diff options
| -rw-r--r-- | changelog.d/10684.bugfix | 1 | ||||
| -rw-r--r-- | synapse/rest/media/v1/thumbnail_resource.py | 26 | ||||
| -rw-r--r-- | tests/rest/media/v1/test_media_storage.py | 39 |
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 |