Skip to content

Fix some webp images improperly marked as animated #29713

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

Merged

Conversation

Petersmit27
Copy link
Contributor

@Petersmit27 Petersmit27 commented Apr 10, 2025

Depending on the webp file, Element incorrectly detects a webp as animated because the wrong byte is looked at.
If GIF autoplay is disabled, it leads to WEBP files getting the "GIF" banner while they are not actually animated.

Before

Image

After

Image

What was wrong

According to the webp spec referenced in the code, the byte highlighted in the image below is checked, but it should be one in the next row (4 bytes later).
Image

Why the unit test passed before

The Static WEBP test in blobIsAnimated in Image-test.ts passed before this change because the tested file is in the VP8L format, which indeed cannot be animated.

The webp file I have is in the VP8X (extended file) format, which may be animated, but in this case is not.
To make the test fail, you can use it (GitHub doesn't let me directly upload webp files) in place of the current test/unit-tests/images/static-logo.webp.
If needed, I could replace the current static-logo.webp with the one I have for this PR, though maybe you would prefer to have a more serious image for that.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

@t3chguy
Copy link
Member

t3chguy commented Apr 10, 2025

This looks sane, good spot on the byte mistake, but would really want a test. Seriousness of the image matters not, copyright-esque issue however do. Keep in mind the CLA you signed for anything you contribute

Copy link
Member

Choose a reason for hiding this comment

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

Well that got a good laugh from me

@Petersmit27
Copy link
Contributor Author

Petersmit27 commented Apr 10, 2025

The added test is pretty minimal, should I add a check that the new file does indeed contain the VP8X bytes? Or should I give some code here for you to verify it? I essentially just added some logs to the blobIsAnimated function

@t3chguy
Copy link
Member

t3chguy commented Apr 10, 2025

@Petersmit27 I trust that you checked it, that's enough

@t3chguy
Copy link
Member

t3chguy commented Apr 10, 2025

Plus in theory coverage will tell us

Copy link
Member

@t3chguy t3chguy 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 this

@t3chguy t3chguy added this pull request to the merge queue Apr 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2025
@t3chguy t3chguy added this pull request to the merge queue Apr 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2025
@t3chguy t3chguy added this pull request to the merge queue Apr 11, 2025
github-merge-queue bot pushed a commit that referenced this pull request Apr 11, 2025
* Fix some webp images improperly marked as animated

* Add unit test for an unanimated webp file in extended file format

* Apply linting to webp test
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2025
@t3chguy
Copy link
Member

t3chguy commented Apr 11, 2025

Sorry about the flakes above, they're unrelated to your change and yet to be investigated by the maintainer

@t3chguy t3chguy added this pull request to the merge queue Apr 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2025
@t3chguy t3chguy added this pull request to the merge queue Apr 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2025
@t3chguy
Copy link
Member

t3chguy commented Apr 11, 2025

Going to wait while @florianduros looks into the flakes

@t3chguy t3chguy added this pull request to the merge queue Apr 11, 2025
Merged via the queue into element-hq:develop with commit ca56c2e Apr 11, 2025
34 checks passed
@Petersmit27 Petersmit27 deleted the fix-webp-animation-identification branch April 14, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants