1509408 - Animated GIFs are visually distorted

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Amy Lee [:amylee] UX]

Attachment: animated_gif_example.gif

Animated GIF is visually distorted when opened in the browser. Attached GIF for reference.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

If I force the recycle rect to be the entire frame, the distortion goes away, so something went wrong with that calculation.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03d006bbc9043d828fb9c73a4a3b70202f396458

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 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 8

[Andrew Osmond [:aosmond] (he/him)]

try for latest changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91dec3ebac84e08bc363020beb8dc64d9e28beee

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c36ca97de3d0
Fix animated image distortion regressions caused by frame recycling. r=tnikkel

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c36ca97de3d0