Fix error when selecting between thumbnails with the same quality (#10684)

Fixes #10318

3 files changed, 56 insertions, 10 deletions

diff --git a/changelog.d/10684.bugfix b/changelog.d/10684.bugfix
new file mode 100644
index 0000000000..311b17601a
--- /dev/null
+++ b/changelog.d/10684.bugfix
@@ -0,0 +1 @@
+Fix long-standing issue which caused an error when a thumbnail is requested and there are multiple thumbnails with the same quality rating.
diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py
index a029d426f0..12bd745cb2 100644
--- a/synapse/rest/media/v1/thumbnail_resource.py
+++ b/synapse/rest/media/v1/thumbnail_resource.py
@@ -15,7 +15,7 @@
 
 
 import logging
-from typing import TYPE_CHECKING, Any, Dict, List, Optional
+from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple
 
 from twisted.web.server import Request
 
@@ -414,9 +414,9 @@ class ThumbnailResource(DirectServeJsonResource):
 
         if desired_method == "crop":
             # Thumbnails that match equal or larger sizes of desired width/height.
-            crop_info_list = []
+            crop_info_list: List[Tuple[int, int, int, bool, int, Dict[str, Any]]] = []
             # Other thumbnails.
-            crop_info_list2 = []
+            crop_info_list2: List[Tuple[int, int, int, bool, int, Dict[str, Any]]] = []
             for info in thumbnail_infos:
                 # Skip thumbnails generated with different methods.
                 if info["thumbnail_method"] != "crop":
@@ -451,15 +451,19 @@ class ThumbnailResource(DirectServeJsonResource):
                             info,
                         )
                     )
+            # Pick the most appropriate thumbnail. Some values of `desired_width` and
+            # `desired_height` may result in a tie, in which case we avoid comparing on
+            # the thumbnail info dictionary and pick the thumbnail that appears earlier
+            # in the list of candidates.
             if crop_info_list:
-                thumbnail_info = min(crop_info_list)[-1]
+                thumbnail_info = min(crop_info_list, key=lambda t: t[:-1])[-1]
             elif crop_info_list2:
-                thumbnail_info = min(crop_info_list2)[-1]
+                thumbnail_info = min(crop_info_list2, key=lambda t: t[:-1])[-1]
         elif desired_method == "scale":
             # Thumbnails that match equal or larger sizes of desired width/height.
-            info_list = []
+            info_list: List[Tuple[int, bool, int, Dict[str, Any]]] = []
             # Other thumbnails.
-            info_list2 = []
+            info_list2: List[Tuple[int, bool, int, Dict[str, Any]]] = []
 
             for info in thumbnail_infos:
                 # Skip thumbnails generated with different methods.
@@ -477,10 +481,14 @@ class ThumbnailResource(DirectServeJsonResource):
                     info_list2.append(
                         (size_quality, type_quality, length_quality, info)
                     )
+            # Pick the most appropriate thumbnail. Some values of `desired_width` and
+            # `desired_height` may result in a tie, in which case we avoid comparing on
+            # the thumbnail info dictionary and pick the thumbnail that appears earlier
+            # in the list of candidates.
             if info_list:
-                thumbnail_info = min(info_list)[-1]
+                thumbnail_info = min(info_list, key=lambda t: t[:-1])[-1]
             elif info_list2:
-                thumbnail_info = min(info_list2)[-1]
+                thumbnail_info = min(info_list2, key=lambda t: t[:-1])[-1]
 
         if thumbnail_info:
             return FileInfo(
diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py
index 6085444b9d..2f7eebfe69 100644
--- a/tests/rest/media/v1/test_media_storage.py
+++ b/tests/rest/media/v1/test_media_storage.py
@@ -21,7 +21,7 @@ from unittest.mock import Mock
 from urllib import parse
 
 import attr
-from parameterized import parameterized_class
+from parameterized import parameterized, parameterized_class
 from PIL import Image as Image
 
 from twisted.internet import defer
@@ -473,6 +473,43 @@ class MediaRepoTests(unittest.HomeserverTestCase):
                 },
             )
 
+    @parameterized.expand([("crop", 16), ("crop", 64), ("scale", 16), ("scale", 64)])
+    def test_same_quality(self, method, desired_size):
+        """Test that choosing between thumbnails with the same quality rating succeeds.
+
+        We are not particular about which thumbnail is chosen."""
+        self.assertIsNotNone(
+            self.thumbnail_resource._select_thumbnail(
+                desired_width=desired_size,
+                desired_height=desired_size,
+                desired_method=method,
+                desired_type=self.test_image.content_type,
+                # Provide two identical thumbnails which are guaranteed to have the same
+                # quality rating.
+                thumbnail_infos=[
+                    {
+                        "thumbnail_width": 32,
+                        "thumbnail_height": 32,
+                        "thumbnail_method": method,
+                        "thumbnail_type": self.test_image.content_type,
+                        "thumbnail_length": 256,
+                        "filesystem_id": f"thumbnail1{self.test_image.extension}",
+                    },
+                    {
+                        "thumbnail_width": 32,
+                        "thumbnail_height": 32,
+                        "thumbnail_method": method,
+                        "thumbnail_type": self.test_image.content_type,
+                        "thumbnail_length": 256,
+                        "filesystem_id": f"thumbnail2{self.test_image.extension}",
+                    },
+                ],
+                file_id=f"image{self.test_image.extension}",
+                url_cache=None,
+                server_name=None,
+            )
+        )
+
     def test_x_robots_tag_header(self):
         """
         Tests that the `X-Robots-Tag` header is present, which informs web crawlers