Created attachment 9027073 [details] animated_gif_example.gif Animated GIF is visually distorted when opened in the browser. Attached GIF for reference.
Looks to be related to recycling -- if I disable it, full frames look fine. The bizarre part is I don't see it on Linux, I was only able to reproduce on Windows.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Was able to reproduce on Linux in a debug build. Seems timing related. Perhaps something is going wrong with the recycle rect calculation.
If I force the recycle rect to be the entire frame, the distortion goes away, so something went wrong with that calculation.
I do the recycle rect calculation at the wrong time. Let's say we insert frame N into our display queue. Once we advance to N+1, frame N's buffer goes into the back of the recycling queue. We pull it out of the recycling queue for frame N+M. The recycle rect is defined as the union of all of the dirty rects from N to N+M-1, or the difference between the contents of what is stored in the frame buffer (N), and the previous frame (N+M-1) to the new frame we are creating (N+M). Right now we calculate the recycle rect when we insert frame N into the recycling queue. However our only guarantee at this point is that frame N+1 is in the display queue -- we might not have frame N+2 .. N+M-1 decode yet. We obviously need those frames produced in order to know what their dirty rects are. We should instead calculate the recycle rect when we pull frame N out of the recycling queue. We know that frame N is now the oldest frame in the display and recycling queues, simply by definition. We know we want to produce frame N+M, so also no frame in the display and recycling queues could be further than N+M-1. Since we store them all sequentially, the aggregate of the display and recycle queues will be N .. N+M-1. Exactly what we need in order to calculate the recycle rect.
This also needs a proper gtest to verify the recycle rect is what we expect to be. I only verified the returned recycle rect to make the internal contents, and not the calculation itself.
Created attachment 9027546 [details] Bug 1509408 - Fix animated image distortion regressions caused by frame recycling. When bug 1508393 landed, it not only enabled producing of full frames on the decoder threads, it also enabled recycling of old animated image frame buffers it would have otherwise discarded. It reuses the contents of the buffer where possible given we know what pixels changed between the old frame and the frame we want to produce. However where this calculation was done was incorrect. We originally calculated it when we advanced the frame, but at that point there is no guarantee that we have all of the necessary information; we may have fallen behind on decoding. As such, we move the calculation to where we actually perform the recycling. At this point we are guaranteed to have all the necessary frames between the recycling and display queues.
try for latest changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91dec3ebac84e08bc363020beb8dc64d9e28beee
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/c36ca97de3d0 Fix animated image distortion regressions caused by frame recycling. r=tnikkel
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox65: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.