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

Discord breaks webp image on upload #159

Closed
nico opened this issue May 10, 2024 · 18 comments
Closed

Discord breaks webp image on upload #159

nico opened this issue May 10, 2024 · 18 comments

Comments

@nico
Copy link

nico commented May 10, 2024

wow.zip

The webp file in the attached zip is an animated lossless webp that displays fine in all browsers I've tried.

If I upload the webp file in the attached zip, Discord:

a) fails to show it
b) turns it into a file that no longer renders in browsers

The webp has different rects for different animation frames – new frames don't store data that's identical to old frames.

For b), it looks like lilliput takes the width and height of the last animation frame and writes that into the vp8x header. That is incorrect, since the last frame is smaller than the animation.

For animated images, ideally the original size from the vp8x header would be written to the output unmodified. Failing that, the dimensions of the first animation frame are probably a better guess than the dimensions of the last animation frame.

It also looks like the flags byte in the vp8x header isn't quite right. On input, they are 0x12. In the file that comes back it's 0x0a.

0xa means "has exif, has animation". Discord added an (empty) EXIF chunk, so it's fine that that bit gets added. But the 0x10 bit, which means has alpha, got cleared. This animation definitely has alpha.

If I manually fix up the dimensions in the vp8x header, the image starts playing in browsers. But e.g. Safari no longer shows the transparent pixels as transparent, because this bit got cleared.

@nico
Copy link
Author

nico commented May 10, 2024

