Closed Bug 595142 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: cers, Assigned: azakai)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
Summary: Animated gif/apng do not animated when used as background unless loaded as regular image as well. → Animated gif/apng do not animate when used as background unless loaded as regular image as well.
confirming with SM trunk on win32
related to bug 595122 / bug 359608
Status: UNCONFIRMED → NEW
Component: General → ImageLib
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → imagelib
Version: unspecified → Trunk
Blocks: 359608
blocking2.0: --- → ?
Keywords: regression
Blocks: 595122
Assignee: nobody → azakai
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #474454 - Flags: review?(bobbyholley+bmo)
> 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?
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?
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.
(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.
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.
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?).
(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?
(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.
Duplicate of this bug: 595867
OS: Mac OS X → All
Hardware: x86 → All
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?
Attachment #474454 - Flags: review?(bobbyholley+bmo) → review+
Attached patch Part 1: Fix itSplinter Review
Carried r+.
Attachment #474454 - Attachment is obsolete: true
Attachment #474833 - Flags: review+
Attached patch Part 2: Test itSplinter Review
> 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.
Attachment #474845 - Flags: review?(bobbyholley+bmo)
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. ;-)
Attachment #474845 - Flags: review?(bobbyholley+bmo) → review?(joe)
Attachment #474845 - Flags: review?(joe) → review+
blocking2.0: ? → final+
If there are no objections, I guess this is ready to land?
Duplicate of this bug: 598935
Blocks: 600107
http://hg.mozilla.org/mozilla-central/rev/1354806a3b6c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 773203
You need to log in before you can comment on or make changes to this bug.