595142 - Animated gif/apng do not animate when used as background unless loaded as regular image as well.

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Christian Sonne [:cers]]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100908 Firefox/4.0b6pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100908 Firefox/4.0b6pre

Background-images fail to animate if not loaded as a regular image on the same page.

Included are several tests, as I have encountered different regressions on different builds.

In some cases, it would work seemingly unless covered with a gradient (or possibly any other background layer), but as I am testing now, it does not work at all unless the image is loaded in an img element on the same page.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.geeksbynature.dk/bugs/stoppedAnimation/
Actual Results:  
Only the currently last two (#7 and #8) displays animated images

Expected Results:  
All (currently 8) boxes should contain animated images

Comment 1

[Matthias Versen [:Matti]]

confirming with SM trunk on win32
related to bug 595122 / bug 359608

Comment 2

[Alon Zakai (:azakai)]

Attachment: Patch

Looks like the animation consumers counting logic was wrong.

Previously, if the proxy did not have an observer, we ignored adding animation consumers to it, and we enforced through assertions that without an observer, it would have no animation consumers. However, you can have the animation consumers added first, and then have the observer. So you need to hold on to them in anticipation of that event.

The patch also fixes bug 595122 (animated personas).

Included is a test.

Patch pushed to try.

Comment 3

[Bobby Holley (:bholley)]

> However, you can have the animation
> consumers added first, and then have the observer. So you need to hold on to
> them in anticipation of that event.

Well, the whole reason for doing things this was was the assumption that there was no way to load an image without an observer and add one later. If that doesn't hold in some cases (sounds like it doesn't), this code would indeed be quite incorrect.

What's the case where that assumption doesn't hold?

Comment 4

[Alon Zakai (:azakai)]

Good point. Looks like the issue is something else.

Normal images get two proxies, the first without an observer, the second with. The second gets added to the nsDocument, so it animates.

Background images get the first proxy, which is added to the nsDocument, but that isn't enough to animate as there is no observer. They later get another proxy with an observer, which is not added to the nsDocument, and also does not have SetImage called. If either one of those two were to occur, it would animate.

Which of those should happen?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Why do we rely on SetImage instead of having Init() do the right thing?  The codepath that creates that second proxy for backgrounds is imgRequestProxy::Clone, fwiw.

Comment 6

[Alon Zakai (:azakai)]

(In reply to comment #5)
> Why do we rely on SetImage instead of having Init() do the right thing?

Hmm, forget what I said earlier about SetImage, that wouldn't help here anyhow.

> The
> codepath that creates that second proxy for backgrounds is
> imgRequestProxy::Clone, fwiw.

Yes, the second is created by cloning it appears. The first clone is short-lived, but another clone is made later (also with an observer), that persists.

Should cloning perhaps (1) add the proxy to the same nsDocuments the original is already present, or (2) copy over the mAnimationConsumers? Either would fix this bug.

Comment 7

[Bobby Holley (:bholley)]

We should register whichever proxy has the observer. If the first one is short-lived, we should de-register it and register the new one when the switchover happens.

If all that is too complicated, we could also just get rid of the HasObservers optimization and just animate active images unconditionally.

Comment 8

[Boris Zbarsky [:bzbarsky]]

In the case of style images, we should NOT animate an image just based on what's in the style struct, imo.  We should start animating it once the nsImageLoader is created (and I thought that was what we did, in that the nsImageLoader is what increments the animation count, no?).

Comment 9

[Alon Zakai (:azakai)]

(In reply to comment #7)
> We should register whichever proxy has the observer. If the first one is
> short-lived, we should de-register it and register the new one when the
> switchover happens.
> 
> If all that is too complicated, we could also just get rid of the HasObservers
> optimization and just animate active images unconditionally.

After some thinking, there doesn't seem to be an easy way to do the HasObservers check. But anyhow, it sort of isn't needed anymore - it used to make sense, as a way to check if you need to animate or not based on having an observer. But now we have a better way of checking that, using the animation consumers. Is there a concrete case where we would do worse than before?

If not, then I am in favor of removing the HasObservers optimization, which is in fact the patch from before.

(In reply to comment #8)
> In the case of style images, we should NOT animate an image just based on
> what's in the style struct, imo.  We should start animating it once the
> nsImageLoader is created (and I thought that was what we did, in that the
> nsImageLoader is what increments the animation count, no?).

The animation consumers is incremented from the nsDocument code, when it knows the image should animate (except for some types of images which aren't handled through the document at all, so we give up on freezing their animation when in the bfcache and so forth). But, I'm not sure how the nsDocument and the nsImageLoader are related, perhaps doing it from one is equivalent to doing it from the other anyhow?

Comment 10

[Bobby Holley (:bholley)]

(In reply to comment #9)

> If not, then I am in favor of removing the HasObservers optimization, which is
> in fact the patch from before.

Sounds fine to me.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 474454 [details] [diff] [review]
Patch

r=bholley on the code changes.

For the tests, a few things:
1-They should be a separate patch (which is good, because you'll need to reflag for review anyway)
2-This image is pretty big. Is it necessary?

3:
>-        if (gImage.framesNotified >= 10) {
>+        if (gImage.framesNotified >= 20) {

What does this change do? It looks like you're only adding a new image to test, not removing the old one. Does the only image still work with this change?

4:
>+      is(Math.abs(image.framesNotified - >gImage.framesNotified)/gImage.framesNotified < 0.1, true,

That fuzz factor makes me nervous. What's the deal with it? Can we eliminate it?

Comment 13

[Alon Zakai (:azakai)]

Attachment: Part 1: Fix it

Carried r+.

Comment 14

[Alon Zakai (:azakai)]

Attachment: Part 2: Test it

> 2-This image is pretty big. Is it necessary?

We need a new image here, to count background stuff separately. Is 65K too much?

> 
> 3:
> >-        if (gImage.framesNotified >= 10) {
> >+        if (gImage.framesNotified >= 20) {
> 
> What does this change do? It looks like you're only adding a new image to test,
> not removing the old one. Does the only image still work with this change?

This is just so the # of frames is nice and round: ~20 before, ~20 after, wait for 4 seconds in the middle, which is potentially 40 frames, but should be silent. So if it does animate there as a bug, should have about 100% difference.

> 
> 4:
> >+      is(Math.abs(image.framesNotified - >gImage.framesNotified)/gImage.framesNotified < 0.1, true,
> 
> That fuzz factor makes me nervous. What's the deal with it? Can we eliminate
> it?

We can't measure the background image's frames directly, since we can't access its imgContainer - unless there is a way to do that? (that would not automatically cause animation anyhow)

If not, then this patch creates a normal image later, and compares frame counts then. I saw no more than 1 frame of discrepancy in my tests, which is 0.025 here, so I thought 0.1 was reasonable. But, raised it to 0.5 in this version, just to be safe. Note that if it does hit even 0.1, that isn't just a random orange, it's an actual bug (it would mean users can't expect images to animate in tandem).

Also improved the comments.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 474845 [details] [diff] [review]
Part 2: Test it

Joe looked at the first tests, so I'll let him look at these too. ;-)

Comment 16

[Alon Zakai (:azakai)]

If there are no objections, I guess this is ready to land?

Comment 18

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/1354806a3b6c