GIF animation timers won't get suspended when GIFs loaded in JavaScript code

NEW
Unassigned

Status

()

9 years ago
24 days ago

People

(Reporter: ilkka.otsala, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.14) Gecko/2009090216 Ubuntu/8.04 (hardy) Firefox/3.0.14
Build Identifier: microb-engine (20091016-1.9.2-4)

All GIF animation timers should get suspended when browser window goes to background, minimized or device goes to power saving mode.

The case is as follow:
1. Page contains some JavaScript code which loads several GIF animations, one of them is shown on page at once.
2. After some time interval, the GIF animation is changed to another.

The problems:
1. When all the GIF animations are loaded in JavaScript code, animations are started for all GIFs, even if certain GIF not shown on the page.
2. When changing the currently shown GIF animation shown on page to another, the timer for new one is started (it has been running since page load) and the timer for previous one is not stopped.

This is pretty common advertisement banner implementation with animated GIFs. This affects to the power management.

Reproducible: Always

Steps to Reproduce:
1. Open e.g. http://www.nettiauto.com/showInHouseBanner.php?na_page=28
2. Minimize the page, place the browser window on background or let the device go to power saving mode.

Actual Results:  
Animation timer are not stopped: imgContainer::Notify gets called continuously for every GIF animation instance.

Expected Results:  
All GIF animation timers should be stopped for saving the CPU and power.
(Reporter)

Comment 1

9 years ago
Added bug #446418 to blocker list and Romaxa to cc list.
Blocks: 446418
(Reporter)

Comment 2

9 years ago
Created attachment 412180 [details] [diff] [review]
Fix proposal

Patch will fix the situation so, that when the GIF animation has been loaded and decoded completely (imgIDecoderObserver::OnStopRequest call back), it is checked whether the GIF is part of the current document or not: if it is not, the animation will be stopped. Also when the image of certain nsHTMLImageElement is changed to another, the animation is stopped for previous image.
Attachment #412180 - Flags: review?(bzbarsky)
Comment on attachment 412180 [details] [diff] [review]
Fix proposal

The container is shared across multiple elements that all show the same image, so this will break sites, no?  In particular those using image preloading and then showing copies of that image in the page; all of those will share a container.
Attachment #412180 - Flags: review?(bzbarsky) → review-
Oh, and I'm pretty sure we have existing bugs on this.  You might want to talk to the imagelib folks about ideas for resolving this....
(Reporter)

Comment 5

