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
|
