Experimental GIF Thumbnailing support #9705
Conversation
synapse/config/repository.py
Outdated
| @@ -70,6 +70,7 @@ def parse_thumbnail_requirements(thumbnail_sizes): | |||
| method = size["method"] | |||
| jpeg_thumbnail = ThumbnailRequirement(width, height, method, "image/jpeg") | |||
| png_thumbnail = ThumbnailRequirement(width, height, method, "image/png") | |||
| gif_thumbnail = ThumbnailRequirement(width, height, method, "image/gif") | |||
There was a problem hiding this comment.
no idea how to go further with this, as this happens in a .config. file, and to "enable" this, you'd need to grab something from the "experimental" config section (as is custom for experimental stuff), however, i dunno how to do this without messing up imports or something
There was a problem hiding this comment.
There are other interdependencies between config sections (see for example ApiConfig which checks self.spaces_enabled, which is part of ExperimentalConfig). As long as ContentRepositoryConfig appears after ExperimentalConfig in the HomeServerConfig definition, this will work. It's not particularly elegant, but 🤷♂️
| @@ -93,27 +93,16 @@ def aspect(self, max_width: int, max_height: int) -> Tuple[int, int]: | |||
| else: | |||
| return (max_height * self.width) // self.height, max_height | |||
|
|
|||
| def _resize(self, width: int, height: int) -> Image: | |||
There was a problem hiding this comment.
(reordering public methods above private ones)
| self.image, output_type, lambda i: self._crop(i, width, height) | ||
| ) | ||
|
|
||
| def _process_animated_aware( |
There was a problem hiding this comment.
rule of three should apply here, but this piece is a bit complex, and the only thing that'd really change between the two functions is changing _resize with _crop, thus the abstraction
| else: | ||
| return self._encode_image(transform(image), output_type) | ||
|
|
||
| def _crop(self, image: Image, width: int, height: int) -> Image: |
There was a problem hiding this comment.
to-private and addition of image to decouple it and make it able to be used as a generic image transformer
|
Are you hoping to get feedback for this or still tweaking it? (Are you looking for a review?) |
|
On above self-reviews, yes, I'm inviting comments, because i need some direction on this before i actually finish this, i wish I could communicate that clearer, but i think putting it on "ready" isn't one of them, as then the PR would be considered done, which it certainly isn't. |
There was a problem hiding this comment.
I'm hesitant about the concept here, even behind an experimental option, unless it's a thing that the matrix protocol actually wants to support. In the best case it becomes a maintenance burden in Synapse; in the worst case lots of people start using gifs (animated or otherwise) and it becomes a thing the matrix protocol has to support forever.
In the interest of trying to stimulate discussion on this from the spec POV, I've commented at https://github.com/matrix-org/matrix-doc/issues/1938#issuecomment-810968551.
|
I agree, I sorta found the way thumbnails are identified/retrieved fairly weird, maybe a comprehensive thumbnail endpoint (f.e. one that supports multiple representations of the same media, animated or not) could definitively solve this. I'll wait for a resolution/MSC, until then, this PR is probably stalled. |
|
This PR almost certainly has bitrotted, and it is still blocked on matrix-org/matrix-spec#453, so i'll close this. |
Hopefully resolves #1278
Adds in some fixes and refactors to
synapse.rest.media.v1.thumbnailer.Thumbnailerthat makes it able to recogniseimage/gifas an output type and transform images accordingly.This is experimental for the following reasons;
For these reasons, GIF thumbnailing support is experimental and disabled by default.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.Signed-off-by: Jonathan de Jong <[email protected]>