Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(previews): Don't crash on animated WEBP images #38364

Conversation

joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented May 18, 2023

Summary

libgd handles animated WEBP images poorly (fatal error and generates a meaningless/misleading error message). As a result we were returning a 500 error for these /preview requests (web) and a fatal error at the command-line (occ). Now we bypass libgd if we detect an animated WEBP image and simply don't generate the preview. No more 500 error. Should fix occ too.

The prospect of incorporating animation handling is an upstream matter:

libgd/libgd#648
https://bugs.php.net/bug.php?id=79809
https://www.php.net/manual/en/function.imagecreatefromwebp.php

TODO

Checklist

lib/private/legacy/OC_Image.php Fixed Show resolved Hide resolved
@szaimen szaimen added this to the Nextcloud 28 milestone May 18, 2023
@szaimen szaimen added the 3. to review Waiting for reviews label May 18, 2023
@szaimen szaimen requested review from rullzer, fancycode, a team, ArtificialOwl, icewind1991 and nfebe and removed request for a team May 18, 2023 19:08
@joshtrichards

This comment was marked as resolved.

@rullzer
Copy link
Member

rullzer commented May 19, 2023

Why not just create a preview provider to do this magic? OC_Image is one of those classes that still needs to die IMO.

@joshtrichards
Copy link
Member Author

Well yeah (#28279 et al + Imaginary support etc), but this PR just fixes what is still a common/still supported setup (particularly in small deployments).

P.S. Is there an official deprecation of OC_Image preview generation? I thought it fell more into the category "we know it's coming, things are happening to build up to it, but not yet deprecated" 👀

@kesselb
Copy link
Contributor

kesselb commented May 19, 2023

WebP preview provider: https://github.com/nextcloud/server/blob/master/lib/private/Preview/WebP.php

Currently the generic implementation, using OC_Image, is used to generate previews for WebP.
I guess it should work to implement this logic in the preview provider. But then it's only available for the preview provider.

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@AndyScherzinger AndyScherzinger force-pushed the jr-preview-libgd-webp-animation-bypass branch from 510ca8a to edf8ca6 Compare February 27, 2024 13:21
@ytoaa

This comment was marked as resolved.

@joshtrichards
Copy link
Member Author

@ytoaa Can you elaborate? What are you experiencing when you're testing this PR doing in your environment?

@joshtrichards joshtrichards force-pushed the jr-preview-libgd-webp-animation-bypass branch from edf8ca6 to 7943ad6 Compare March 9, 2024 12:58
@joshtrichards joshtrichards changed the title (fix) Generating previews for animated WEBP images was failing fix(previews): Prevent 500 error when generating previews for animated WEBP images Mar 9, 2024
@ytoaa
Copy link

ytoaa commented Mar 9, 2024

@ytoaa Can you elaborate? What are you experiencing when you're testing this PR doing in your environment?

Oh I think I'm mistaken. When will this merge into the main release?

This was referenced Mar 12, 2024
@skjnldsv skjnldsv requested review from come-nc and removed request for fancycode May 2, 2024 15:30
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. Didn't have a look at the code though.

To be honest, I don't like rawdogging the decoder logic but I don't see a better way for now.

@st3iny
Copy link
Member

st3iny commented May 2, 2024

@joshtrichards Please squash and use a conventional commit message. E.g. fix(previews): Skip generating previews for animated WEBP images

@joshtrichards joshtrichards force-pushed the jr-preview-libgd-webp-animation-bypass branch from 8cc107f to cff4727 Compare May 27, 2024 15:17
@joshtrichards
Copy link
Member Author

/backport to stable29

@joshtrichards
Copy link
Member Author

/backport to stable28

@joshtrichards joshtrichards linked an issue May 27, 2024 that may be closed by this pull request
8 tasks
@joshtrichards joshtrichards self-assigned this May 27, 2024
@joshtrichards joshtrichards changed the title fix(previews): Prevent 500 error when generating previews for animated WEBP images fix(previews): Don't crash on animated WEBP images May 27, 2024
@joshtrichards joshtrichards force-pushed the jr-preview-libgd-webp-animation-bypass branch 3 times, most recently from bc97193 to e2aaf27 Compare May 29, 2024 15:21
Fixes nextcloud#30029 and nextcloud#37263

libgd handles animated WEBP images poorly and generates a meaningless error message as a result. We were returning a 500 error for these preview requests (web) and a fatal error at the command-line (occ). Now we bypass libgd if the we detect an animated WEBP image (and simply don't generate the preview). No more 500 error. Should fix occ too.

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny force-pushed the jr-preview-libgd-webp-animation-bypass branch from e2aaf27 to 046fe8d Compare May 30, 2024 05:48
@st3iny
Copy link
Member

st3iny commented May 30, 2024

I don't get why cypress keeps failing. Could this be because the action is run from a fork?

@come-nc
Copy link
Contributor

come-nc commented Jun 3, 2024

I don't get why cypress keeps failing. Could this be because the action is run from a fork?

Yes iirc our cypress setup does not support forks

@st3iny
Copy link
Member

st3iny commented Jun 3, 2024

Alright, thanks for clarifying.

@joshtrichards I'm sorry for the inconvenience but do you mind opening another PR from a branch on this repository directly? AFAIK, you should have permissions to do so.

This was referenced Jul 30, 2024
@Altahrim Altahrim mentioned this pull request Aug 5, 2024
@skjnldsv skjnldsv merged commit 8a5bc47 into nextcloud:master Aug 6, 2024
145 of 154 checks passed
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 6, 2024
@Altahrim Altahrim mentioned this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: previews and thumbnails
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: cron fails because of gd-webp Generating previews for animated webp images fails
9 participants