Closed Bug 1509408 Opened 3 years ago Closed 3 years ago

Animated GIFs are visually distorted


(Core :: ImageLib, defect, P3)

65 Branch



Tracking Status
firefox65 --- fixed


(Reporter: amylee, Assigned: aosmond)




(2 files)

Animated GIF is visually distorted when opened in the browser. Attached GIF for reference.
Blocks: 1508393
Flags: needinfo?(aosmond)
Priority: -- → P3
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
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.
Flags: needinfo?(aosmond)
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.
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.
Pushed by
Fix animated image distortion regressions caused by frame recycling. r=tnikkel
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.