diff --git a/changelog.d/20025.bugfix b/changelog.d/20025.bugfix new file mode 100644 index 0000000000..b5feabd30b --- /dev/null +++ b/changelog.d/20025.bugfix @@ -0,0 +1 @@ +Fix thumbnail generation failing for MPO images. Animations that cannot be decoded now fall back to a static thumbnail. diff --git a/synapse/media/thumbnailer.py b/synapse/media/thumbnailer.py index d60ca16f26..27f016931a 100644 --- a/synapse/media/thumbnailer.py +++ b/synapse/media/thumbnailer.py @@ -79,6 +79,10 @@ class Thumbnailer: # format in this list becomes part of our trusted computing base. PILLOW_FORMATS = ("jpeg", "png", "webp", "gif") + # Pillow reports MPO (a JPEG holding a stereo pair) as multi-frame, so + # frame count alone doesn't tell us whether something is an animation. + ANIMATED_FORMATS = frozenset({"GIF", "PNG", "WEBP"}) + @staticmethod def set_limits(max_image_pixels: int) -> None: Image.MAX_IMAGE_PIXELS = max_image_pixels @@ -86,6 +90,12 @@ class Thumbnailer: def __init__(self, input_path: str): # Have we closed the image? self._closed = False + # Whether attempting to thumbnail the image failed for some reason. The + # thumbnailing code should fallback to treating the image as static in + # this case. + # + # Cached so a broken animation isn't re-decoded for every thumbnail size. + self._animation_broken = False try: self.image = Image.open(input_path, formats=self.PILLOW_FORMATS) @@ -166,30 +176,52 @@ class Thumbnailer: @property def is_animated(self) -> bool: + if self._animation_broken: + return False + if self.image.format not in self.ANIMATED_FORMATS: + return False return getattr(self.image, "is_animated", False) def _encode_animated_from( self, transform: Callable[[Image.Image], Image.Image] - ) -> BytesIO: + ) -> BytesIO | None: """Apply `transform` to every frame of the source image and encode the - result as an animated thumbnail.""" + result as an animated thumbnail, or None if the source could not be + decoded as an animation and the caller should fall back to a static one. + """ frames = [] durations = [] loop = self.image.info.get("loop", 0) - for frame in ImageSequence.Iterator(self.image): - # Copy the frame to avoid referencing the original image memory. - f = frame.copy() - if f.mode != "RGBA": - f = f.convert("RGBA") - frames.append(transform(f)) - # A duration of 0 is valid (interpretation is implementation-defined, - # see RFC 9649 section 2.7.1.1), so only fall back when unset. The - # 100ms default matches libwebp's animation tools. - duration = frame.info.get("duration") - if duration is None: - duration = self.image.info.get("duration", 100) - durations.append(duration) - return self._encode_animated(frames, durations, loop) + try: + for frame in ImageSequence.Iterator(self.image): + # Copy the frame to avoid referencing the original image memory. + f = frame.copy() + if f.mode != "RGBA": + f = f.convert("RGBA") + frames.append(transform(f)) + # A duration of 0 is valid (interpretation is implementation-defined, + # see RFC 9649 section 2.7.1.1), so only fall back when unset. The + # 100ms default matches libwebp's animation tools. + duration = frame.info.get("duration") + if duration is None: + duration = self.image.info.get("duration", 100) + durations.append(duration) + return self._encode_animated(frames, durations, loop) + except Exception as e: + logger.warning( + "Failed to generate an animated thumbnail, falling back to a " + "static one: %s", + e, + ) + self._animation_broken = True + + try: + # Leave the source on its first frame for the static fallback. + self.image.seek(0) + except Exception as e: + logger.warning("Failed to rewind image to its first frame: %s", e) + + return None @trace def scale( @@ -204,9 +236,11 @@ class Thumbnailer: The bytes of the encoded image ready to be written to disk """ if animated and self.is_animated: - return self._encode_animated_from( + output = self._encode_animated_from( lambda f: self._resize_image(f, width, height) ) + if output is not None: + return output with self._resize_image(self.image, width, height) as scaled: return self._encode_image(scaled, output_type) @@ -242,9 +276,11 @@ class Thumbnailer: crop = (crop_left, 0, crop_right, height) if animated and self.is_animated: - return self._encode_animated_from( + output = self._encode_animated_from( lambda f: self._resize_image(f, scaled_width, scaled_height).crop(crop) ) + if output is not None: + return output with self._resize_image( self.image, scaled_width, scaled_height diff --git a/tests/media/test_media_storage.py b/tests/media/test_media_storage.py index 54d1262054..430a1d6789 100644 --- a/tests/media/test_media_storage.py +++ b/tests/media/test_media_storage.py @@ -1428,6 +1428,22 @@ def _make_animated_gif() -> bytes: return out.getvalue() +def _make_mpo() -> bytes: + """Build a two-image MPO: a JPEG holding a stereo pair, not an animation.""" + frames = [Image.new("RGB", (64, 64), color) for color in ((255, 0, 0), (0, 0, 255))] + out = BytesIO() + frames[0].save(out, format="MPO", save_all=True, append_images=frames[1:]) + return out.getvalue() + + +def _make_stale_mpo() -> bytes: + """Build an MPO whose trailing image is stripped but still advertised.""" + data = _make_mpo() + with Image.open(BytesIO(data)) as image: + primary_size = image.mpinfo[0xB002][0]["Size"] # type: ignore[attr-defined] + return data[:primary_size] + + class ThumbnailerAnimatedTestCase(unittest.TestCase): """Tests that the thumbnailer only animates when explicitly asked to.""" @@ -1444,6 +1460,24 @@ class ThumbnailerAnimatedTestCase(unittest.TestCase): with open(self.png_path, "wb") as f: f.write(SMALL_PNG) + self.mpo_path = os.path.join(self.tempdir, "stereo.jpg") + with open(self.mpo_path, "wb") as f: + f.write(_make_mpo()) + + self.stale_mpo_path = os.path.join(self.tempdir, "stale.jpg") + with open(self.stale_mpo_path, "wb") as f: + f.write(_make_stale_mpo()) + + def assert_is_first_frame(self, output: BytesIO) -> None: + """Raises an `AssertionError` unless the given image is red.""" + pixel = Image.open(output).convert("RGB").getpixel((16, 16)) + assert isinstance(pixel, tuple) + red, green, blue = pixel + # The first frame of every source here is red. + # WebP is lossy, so allow *some* green/blue to be present. + self.assertGreater(red, 200) + self.assertLess(max(green, blue), 50) + def test_scale_static_by_default(self) -> None: """An animated source produces a static thumbnail unless animated=True.""" with Thumbnailer(self.gif_path) as thumbnailer: @@ -1475,3 +1509,83 @@ class ThumbnailerAnimatedTestCase(unittest.TestCase): out = thumbnailer.scale(1, 1, ANIMATED_THUMBNAIL_TYPE, animated=True) result = Image.open(out) self.assertFalse(getattr(result, "is_animated", False)) + + @parameterized.expand([("GIF", "gif"), ("PNG", "apng"), ("WEBP", "webp")]) + def test_animated_formats(self, fmt: str, ext: str) -> None: + """Every animated format we accept produces an animated thumbnail.""" + frames = [ + Image.new("RGBA", (64, 64), color) + for color in ((255, 0, 0, 255), (0, 0, 255, 255)) + ] + out = BytesIO() + frames[0].save( + out, + format=fmt, + save_all=True, + append_images=frames[1:], + duration=100, + loop=0, + ) + path = os.path.join(self.tempdir, f"animated.{ext}") + with open(path, "wb") as f: + f.write(out.getvalue()) + + with Thumbnailer(path) as thumbnailer: + self.assertTrue(thumbnailer.is_animated) + thumbnail = thumbnailer.scale( + 32, 32, ANIMATED_THUMBNAIL_TYPE, animated=True + ) + result = Image.open(thumbnail) + self.assertEqual(result.format, "WEBP") + self.assertTrue(getattr(result, "is_animated", False)) + self.assertEqual(getattr(result, "n_frames", 1), 2) + + @parameterized.expand(["scale", "crop"]) + def test_mpo_is_not_animated(self, method: str) -> None: + """An MPO packs several stills into one JPEG; not an animation.""" + with Thumbnailer(self.mpo_path) as thumbnailer: + self.assertFalse(thumbnailer.is_animated) + out = getattr(thumbnailer, method)( + 32, 32, ANIMATED_THUMBNAIL_TYPE, animated=True + ) + self.assertFalse(getattr(Image.open(out), "is_animated", False)) + self.assert_is_first_frame(out) + + @parameterized.expand(["scale", "crop"]) + def test_stale_mpo_index_does_not_raise(self, method: str) -> None: + """An MPO advertising frames that are not in the file still thumbnails. + + Regression test for https://github.com/element-hq/synapse/issues/20024. + """ + with Thumbnailer(self.stale_mpo_path) as thumbnailer: + out = getattr(thumbnailer, method)( + 32, 32, ANIMATED_THUMBNAIL_TYPE, animated=True + ) + self.assertEqual(Image.open(out).format, "WEBP") + self.assert_is_first_frame(out) + + def test_fallback_thumbnails_the_first_frame(self) -> None: + """Failing after the frames are read leaves the source parked on the + last one, so the fallback has to rewind.""" + with patch.object(Thumbnailer, "_encode_animated", side_effect=ValueError): + with Thumbnailer(self.gif_path) as thumbnailer: + out = thumbnailer.scale(32, 32, ANIMATED_THUMBNAIL_TYPE, animated=True) + self.assert_is_first_frame(out) + + @parameterized.expand(["scale", "crop"]) + def test_undecodable_animation_falls_back_to_static(self, method: str) -> None: + """If the frames can't be decoded we still serve a static thumbnail of + the first frame rather than failing the request.""" + # Force the stale MPO down the animated path so decoding it fails. + with patch.object(Thumbnailer, "ANIMATED_FORMATS", frozenset({"MPO"})): + with Thumbnailer(self.stale_mpo_path) as thumbnailer: + self.assertTrue(thumbnailer.is_animated) + out = getattr(thumbnailer, method)( + 32, 32, ANIMATED_THUMBNAIL_TYPE, animated=True + ) + # A broken source isn't retried for every other thumbnail size. + self.assertFalse(thumbnailer.is_animated) + + self.assertEqual(Image.open(out).format, "WEBP") + self.assertFalse(getattr(Image.open(out), "is_animated", False)) + self.assert_is_first_frame(out) diff --git a/tests/rest/client/test_media.py b/tests/rest/client/test_media.py index 1cf401ed4f..b20417a6ed 100644 --- a/tests/rest/client/test_media.py +++ b/tests/rest/client/test_media.py @@ -3441,6 +3441,23 @@ class AnimatedThumbnailTestCase(unittest.HomeserverTestCase): ) return out.getvalue() + def _make_mpo(self, stale: bool = False) -> bytes: + """Build a two-image MPO: a JPEG holding a stereo pair, not an + animation. If `stale` is set, the trailing image is stripped but still + advertised.""" + frames = [ + Image.new("RGB", (64, 64), color) for color in ((255, 0, 0), (0, 0, 255)) + ] + out = io.BytesIO() + frames[0].save(out, format="MPO", save_all=True, append_images=frames[1:]) + data = out.getvalue() + if not stale: + return data + + with Image.open(io.BytesIO(data)) as image: + primary_size = image.mpinfo[0xB002][0]["Size"] # type: ignore[attr-defined] + return data[:primary_size] + def _upload(self, data: bytes, content_type: str) -> MXCUri: return self.get_success( self.repo.create_or_update_content( @@ -3515,6 +3532,37 @@ class AnimatedThumbnailTestCase(unittest.HomeserverTestCase): result = Image.open(io.BytesIO(channel.result["body"])) self.assertFalse(getattr(result, "is_animated", False)) + @parameterized.expand([("intact", False), ("stale_index", True)]) + def test_mpo_is_thumbnailed_as_a_still(self, _name: str, stale: bool) -> None: + """An MPO holds several stills rather than an animation, so it never + gets an animated thumbnail. + + Regression test for https://github.com/element-hq/synapse/issues/20024. + """ + mpo_uri = self._upload(self._make_mpo(stale=stale), "image/jpeg") + + thumbnails = self.get_success( + self.store.get_local_media_thumbnails(mpo_uri.media_id) + ) + self.assertTrue(thumbnails) + self.assertNotIn("image/webp", [info.type for info in thumbnails]) + + channel = self._thumbnail(mpo_uri, animated="true") + result = Image.open(io.BytesIO(channel.result["body"])) + self.assertFalse(getattr(result, "is_animated", False)) + + @parameterized.expand([("intact", False), ("stale_index", True)]) + @override_config({"dynamic_thumbnails": True}) + def test_dynamic_thumbnails_of_mpo_are_static( + self, _name: str, stale: bool + ) -> None: + """The same holds when the thumbnail is generated on demand.""" + mpo_uri = self._upload(self._make_mpo(stale=stale), "image/jpeg") + + channel = self._thumbnail(mpo_uri, animated="true") + result = Image.open(io.BytesIO(channel.result["body"])) + self.assertFalse(getattr(result, "is_animated", False)) + @override_config({"dynamic_thumbnails": True}) def test_dynamic_thumbnails_generates_and_caches_animated(self) -> None: """With dynamic thumbnails, animated thumbnails are generated on demand