9 years ago
(In reply to comment #3)
> The container is shared across multiple elements that all show the same image,
> so this will break sites, no?  In particular those using image preloading and
> then showing copies of that image in the page; all of those will share a
> container.
This might be valid on desktop browsers where it is possible to have several windows open and shown at the same time, but for microb-engine there is always only one window shown. I tried to create a scenario where I could reproduce possible regressions with this issue, but I wasn't able. Any hints?

(In reply to comment #4)
> Oh, and I'm pretty sure we have existing bugs on this.  You might want to talk
> to the imagelib folks about ideas for resolving this....
Added Imagelib folks to cc, could you please comment this case, or give some ideas how this one should be fixed?

Probably one way to do this could be to stop all animation timers in nsPresContext::SetImageAnimationModeInternal, where currently timers for visible animations are suspended. But I don't know how can I access all imgContainer's, especially the not visible ones; ideas?
> here it is possible to have several windows open

What do multiple windows have to do with this?  I'm talking about multiple images in the same document.  Microb does support _that_ last I checked.

> Any hints?

  <script>
    var i1 = new Image("my-animated-image");
    var i2 = new Image("my-animated-image");
    document.body.appendChild(i2);
  </script>


As for solving this...  The right way is probably to hook up image animations to nsRefreshDriver (so make them pull, not push) and disable the refresh driver in background tabs or something, no?  Not sure how that fits in with imagelib's goals for non-gecko embedding, etc.
Imagelib has no plans for non-gecko embedding. Also, I had no idea nsRefreshDriver existed.
That's probably because it didn't until about a month ago.  ;)

To be clear "non-gecko-embedding" would include things like "animating images not associated with a presshell", since refresh driver is per-presshell.
> As for solving this...  The right way is probably to hook up image animations
> to nsRefreshDriver (so make them pull, not push) and disable the refresh driver

We need this fix reviewed properly for 1.9.2 branch, and nsRefreshDriver is not available for 1.9.2.

Can you review this patch for 1.9.2, that we can integrate it properly into MicroB engine, and possibly for fennec 1.9.2 branch...
(Reporter)

Comment 10

9 years ago
Created attachment 412819 [details]
appendChild test page

Tested the document.body.appendChild scenario with attached test page.

Scenario goes as follow:
1. 2 gifs are preloaded, animation for those stopped because those are not part of the document.
2. After the button is pressed, the animation will be appended to document. In that sequence a method nsImageFrame::Init will be called and there it is ensured that the animation is started.

So far this is not breaking anything(?). Please let me know if there is something else that might break the functionality of animating gifs; I can do the needed tests.
> We need this fix reviewed properly for 1.9.2 branch

OK.  In that case you need a patch that doesn't break things.

> Tested the document.body.appendChild scenario with attached test page.

No, it's not. The key is to append immediately after creating as in comment 6, so that the OnStopRequest for the image not in a document comes after the other image has been inserted into the document.
(Reporter)

Comment 12

9 years ago
Created attachment 413062 [details]
another appendChild test page

Changed the test page as suggested; sequence is slightly different, but still not breaking anything. 

I don't know the exact sequence when the image has become a part of the document, but in this case it seems to be before the OnStopRequest is called. 

Looks like the image has been bind to nsImageFrame before the loading has been completed. The animation starting method is first time called from nsImageFrame::OnStartContainer. As I have understood the imgContainer is bind to nsImageFrame when it is part of the document? Correct me if I'm wrong.
Attachment #412819 - Attachment is obsolete: true
You new testcase doesn't have an image that's not in a document.  You need both: an image in the document (which will start the animation) and an image not in the document (which will stop it when it gets OnStopRequest).  I'm not quite sure why just copying and pasting the code from comment 6 is being so difficult...
(Reporter)

Comment 14

9 years ago
Oh yes, now I got it reproduced. I didn't understood that the image has to be the same one. 

So, any ideas how to fix or use some different approach to the problem?
> So, any ideas how to fix or use some different approach to the problem?

If I'd had ideas, I would have mentioned them.  I'm not trying to hide things from you here....

I strongly suggest talking this over with the imagelib peers.  Fundamentally, it seems that we want to have two different kinds of image observers, ones that care about animations and ones that don't, and the animation should stop as soon as the count of the former goes to 0 (just like it does now when the image observer count goes to 0).
I'm not sure what chrome's relationship to presshells are, but if chrome doesn't have one, then we can't use nsRefreshDriver.

Bug 487667 adds "static" image requests, images that aren't animated. But that's not what we want here.

I am quite surprised we don't have a bug on this already (well, at least I can't find one). Yes, it's a known problem that we don't stop the animation timer when images are unused, but what you've been doing here is wrong.

Right now we don't have a way of telling whether an image is in use by a document, or just in js. Imagelib has a count of imgRequestProxys (= users) that are attached to a particular image, but var i = new Image("foo.png") creates an imgRequestProxy even if we're not inserted into a document. So just counting imgRequestProxys will not do what you want.

I do not know enough about content to say whether it's possible for a given DOM node to know if it's part of the document. I'd be open to an "activity count" on images, which if it reaches zero will stop the animation. I just don't know where to insert that in the DOM code.
Maybe I was unclear.  If an xpcom component calls into imgILoader and gets an image, etc, should it be able to get animation notifications from that imgIRequest?  There's no presshell there.

> I do not know enough about content to say whether it's possible for a given DOM
> node to know if it's part of the document.

It sure is. BindToTree and UnbindFromTree.  I can point people to good places to insert stuff in the DOM as needed, if there's an imagelib API for it.
(In reply to comment #17)
> If an xpcom component calls into imgILoader and gets an
> image, etc, should it be able to get animation notifications from that
> imgIRequest?  There's no presshell there.

Yes, it should be possible. Whether we should do it is maybe up for debate, but the possibility should absolutely be there.
OK, then those can't come off a refresh driver...
Which is a little sad, since the refresh driver approach is really better for in-page stuff, imo.
(In reply to comment #18)
> (In reply to comment #17)
> > If an xpcom component calls into imgILoader and gets an
> > image, etc, should it be able to get animation notifications from that
> > imgIRequest?  There's no presshell there.
> 
> Yes, it should be possible. Whether we should do it is maybe up for debate, but
> the possibility should absolutely be there.

Can we have that debate now?

Are people doing this today, and is there a use-case we care about?
Seems to me like it wouldn't be all that hard to just do both.

In the common case, we use the refresh driver. A bit is set on the image, and we just track the timestamp of the last refresh. When we get another one, we compare timestamps and fast-forward the image to the appropriate frame.

In the not-common case, we can just keep using the timer callback that we use now (not much extra work to keep it around). This should eliminate most of the overhead of clogging the event queue with timers, which is the main problem we care about from a perf standpoint.
(Reporter)

Comment 23

9 years ago
Created attachment 416890 [details] [diff] [review]
Fix proposal V2

New fix proposal for 1.9.2: 
Added reference array to imgContainer for elements that has the certain image visible on document. Having that information in imgContainer makes possible to stop animations that are not visible.

The reference is added to imgContainer in OnStopRequest or BindToTree methods: if the image is not loaded to cache BindToTree gets called before the imgContainer exists, in that case the reference is added in OnStopRequest. If the image has already been loaded to cache, OnStopRequest gets called before BindToTree and the reference is in that case added in BindToTree. Setting the element pointer as reference handle, double addition can be avoided. Removing reference from imgContainer is done in UnBindFromTree.

I'm not sure about the addition of new methods in imgIContainer, is there something specific that I have to take care of, since that is a public interface?

Could you please check the patch for comments?
Attachment #412180 - Attachment is obsolete: true
Attachment #416890 - Flags: review?
(Reporter)

Updated

9 years ago
Attachment #416890 - Flags: review? → review?(bzbarsky)
Probably Bobby Holley or Joe Drew should review this.
(Reporter)

Comment 25

9 years ago
Comment on attachment 416890 [details] [diff] [review]
Fix proposal V2

(In reply to comment #24)
> Probably Bobby Holley or Joe Drew should review this.
Changed reviewer to Joe Drew.
Attachment #416890 - Flags: review?(bzbarsky) → review?(joe)
I'm a little confused about why the content side of this wasn't done in nsImageLoadingContext, and instead in only one of its many subclasses...

Comment 27

9 years ago
unsure if this is solely about content, but i'm having a similar problem as per Bug 535583 in chrome.
This isn't just about content, but _is_ just about <html:img> elements.  The tree setup is weird enough it'll need a totally different approach.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 29

9 years ago
I am not really sure are you commenting the patch or the issue overall? Probably my fix isn't valid for 1.9.3 branch since many things have been changed there, but we really need to fix this issue for Microb branch.

As I have tested the patch, I haven't found any possible regressions, could you imagine any use case where something might get broken?
> I am not really sure are you commenting the patch or the issue overall?

In which comment?
(Reporter)

Comment 31

9 years ago
(In reply to comment #30)
> > I am not really sure are you commenting the patch or the issue overall?
> 
> In which comment?
In comments after I attached the fix proposal V2 (after comment #23). I would really like get some feedback about the patch; can you see any possible problems with this patch? Is it ok or not?
Comment 26 applies to any branch you're likely to make this change on.
(Reporter)

Comment 33

9 years ago
(In reply to comment #26)
> I'm a little confused about why the content side of this wasn't done in
> nsImageLoadingContext, and instead in only one of its many subclasses...

Reference count handling for imgContainer is needed to be done when the attribute of element has been set or changed (in nsHTMLImageElement::SetAttr), when the element is bind to document (nsHTMLImageElement::BindToTree) and when the element is unbind from the document (nsHTMLImageElement::UnbindFromTree). These can only be done in html element (as I have understood). The OnStopRequest handling part could be moved to nsImageLoadingContent, but does it make any difference? The loading content isn't aware when certain element is bind and when unbind.

One change that could be that I move the implementation of adding and removing a reference to nsImageLoadingContent as own methods, and call those from nsHTMLImageElement. Is that what you meant?

I don't know the design of this component so well that I could understand what you mean with your comment and what you have on your mind. More detailed explanation could really help.
The loading content is notified on BindToTree, SetAttr, and UnsetAttr (via LoadImage and CancelImageRequests).  You could also add a notification in UnbindFromTree.

> One change that could be that I move the implementation of adding and removing
> a reference to nsImageLoadingContent as own methods, and call those from
> nsHTMLImageElement.

This would be a big step up, yes. Then other subclasses (inputs, objects, SVG <image> elements) could more easily implement the same behavior.

> More detailed explanation could really help.

The basic goals, imo, should be:

1)  All images (<html:img>, <html:input type="image">, <html:object>,
    <svg:image>) behave the same way.
2)  There is minimal code duplication required to achieve goal #1.
(Reporter)

Comment 35

9 years ago
Created attachment 420537 [details] [diff] [review]
Fix proposal V3

Did the changes. Now, most of the visibility reference addition or removal implementation is located in nsImageLoadingContent, common for <html:img>, <html:input type="image">, <html:object> type of elements. Reference removel for element's UnbindFromTree is still called from sub-classes, since image loading content not involved with that.

This fix is not valid for svg animations, since those does not use imgContainer; there is separate controller for SMIL animations.

New patch attached, please take a look.

Comments?
Attachment #416890 - Attachment is obsolete: true
Attachment #420537 - Flags: review?
Attachment #416890 - Flags: review?(joe)

Updated

9 years ago
Attachment #420537 - Flags: review? → review?(bzbarsky)
Comment on attachment 420537 [details] [diff] [review]
Fix proposal V3

This needs review from the imagelib owner and getting the API there sorted out before I can even look at the content side.
Attachment #420537 - Flags: review?(bzbarsky) → review?(joe)
(Reporter)

Comment 37

9 years ago
(In reply to comment #36)
> (From update of attachment 420537 [details] [diff] [review])
> This needs review from the imagelib owner and getting the API there sorted out
> before I can even look at the content side.
Any possibilities to speed up the reviews? Joe?
I'm not overjoyed at having another list of things to keep track of in imgContainer (I'm trying to _reduce_ complexity!), but if I can't come up with a better solution we should probably take this.
After considering it, I now think our best bet is to use nsRefreshDriver, and not animate images that aren't part of a document. This would make it so that we don't need to explicitly turn off redrawing on GIFs in the way the patches in this bug do; instead, it would just happen "magically".

Ilkka, do you want to work on such a patch?
Right. I'd forgotten.

I'm not really in favour of adding a ton of complexity to a branch that won't have a lot of testing before being thrown in front of hundreds of millions of users. But if backporting nsRefreshDriver is impossible, I guess I'll accept it.
Comment on attachment 420537 [details] [diff] [review]
Fix proposal V3

What we need here is a trunk version, implemented using nsRefreshDriver, so we know what a "good" API looks like. Then we can backport and/or modify the solution for 1.9.2.

I suspect it will be easier to backport nsRefreshDriver than to engineer a hacky fix like this.
Attachment #420537 - Flags: review?(joe) → review-

Comment 43

6 years ago
Quite some time has passed, quite some bugs fixed, is this bug still valid?

Updated

29 days ago
Product: Core → Core Graveyard

Updated

24 days ago
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.