From 4dc6e1a90a46cc18c97791aa8f28c82c5d5c0129 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 13 Jun 2022 16:24:56 -0500 Subject: [PATCH 01/13] Provide more info when we don't have any thumbnails to serve Fix https://github.com/matrix-org/synapse/issues/13016 --- synapse/config/repository.py | 25 ++++++++-- synapse/rest/media/v1/thumbnail_resource.py | 51 +++++++++++++++++-- tests/rest/media/v1/test_media_storage.py | 54 ++++++++++++++++++--- 3 files changed, 115 insertions(+), 15 deletions(-) diff --git a/synapse/config/repository.py b/synapse/config/repository.py index 98d8a16621ad..6aa636cb79a4 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -42,6 +42,14 @@ # method: %(method)s """ +THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP = { + "image/jpeg": "jpeg", + "image/jpg": "jpeg", + "image/webp": "jpeg", + "image/gif": "png", + "image/png": "png", +} + HTTP_PROXY_SET_WARNING = """\ The Synapse config url_preview_ip_range_blacklist will be ignored as an HTTP(s) proxy is configured.""" @@ -81,11 +89,18 @@ def parse_thumbnail_requirements( method = size["method"] jpeg_thumbnail = ThumbnailRequirement(width, height, method, "image/jpeg") png_thumbnail = ThumbnailRequirement(width, height, method, "image/png") - requirements.setdefault("image/jpeg", []).append(jpeg_thumbnail) - requirements.setdefault("image/jpg", []).append(jpeg_thumbnail) - requirements.setdefault("image/webp", []).append(jpeg_thumbnail) - requirements.setdefault("image/gif", []).append(png_thumbnail) - requirements.setdefault("image/png", []).append(png_thumbnail) + + for format, thumbnail_format in THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.items(): + requirement = requirements.setdefault(format, []) + if thumbnail_format == "jpeg": + requirement.append(jpeg_thumbnail) + elif thumbnail_format == "png": + requirement.append(png_thumbnail) + else: + raise Exception( + "Unknown thumbnail mapping from %s to %s. This is a Synapse problem, please report!" + % (format, thumbnail_format) + ) return { media_type: tuple(thumbnails) for media_type, thumbnails in requirements.items() } diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 53b156524375..1707569c1647 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -17,11 +17,16 @@ import logging from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple -from synapse.api.errors import SynapseError -from synapse.http.server import DirectServeJsonResource, set_cors_headers +from synapse.api.errors import Codes, SynapseError, cs_error +from synapse.http.server import ( + DirectServeJsonResource, + set_cors_headers, + respond_with_json, +) from synapse.http.servlet import parse_integer, parse_string from synapse.http.site import SynapseRequest from synapse.rest.media.v1.media_storage import MediaStorage +from synapse.config.repository import THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP from ._base import ( FileInfo, @@ -304,6 +309,14 @@ async def _select_and_respond_with_thumbnail( url_cache: True if this is from a URL cache. server_name: The server name, if this is a remote thumbnail. """ + logger.debug( + "_select_and_respond_with_thumbnail: media_id=%s desired=%sx%s (%s) thumbnail_infos=%s", + media_id, + desired_width, + desired_height, + desired_method, + thumbnail_infos, + ) if thumbnail_infos: file_info = self._select_thumbnail( desired_width, @@ -379,8 +392,40 @@ async def _select_and_respond_with_thumbnail( file_info.thumbnail.length, ) else: + # This might be because: + # 1. We can't create thumbnails for the given media (corrupted or + # unsupported file type) + # 2. The thumbnailing process never ran or errored out initially + # when the media was first uploaded (these bugs should be + # reported and fixed). + # 3. `dynamic_thumbnails` is disabled (see `homeserver.yaml`) and we + # can't try to generate one. If this was enabled, Synapse would + # go down a different code path. logger.info("Failed to find any generated thumbnails") - respond_404(request) + + # We could get away with putting this error text directly in the + # error below because it never hits this code path otherwise when + # `dynamic_thumbnails` is disabled. But it would be good not to + # always assume this will be the case in the future so we provide + # accurate information to the user. + dynamic_thumbnails_disabled_warning_text = "" + if not self.dynamic_thumbnails: + dynamic_thumbnails_disabled_warning_text = " We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)." + + respond_with_json( + request, + 400, + cs_error( + "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed.%s" + % ( + request.postpath, + ", ".join(THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.keys()), + dynamic_thumbnails_disabled_warning_text, + ), + code=Codes.UNKNOWN, + ), + send_cors=True, + ) def _select_thumbnail( self, diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 7204b2dfe075..0d0d9dfbf12d 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -133,6 +133,7 @@ class _TestImage: expected_cropped: Optional[bytes] = None expected_scaled: Optional[bytes] = None expected_found: bool = True + unable_to_thumbnail: bool = False @parameterized_class( @@ -190,6 +191,7 @@ class _TestImage: b"image/gif", b".gif", expected_found=False, + unable_to_thumbnail=True, ), ), ], @@ -364,18 +366,29 @@ def test_disposition_none(self) -> None: def test_thumbnail_crop(self) -> None: """Test that a cropped remote thumbnail is available.""" self._test_thumbnail( - "crop", self.test_image.expected_cropped, self.test_image.expected_found + "crop", + self.test_image.expected_cropped, + expected_found=self.test_image.expected_found, + unable_to_thumbnail=self.test_image.unable_to_thumbnail, ) def test_thumbnail_scale(self) -> None: """Test that a scaled remote thumbnail is available.""" self._test_thumbnail( - "scale", self.test_image.expected_scaled, self.test_image.expected_found + "scale", + self.test_image.expected_scaled, + expected_found=self.test_image.expected_found, + unable_to_thumbnail=self.test_image.unable_to_thumbnail, ) def test_invalid_type(self) -> None: """An invalid thumbnail type is never available.""" - self._test_thumbnail("invalid", None, False) + self._test_thumbnail( + "invalid", + None, + expected_found=False, + unable_to_thumbnail=self.test_image.unable_to_thumbnail, + ) @unittest.override_config( {"thumbnail_sizes": [{"width": 32, "height": 32, "method": "scale"}]} @@ -384,7 +397,12 @@ def test_no_thumbnail_crop(self) -> None: """ Override the config to generate only scaled thumbnails, but request a cropped one. """ - self._test_thumbnail("crop", None, False) + self._test_thumbnail( + "crop", + None, + expected_found=False, + unable_to_thumbnail=self.test_image.unable_to_thumbnail, + ) @unittest.override_config( {"thumbnail_sizes": [{"width": 32, "height": 32, "method": "crop"}]} @@ -393,14 +411,22 @@ def test_no_thumbnail_scale(self) -> None: """ Override the config to generate only cropped thumbnails, but request a scaled one. """ - self._test_thumbnail("scale", None, False) + self._test_thumbnail( + "scale", + None, + expected_found=False, + unable_to_thumbnail=self.test_image.unable_to_thumbnail, + ) def test_thumbnail_repeated_thumbnail(self) -> None: """Test that fetching the same thumbnail works, and deleting the on disk thumbnail regenerates it. """ self._test_thumbnail( - "scale", self.test_image.expected_scaled, self.test_image.expected_found + "scale", + self.test_image.expected_scaled, + expected_found=self.test_image.expected_found, + unable_to_thumbnail=self.test_image.unable_to_thumbnail, ) if not self.test_image.expected_found: @@ -457,7 +483,11 @@ def test_thumbnail_repeated_thumbnail(self) -> None: ) def _test_thumbnail( - self, method: str, expected_body: Optional[bytes], expected_found: bool + self, + method: str, + expected_body: Optional[bytes], + expected_found: bool, + unable_to_thumbnail: bool = False, ) -> None: params = "?width=32&height=32&method=" + method channel = make_request( @@ -488,6 +518,16 @@ def _test_thumbnail( else: # ensure that the result is at least some valid image Image.open(BytesIO(channel.result["body"])) + elif unable_to_thumbnail: + # A 400 with a JSON body. + self.assertEqual(channel.code, 400) + self.assertEqual( + channel.json_body, + { + "errcode": "M_UNKNOWN", + "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or your media is corrupted and cannot be thumbnailed. We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`).", + }, + ) else: # A 404 with a JSON body. self.assertEqual(channel.code, 404) From 4bf53bcde6cbb63e57370b3bd66d6d3c716ff41e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 13 Jun 2022 16:29:01 -0500 Subject: [PATCH 02/13] Add changelog --- changelog.d/13038.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13038.feature diff --git a/changelog.d/13038.feature b/changelog.d/13038.feature new file mode 100644 index 000000000000..1278f1b4e992 --- /dev/null +++ b/changelog.d/13038.feature @@ -0,0 +1 @@ +Provide more info why we don't have any thumbnails to serve. From 9348242a3b12a3210fae2d6e42601f87601c2638 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 13 Jun 2022 16:36:40 -0500 Subject: [PATCH 03/13] Fix lints --- synapse/rest/media/v1/thumbnail_resource.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 1707569c1647..d506a0ed65a6 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -18,15 +18,15 @@ from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple from synapse.api.errors import Codes, SynapseError, cs_error +from synapse.config.repository import THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP from synapse.http.server import ( DirectServeJsonResource, - set_cors_headers, respond_with_json, + set_cors_headers, ) from synapse.http.servlet import parse_integer, parse_string from synapse.http.site import SynapseRequest from synapse.rest.media.v1.media_storage import MediaStorage -from synapse.config.repository import THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP from ._base import ( FileInfo, From 9a6b85b7f5232a6f19551ff310c5f6791cbe0757 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 6 Jul 2022 09:14:57 -0500 Subject: [PATCH 04/13] Clarify why gif -> png See https://github.com/matrix-org/synapse/pull/13038#discussion_r900011373 --- synapse/config/repository.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/config/repository.py b/synapse/config/repository.py index 97bf9e75bd3e..5860d6c3c905 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -46,6 +46,8 @@ "image/jpeg": "jpeg", "image/jpg": "jpeg", "image/webp": "jpeg", + # Thumbnails can only be jpeg or png. We choose png thumbnails for gif + # because it can have transparency. "image/gif": "png", "image/png": "png", } From f881c39cd43ea5c887ce0146e54e527cb77682c5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 6 Jul 2022 09:16:15 -0500 Subject: [PATCH 05/13] Document what the map represents See https://github.com/matrix-org/synapse/pull/13038#discussion_r900000704 --- synapse/config/repository.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/config/repository.py b/synapse/config/repository.py index 5860d6c3c905..59a473608fe6 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -42,6 +42,8 @@ # method: %(method)s """ +# A map from the given media type to the type of thumbnail we should generate +# for it. THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP = { "image/jpeg": "jpeg", "image/jpg": "jpeg", From ad5aa809f18a579822ed5772ee118132e03fcbfe Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 6 Jul 2022 09:39:58 -0500 Subject: [PATCH 06/13] Just assert the environment we expect this function to be run in See https://github.com/matrix-org/synapse/pull/13038#discussion_r900008532 --- synapse/rest/media/v1/thumbnail_resource.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index d506a0ed65a6..8d17dcdd56e0 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -317,6 +317,11 @@ async def _select_and_respond_with_thumbnail( desired_method, thumbnail_infos, ) + + # If `dynamic_thumbnails` is enabled, we expect Synapse to go down a + # different code path to handle it. + assert not self.dynamic_thumbnails + if thumbnail_infos: file_info = self._select_thumbnail( desired_width, @@ -399,28 +404,18 @@ async def _select_and_respond_with_thumbnail( # when the media was first uploaded (these bugs should be # reported and fixed). # 3. `dynamic_thumbnails` is disabled (see `homeserver.yaml`) and we - # can't try to generate one. If this was enabled, Synapse would + # can't generate one. If this was enabled, Synapse would # go down a different code path. logger.info("Failed to find any generated thumbnails") - # We could get away with putting this error text directly in the - # error below because it never hits this code path otherwise when - # `dynamic_thumbnails` is disabled. But it would be good not to - # always assume this will be the case in the future so we provide - # accurate information to the user. - dynamic_thumbnails_disabled_warning_text = "" - if not self.dynamic_thumbnails: - dynamic_thumbnails_disabled_warning_text = " We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)." - respond_with_json( request, 400, cs_error( - "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed.%s" + "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed. We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)." % ( request.postpath, ", ".join(THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.keys()), - dynamic_thumbnails_disabled_warning_text, ), code=Codes.UNKNOWN, ), From ac9285f0f5deabf643533fdf67aee1b187d62eb6 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 6 Jul 2022 09:57:08 -0500 Subject: [PATCH 07/13] Add docstrings See - https://github.com/matrix-org/synapse/pull/13038#discussion_r900012686 - https://github.com/matrix-org/synapse/pull/13038#discussion_r900013735 --- synapse/rest/media/v1/thumbnail_resource.py | 2 +- tests/rest/media/v1/test_media_storage.py | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 8d17dcdd56e0..24e1b4648f79 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -412,7 +412,7 @@ async def _select_and_respond_with_thumbnail( request, 400, cs_error( - "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed. We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)." + "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed. We also cannot generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)." % ( request.postpath, ", ".join(THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.keys()), diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 0d0d9dfbf12d..65d5e472707d 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -124,7 +124,9 @@ class _TestImage: expected_scaled: The expected bytes from scaled thumbnailing, or None if test should just check for a valid image returned. expected_found: True if the file should exist on the server, or False if - a 404 is expected. + a 404/400 is expected. + unable_to_thumbnail: True if we expect the thumbnailing to fail (400), or + False if the thumbnailing should succeed or a normal 404 is expected. """ data: bytes @@ -489,6 +491,18 @@ def _test_thumbnail( expected_found: bool, unable_to_thumbnail: bool = False, ) -> None: + """Test the given thumbnailing method works as expected. + + Args: + method: The thumbnailing method to use (crop, scale). + expected_body: The expected bytes from cropped thumbnailing, or None if + test should just check for a valid image. + expected_found: True if the file should exist on the server, or False if + a 404/400 is expected. + unable_to_thumbnail: True if we expect the thumbnailing to fail (400), or + False if the thumbnailing should succeed or a normal 404 is expected. + """ + params = "?width=32&height=32&method=" + method channel = make_request( self.reactor, @@ -525,7 +539,7 @@ def _test_thumbnail( channel.json_body, { "errcode": "M_UNKNOWN", - "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or your media is corrupted and cannot be thumbnailed. We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`).", + "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or your media is corrupted and cannot be thumbnailed. We also cannot generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`).", }, ) else: From 67c13abd1498211fcd17b2d7e8522623eba52376 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 6 Jul 2022 10:03:01 -0500 Subject: [PATCH 08/13] Simpler dynamic thumbnail disabled error message See https://github.com/matrix-org/synapse/pull/13038#discussion_r900013954 --- synapse/rest/media/v1/thumbnail_resource.py | 2 +- tests/rest/media/v1/test_media_storage.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 24e1b4648f79..1b2ee7145aa9 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -412,7 +412,7 @@ async def _select_and_respond_with_thumbnail( request, 400, cs_error( - "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed. We also cannot generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)." + "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed. Additionally, dynamic thumbnails are disabled on this server." % ( request.postpath, ", ".join(THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.keys()), diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 65d5e472707d..644289d585d5 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -539,7 +539,7 @@ def _test_thumbnail( channel.json_body, { "errcode": "M_UNKNOWN", - "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or your media is corrupted and cannot be thumbnailed. We also cannot generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`).", + "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or your media is corrupted and cannot be thumbnailed. Additionally, dynamic thumbnails are disabled on this server.", }, ) else: From d3308f8f5237926347fb059a49ad17f96479e063 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 6 Jul 2022 10:10:28 -0500 Subject: [PATCH 09/13] Fix copy-paste error --- tests/rest/media/v1/test_media_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index a7f0e7a571cc..edfe3a2bf9b3 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -495,7 +495,7 @@ def _test_thumbnail( Args: method: The thumbnailing method to use (crop, scale). - expected_body: The expected bytes from cropped thumbnailing, or None if + expected_body: The expected bytes from thumbnailing, or None if test should just check for a valid image. expected_found: True if the file should exist on the server, or False if a 404/400 is expected. From 990450fe46ea445cba481fc9c7727cac4f6f2fa7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 12 Jul 2022 10:49:01 -0500 Subject: [PATCH 10/13] Inline requirements See https://github.com/matrix-org/synapse/pull/13038#discussion_r918196612 --- synapse/config/repository.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/config/repository.py b/synapse/config/repository.py index ae1593053cf2..1033496bb43d 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -91,15 +91,17 @@ def parse_thumbnail_requirements( width = size["width"] height = size["height"] method = size["method"] - jpeg_thumbnail = ThumbnailRequirement(width, height, method, "image/jpeg") - png_thumbnail = ThumbnailRequirement(width, height, method, "image/png") for format, thumbnail_format in THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.items(): requirement = requirements.setdefault(format, []) if thumbnail_format == "jpeg": - requirement.append(jpeg_thumbnail) + requirement.append( + ThumbnailRequirement(width, height, method, "image/jpeg") + ) elif thumbnail_format == "png": - requirement.append(png_thumbnail) + requirement.append( + ThumbnailRequirement(width, height, method, "image/png") + ) else: raise Exception( "Unknown thumbnail mapping from %s to %s. This is a Synapse problem, please report!" From 6a4292574973d40a4c9de3691bf607b692a4d65a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 12 Jul 2022 10:52:17 -0500 Subject: [PATCH 11/13] More clear comment --- synapse/rest/media/v1/thumbnail_resource.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 419894bb091f..2f9df106a82d 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -406,8 +406,9 @@ async def _select_and_respond_with_thumbnail( # when the media was first uploaded (these bugs should be # reported and fixed). # 3. `dynamic_thumbnails` is disabled (see `homeserver.yaml`) and we - # can't generate one. If this was enabled, Synapse would - # go down a different code path. + # failed to generate thumbnails on initial upload. If + # `dynamic_thumbnails` was enabled, Synapse would go down a + # different code path. logger.info("Failed to find any generated thumbnails") respond_with_json( From 52ee48f76bb2577287887807315d4a451b43a9b4 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 12 Jul 2022 10:59:00 -0500 Subject: [PATCH 12/13] Update error to be a little bit more generic and friendly See https://github.com/matrix-org/synapse/pull/13038#discussion_r918208305 --- synapse/rest/media/v1/thumbnail_resource.py | 2 +- tests/rest/media/v1/test_media_storage.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 2f9df106a82d..5053901e7383 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -415,7 +415,7 @@ async def _select_and_respond_with_thumbnail( request, 400, cs_error( - "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed. Additionally, dynamic thumbnails are disabled on this server." + "Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or that thumbnailing failed for some other reason. (Dynamic thumbnails are disabled on this server.)" % ( request.postpath, ", ".join(THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.keys()), diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index edfe3a2bf9b3..ab18c70a7b29 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -545,7 +545,7 @@ def _test_thumbnail( channel.json_body, { "errcode": "M_UNKNOWN", - "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or your media is corrupted and cannot be thumbnailed. Additionally, dynamic thumbnails are disabled on this server.", + "error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or that thumbnailing failed for some other reason. (Dynamic thumbnails are disabled on this server.)", }, ) else: From 06405dbc65428d1cfd62846b7c651ad90346b237 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 14 Jul 2022 13:03:07 -0500 Subject: [PATCH 13/13] Updated description of why no thumbnail See https://github.com/matrix-org/synapse/pull/13038#discussion_r920934393 --- synapse/rest/media/v1/thumbnail_resource.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 5053901e7383..5f725c76009f 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -401,14 +401,12 @@ async def _select_and_respond_with_thumbnail( else: # This might be because: # 1. We can't create thumbnails for the given media (corrupted or - # unsupported file type) + # unsupported file type), or # 2. The thumbnailing process never ran or errored out initially # when the media was first uploaded (these bugs should be # reported and fixed). - # 3. `dynamic_thumbnails` is disabled (see `homeserver.yaml`) and we - # failed to generate thumbnails on initial upload. If - # `dynamic_thumbnails` was enabled, Synapse would go down a - # different code path. + # Note that we don't attempt to generate a thumbnail now because + # `dynamic_thumbnails` is disabled. logger.info("Failed to find any generated thumbnails") respond_with_json(