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.
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....
(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...
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.
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...
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.
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 #416890 - Flags: review? → review?(bzbarsky)
Probably Bobby Holley or Joe Drew should review this.
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...
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
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?
(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.
(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.
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?
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)
(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?
Joe, see comment 9.
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-
Quite some time has passed, quite some bugs fixed, is this bug still valid?
You need to log in before you can comment on or make changes to this bug.