Animated GIFs are visually distorted

RESOLVED FIXED in Firefox 65

Status

()

P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: amylee, Assigned: aosmond)

Tracking

65 Branch
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 months ago
Created attachment 9027073 [details]
animated_gif_example.gif

Animated GIF is visually distorted when opened in the browser. Attached GIF for reference.
Blocks: 1508393
Flags: needinfo?(aosmond)
Priority: -- → P3
(Assignee)

Comment 1

3 months ago
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)

Updated

3 months ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 months ago
Was able to reproduce on Linux in a debug build. Seems timing related. Perhaps something is going wrong with the recycle rect calculation.
(Assignee)

Comment 3

3 months ago
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)
(Assignee)

Comment 4

3 months ago
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.
(Assignee)

Comment 5

3 months ago
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.
(Assignee)

Comment 7

3 months ago
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.

Comment 9

3 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36ca97de3d0
Fix animated image distortion regressions caused by frame recycling. r=tnikkel

Comment 10

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c36ca97de3d0
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.