GNOME Bugzilla – Bug 769960
animated WebP loader is not properly handling blending and transparency
Last modified: 2016-09-07 15:22:35 UTC
Created attachment 333386 [details] animated webp file demonstrating the reconstruction problem the current webp file-loader is not reconstructing frame correctly. Attached, a small (well-known? :)) animation that demonstrate the problem. Using Filters -> Animation -> Playback tool, once can see the first frame is scrambled. Using WebPAnimDecoder API solves the problem (reconstruction is handled by libwebpdemux directly). Fix patch following up.
Created attachment 333387 [details] [review] patch for fixing animation decoding switch to WebPAnimDecoder API.
Created attachment 333434 [details] [review] updated patch fixing animation decoding Here's a rebased patch, with coding style fixes
For authorship, could you tell us your full name? I assume this is Pascal Massimino, from the email address. But just to be sure…
indeed, that's Pascal Massimino.
Review of attachment 333434 [details] [review]: Looks good. A couple of optional comments... ::: plug-ins/file-webp/file-webp-load.c @@ +168,1 @@ + dec = WebPAnimDecoderNew (&wp_data, NULL); I'd suggest adding a note that this defaults to "RGBA" color mode. @@ +177,3 @@ + g_printerr ("Failed to decode animated WebP information\n"); + WebPAnimDecoderDelete (dec); + return -1; If 'goto' is allowed, maybe 'goto Err' here and the cleanup can be handled in one place?
@pascal What version of libwebp were you using? Also did you test this patch to make sure that the metadata and icc profiles still load properly?
@Pascal For the metadata requires installing exiv2 from git to test with.
Created attachment 333442 [details] [review] updated patch fixing animation decoding updated patch fixing animation decoding
@Ben the metadata code didn't change (it's before and after the section i modified). I just modified the code handling decoding of individual frames. I'm testing with latest libwebp 0.5.1.
Created attachment 333870 [details] torture test bitstream for webp animation + ICC generated using the initial 'farbkreis.jpg' image reported here: https://bugs.chromium.org/p/chromium/issues/detail?id=143
Please find attached a 'torture' bitstream anim_icc.webp for exercising webp+animation+alpha+ICC+strange timestamps. I generated this bitstream starting from the infamous 'farbkreis.jpg' image that was once reported on Chromium project[1]. Then i just created a rotating animation of it, transferring the ICC data into anim_icc.webp. In GIMP, the labeled texts should be displayed with the correct colors if ICC is supported. The new patch didn't break this behaviour. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=143
It works fine here, i'm not seeing any problems. I see in GIMP the same i did in Chromium and the colors match the sections of the wheel. You have to keep the profile when it loads in GIMP or else the profile will default to GIMPs default SRGB profile (or whatever is your default set to).
It does seem to fail in other browsers but how is that GIMPs problem if color management isn't respected in other software?
Converting it to SRGB seems makes it work in all browsers. So i'm not really sure what the issue is?
Embarrassingly, animated WebP + ICC is *not* supported by Chrome. This is a long-standing unresolved issue (see code [1]). The main reason for this gap is performance: having to apply the ICC profile to all pixels, in combination with alpha, is involved and CPU-consuming. It was dropped, being considered a corner case, pretty much like ICC is not supported in animated GIFs (same reason). [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp?q=webpimagedecoder&dr=CSs&l=230
Lol. Maybe my coffee is defective this morning but I still don't get what this has to do with GIMP?
i mentioned Chromium's code because anim_icc.webp is an example of bitstream that will show differently in Chrome and GIMP (that's related to comment #12). And also why this is likely to stay so for a while.
Pascal > I don't see big issues in your patch but for minor styling issues. Like you forgot a space between a function and the parentheses (at least once), and we try to align variable declarations. Also please when initializing a pointer, that's NULL, not 0. Ben > I think Pascal just made a digression to answer your question in comment 6 (i.e. does it still load ICC profiles properly?). So basically just showing that the profile was still properly applied on the webm before and after the patch (not saying there is a bug here). Anyway testing, it does look like it indeed fixes the first frame being scrambled on mosaic.webp (assuming this is not how it was supposed to look. I did not look at the webp format itself). Should we apply the commit?
#18: Jehan, let me fix the few style issues in a subsequent patch.
Created attachment 334064 [details] [review] WebP animation decoding fix updated patch with coding style fixes
Thanks, fixed in master with some cleanup: commit 252da1bdd5724789428dd95689de55bed8e5abb8 Author: Pascal Massimino <pascal.massimino@gmail.com> Date: Wed Aug 24 11:29:58 2016 +0200 Bug 769960 - animated WebP loader is not properly handling blending... ...and transparency Use most recent API for decoding WebP animation. WebPAnimDecoder handles transparency and blending automatically. plug-ins/file-webp/file-webp-load.c | 89 ++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 41 deletions(-)
Thanks for merging the patch! Next, I posted a follow-up patch for the webp-saving part: https://bugzilla.gnome.org/show_bug.cgi?id=771012 skal/