-
Notifications
You must be signed in to change notification settings - Fork 123
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
Comments
(It'd generally be nice if animated webp would work like animated gif.) |
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). |
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.)
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.)
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 Thank you for reporting this issue, lots of great detail :) I'll take a look at this more closely in the next week. |
Let me know if I can help answer any questions. I know a bit about webp format internals. |
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. |
كيف اديرها |
@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. |
@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! |
Super cool, thanks! I can confirm that uploaded animated webps are no longer getting corrupted. Looking forward to full support! :) |
(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! |
@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. |
@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 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? |
@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:
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 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! |
I just tested a linked animated Webp attachment and it does now animate. Attaching a webp directly also correctly animated. |
@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. |
Here's a quick note on where we're at: 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 😀 |
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's0x0a
.0xa
means "has exif, has animation". Discord added an (empty) EXIF chunk, so it's fine that that bit gets added. But the0x10
bit, which meanshas 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.
The text was updated successfully, but these errors were encountered: