580182 - (animation) problem with dispose=previous

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Max Stepin]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100718 Minefield/4.0b2pre
Build Identifier: 

See the attached example: flags.gif 

It's structure:

1. show Blue (250x150), dispose = none
2. show "UK" (250x150), dispose = previous
3. show "EU" (128x128), dispose = none

dispose = previous means that after UK is shown, the image should go back to previous (Blue) frame, and then EU stars should appear on top of that Blue background.

But dispose = previous for some reason clears the background, turning it transparent. So EU stars appear on transparent background.

This GIF shouldn't have *any* transparency, because:
1. there is no transparent color.
2. there is no dispose = clear.

I tested it with many image viewers and browsers: they all show it correctly.
Only Firefox seems to have that problem. 
And it's not new, I went as far back as Firefox 2, the bug is there.
Also, it's not just that particular GIF, because when I converted it to 24-bit (no alpha) apng, the bug is still there.


Reproducible: Always

Comment 1

[Max Stepin]

Attachment: flags.gif example

Comment 2

[Boris Zbarsky [:bzbarsky]]

I can confirm the behavior (though every single webkit-based browser I have here, plus Preview, all show the third frame on a _green_ background for some reason; that can't possibly be correct).

Bobby, any idea what's up here?

Comment 3

[Bobby Holley (:bholley)]

Just took a quick look through this with gdb. It appears that it's doing the dispose=previous stuff correctly, but that for some reason the previous frame it restores is straight RGBA zeros (consistent with the behavior we're seeing). I'll figure out what that's about after I finish doing some reviews for joe.

Also, bz - are you sure you're seeing green on mac/Preview? I get a lighter blue...

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Also, bz - are you sure you're seeing green on mac/Preview?

Very sure, yes.  I'm on 10.5; that might matter.

Comment 5

[Max Stepin]

I'm seeing the same things as Bobby. I think the very first frame is not stored in compositingFrame, because it's displayed "as is" instead (for optimization purposes).

Maybe right before this:
    CopyFrameImage(mAnim->compositingFrame, mAnim->compositingPrevFrame);
there needs to be a check for first frame...
Or maybe a check for mAnim->lastCompositedFrameIndex ...


P.S. I think the light blue is color #0 (rgb=56,83,153), maybe webkit simply fills the buffer with it before decoding.

Comment 6

[Max Stepin]

Attachment: a_example.png

Another example, but dispose=previous only clears the frame on 2nd loop. The 1st loop is displayed correctly. That makes me think the reason could be a little different, but I'm pretty sure it's located somewhere in imgContainer::DoComposite() too.

There are a number of optimizations in DoComposite(), and it seems that some of them are not 100% correct.

Comment 7

[Bobby Holley (:bholley)]

Investigating this is on my todo list, but it'll probably be a while unless this becomes a blocker. I'll expedite reviews on any patches though.

Comment 8

[Max Stepin]

Attachment: proposed patch

The patch has 3 parts:

1. Turns off "doDisposal = PR_FALSE" optimization if next frame has 
   dispose=previous.

   It corrects flags.gif problem, as doDisposal = PR_TRUE will cause 
   compositingFrame to contain "this frame after disposal op", as intended.
   It will be stored later in compositingPrevFrame and restored from it nicely.

2. Typo in NS_WARNING() 

3. Turns off aPrevFrame->SetFrameDisposalMethod(kDisposeClearAll) optimization
   if next frame has dispose=previous.

   It corrects a_example.png problem, as next loop will not be messed up 
   by that ClearAll optimization.

Comment 9

[Bobby Holley (:bholley)]

(In reply to comment #8)
> Created attachment 466982 [details] [diff] [review]
> proposed patch
> 
> The patch has 3 parts:
> 
> 1. Turns off "doDisposal = PR_FALSE" optimization if next frame has 
>    dispose=previous.
> 
>    It corrects flags.gif problem, as doDisposal = PR_TRUE will cause 
>    compositingFrame to contain "this frame after disposal op", as intended.
>    It will be stored later in compositingPrevFrame and restored from it nicely.
> 
> 2. Typo in NS_WARNING() 
> 
> 3. Turns off aPrevFrame->SetFrameDisposalMethod(kDisposeClearAll) optimization
>    if next frame has dispose=previous.
> 
>    It corrects a_example.png problem, as next loop will not be messed up 
>    by that ClearAll optimization.

This looks good to me, but Joe's the one who wrote this stuff. Joe, what do you think?

Comment 10

[Joe Drew (not getting mail)]

This looks great. I'd love to see some modifications to the comments above the code you're changing (right now it only says "transparency" and "8bit"), and we'll need a test for this too.

Comment 11

[Max Stepin]

Attachment: proposed patch, v2

Added some comments to reflect the changes. 

Also fixed the comment I forgot to change in bug 433047.

Comment 12

[Max Stepin]

Joe, I think the first 2 attachments are good tests for both issues,
but let me know if you want to see then modified in some way...

Comment 13

[Joe Drew (not getting mail)]

Totally agreed - we just need them to be put into our automated tests. Best option is probably a reftest.

Comment 14

[Bobby Holley (:bholley)]

(In reply to comment #13)
> Totally agreed - we just need them to be put into our automated tests. Best
> option is probably a reftest.

More specifically, what we should do is modify the test cases so that they're as short as possible, so that the frame that visually differs is the last one, and so that it doesn't loop.

That way, we can write a simple delay test, where we let the animation run its course, and then compare what's left on the screen with what the final frame should look like.

See modules/libpr0n/test/reftest/apng/delaytest.html for an example of how this should work.

Comment 15

[Max Stepin]

Attachment: bug580182-1.gif

the same as flags.gif but much faster and with only 1 loop.

Comment 16

[Max Stepin]

Attachment: bug580182-2.png

the same as a_example.png but much faster and with only 2 loops.

Comment 17

[Max Stepin]

I don't know how to write a reftest myself, but if someone else is willing to try, you can use these animations, which are basically the same as originals, but faster and they stop very soon. If the last frame has a transparent pixel(0,0), it's a bug. Hope that's enough.

Comment 18

[Bobby Holley (:bholley)]

If you can also post the reference images (ie, what the images _should_ end up looking like), I'll write the reftest.

Comment 19

[Max Stepin]

Attachment: bug580182-1-ref.gif

correct last frame, for reference

Comment 20

[Max Stepin]

Attachment: bug580182-2-ref.png

correct last frame, for reference

Comment 21

[Max Stepin]

I see the reftest is currently using 100ms delay, but you might want to increase it to 300ms or so, to let the second test finish 2 loops.

Btw, I fixed another DoComposite() problem recently (see bug 433047), I can make a similar test for it too, if you think it would be useful.

Comment 22

[Joe Drew (not getting mail)]

Comment on attachment 467096 [details] [diff] [review]
proposed patch, v2

More details in the comments as to *why* things are different when we're using kDisposeRestorePrevious would be helpful too.

Comment 23

[Max Stepin]

Attachment: proposed patch, v3

Expanded comments, renamed imgContainer into RasterImage

Comment 24

[Bobby Holley (:bholley)]

Attachment: tests v1

Added tests. These fail without the patch, and pass with it (Good Thing).

However, they make use of delaytest, so I fear they'll suffer from the same
problem as bug 558678. As such, I don't think they should be checked in right
now (though this patch certainly should).

Joe, after we stop working on ff4, what do you say we spend some serious time
cleaning up our test situation? ;-)

Comment 25

[Bobby Holley (:bholley)]

Requesting a2.0.

This patch is very low risk, and has tests. It fixes animation behavior and makes us more correct. This patch was submitted by a contributor, and thus does not represent a failing of any moco employee to be working on blockers.

Comment 26

[Joe Drew (not getting mail)]

Comment on attachment 467912 [details] [diff] [review]
proposed patch, v3

Much better, thanks :)

Comment 27

[Joe Drew (not getting mail)]

Comment on attachment 467912 [details] [diff] [review]
proposed patch, v3

well, except for Bobby, who worked on the test!!!!! :D

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 467932 [details] [diff] [review]
tests v1

What's the reason for changing the apng delaytest to 500ms?

Comment 29

[Bobby Holley (:bholley)]

(In reply to comment #28)
> Comment on attachment 467932 [details] [diff] [review]
> tests v1
> 
> What's the reason for changing the apng delaytest to 500ms?

comment 21, with an extra safety margin.

Either way, I don't think it's worth quibbling over now, since in the end we're probably going to make the reftest poll anyway.

Comment 30

[Boris Zbarsky [:bzbarsky]]

So what needs to happen at this point to land this?  Just adding checkin-needed?  Or can I just push the patches?

Comment 31

[Bobby Holley (:bholley)]

(In reply to comment #30)
> So what needs to happen at this point to land this?  Just adding
> checkin-needed?  Or can I just push the patches?

Should be good to go AFAIK.

Comment 32

[Bobby Holley (:bholley)]

(though probably not the tests, because they use delaytest, and will probably go intermittently orange)

Comment 33

[Boris Zbarsky [:bzbarsky]]

Pushed fix as http://hg.mozilla.org/mozilla-central/rev/d43112f63698

The test still needs to land.

Bobby, file a separate bug on tracking that, perhaps?

Comment 34

[Bobby Holley (:bholley)]

(In reply to comment #33) 
> Bobby, file a separate bug on tracking that, perhaps?

Filed bug 594147.

Comment 35

[Max Stepin]

Thanks for landing. Everything works fine in Minefield:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100908 Firefox/4.0b6pre