Animated thumbnails #18831
Animated thumbnails #18831
Conversation
Signed-off-by: cat <[email protected]>
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds animated image support to the media thumbnailer by enabling generation of animated thumbnails that preserve animation, loop count, and frame durations. The changes extend the existing thumbnail functionality to handle animated images like GIFs while maintaining backward compatibility with static images.
Key changes:
- Refactored image resizing logic to support both single frames and animated sequences
- Added animated thumbnail encoding that preserves animation properties
- Enhanced both scale and crop operations to handle animated images
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| synapse/media/thumbnailer.py | Core implementation adding animated image processing to scale/crop methods and new encoding functionality |
| changelog.d/18831.feature | Changelog entry documenting the new animated image support feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| frames = [] | ||
| durations = [] | ||
| loop = self.image.info.get("loop", 0) | ||
| transparency = self.image.info.get("transparency", None) | ||
| for frame in ImageSequence.Iterator(self.image): | ||
| # Copy the frame to avoid referencing the original image memory | ||
| f = frame.copy() | ||
| if f.mode != "RGBA": | ||
| f = f.convert("RGBA") | ||
| resized = self._resize_image(f, width, height) | ||
| frames.append(resized) | ||
| durations.append( | ||
| frame.info.get("duration") or self.image.info.get("duration") or 100 | ||
| ) | ||
| return self._encode_animated(frames, durations, loop, transparency) |
There was a problem hiding this comment.
The animated processing logic is duplicated between scale() and crop() methods. Consider extracting this into a shared helper method to reduce code duplication.
| frames = [] | |
| durations = [] | |
| loop = self.image.info.get("loop", 0) | |
| transparency = self.image.info.get("transparency", None) | |
| for frame in ImageSequence.Iterator(self.image): | |
| # Copy the frame to avoid referencing the original image memory | |
| f = frame.copy() | |
| if f.mode != "RGBA": | |
| f = f.convert("RGBA") | |
| resized = self._resize_image(f, width, height) | |
| frames.append(resized) | |
| durations.append( | |
| frame.info.get("duration") or self.image.info.get("duration") or 100 | |
| ) | |
| return self._encode_animated(frames, durations, loop, transparency) | |
| return self._process_animated(width, height, self._resize_image) |
Copilot uses AI. Check for mistakes.
| if transparency is not None: | ||
| save_kwargs["transparency"] = transparency | ||
|
|
||
| paletted_frames[0].save(output_bytes_io, **cast(dict, save_kwargs)) |
There was a problem hiding this comment.
The cast(dict, save_kwargs) is unnecessary since save_kwargs is already declared as a dict. This cast adds complexity without benefit.
| paletted_frames[0].save(output_bytes_io, **cast(dict, save_kwargs)) | |
| paletted_frames[0].save(output_bytes_io, **save_kwargs) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
When you try the suggestion, you run into:
$ poetry run mypy synapse/media/thumbnailer.py
synapse/media/thumbnailer.py:311: note: Revealed type is "builtins.dict[builtins.str, builtins.object]"
synapse/media/thumbnailer.py:313: error: Argument 2 to "save" of "Image" has incompatible type "**dict[str, object]"; expected "Optional[str]" [arg-type]
synapse/media/thumbnailer.py:313: error: Argument 2 to "save" of "Image" has incompatible type "**dict[str, object]"; expected "bool" [arg-type]
synapse/media/thumbnailer.py:313: error: Argument 2 to "save" of "Image" has incompatible type "**dict[str, object]"; expected "Literal['bmp', 'png']" [arg-type]But overall, the cast isn't very good.
It would be better to annotate the correct type where it is declared save_kwargs: Dict[str, Any] = {
Pillow types:
- We're using
types-Pillowbut it has since been removed from thetypeshedin favor of the native types: https://github.com/python/typeshed/pull/12732/files#diff-202ba5663fbebd32c9087d7510b31f3ff145d155c304a876ff10e554dd49de74L266-L275 - The native types (but we're not using these yet): https://github.com/python-pillow/Pillow/blob/b7e0570cb11901fbfdc9d5bc1ae2918cb32cc9c8/src/PIL/Image.py#L2459-L2461
There was a problem hiding this comment.
When you try the suggestion, you run into:
$ poetry run mypy synapse/media/thumbnailer.py synapse/media/thumbnailer.py:311: note: Revealed type is "builtins.dict[builtins.str, builtins.object]" synapse/media/thumbnailer.py:313: error: Argument 2 to "save" of "Image" has incompatible type "**dict[str, object]"; expected "Optional[str]" [arg-type] synapse/media/thumbnailer.py:313: error: Argument 2 to "save" of "Image" has incompatible type "**dict[str, object]"; expected "bool" [arg-type] synapse/media/thumbnailer.py:313: error: Argument 2 to "save" of "Image" has incompatible type "**dict[str, object]"; expected "Literal['bmp', 'png']" [arg-type]But overall, the
castisn't very good.It would be better to annotate the correct type where it is declared
save_kwargs: Dict[str, Any] = {
Pillowtypes:* We're using `types-Pillow` but it has since been removed from the `typeshed` in favor of the native types: https://github.com/python/typeshed/pull/12732/files#diff-202ba5663fbebd32c9087d7510b31f3ff145d155c304a876ff10e554dd49de74L266-L275 * The native types (but we're not using these yet): https://github.com/python-pillow/Pillow/blob/b7e0570cb11901fbfdc9d5bc1ae2918cb32cc9c8/src/PIL/Image.py#L2459-L2461
Feel free to modify my PR however you like, I'm no expert
There was a problem hiding this comment.
@catfromplan9 Are you interested in continuing the PR further?
There's bound to be more review points to address once this gets a full round of review. While improving the media repo in Synapse itself is desirable, this isn't necessarily something the team would take over and put effort into ourselves. Happy to review and push this along if you plan on making the changes though 🙂
There was a problem hiding this comment.
@catfromplan9 Are you interested in continuing the PR further?
There's bound to be more review points to address once this gets a full round of review. While improving the media repo in Synapse itself is desirable, this isn't necessarily something the team would take over and put effort into ourselves. Happy to review and push this along if you plan on making the changes though 🙂
Can you outline to me what exactly you would need me to change? I will do so.
There was a problem hiding this comment.
@catfromplan9 thanks for creating this PR and apologies for the slow feedback. I've tested it locally and it works well!
However I'm hesitant to accept this PR as-is, as clients will now animate avatars with no way to disable them (either client-side or server-side). This is partly the fault of clients (especially Element) not making use of the animated query parameter on the GET /_matrix/client/v1/media/thumbnail/{serverName}/{mediaId}. But this PR also doesn't take said query parameter into account.
I would like to see support for the animated query parameter added so that clients can easily and individually enable the feature. It should default to false if not provided, so that animated avatars are not returned unless the clients asks for them.
Disabling the feature by default prevents avatars from suddenly becoming animated in every deployment, without a good way to disable it today, which would likely upset users.
The need to provide the animated query parameter will also encourage clients to actually implement support for it.
So I would need to now also modify the code for the thumbnail request handling and pass whether it should thumbnail along? I have also noticed that the thumbnails seem to as-is cache themselves when requested, so I would need to always do animated one and instead return the first frame if not animated I guess. I would also like to propose that the spec sets animated to true if not specified, since in my testing any client that does not support animated thumbnail feature will either animate it or display first frame, but I'm not going to because I doubt this change would be made. Well, when I have the time, I will work towards using animated param, it would help me greatly if you wouldn't mind outlining the files I should take a look at and how you think I should handle it (just rough outline) so that I can get this PR done in a way that conforms to what you expect. |
|
Afaik these files are the ones I'd need to change to add in that that animated parameter |
4 participants
Add animated image support to the media thumbnailer.
Fix #18826