Closed Bug 1504237 Opened 2 years ago Closed 2 years ago
Some animation Web
Ps with lossy-compression aren't rendered correctly
47 bytes, text/x-phabricator-request
|Details | Review|
I noticed it while checking Bug 1503935 was fixed. STR: 1. Set image.webp.enabled to true. 2. Open http://littlesvr.ca/apng/gif_apng_webp3.html Expected result: All animation plays normally. Actual result: Animation with lossy webp sometimes shows black blinks. This happens in Test5 also and doesn't happen in Test 1,2,4.
I thought that bug fixed this as I wasn't able to reproduce with it, but I saw it just now. Investigating.
Assignee: nobody → aosmond
Status: UNCONFIRMED → NEW
Ever confirmed: true
It it consistently reproducible in a debug build for me. I set the discarding threshold to be very high so it never discards the frames after producing them. Setting image.animated.generate-full-frames to true seems to resolve the issue (for me), so I wonder if it is a bug in FrameAnimator.
(In reply to Andrew Osmond [:aosmond] from comment #2) > Setting image.animated.generate-full-frames to true seems to > resolve the issue (for me), so I wonder if it is a bug in FrameAnimator. Confirmed that this works as a workaround on current nightly build(2018-11-02-10), too.
(In reply to Toshihiro Yamada from comment #3) > (In reply to Andrew Osmond [:aosmond] from comment #2) > > Setting image.animated.generate-full-frames to true seems to > > resolve the issue (for me), so I wonder if it is a bug in FrameAnimator. > > Confirmed that this works as a workaround on current nightly > build(2018-11-02-10), too. Awesome, thanks. I think what is going wrong (and there should be a similar bug in APNG) is that the partial frames are incorrectly produced. APNG and WebP currently allocate a full sized frame buffer, but move the partial frame contents to the right location; soon to be historical, the pref I mentioned fixes that. The rest of the frame data should all be zeroes. However if the frame is marked as having no alpha, then it will actually use BGRX and memset everything to 0xFF, which is solid white. Hence the distortions. What bothers me is why it seems inconsistent about it from decode to decode. It should be very consistent.
For decoders which produce unpaletted partial frames (APNG, WebP), the surface format should always be BGRA.
So this is actually a bug in RemoveFrameRectFilter. If it needed to buffer inside the filter, it does the right thing: https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/image/SurfaceFilters.h#923 But if it could reuse the underlying buffer, it will only write the pixels that are provided -- it leaves the prefix and postfix of the row untouched. If we set the surface format to BGRA, we don't need to worry about writing them.
After some thought, it is both, although mainly the surface format's fault. If it comes in a BGRX, the pixman blending operation actually overwrites the entire compositing buffer with the new frame's data. Which is sort of garbage. Since pixman ignores the alpha channel, some of it will be black and some of it will be white (due to the RemoveFrameRectFilter bug). We don't see this immediately because animated images provide a dirty rect of what changed -- typically that means Gecko will only redraw what changed. However Gecko is free to repaint as much as it wishes. When there are multiple things animated, it may choose to repaint broader areas as a result. This is probably why it is only observed sometimes, and with seemingly random severity. Ideally to go with the format change, I can write a mochitest that forces a full repaint, and confirms the compositing buffer wasn't overwritten with garbage.
It is pretty easy to observe on an animated image that ends with a bad frame, and setting image.animation_mode to once. If you zoom in and out, it forces a full repaint, and you see the bad contents.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a8912441d21f8336dba704a248dedc99d205b9e Wrote the tests as gtests instead, BlendWebPWithAnimator failed before, and passes after the decoder fix.
Can anybody else see a significant delay to load/render WebP images? Pageload seems to hang when they are fist loaded. Loading them from cache seems to be no problem, so a page refresh fixes the problem for me.
Same as sjw: on first load it doesn't show the animated webp, I have to reload from cache for them to be displayed.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb87d2312ea Ensure partial animated frames always use BGRA instead of BGRX. r=tnikkel
I can confirm this behavior (sJw and Robert-André). I also mentioned it in: https://bugzilla.mozilla.org/show_bug.cgi?id=1504120 Just an example webp animation https://pullzone1-corydowdywebdesi.netdna-ssl.com/assets/blog/apngwebp/squirrel.default.webp It only loads when it's in the cache. For the first load or if you ignore caches (strg + F5) it just keeps on loading (endlessly) without displaying anything This part of the bug is not fixed in this nightly: 65.0a1 (2018-11-06) (64-Bit) It seems to be the the case for every webp animation and not just some.
(In reply to Djfe from comment #14) > I can confirm this behavior (sJw and Robert-André). I also mentioned it in: > https://bugzilla.mozilla.org/show_bug.cgi?id=1504120 > > Just an example webp animation > https://pullzone1-corydowdywebdesi.netdna-ssl.com/assets/blog/apngwebp/ > squirrel.default.webp > It only loads when it's in the cache. For the first load or if you ignore > caches (strg + F5) it just keeps on loading (endlessly) without displaying > anything > > This part of the bug is not fixed in this nightly: 65.0a1 (2018-11-06) > (64-Bit) > > It seems to be the the case for every webp animation and not just some. Sounds like bug 1504448.
You need to log in before you can comment on or make changes to this bug.