(It'd generally be nice if animated webp would work like animated gif.)

@nico
Copy link
Author

nico commented May 10, 2024

Also, it looks like the EXIF chunk is added at the front. Per https://developers.google.com/speed/webp/docs/riff_container#extended_file_format that's invalid: EXIF must follow image data (which for animated files is a sequence of ANMF chunks).

nico added a commit to nico/serenity that referenced this issue May 12, 2024
For example, for 7z7c.gif, we now store one 500x500 frame and then
a 94x78 frame at (196, 208) and a 91x78 frame at (198, 208).

This reduces how much data we have to store.

We currently store all pixels in the rect with changed pixels.
We could in the future store pixels that are equal in that rect
as transparent pixels. When inputs are gif files, this would
guaranteee that new frames only have at most 256 distinct colors
(since GIFs require that), which would help a future color indexing
transform. For now, we don't do that though.

The API I'm adding here is a bit ugly:

* WebPs can only store x/y offsets that are a multiple of 2. This
  currently leaks into the AnimationWriter base class.
  (Since we potentially have to make a webp frame 1 pixel wider
  and higher due to this, it's possible to have a frame that has
  <= 256 colors in a gif input but > 256 colors in the webp,
  if we do the technique above.)

* Every client writing animations has to have logic to track
  previous frames, decide which of the two functions to call, etc.

This also adds an opt-out flag to `animation`, because:

1. Some clients apparently assume the size of the last VP8L
   chunk is the size of the image
   (see discord/lilliput#159).

2. Having incremental frames is good for filesize and for
   playing the animation start-to-end, but it makes it hard
   to extract arbitrary frames (have to extract all frames
   from start to target frame) -- but this is mean tto be a
   delivery codec, not an editing codec. It's also more vulnerable to
   corrupted bytes in the middle of the file -- but transport
   protocols are good these days.
   (It'd also be an idea to write a full frame every N frames.)

For https://giphy.com/gifs/XT9HMdwmpHqqOu1f1a (an 184K gif),
output webp size goes from 21M to 11M.

For 7z7c.gif (an 11K gif), output webp size goes from 2.1M to 775K.

(The webp image data still isn't compressed at all.)
nico added a commit to nico/serenity that referenced this issue May 12, 2024
For example, for 7z7c.gif, we now store one 500x500 frame and then
a 94x78 frame at (196, 208) and a 91x78 frame at (198, 208).

This reduces how much data we have to store.

We currently store all pixels in the rect with changed pixels.
We could in the future store pixels that are equal in that rect
as transparent pixels. When inputs are gif files, this would
guaranteee that new frames only have at most 256 distinct colors
(since GIFs require that), which would help a future color indexing
transform. For now, we don't do that though.

The API I'm adding here is a bit ugly:

* WebPs can only store x/y offsets that are a multiple of 2. This
  currently leaks into the AnimationWriter base class.
  (Since we potentially have to make a webp frame 1 pixel wider
  and higher due to this, it's possible to have a frame that has
  <= 256 colors in a gif input but > 256 colors in the webp,
  if we do the technique above.)

* Every client writing animations has to have logic to track
  previous frames, decide which of the two functions to call, etc.

This also adds an opt-out flag to `animation`, because:

1. Some clients apparently assume the size of the last VP8L
   chunk is the size of the image
   (see discord/lilliput#159).

2. Having incremental frames is good for filesize and for
   playing the animation start-to-end, but it makes it hard
   to extract arbitrary frames (have to extract all frames
   from start to target frame) -- but this is mean tto be a
   delivery codec, not an editing codec. It's also more vulnerable to
   corrupted bytes in the middle of the file -- but transport
   protocols are good these days.
   (It'd also be an idea to write a full frame every N frames.)

For https://giphy.com/gifs/XT9HMdwmpHqqOu1f1a (an 184K gif),
output webp size goes from 21M to 11M.

For 7z7c.gif (an 11K gif), output webp size goes from 2.1M to 775K.

(The webp image data still isn't compressed at all.)
trflynn89 pushed a commit to SerenityOS/serenity that referenced this issue May 14, 2024
For example, for 7z7c.gif, we now store one 500x500 frame and then
a 94x78 frame at (196, 208) and a 91x78 frame at (198, 208).

This reduces how much data we have to store.

We currently store all pixels in the rect with changed pixels.
We could in the future store pixels that are equal in that rect
as transparent pixels. When inputs are gif files, this would
guaranteee that new frames only have at most 256 distinct colors
(since GIFs require that), which would help a future color indexing
transform. For now, we don't do that though.

The API I'm adding here is a bit ugly:

* WebPs can only store x/y offsets that are a multiple of 2. This
  currently leaks into the AnimationWriter base class.
  (Since we potentially have to make a webp frame 1 pixel wider
  and higher due to this, it's possible to have a frame that has
  <= 256 colors in a gif input but > 256 colors in the webp,
  if we do the technique above.)

* Every client writing animations has to have logic to track
  previous frames, decide which of the two functions to call, etc.

This also adds an opt-out flag to `animation`, because:

1. Some clients apparently assume the size of the last VP8L
   chunk is the size of the image
   (see discord/lilliput#159).

2. Having incremental frames is good for filesize and for
   playing the animation start-to-end, but it makes it hard
   to extract arbitrary frames (have to extract all frames
   from start to target frame) -- but this is mean tto be a
   delivery codec, not an editing codec. It's also more vulnerable to
   corrupted bytes in the middle of the file -- but transport
   protocols are good these days.
   (It'd also be an idea to write a full frame every N frames.)

For https://giphy.com/gifs/XT9HMdwmpHqqOu1f1a (an 184K gif),
output webp size goes from 21M to 11M.

For 7z7c.gif (an 11K gif), output webp size goes from 2.1M to 775K.

(The webp image data still isn't compressed at all.)
@skidder
Copy link
Contributor

skidder commented May 20, 2024

@nico Thank you for reporting this issue, lots of great detail :) I'll take a look at this more closely in the next week.

@nico
Copy link
Author

nico commented Jun 12, 2024

Let me know if I can help answer any questions. I know a bit about webp format internals.

@skidder
Copy link
Contributor

skidder commented Aug 14, 2024

Quick update, I'm making some improvements & fixes to Lilliput's support for animated image formats, and hope to cover this animated WebP issue as part of that work in the coming weeks.

@6446plm
Copy link

6446plm commented Aug 21, 2024

كيف اديرها

@skidder
Copy link
Contributor

skidder commented Aug 23, 2024

@nico I too was able to reproduce this issue consistently in Discord. Although Lilliput currently lacks support for transformations of animated WebP content, Discord should not be corrupting animated WebP files uploaded by users.

The root-cause of the corruption is actually external to Lilliput. Discord uses the Piexif library to remove EXIF data from media uploads. The corruption stems from two defects I found in Piexif:

I will close this issue out once those defects have been corrected in our fork of Piexif.

@skidder
Copy link
Contributor

skidder commented Aug 28, 2024

@nico this has been fixed in Discord, you should now see animated WebP uploads presented as a single frame in Discord, and viewing the original animated WebP in browser (or any other WebP viewer) should work. We're working on full animated WebP support in Discord, soon 😄

Thanks again for raising this, and please file an issue should anything else come up!

@skidder skidder closed this as completed Aug 28, 2024
@nico
Copy link
Author

nico commented Sep 6, 2024

Super cool, thanks! I can confirm that uploaded animated webps are no longer getting corrupted.

Looking forward to full support! :)

@nico
Copy link
Author

nico commented Sep 6, 2024

(ps: Is there an issue I could follow to get updates for full support?)

@skidder
Copy link
Contributor

skidder commented Sep 6, 2024

(ps: Is there an issue I could follow to get updates for full support?)

Definitely! Earlier this week I merged in full support for animated WebP support to Lilliput, and we're in the process of integrating those changes with the rest of Discord infrastructure.

I plan on partial integration of animated WebP (a subset of use-cases (e.g. only attachments), likely as an experiment) by end of September. Naturally we want to be cautious rolling out such a big change, as well as ensure we can measure any changes in performance (client & server). Most images generated by Discord are non-animated WebP, and it's important that we not break that behavior.

The Discord engineering blog has a monthly patch-notes post with recent changes. Animated WebP support will be reported in the 'Media' section. I'll also try to tag you on this Github issue once fully rolled out.

Thanks again!

@skidder
Copy link
Contributor

skidder commented Sep 26, 2024

@nico fyi you should see animated WebP attachments display on web & Discord desktop clients! We plan to enable retrieval of animated WebP attachments on mobile clients very soon. Please open an issue against Lilliput if you see any issues or areas to improve.

@goodusername123
Copy link

@skidder Hello. 3 weeks ago when animated WebP support was rolled out to desktop clients it functioned almost flawlessly even after testing it for an hour or so against a few various test cases it came out on top with the only sole issue I could notice being they would not animate in the "GIF" picker, However a few days or so later support for animated WebPs became crippled in functionality with them proceeding to only animate if they where a directly uploaded attachment in a message and would no longer function when linked to and also lost the favorite star button ability as well. an example of a version of Discord that worked fine was stable 331573 (1886297) while an example of a version that does not function is stable 333307 (1c1ea47) (maybe not exactly when it started) and anything else recent.

Is this new less/barely functional animated WebP support intended? or has full animated WebP support been temporarily disabled until support is ready for mobile clients in which case a full rollout across platforms is planned maybe?

@skidder
Copy link
Contributor

skidder commented Oct 16, 2024

@goodusername123 first off, thank you for the feedback - this is a new capability, and hearing from users helps ensure things work intuitively and correctly.

Here's a list of the issues I see:

  1. Linked animated WebP attachments don't animate
  2. Animated WebP attachments cannot be favorited for the GIF picker
  3. Previously, animated WebP attachments in GIF picker didn't animate

As for how/why things regressed, with the initial rollout of animated WebP support we didn't signal to the Discord client whether it's an animated or still WebP. Previously the Discord client could assume all WebP files were still, which is no longer the case. That led to all static WebP files being shown as animated, even those that were still, which isn't right.

We've since begun signaling to the client whether a WebP attachment is animated, and the client requests an animated WebP by including the animated=true query parameter; without that parameter, a still WebP is returned. This relates to the first bug listed above, where links need to include the animated=true query parameter to animate.

I totally acknowledge all of these, and we'll tackle them all soon. Our first goal with this change was to support animated WebP attachments that had been completely unsupported (aside from just showing a still image). We're in a good spot there, but these client-facing issues remain.

The Discord client app is a complex bit of software, and these client facing changes are nuanced and can take time to uncover. 😅

I assure you, we'll fix these issues in short order, and I appreciate you reporting them!

@jobbio
Copy link

jobbio commented Nov 27, 2024

I just tested a linked animated Webp attachment and it does now animate. Attaching a webp directly also correctly animated.

@nico
Copy link
Author

nico commented Dec 1, 2024

@skidder This is so so cool, thanks! Seems to work great. Great work, this is super exciting to see 🙂

@jobbio
Copy link

jobbio commented Dec 1, 2024

@skidder This is so so cool, thanks! Seems to work great. Great work, this is super exciting to see 🙂

I meant to add after my previous post that there seems to be different experiences on the mobile apps. Android webp attachments do animate, but the iOS app they do not. Android does not animate linked webp images, either and it also doesn't work on iOS.

@skidder
Copy link
Contributor

skidder commented Dec 1, 2024

Thank you @nico and @jobbio !

Here's a quick note on where we're at:
Attachments: Animated WebP attachments are supported on desktop/web and mobile (iOS, Android).

Emojis: we've been requesting all animated emojis as animated WebP, converting animated GIF emojis to animated WebP. We currently still require emoji uploads be provided as GIF for backward compatibility, as WebP -> GIF is not currently a supported transformation.

Embeds: we're currently running an experiment with animated WebP embeds on desktop/web, and will likely enable it globally for desktop/web early next week. Mobile handling of embeds is rolling out in the next stable release behind an experiment and will likely be enabled in the next week or two as well.

Thanks for your patience in seeing this roll out in the Discord app. There's some nuance in tracking whether source media is animated or not so it can be displayed correctly in the Discord app (reduced-motion setting, app focus change, etc). The animated- and still-image capabilities in WebP is both really cool and frustrating since the animated-nature of a file is known only by decoding the container 😅

Good news is that a lot of the work done to support animated WebP makes it relatively easy to support related formats, such as AVIF, for which I have a working spike that will convert AVIF (animated, still) to WebP 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants