Fix thumbnailing MPO images (#20025)

Signed-off-by: cat <cat@plan9.rocks>

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.

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

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)

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