Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Thumbnail WebP images as WebP #14890

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

tastytea
Copy link

I'm testing it since about 3 months (on a single user server) and it works fine.

Closes: #11840
Signed-off-by: Ronny (tastytea) Gutbrod [email protected]

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@tastytea tastytea requested a review from a team as a code owner January 21, 2023 21:16
@clokep
Copy link
Member

clokep commented Jan 25, 2023

At #7586 (comment) @erikjohnston said:

On the other hand, it might make sense to generate webp previews for webp images. This one also works fine. What do you think?

Given some devices can't render webp I think it makes sense for thumbnails to be in a more widely supported format.

I think in the (almost) 3 years since that comment support for WebP has increased, back in May of 2020 Safari (on both iOS and macOS) didn't support WebP, where it looks like it is widely supported now. The only folks who might have an issue are people on macOS < 11 where transparency of WebP is not supported. This shouldn't be any worse than what we currently do of thumbnailing to JPEG! (Latest IE 11 and KaiOS Browser 2.5 also don't support WebP, but those are hardly used.) I'm a little less confident about mobile support (I'm not sure how mobile clients usually render images), but have asked internally about that.

tl;dr I think it is reasonable to support this, but I have a few questions:

  • Do you know if our minimum version of pillow supports thumbnailing to WebP?
  • Do we need to ensure any operating system libraries are installed to support WebP?

Can we now update the tests at tests/rest/media/v1/test_media_storage.py to have expected output? It seems that in #7586 it was not "deterministic" so was left out?

@clokep clokep removed the request for review from a team January 25, 2023 12:30
@clokep clokep added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jan 25, 2023
@tastytea
Copy link
Author

tastytea commented Jan 25, 2023

tl;dr I think it is reasonable to support this, but I have a few questions:

  • Do you know if our minimum version of pillow supports thumbnailing to WebP?
  • Do we need to ensure any operating system libraries are installed to support WebP?

Looks like it is supported since pillow 2.4.0 from 2014 and it needs libwebp.

@clokep clokep requested a review from a team January 30, 2023 16:54
@clokep clokep removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jan 30, 2023
@squahtx squahtx changed the title thumbnail WebP as WebP Thumbnail WebP images as WebP Feb 1, 2023
Copy link
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Now that we're relying on libwebp, we should also:

@clokep
Copy link
Member

clokep commented Feb 2, 2023

Cross-referencing matrix-org/matrix-spec#453.

@tastytea
Copy link
Author

tastytea commented Feb 2, 2023

Updated instructions and added check; also fixed the zlib check error message while i was there.

@tastytea tastytea force-pushed the webp-thumbnail branch 2 times, most recently from 83543e2 to 136ab36 Compare February 2, 2023 20:25
Copy link
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the docs so thoroughly!

Just one minor thing left.

synapse/rest/media/v1/__init__.py Outdated Show resolved Hide resolved
tastytea and others added 5 commits March 18, 2023 21:03
Closes: matrix-org#11840
Signed-off-by: Ronny (tastytea) Gutbrod <[email protected]>
Co-authored-by: Sean Quah <[email protected]>
Signed-off-by: tastytea <[email protected]>
@tastytea
Copy link
Author

updated since the location of synapse/rest/media/v1/thumbnailer.py changed to synapse/media/thumbnailer.py.

@clokep
Copy link
Member

clokep commented May 15, 2023

I suspect this needs something like MSC4011 by the way to unstick the current issue.

@clokep clokep added the Z-Spec-Blocked This change is blocked on specification (e.g. an MSC). label Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z-Spec-Blocked This change is blocked on specification (e.g. an MSC).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WEBPs are thumbnailed as JPEGs, losing transparency/animation
3 participants