Closed Bug 359608 Opened 18 years ago Closed 14 years ago

Animated GIFs are animated even when user navigates to another page

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+
fennec 2.0+ ---

People

(Reporter: alex+list, Assigned: azakai)

References

()

Details

(Keywords: perf, testcase)

Attachments

(2 files, 23 obsolete files)

102.33 KB, patch
bholley
: review+
Details | Diff | Splinter Review
40.62 KB, patch
azakai
: review+
azakai
: superreview+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061104 BonEcho/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061104 BonEcho/2.0

If a user visits a page with an animated GIF embedded in it, and then navigates to another page, then presses back, the animated image will have advanced further, as though the image had been animating the entire time.

(Sorry if that's hard to understand, I can't seem to word it well. Take a look at the URL and steps to reproduce.)

This is probably using more CPU than it needs to, because we don't need images to be animated while we're not looking at them.

Reproducible: Always

Steps to Reproduce:
1. Visit a page with an animated GIF, like http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes
2. Watch the animation and remember where in the animated loop it is.
3. Click a link.
4. Wait a few seconds, then click Back.
5. The image has been animated the entire time.

Actual Results:  
Image has advanced while the page was not loaded.

Expected Results:  
The image loop would restart or it would start where it had been left off.
Happens on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061106 Minefield/3.0a1
confirmed on Windows Minefield build 20061107 and Mac Bon Echo nightly 20061101
OS: Windows XP → All
Hardware: PC → All
And WFM on both 2.0 builds I have:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061107 BonEcho/2.0
Status: UNCONFIRMED → NEW
Ever confirmed: true
Does this still happen if you disable bfcache (set browser.sessionhistory.max_total_viewers = 0 and restart)?
Gavin: no, that "fixes" it.
Assignee: pavlov → nobody
Component: Image: GFX → History: Session
QA Contact: history.session
Another data point: this behavior is also seen with SeaMonkey v1.1.1.

Breakpoints set on the animation code continue to be hit while the images are off-screen.
I don't know if there's another bug for this, but IMO, this should also apply to minimized windows and inactive tabs. I regularly visit sites which use large numbers of very small animated gifs, and constantly processing them even when they're not being viewed seems like an awful performance waste, and I regularly experience very jerky browser performance on these sites.
(In reply to comment #7)
> I don't know if there's another bug for this, but IMO, this should also apply
> to minimized windows and inactive tabs. 

bug 120154?
Blocks: 119597
Keywords: perf, testcase
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
Still in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090420041629

Animated gif should 'pause' when leaving page (or switching tab) and continue once user is back.
I'm not sure about the switching tab one. Let's take an extreme scenario, where there's an animation that is an half hour long and the user wants that to run in the background until it reaches a certain point.

I think that it should run as long as the homepage is open in anyway (visible or not), but when closed, it should stop.
(In reply to comment #10)
> Animated gif should 'pause' when leaving page (or switching tab) and continue
> once user is back.

FWIW, this is the behavior that we have for SMIL-animated SVG content -- animations "pause" when the user navigates away, and "resume" when the user returns.  It seems like this would be an area where we should be consistent between types of animated images, if possible, though it's probably not very high-priority.
When GIFs are being animated in a page that's not visible, or has been navigated away from, are they still being decoded?

That is, Firefox could be doing the decoding and drawing needlessly, or it could just be advancing the frame counter and not doing any work. Since lots of animated GIFs are used, the first case would be a perf problem.

I initially reported this because I was afraid of the perf problem, but of course I don't know how GIFs are animated, so it may not be a problem at all, just a matter of consistency.
Blocks: 567339
It seems that this is closely related (or perhaps identical) to bug 567339. Not much CPU is being used, but aside from consistency (as mentioned in previous comments here), there is also the issue of the animation timers continuing to fire (which is the focus of the just mentioned bug).

How does SMIL-animated SVG content achieve proper pause/resume behavior (mentioned by dholbert) - would it be feasible to utilize that here?

If not, random thought: there is properly working pause/resume functionality in nsGlobalWindow, for script timeouts. It seems that could be generalized to handle other timers (and so, animation timers) as well in a simple way - would that make sense?
(In reply to comment #14)
> How does SMIL-animated SVG content achieve proper pause/resume behavior
> (mentioned by dholbert) - would it be feasible to utilize that here?

When we navigate away from a SMIL-animated SVG document, we:
  (a) unregister with the refresh driver (the timer that gives us callbacks)
  (b) record the current time.
When we return to that page, we
  (c) re-register with the refresh driver
  (d) record the time that's elapsed during pausing, so we can offset our timeline by that much, and behave as if no time has passed.

Rethinking the argument from comment 10 & 12, though -- I don't actually think "pause-on-navigating-away" would work for animated GIFs right now, because (as i understand it) we create a single imgIContainer object for *all* instances of a particular animated GIF, across *all* tabs.  If we were to pause a GIF when the user clicks a link in one tab, that would actually pause the GIF everywhere (in other tabs), and we don't want that.  (You can see what this would be like by opening a few tabs that reference the same GIF, and press "esc" in one of them to pause it -- note that it pauses the gif elsewhere, too.)
The patch makes use of the refresh driver to stop animated images when they should stop, and restarts them when necessary. Tested on fennec/e10s, works ok as far as I can see (including multiple tabs etc.). Notes:

* A single tiny method is added to the refresh driver, IsFrozen()

* Otherwise the only change is some added code in nsImageFrame::FrameChanged. nsImageFrame receives animation notifications from images, and also has knowledge of the relevant document/.../refresh driver, so it has everything it needs (whereas the imgContainers do not).

* The change checks if we should freeze; if so, we freeze and listen on the refresh driver. Once it sends a single animation update, we stop listening and start the animation again.

This approach doesn't use the refresh driver to drive the animations, but only to be notified when to sleep and awaken. The reason is that
keeps the animations running normally at their regular rates, whereas the refresh driver would run a timer at 50Hz (which would be a regression in terms of CPU&power usage).
Comment on attachment 452023 [details] [diff] [review]
Use refresh driver to stop animating when relevant

Is there a reason you can't just get the prescontext via PresContext()?
(In reply to comment #17)
> (From update of attachment 452023 [details] [diff] [review])
> Is there a reason you can't just get the prescontext via PresContext()?

Thanks, you are absolutely right, I missed PresContext(). The patch should do it that way.
That patch doesn't work for animated background images, border images, list-style-image images, etc, etc, etc, right?
Furthermore, it breaks situations where an image is both on the preceding page and on this one, since they share a single imgIContainer, and this code will freeze it.
Improved version of previous patch, following bz's comments.

1. Works for regular HTML images and background images as well. I am not sure how to test border images and the rest, but they might already work; if not, supporting them is hopefully just a single line of code (see below).

2. Works with shared imgContainers.

The patch allows notifying imgContainers of their relevant refresh drivers, with a new method AddRefreshDriver (currently called in ImageFrame and ImageLoader). When any of them are active, we will animate; if all are suspended, we freeze ourselves. So if we have a shared imgContainer, it will have 2 refresh drivers, and we will only pause if both are paused.
Attachment #452023 - Attachment is obsolete: true
Attachment #452922 - Flags: feedback?
Best to ask feedback from a specific person as a feedback? flag with no person is likely to go unseen.
Attachment #452922 - Flags: feedback? → feedback?(bzbarsky)
A few things:
 (A) imgContainer::mAnimationSuspender should be an nsRefPtr rather than a raw pointer, since it's a ref-counted object -- and it should be freed automagically through refcounting, rather than an explicit 'delete' call.  (otherwise we're opening ourselves up to double-frees if we make mistaken assumptions about lifetimes etc.)

 (B) I don't understand the paired AddRef() in the constructor & Release() in WillRefresh.  Was this necessary to keep the object alive?  If so, that should be fixed by (A).

 (C) s/true/PR_TRUE/, inside of AnimationSuspender::Cancel().

 (D) mWokenUp and mCanceled should be PRPackedBool, not PRBool

 (E) Method names in .cpp/.h files ('observe', 'unobserve', 'checkRefreshDriver') should be capitalized

 (F) I'd very much like for the "AnimationSuspender" class to be non-imglib-private -- I'll need access to it in bug 276431 (SVG-as-image), which has an imgIContainer implementation that lives in /layout/.  Perhaps AnimationSuspender could live in its own .h file, in modules/libpr0n/public/?
dholbert, thanks for the comments.

New patch addresses those comments, and rebases against current m-c.
Attachment #452922 - Attachment is obsolete: true
Attachment #460630 - Flags: review?(bzbarsky)
Attachment #452922 - Flags: feedback?(bzbarsky)
So just to make sure I have the ownership model straight:

1)  imgContainer holds strong refs to some refresh drivers.  Shouldn't we be
    calling RemoveRefreshDriver sometimes?  Why do we need to ensure no
    repeated drivers in the array?
2)  On each timer notification for the animation (which still comes from our
    built-in timer) we poll all the relevant refresh drivers and check whether
    they're frozen.  If not, we do nothing.
3)  If they're frozen, we create an animation suspender (owned by the
    imgContainer) which registers with all the refresh drivers and then waits to
    see whether they start notifying, at which point it unregisters and destroys
    itself.

Sound about right?
(In reply to comment #25)
> So just to make sure I have the ownership model straight:
> 
> 1)  imgContainer holds strong refs to some refresh drivers.  Shouldn't we be
>     calling RemoveRefreshDriver sometimes?

Well, the strong refs will be dropped when the imgContainer is destroyed. We could also call RemoveRefreshDriver so they are dropped earlier, I'm not sure how important that is, or what point in the code would be the right place.

>     Why do we need to ensure no
>     repeated drivers in the array?

Just for performance reasons, to keep the array from growing needlessly.

> 2)  On each timer notification for the animation (which still comes from our
>     built-in timer) we poll all the relevant refresh drivers and check whether
>     they're frozen.  If not, we do nothing.

Yes. (Now that I think of it, maybe it makes sense to do this only once/second or so, to save some CPU?)

> 3)  If they're frozen, we create an animation suspender (owned by the
>     imgContainer) which registers with all the refresh drivers and then waits
> to
>     see whether they start notifying, at which point it unregisters and
> destroys
>     itself.
> 
> Sound about right?

Yes, that is exactly right.
> Well, the strong refs will be dropped when the imgContainer is destroyed.

Yes, but in the meantime it can accumulate arbitrary numbers of refresh drivers that would otherwise be defunct, right?

> Just for performance reasons, to keep the array from growing needlessly.

But it introduces a different performance problem: building an array of length N becomes O(N^2)...   Perhaps an array is the wrong data structure for this?  Maybe a hash set?

> Now that I think of it, maybe it makes sense to do this only once/second
> or so, to save some CPU?

I'd fully expect the common case to only have one refresh driver involved.  So the main CPU cost is from the timer wakeup in the first place.
tracking-fennec: --- → 2.0+
New patch with some fixes:

* IsDefunct() function on RefreshDrivers. imgContainers will automatically remove such RefreshDrivers (see below), so unneeded ones are no longer kept.

* Fixed a bug with the AnimationSuspender already existing, when the animation is manually restarted (noticed this on try server).


(In reply to comment #27)
> > Well, the strong refs will be dropped when the imgContainer is destroyed.
> 
> Yes, but in the meantime it can accumulate arbitrary numbers of refresh drivers
> that would otherwise be defunct, right?

Good point. As mentioned above, fixed in latest patch.

> 
> > Just for performance reasons, to keep the array from growing needlessly.
> 
> But it introduces a different performance problem: building an array of length
> N becomes O(N^2)...   Perhaps an array is the wrong data structure for this? 
> Maybe a hash set?

I did some testing, both manually and with the existing mochitests. The most RefreshDrivers I saw was 2. Given that, maybe it doesn't matter to move this to a hash set, as it's so small?

> 
> > Now that I think of it, maybe it makes sense to do this only once/second
> > or so, to save some CPU?
> 
> I'd fully expect the common case to only have one refresh driver involved.  So
> the main CPU cost is from the timer wakeup in the first place.

Yes, in the tests I did it was almost always 1, rarely 2. So maybe there isn't a problem here. On the other hand, to remove defunct RefreshDrivers a little additional work is now done in each timer wakeup.
Attachment #460630 - Attachment is obsolete: true
Attachment #461340 - Flags: review?(bzbarsky)
Attachment #460630 - Flags: review?(bzbarsky)
Drive-by comment.

I apologize for not understanding the Fx image code enough to know precisely what behavior you're implementing.  But if you are implementing "freeze animations in background tabs", then you should be aware that, at least in the past, this behavior has caused compatibility issues with sites that sync animated GIFs with other resources (e.g. sound loops).

The reason I know this is because I fixed this bug in WebKit a couple years ago -- WebKit now pauses animations that are invisible, but when they become visible again, if they're less than five minutes out of date, it "spins them forward" (without drawing, of course) to the point where they should have been had they kept animating.

I don't have any examples onhand of sites that rely on this (ytmnd.com used to be one, but they've switched to using Flash for everything), but you may want to avoid introducing this kind of problem.
We're freezing animations for resources in back/forward cache; this is doing nothing about background tabs for now.

> The most RefreshDrivers I saw was 2.

You want me to write you some testcases with a lot more than that?  All that needs to happen is that a page has lots of frames, each of which uses the image...  It's easy to get the number into the thousands.

That situation will get better once we share the same refresh driver for all subframes, but you could still hit it with lots of tabs.  Note that mochitests don't really test the "hundreds of tabs with web content" use case much; I bet your manual testing didn't either.
blocking2.0: --- → ?
Blocks: 276431
This needs to block beta 5, because it blocks bug 276431.
Assignee: nobody → azakai
blocking2.0: ? → beta5+
Uses a hash set to avoid slowdowns with large numbers of refresh drivers.

Also cleaned up the code and prevented some edge cases.
Attachment #461340 - Attachment is obsolete: true
Attachment #463403 - Flags: review?(bzbarsky)
Attachment #461340 - Flags: review?(bzbarsky)
Fixed a crash shown on try server.
Attachment #463403 - Attachment is obsolete: true
Attachment #463714 - Flags: review?(bzbarsky)
Attachment #463403 - Flags: review?(bzbarsky)
I'm probably not going to get to this review before vacation.
Comment on attachment 463714 [details] [diff] [review]
Use refresh driver to stop animating when relevant v6

>+++ b/layout/base/nsRefreshDriver.h
>+ * Simple implementation of refcounting in the nsARefreshObserver
>+ * schema.

 Simple implementation of refcounting for an nsARefreshObserver

>+  int mRefs;

nsAutoRefCnt, please.

>+  NS_IMETHOD_(nsrefcnt) AddRef(void) {
>+    mRefs++;
>+    return NS_OK;

You probably want to do something closer to what NS_IMPL_ADDREF does.  Can probably skip the owning thread bit, but you do want the refcount logging, and the return value here is all wrong.

>+  NS_IMETHOD_(nsrefcnt) Release(void) {

Again, see what NS_IMPL_RELEASE does.

In fact, is there a reason you can't just use NS_IMPL_ADDREF and NS_IMPL_RELEASE here?

There are probably existing refresh observers that could use this code.  Followup bug to make them to do?

>+  PRBool IsDefunct() { return mPresContext == nsnull; }

const method, right?

>+  PRBool IsFrozen() { return mFrozen; }

And this.

Was there a reason to not add the refresh driver in nsBulletFrame, nsImageBoxFrame, nsTreeImageListener?

>+++ b/modules/libpr0n/public/AnimationSuspender.h
>+/**
>+ * Handles suspending animations for an imgContainer. Listens on all relevant
>+ * nsRefreshDrivers for when we should start animating again.
>+ */

Put this comment before the ifdef guards, right after the license.  Then MXR will show it in file listings.

You should make it clear that this object holds no strong refs to anything and that the constructor arguments need to outlive this object.  Or rather, since this can have arbitrary lifetime (it's refcounted) make sure it can deal with its nsARefreshObserver methods being called after the imgContainer has gone away.  This may already be the case, but if so worth documenting how it works.

Given that this doesn't have an "ns" prefix, should it be in a namespace?

>+   *                        removing it from drivers you remove. Or,
>+   *                        just stop the animation suspender

Stop how?  By calling Cleanup()?  Might be good to be more explicit.

>+                     nsTHashtable<nsISupportsHashKey>* aRefreshDrivers);

I'd create AnimationSuspender::RefresDriverTable as a typedef for that nsTHashtable<nsISupportsHashKey> business.  But either way.

>+   * If this function is never called, we will never restart the
>+   * animation. This is what you want, if the imgContainer is going
>+   * away and you no longer care about it or the AnimationSuspender.
>+   * You should call Cleanup() in that case (WillRefresh will call Cleanup()
>+   * itself.)

I don't quite follow how that last sentence connects with the rest of the comment.  Does it?

>+   * Clean up (remove) the refresh driver listenings.

"Stop listening to the refresh drivers."  

>+  PRPackedBool mWokenUp;

Is this really needed?  I guess it's somewhat safer....

>+EXPORTS		= ImageErrors.h ImageLogging.h AnimationSuspender.h

Why does this need to be exported?

>+++ b/modules/libpr0n/public/imgIContainer.idl

Need to change the iid, right?

>+   * Adds a refresh driver that is relevant for us. When any of our refresh
>+   * drivers is active, we should be active; if they are all inactive, then
>+   * we can freeze ourself.

Maybe "pause our animation, if any" instead of "freeze ourself"?

>+++ b/modules/libpr0n/src/AnimationSuspender.cpp

>+ * The Initial Developer of the Original Code is Mozilla.

Mozilla Foundation. And anywhere else where you added new licenses.

>+#include "imgContainer.h"
>+
>+AnimationSuspender::AnimationSuspender(imgIContainer* aContainer,

Why not just take an imgContainer* here and store an imgContainer*?  Then you won't need the cast you end up doing below.

>+  static_cast<nsRefreshDriver*>(aEntry->GetKey())->AddRefreshObserver(static_cast<nsARefreshObserver*>(userArg), Flush_Style);

Please file a followup bug on creating an nsRefPtrHashKey<T> and using it here, so we can avoid the nasty non-typesafe casts?

You should cast userArg to AnimationSuspender*; same in Unobserve.  That way you won't have to worry about someone adding more stuff AnimationSuspender inherits from.

Should this really be a Flush_Style observer?  Flush_Display seems more appropriate.

>+++ b/modules/libpr0n/src/imgContainer.cpp
>+NS_IMETHODIMP imgContainer::AddRefreshDriver(nsRefreshDriver* aRefreshDriver)
>+{
>+  // If currently suspended, reboot the suspender

Seems like there's no need to do that in the common case that aRefreshDriver is already in the hashtable, right?

>+  aRefreshDriver->AddRef();

Why?  That looks like it will leak...

>+NS_IMETHODIMP imgContainer::RemoveRefreshDriver(nsRefreshDriver* aRefreshDriver)

No one calls this.  When would someone call it, exactly?  I don't think it's needed.

>+  if (refreshDriver->IsDefunct()) {
>+    return (PLDHashOperator) (PL_DHASH_REMOVE | PL_DHASH_NEXT);

Just REMOVE.  No point or-ing in NEXT here.

>+  if (!(refreshDriver->IsFrozen())) {

Remove the extra parens.

>+    PRBool* hasActiveDriver = static_cast<PRBool*>(userArg);
>+    *hasActiveDriver = PR_TRUE;
>+    return PL_DHASH_STOP;

This will fail to clean up defunct drivers, no?  I think this should fall through to returning PL_DHASH_NEXT.

r- pending us sorting out that addref in AddRefreshDriver....
Attachment #463714 - Flags: review?(bzbarsky) → review-
This is probably going to need some reworking on top of bug 584841, which is going to land sometime this afternoon. Once bz's issues are sorted out, you should probably have one of the imagelib people (me or joe) review it too.
(bholley - I added you as a reviewer now, and not after bz as you asked, since bz is currently on vacation, and the major concern he had - the AddRef - is resolved.)

(In reply to comment #35)
> There are probably existing refresh observers that could use this code
> [nsASimpleRefreshObserver]. Followup bug to make them to do?

Will do.

> 
> Was there a reason to not add the refresh driver in nsBulletFrame,
> nsImageBoxFrame, nsTreeImageListener?

I added a call in the first two, but I don't see a way to get
at the relevant refresh driver in nsTreeImageListener?

> 
> Given that this doesn't have an "ns" prefix, should it be in a namespace?

Ok, moved to the imagelib namespace.
 
> 
> >+EXPORTS		= ImageErrors.h ImageLogging.h AnimationSuspender.h
> 
> Why does this need to be exported?

It won't build without it - am I missing something here?

> >+  static_cast<nsRefreshDriver*>(aEntry->GetKey())->AddRefreshObserver(static_cast<nsARefreshObserver*>(userArg), Flush_Style);
> 
> Please file a followup bug on creating an nsRefPtrHashKey<T> and using it here,
> so we can avoid the nasty non-typesafe casts?

Will do.

> >+  aRefreshDriver->AddRef();
> 
> Why?  That looks like it will leak...

That was an embarrassing copy-paste mistake from a much older version of this
patch... please ignore. Removed.
Attachment #463714 - Attachment is obsolete: true
Attachment #466503 - Flags: review?(bzbarsky)
Attachment #466503 - Flags: review?(bobbyholley+bmo)
(In reply to comment #37)
> > >+EXPORTS		= ImageErrors.h ImageLogging.h AnimationSuspender.h
> > 
> > Why does this need to be exported?
> 
> It won't build without it - am I missing something here?

(I think bz implicitly meant "why does it need to be in /public/"?)

The answer to that is: in comment 23, I'd asked that it be in /public/ (and hence exported) so that I could use it for SVG-as-image.

However, my SVG-as-image stuff is going to live in imagelib now -- so AnimationSuspender.h can move back to /src/ (and won't need to be exported).
Rather than observing refresh drivers, why can't we just use the refresh driver to drive the animation?
(In reply to comment #38)
> (In reply to comment #37)
> > > >+EXPORTS		= ImageErrors.h ImageLogging.h AnimationSuspender.h
> > > 
> > > Why does this need to be exported?
> > 
> > It won't build without it - am I missing something here?
> 
> (I think bz implicitly meant "why does it need to be in /public/"?)
> 
> The answer to that is: in comment 23, I'd asked that it be in /public/ (and
> hence exported) so that I could use it for SVG-as-image.
> 
> However, my SVG-as-image stuff is going to live in imagelib now -- so
> AnimationSuspender.h can move back to /src/ (and won't need to be exported).

Got it. I'll make a patch with it in /src/ then.

(In reply to comment #39)
> Rather than observing refresh drivers, why can't we just use the refresh driver
> to drive the animation?

The main issue with that is the Refresh Driver uses a single fixed 50Hz timer. So it would take some work to rewrite it to use multiple timers of various frequencies.
Comment on attachment 466503 [details] [diff] [review]
Use refresh driver to stop animating when relevant v7

bholley tells me there might be an easier way to do this patch, using the recently landed bug 512260. So I will look into that.

(I can't see a way to obsolete this patch - is there?)
Attachment #466503 - Flags: review?(bzbarsky)
Attachment #466503 - Flags: review?(bobbyholley+bmo)
Attachment #466503 - Attachment is obsolete: true
Blocks: 588975
Attached patch Patch using bug 512260 (obsolete) — Splinter Review
Rewritten patch, using bug 512260 - shorter and simpler.
Attachment #467590 - Flags: review?(bzbarsky)
Attachment #467590 - Flags: review?(bobbyholley+bmo)
Comment on attachment 467590 [details] [diff] [review]
Patch using bug 512260

>diff -r 3be451ad56d7 content/base/public/nsIDocument.h
>--- a/content/base/public/nsIDocument.h	Thu Aug 19 08:22:46 2010 -0700
>+++ b/content/base/public/nsIDocument.h	Thu Aug 19 16:18:56 2010 -0700
>@@ -1450,6 +1450,15 @@
>   // state is unlocked/false.
>   virtual nsresult SetImageLockingState(PRBool aLocked) = 0;
> 
>+  // Makes the images on this document capable of having their animation
>+  // active or suspended. An Image will animate as long as at least one of its
>+  // owning Documents needs it to animate; otherwise it can suspend.
>+  // TODO: Instead of animating/suspended, add a third state, of being in
>+  // a background tab. In that case, no need to actually show the animation, but
>+  // do need to keep track of timing, so if the tab is brought to the foreground,
>+  // we can fast-forward to be in sync.
>+  virtual nsresult SetImageAnimatingState(PRBool aAnimating) = 0;
>+
> protected:
>   ~nsIDocument()
>   {
  
Given that nsDocument now wants to do different things depending on whether we're frozen/active and frozen/inactive, I think it makes more sense to just teach nsDocument those concepts. So I'd support a SetIsActive() and SetIsFrozen() here instead of SetImageLockingState and SetImageAnimatingState.

Note - I'm not a content/layout peer, so the review comments for those files are not binding, nor is my review sufficient on those parts.


>diff -r 3be451ad56d7 modules/libpr0n/public/imgIContainer.idl
>--- a/modules/libpr0n/public/imgIContainer.idl	Thu Aug 19 08:22:46 2010 -0700
>+++ b/modules/libpr0n/public/imgIContainer.idl	Thu Aug 19 16:18:56 2010 -0700
>@@ -230,6 +230,22 @@
>   void unlockImage();
> 
>   /**
>+    * Tells the image that it is required to be animated in one of the places
>+    * it is being shown. When at least one of those places asks for animation,
>+    * we will animate; otherwise, we can suspend the animation.
>+    *
>+    * Every call to this function should be matched with a call to
>+    * UnrequestAnimation, when the animation is no longer needed.
>+    */
>+  void requestAnimation();
>+
>+  /**
>+    * Tells the image that it is no longer required to be animated in one of
>+    * the places it is being shown, which previously called requestAnimation.
>+    */
>+  void UnrequestAnimation();
>+
>+  /**
>    * Animation mode Constants
>    *   0 = normal
>    *   1 = don't animate


This stuff shouldn't be in imgIContainer - it should go in Image. I've filed bug 589345 for moving lockImage/unlockImage/requestDecode there as well.

>diff -r 3be451ad56d7 modules/libpr0n/public/imgIRequest.idl
>--- a/modules/libpr0n/public/imgIRequest.idl	Thu Aug 19 08:22:46 2010 -0700
>+++ b/modules/libpr0n/public/imgIRequest.idl	Thu Aug 19 16:18:56 2010 -0700
>@@ -174,6 +174,20 @@
>   void unlockImage();
> 
>   /**
>+   * Requests that the image animate (if it has an animation).
>+   *
>+   * @see imgIContainer::requestAnimation for documentation of the underlying call.
>+   */
>+  void requestAnimation();
>+
>+  /**
>+   * Tell the image it can forget about a requests that the image animate.
>+   *
>+   * @see imgIContainer::unrequestAnimation for documentation of the underlying call.
>+   */
>+  void unrequestAnimation();
>+
>+  /**
>    * If this request is for an animated image, the method creates a new
>    * request which contains the current frame of the image.
>    * Otherwise returns the same request.

How about incrementAnimationConsumers() and decrementAnimationConsumers() instead? Some variable and method names will need to change as well.

Also, these @see's will need to be changed.

>diff -r 3be451ad56d7 modules/libpr0n/src/Image.cpp
>--- a/modules/libpr0n/src/Image.cpp	Thu Aug 19 08:22:46 2010 -0700
>+++ b/modules/libpr0n/src/Image.cpp	Thu Aug 19 16:18:56 2010 -0700
>@@ -43,9 +43,31 @@
>+nsresult
>+Image::RequestAnimation()
>+{
>+  mAnimationRequests ++;
>+  if (mAnimationRequests == 1)
>+    StartAnimation();
>+
>+  return NS_OK;
>+}

Remove the space before the ++, and add an NS_ABORT_IF_FALSE(!mAnim || !mAnim->animating, ...) within the if block.

>+
>+nsresult
>+Image::UnrequestAnimation()
>+{
>+  mAnimationRequests --;

Remove the space.

>+  NS_ASSERTION(mAnimationRequests >= 0, "Invalid no. of animation requests!");

use NS_ABORT_IF_FALSE in all new code - NS_ASSERTION is non-fatal.

>diff -r 3be451ad56d7 modules/libpr0n/src/RasterImage.cpp
>--- a/modules/libpr0n/src/RasterImage.cpp	Thu Aug 19 08:22:46 2010 -0700
>+++ b/modules/libpr0n/src/RasterImage.cpp	Thu Aug 19 16:18:56 2010 -0700
>@@ -2481,6 +2481,18 @@

>+nsresult
>+RasterImage::RequestAnimation()
>+{
>+  return Image::RequestAnimation();
>+}
>+
>+nsresult
>+RasterImage::UnrequestAnimation()
>+{
>+  return Image::UnrequestAnimation();
>+}
>+

RasterImage inherits Image right? So why do we need this?


>diff -r 3be451ad56d7 modules/libpr0n/src/imgRequest.h
>--- a/modules/libpr0n/src/imgRequest.h	Thu Aug 19 08:22:46 2010 -0700
>+++ b/modules/libpr0n/src/imgRequest.h	Thu Aug 19 16:18:56 2010 -0700
>@@ -119,6 +119,8 @@
>   nsresult LockImage();
>   nsresult UnlockImage();
>   nsresult RequestDecode();
>+  nsresult RequestAnimation();
>+  nsresult UnrequestAnimation();

These don't need to be on imgRequest. (I've filed bug 589352 to remove LockImage/UnlockImage from imgRequest).


>diff -r 3be451ad56d7 modules/libpr0n/src/imgRequestProxy.cpp
>--- a/modules/libpr0n/src/imgRequestProxy.cpp	Thu Aug 19 08:22:46 2010 -0700
>+++ b/modules/libpr0n/src/imgRequestProxy.cpp	Thu Aug 19 16:18:56 2010 -0700
>@@ -329,6 +329,18 @@
>   return mImage->UnlockImage();
> }
> 
>+NS_IMETHODIMP
>+imgRequestProxy::RequestAnimation()
>+{
>+  return mImage->RequestAnimation();
>+}
>+
>+NS_IMETHODIMP
>+imgRequestProxy::UnrequestAnimation()
>+{
>+  return mImage->UnrequestAnimation();
>+}
>+
> /* void suspend (); */
> NS_IMETHODIMP imgRequestProxy::Suspend()
> {

imgRequestProxy should keep track of the number of animation consumers, in a manner similar to mLocksHeld. You should assert in DecrementAnimationConsumers that the count isn't dropping below 0, and either have the destructor call DecrementAnimationConsumers the appropriate number of times, or assert that it is 0 at destruction-time. This helps isolate any unmatched calls.

Finally, you need to handle imgRequestProxy::ChangeOwner(). ping me or joe on IRC if you don't understand what it's doing.
Attachment #467590 - Flags: review?(bzbarsky)
Attachment #467590 - Flags: review?(bobbyholley+bmo)
Attachment #467590 - Flags: review-
Attached patch Patch using bug 512260 v2 (obsolete) — Splinter Review
Ok, made the requested changes, except for:

> >diff -r 3be451ad56d7 content/base/public/nsIDocument.h
> >--- a/content/base/public/nsIDocument.h	Thu Aug 19 08:22:46 2010 -0700
> >+++ b/content/base/public/nsIDocument.h	Thu Aug 19 16:18:56 2010 -0700
> >@@ -1450,6 +1450,15 @@
> >   // state is unlocked/false.
> >   virtual nsresult SetImageLockingState(PRBool aLocked) = 0;
> > 
> >+  // Makes the images on this document capable of having their animation
> >+  // active or suspended. An Image will animate as long as at least one of its
> >+  // owning Documents needs it to animate; otherwise it can suspend.
> >+  // TODO: Instead of animating/suspended, add a third state, of being in
> >+  // a background tab. In that case, no need to actually show the animation, but
> >+  // do need to keep track of timing, so if the tab is brought to the foreground,
> >+  // we can fast-forward to be in sync.
> >+  virtual nsresult SetImageAnimatingState(PRBool aAnimating) = 0;
> >+
> > protected:
> >   ~nsIDocument()
> >   {
> 
> Given that nsDocument now wants to do different things depending on whether
> we're frozen/active and frozen/inactive, I think it makes more sense to just
> teach nsDocument those concepts. So I'd support a SetIsActive() and
> SetIsFrozen() here instead of SetImageLockingState and SetImageAnimatingState.
> 

That does make sense I think, but there should probably be simultaneous changes in PresShell and Document (for one thing, in PresShell, SetImageLockingState isn't the same as SetIsActive - UpdateImageLockingState is called with True only if both Active and not Frozen). I'd suggest a separate bug if we want to do this cleanup.

> 
> >diff -r 3be451ad56d7 modules/libpr0n/src/Image.cpp
> >--- a/modules/libpr0n/src/Image.cpp	Thu Aug 19 08:22:46 2010 -0700
> >+++ b/modules/libpr0n/src/Image.cpp	Thu Aug 19 16:18:56 2010 -0700
> >@@ -43,9 +43,31 @@
> >+nsresult
> >+Image::RequestAnimation()
> >+{
> >+  mAnimationRequests ++;
> >+  if (mAnimationRequests == 1)
> >+    StartAnimation();
> >+
> >+  return NS_OK;
> >+}
> 
> Remove the space before the ++, and add an NS_ABORT_IF_FALSE(!mAnim ||
> !mAnim->animating, ...) within the if block.

Fixed the spacing, but there is no mAnim in Image, only in RasterImage.
The call to StartAnimation will handle that check.

--

Aside from the requested changes, the patch has one additional change,

 RasterImage::StartAnimation()
 {
   if (mError)
     return NS_ERROR_FAILURE;

   if (mAnimationMode == kDontAnimMode || 
       (mAnim && (mAnim->timer || mAnim->animating)))
     return NS_OK;

+  if (mAnimationConsumers == 0)
+    return NS_OK;

StartAnimation() is called in several places, basically to ensure that
the animation starts if one is needed. This check will prevent that
from accidentally starting an animation when we don't actually want
one (and if we want one later, it'll be started automatically anyhow).
Attachment #467590 - Attachment is obsolete: true
Attachment #468388 - Flags: review?(bzbarsky)
Attachment #468388 - Flags: review?(bobbyholley+bmo)
(In reply to comment #44)

> That does make sense I think, but there should probably be simultaneous changes
> in PresShell and Document (for one thing, in PresShell, SetImageLockingState
> isn't the same as SetIsActive - UpdateImageLockingState is called with True
> only if both Active and not Frozen). I'd suggest a separate bug if we want to
> do this cleanup.

Well, you're already changing both PresShell and Document, right? I'm just saying that it makes sense to streamline this communication while you're at it: have the presshell just pass the values of mIsActive and mIsFrozen over to the document, and let it make the appropriate changes based on the two values. Again though, I'll let bz be the final word here.

> 
> > 
> > >diff -r 3be451ad56d7 modules/libpr0n/src/Image.cpp
> > >--- a/modules/libpr0n/src/Image.cpp	Thu Aug 19 08:22:46 2010 -0700
> > >+++ b/modules/libpr0n/src/Image.cpp	Thu Aug 19 16:18:56 2010 -0700
> > >@@ -43,9 +43,31 @@
> > >+nsresult
> > >+Image::RequestAnimation()
> > >+{
> > >+  mAnimationRequests ++;
> > >+  if (mAnimationRequests == 1)
> > >+    StartAnimation();
> > >+
> > >+  return NS_OK;
> > >+}
> > 
> > Remove the space before the ++, and add an NS_ABORT_IF_FALSE(!mAnim ||
> > !mAnim->animating, ...) within the if block.
> 
> Fixed the spacing, but there is no mAnim in Image, only in RasterImage.
> The call to StartAnimation will handle that check.
> 
> --
> 
> Aside from the requested changes, the patch has one additional change,
> 
>  RasterImage::StartAnimation()
>  {
>    if (mError)
>      return NS_ERROR_FAILURE;
> 
>    if (mAnimationMode == kDontAnimMode || 
>        (mAnim && (mAnim->timer || mAnim->animating)))
>      return NS_OK;
> 
> +  if (mAnimationConsumers == 0)
> +    return NS_OK;
> 
> StartAnimation() is called in several places, basically to ensure that
> the animation starts if one is needed. This check will prevent that
> from accidentally starting an animation when we don't actually want
> one (and if we want one later, it'll be started automatically anyhow).

If this check is to prevent callers from accidentally doing something, then it should be an assertion. Why not add:

NS_ABORT_IF_FALSE(mAnimationConsumers == 1, ...)

to StartAnimation?

Also, I would like the check:

>(mAnim && (mAnim->timer || mAnim->animating)

to be turned into an assertion as well, since we shouldn't be calling StartAnimation if we already did.
> > StartAnimation() is called in several places, basically to ensure that
> > the animation starts if one is needed. This check will prevent that
> > from accidentally starting an animation when we don't actually want
> > one (and if we want one later, it'll be started automatically anyhow).
> 
> If this check is to prevent callers from accidentally doing something, then it
> should be an assertion.

Well no, StartAnimation basically means "If you have an animation, please start it now; if not, forget I mentioned it". It is called in various places in the code, basically where enough of the image data is loaded so we can know if animation is needed (might be some other places too, I didn't go over them all). So making this an assertion would break a lot of existing stuff (not related to this bug at all). And so the new check I added into StartAnimation works like all the other checks there.

I was thinking as I was working on this patch, it might be nice to clean all that other stuff up at some point. However doing so would touch code spread around the tree, and I'm not sure if it wouldn't require a lot of careful debugging or not. But I'd be happy to work on that in a followup bug if it's agreed that is important.
Ah right, those darn StartAnimation() calls scattered through the tree. :\

I think that the only change that needs to be made to the rest of the tree is to replace StartAnimation() with IncrementAnimationConsumers(). That's essentially what all those consumers are trying to do anyway, but they just didn't have a good way of doing it until your patch.

I understand not wanting to bloat your patch with a bunch of other stuff, but this is a case where I'm dubious about getting this right without making those changes. What you're essentially doing is providing a more abstract interface to animation consumers, which lets imagelib manage the animation. However, until those StartAnimation() calls go away, we'll also have consumers occasionally reaching in and trying to manage things themselves.

I think this patch has the opportunity to make things a lot a lot better, and all it has to do is switch the StartAnimation() calls to IncrementAnimationConsumer() calls, and then remove startAnimation and stopAnimation from imgIContainer.idl. So I'd like to do that, unless there are good reasons to the contrary.
The way to avoid bloating the patch is to have multiple patches:

1)  Add the new IncrementAnimationConsumers api
2)  Switch consumers to it
3)  Whatever else
Attachment #468388 - Attachment is obsolete: true
Attachment #468555 - Flags: review?(bobbyholley+bmo)
Attachment #468388 - Flags: review?(bzbarsky)
Attachment #468388 - Flags: review?(bobbyholley+bmo)
Attachment #468557 - Flags: review?(bobbyholley+bmo)
Attachment #468558 - Flags: review?(bzbarsky)
Attachment #468558 - Flags: review?(bobbyholley+bmo)
Comment on attachment 468555 [details] [diff] [review]
Part 1: Remove Start/StopAnimation from imgIContainer

>diff --git a/modules/libpr0n/public/imgIContainer.idl b/modules/libpr0n/public/imgIContainer.idl
>--- a/modules/libpr0n/public/imgIContainer.idl
>+++ b/modules/libpr0n/public/imgIContainer.idl
>@@ -242,7 +242,5 @@ interface imgIContainer : nsISupports
>   attribute unsigned short animationMode;
> 
>   /* Methods to control animation */
>-  void startAnimation();
>-  void stopAnimation();
>   void resetAnimation();
> };
>diff --git a/modules/libpr0n/src/Image.h b/modules/libpr0n/src/Image.h
>--- a/modules/libpr0n/src/Image.h
>+++ b/modules/libpr0n/src/Image.h
>@@ -119,6 +119,9 @@ public:
>   };
>   static eDecoderType GetDecoderType(const char *aMimeType);
> 
>+  NS_IMETHOD StartAnimation() = 0;
>+  NS_IMETHOD StopAnimation() = 0;
>+

s/NS_IMETHOD/virtual nsresult/


>diff --git a/modules/libpr0n/src/RasterImage.h b/modules/libpr0n/src/RasterImage.h
>--- a/modules/libpr0n/src/RasterImage.h
>+++ b/modules/libpr0n/src/RasterImage.h
>@@ -159,6 +159,9 @@ public:
>   RasterImage();
>   virtual ~RasterImage();
> 
>+  NS_IMETHOD StartAnimation();
>+  NS_IMETHOD StopAnimation();
>+

s/NS_IMETHOD/nsresult/

r=bholley with those changes.

This will need sr from bz.
Attachment #468555 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 468557 [details] [diff] [review]
Part 2: Increment/DecrementAnimationConsumers framework


>+NS_IMETHODIMP
>+Image::IncrementAnimationConsumers()

No NS_IMETHODIMP here.

>+  mAnimationConsumers--;
>+  NS_ABORT_IF_FALSE(mAnimationConsumers >= 0, "Invalid no. of animation consumers!");

mAnimationConsumers is a PRUint32, so the assertion should come first here. Otherwise, it'll never catch anything.

>+  NS_IMETHOD IncrementAnimationConsumers();
>+  NS_IMETHOD DecrementAnimationConsumers();

No NS_IMETHOD here. In fact, I don't see a compelling reason for these to return an nsresult. How about making them bonafide void methods?


>   PRPackedBool       mInitialized;   // Have we been initalized?
>+  PRUint32           mAnimationConsumers;

Not a big deal right now, but worth staying on top of anyway - keep PRPackedBools at the end so that we don't lose space due to alignment issues. 

>+
>+  if (mAnimationConsumers == 0)
>+    return NS_OK;

Now that we're managing who calls StartAnimation(), we should turn this check into an assertion, as well as the mAnim thing. Can you address comment 45, or is there something I'm missing?


>+  // Unrequest the animation the proper number of times. Each call decrements
>+  // mAnimationConsumers.

I'm not big on the word "unrequest".  How about just: // Each call decrements mAnimationConsumers.
Attachment #468557 - Flags: review?(bobbyholley+bmo) → review-
(In reply to comment #51)
> Created attachment 468558 [details] [diff] [review]
> Part 3: Call Increment/DecrementAnimationConsumer

This patch does exactly what I suggested, but there are some things I'd like the check with bz on. Specifically, I want to verify the following things:

1-that nsImageLoader is only used for CSS images, which we register with the Document. I believe that CSS images for non-content Documents should be OK, because Documents default to animating.

2-that it's ok that we don't animate images for border-image, list-style-image, and cursor-image (these are images we don't bother registering with the Document, though we could if we wanted to pretty easily).

Also, I'd like to figure out what we should do with nsBulletFrame. Looking at things more closely, I'm pretty sure that we need to handle it somehow, since it seems to be separate from nsImageLoadingContent.

If we want to support animated bullet frames, but they're uncommon, a decent approach would be to handle them like XUL for now, and file a followup bug to register them with their documents.
Oh, I just realized something big about patch 2 that was bouncing around my head earlier but was somehow never expressed.

Given that StartAnimation is a no-op if we don't have more than 1 frame yet, we need some way to make sure the StartAnimation triggered by IncrementAnimationConsumers() gets applied with the time is right. My guess is that we need something similar to mDecodedRequested in imgRequest, but to apply it in OnStopFrame. So patch #2 is going to have to handle that as well.
Made changes, carried over r+, requested sr.
Attachment #468555 - Attachment is obsolete: true
Attachment #468876 - Flags: superreview?(bzbarsky)
Attachment #468876 - Flags: review+
(In reply to comment #55)
> Oh, I just realized something big about patch 2 that was bouncing around my
> head earlier but was somehow never expressed.
> 
> Given that StartAnimation is a no-op if we don't have more than 1 frame yet, we
> need some way to make sure the StartAnimation triggered by
> IncrementAnimationConsumers() gets applied with the time is right. My guess is
> that we need something similar to mDecodedRequested in imgRequest, but to apply
> it in OnStopFrame. So patch #2 is going to have to handle that as well.

I believe this is really the same issue as with making the checks&returns in StartAnimation into assertions (the old code fixed the issue you just mentioned, using multiple calls into StartAnimation).

How about this solution: Rename StartAnimation into EnsureAnimation. EnsureAnimation will be called in every call to IncrementAnimationConsumers, and from a few existing places in RasterImage.cpp (like when you get to 2 frames, or know more about the animation type, etc.). EnsureAnimation's stated goal will be to ensure that the animation is running, if there should be one, and so the old checks&returns are appropriate.
> 1-that nsImageLoader is only used for CSS images,

They're only used for background images and border images.  Note that it doesn't do any actual loading anymore, right?

> 2-that it's ok that we don't animate images for border-image,
> list-style-image, and cursor-image

Why?  At least the first two seem broken to me if not animating; we certainly go to some pain to make it work.  I doubt animating cursors works right now (nothing in place to trigger a repaint as it animates, right?).

> Also, I'd like to figure out what we should do with nsBulletFrame

Do in what sense?

> since it seems to be separate from nsImageLoadingContent.

Yes, it gets its image from the style data; the actual load is performed from CSS code.

> If we want to support animated bullet frames, but they're uncommon

We do, and image bullets in general are uncommon.

I have a hard time believing the part numbering on the attachments is correct.  Is it?  I'd like to at least understand that before trying to review them.

Also, is there a description at this point of what the patches are trying to implement?  Or do I just need to read through the whole bug to sort that out?
And in particular, none of the non-obsolete patches on this bug seem to have _anything_ to do with the patch I reviewed for this bug earlier.  What's going on?  Did we decide to take some other approach?  Is this last bunch of patches supposed to apply on top of something else?  Did we mutate the bug?
(In reply to comment #59)
> And in particular, none of the non-obsolete patches on this bug seem to have
> _anything_ to do with the patch I reviewed for this bug earlier.  What's going
> on?  Did we decide to take some other approach?

Yes. Given bug 512260, it didn't make sense to have a duplicate registrar of images listening for this type of thing.
To summarize the last set of comments: Meanwhile some patches landed that handle keeping track of Images in Documents. Their purpose was for locking those images. But bholley thought it might be useful for this patch as well, both in terms of making the patch shorter, and more aligned with the (just-changed) Image code.

Basically it is possible to, when something happens to a document - like it becomes active, or goes in the bfcache - to do an operation on all the Images in it. The patches that landed locked images when the tab became active. bholley's idea was to tell images in documents that are sent into the bfcache that their animation is no longer needed. The same list of Images per Document is used in both cases.

The benefit of this approach is that images no longer need to track their refresh managers, as the previous approach did. So less code and less memory is used. Also no polling of refresh managers, so probably less CPU is used.
Aha.  Thank you for explaining that!

I assume the actual order of those three patches, as you plan to land them, is 2, 3, 1?
(In reply to comment #57)
> (In reply to comment #55)

> I believe this is really the same issue as with making the checks&returns in
> StartAnimation into assertions (the old code fixed the issue you just
> mentioned, using multiple calls into StartAnimation).
> 
> How about this solution: Rename StartAnimation into EnsureAnimation.
> EnsureAnimation will be called in every call to IncrementAnimationConsumers,
> and from a few existing places in RasterImage.cpp (like when you get to 2
> frames, or know more about the animation type, etc.). EnsureAnimation's stated
> goal will be to ensure that the animation is running, if there should be one,
> and so the old checks&returns are appropriate.

Great idea. Though a name like EnsureAnimation is misleading, since it doesn't actually ensure anything, it just MaybeEnsures it. ;-)

So how about making a method called Image::EvaluteAnimation(), which does something like this:

shouldBeAnimating = (mAnimationConsumers > 0) && (have the stuff to animate)
if (shouldBeAnimation == isAnimating) return
if (shouldBeAnimating) StartAnimation(), else StopAnimation()

then call EvaluateAnimation() after increment/decrementing mAnimationConsumers, and in the appropriate places in RasterImage.

This will mean that we can remove those "maybe-not" checks from StartAnimation() and StopAnimation() (and replace them with asserts), and have them just do what they say they do.
(In reply to comment #62)
> I assume the actual order of those three patches, as you plan to land them, is
> 2, 3, 1?

Hmm, I didn't think much about the order - thought they would all be landed together. The main reason for splitting up into these 3 is to make reviewing easier.

#1 does some cleanup of the Start|StopAnimation methods that are used in #2 and #3. That's why I put it first. But I guess it could be last, since I don't think it changes something the others depend on or vice versa.
Landed together as in one changeset?  Or landed together as in one push of three changesets?

If the latter, then the ordering I said seems to be the only one that won't break compiles during bisect, unless I'm missing something?
(In reply to comment #65)
> Landed together as in one changeset?  Or landed together as in one push of
> three changesets?
> 
> If the latter, then the ordering I said seems to be the only one that won't
> break compiles during bisect, unless I'm missing something?

You're correct.
(In reply to comment #58)

> > 2-that it's ok that we don't animate images for border-image,
> > list-style-image, and cursor-image
> 
> Why?  At least the first two seem broken to me if not animating; we certainly
> go to some pain to make it work.  I doubt animating cursors works right now
> (nothing in place to trigger a repaint as it animates, right?).
> 

Ok. In this case we'll need to add layout/style code to register those types of images with the document. Note that this shouldn't be particularly difficult - dbaron said they'd be a fair bit easier than bug 512260 parts 3 and 4. I'm down to help do it, but I'm pretty swamped for the next couple of days.


> > Also, I'd like to figure out what we should do with nsBulletFrame
> 
> Do in what sense?
> 
> > since it seems to be separate from nsImageLoadingContent.
> 
> Yes, it gets its image from the style data; the actual load is performed from
> CSS code.
> 
> > If we want to support animated bullet frames, but they're uncommon
> 
> We do, and image bullets in general are uncommon.

So the easy answer here is just to IncrementAnimationConsumers() here, as in the XUL case. If we want to be able to stop animation on these too, we'd need to add code to register/unregister the images with the Document at the appropriate times.
> I'm down to help do it, but I'm pretty swamped for the next couple of days.

OK.  I'm actually a bit surprised we have no tests for that stuff (using MozReftestInvalidate and images that animate from one state to another and then stop or something).

> So the easy answer here is just to IncrementAnimationConsumers() here

Makes sense to me.
(In reply to comment #65)
> Landed together as in one changeset?  Or landed together as in one push of
> three changesets?
> 
> If the latter, then the ordering I said seems to be the only one that won't
> break compiles during bisect, unless I'm missing something?

I meant to fold them all into one patch for pushing - separate patches was just to simplify review. But, given the last few comments I will need to rewrite all 3 patches, in ways that intersect, so I will abandon the separate patches.
OK, given that are you still waiting on my review?
(In reply to comment #70)
> OK, given that are you still waiting on my review?

I will have a new patch up in a few minutes. Please ignore the existing ones.
Attached patch Patch v9 (obsolete) — Splinter Review
Updated patch.

Moved to EvaluateAnimation(). This required various small changes in the image code. But now the StartAnimation/StopAnimation calls do exactly what their name says.

Handles the case of deferred animation consumers calls, to parallel recent changes in image locking to handle deferred locks.

Added an IncrementAnimationConsumers to BulletFrame, following the last comments.
Attachment #468557 - Attachment is obsolete: true
Attachment #468558 - Attachment is obsolete: true
Attachment #468876 - Attachment is obsolete: true
Attachment #469131 - Flags: superreview?(bzbarsky)
Attachment #469131 - Flags: review?(bzbarsky)
Attachment #469131 - Flags: review?(bobbyholley+bmo)
Attachment #468558 - Flags: review?(bzbarsky)
Attachment #468558 - Flags: review?(bobbyholley+bmo)
Attachment #468876 - Flags: superreview?(bzbarsky)
Comment on attachment 469131 [details] [diff] [review]
Patch v9

Maybe call the nsIDocument API SetImagesNeedAnimating?

>+++ b/content/base/src/nsDocument.cpp
>+  if ((oldCount == 0) && mAnimatingImages) {

Please don't overparenthesize.

>@@ -8018,16 +8026,21 @@ nsDocument::RemoveImage(imgIRequest* aIm
>+  if ((count == 0) && mAnimatingImages)

Likewise.

>+PLDHashOperator AnimateEnumerator(imgIRequest* aKey,

This is probably fine....

>+PLDHashOperator UnanimateEnumerator(imgIRequest* aKey,

StopAnimationEnumerator?  And call the other StartAnimationEnumerator by analogy?

>+++ b/layout/base/nsPresShell.cpp
>+  nsresult UpdateImageAnimatingState();

And call this UpdateImagesNeedAnimating?

>+ * Determines the current image animating state. Called when one of the
>+ * dependent factors changes.

I have no idea what this comment means....

Why not just make the nsIDocument calls from Freeze() and Thaw() directly?

Further, a document already knows when its going into bfcache or coming out of it.  Why does this have to go via the presshell and new nsIDocument API at all?

I didn't review the imagelib changes all that carefully.

"decement" is not a word.  Fix that as needed?

Why do we need separate deferred and nondeferred consumer counts?  Can't we just have a single count?  What's the benefit of having both?

This patch breaks animating border-images but nothing else, right?  Is there a plan to fix that?
Comment on attachment 469131 [details] [diff] [review]
Patch v9

>diff --git a/modules/libpr0n/src/Image.cpp b/modules/libpr0n/src/Image.cpp
>--- a/modules/libpr0n/src/Image.cpp
>+++ b/modules/libpr0n/src/Image.cpp
>@@ -37,17 +37,19 @@
> 
> #include "Image.h"
> 
> namespace mozilla {
> namespace imagelib {
> 
> // Constructor
> Image::Image(imgStatusTracker* aStatusTracker) :
>-  mInitialized(PR_FALSE)
>+  mAnimationConsumers(0),
>+  mInitialized(PR_FALSE),
>+  mAnimating(PR_FALSE)

Good idea adding mAnimating. Just as a note - you'll be racing with dholbert's <img src="foo.svg"> (bug 276431) which adds VectorImage : public Image, so make sure the patch builds and works before pushing it.

>+
>+void
>+Image::EvaluateAnimation()
>+{
>+  if (!mAnimating && ShouldAnimate()) {
>+    mAnimating = NS_SUCCEEDED(StartAnimation());

I'd prefer to store an rv and make the NS_SUCCEEDED() check a separate line.

>+nsresult
> RasterImage::StartAnimation()
> {
>   if (mError)
>     return NS_ERROR_FAILURE;
> 
>-  if (mAnimationMode == kDontAnimMode || 
>-      (mAnim && (mAnim->timer || mAnim->animating)))
>-    return NS_OK;
>+  if (!ensureAnimExists())
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  NS_ASSERTION(ShouldAnimate(), "Should not animate!");
>+
>+  NS_ASSERTION(mAnim && !mAnim->timer, "Anim must exist and not have a timer yet");

NS_ABORT_IF_FALSE on both of these. We shouldn't be adding new NS_ASSERTIONs anywhere in mozilla.

Also, move the ShouldAnimate() assertion before the call to ensureAnimExists() - calling ensureAnimExists() when we shouldn't animate could crash, so we should check our state first.

>+  // Default timeout to 100: the timer notify code will do the right
>+  // thing, so just get that started.
>+  PRInt32 timeout = 100;
>+  imgFrame *currentFrame = GetCurrentImgFrame();
>+  if (currentFrame) {
>+    timeout = currentFrame->GetTimeout();
>+    if (timeout <= 0) // -1 means display this frame forever
>+      return NS_OK;

3 comments:
1 - I know it was wrong in the old code, but this check should be a strict inequality (timeout < 0).
2 - In this case, we should also set mAnimationFinished (see below), otherwise we'll get confused as to why we thought we should have been animating and but aren't. 
3 - We should return something other than NS_OK, because EvaluateAnimation uses the return value to set mAnimating, and we don't want to set mAnimating

> RasterImage::StopAnimation()
> {
>+  NS_ASSERTION(!ShouldAnimate(), "Should not need to animate!");

NS_ABORT_IF_FALSE, and check mAnimating too.

>+
>   if (mError)
>     return NS_ERROR_FAILURE;
> 
>   if (mAnim) {

We should have mAnimating == true here, so no need to check mAnim anymore.

>@@ -1167,36 +1150,34 @@ RasterImage::ResetAnimation()
>+  PRBool oldAnimating = mAnimating;
>+  mResettingAnimation = PR_TRUE;
>+  EvaluateAnimation(); // Will stop it, if we were animating

>-    return StartAnimation();
>+  mResettingAnimation = PR_FALSE;
>+  EvaluateAnimation(); // Will restart it, if we were animating

This seems unnecessarily complicated. Why not just do:

if (mAnimating)
  StopAnimation();
if (ShouldAnimate())
  StartAnimation();

It will involve the removal of the !ShouldAnimate() assertion in StopAnimation(), but I don't think that's a big deal.

>@@ -1406,23 +1387,24 @@ NS_IMETHODIMP
> RasterImage::Notify(nsITimer *timer)
> {
>   // This should never happen since the timer is only set up in StartAnimation()
>   // after mAnim is checked to exist.
>   NS_ENSURE_TRUE(mAnim, NS_ERROR_UNEXPECTED);
>   NS_ASSERTION(mAnim->timer == timer,
>                "RasterImage::Notify() called with incorrect timer");

While you're at it, can you make the above block go like this?
assert mAnim
assert timer
assert mAnim->timer == timer
(all fatal assertions of course)

> 
>   nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
>   if (!observer) {
>     // the imgRequest that owns us is dead, we should die now too.
>-    StopAnimation();
>+    mAnimationCancelled = PR_TRUE;
>+    EvaluateAnimation();

If the imgRequest is dead, we should have no more animation consumers, because imgRequest is the link between Image and imgRequestProxy. So what I think we should do here is:
assert mAnimationConsumers == 0
if (mAnimating)
  StopAnimation();

>@@ -1435,17 +1417,18 @@ RasterImage::Notify(nsITimer *timer)
>   // finished decoding (see EndFrameDecode)
>   if (mAnim->doneDecoding || 
>       (nextFrameIndex < mAnim->currentDecodingFrameIndex)) {
>     if (mFrames.Length() == nextFrameIndex) {
>       // End of Animation
> 
>       // If animation mode is "loop once", it's time to stop animating
>       if (mAnimationMode == kLoopOnceAnimMode || mLoopCount == 0) {
>-        StopAnimation();
>+        mAnimationCancelled = PR_TRUE;
>+        EvaluateAnimation();

This changes behavior, because it means that new consumers to finite-length animations will get zero loops instead of one. But joe and I decided that this is what we want. Yay!

>- * with StartAnimation().
>+ * with EnsureAnimation().
>- * timer calls Notify when the specified frame delay time is up.
>+ * EnsureAnimation() checks if animating is required, and if so calls

It's not called that anymore, right?

>+  // Whether the animation can stop, due to running out
>+  // of frames, or no more owning request
>+  PRPackedBool               mAnimationCancelled:1;

Given how it's used, I would prefer to call this "mAnimationFinished".

>diff --git a/modules/libpr0n/src/imgRequest.cpp b/modules/libpr0n/src/imgRequest.cpp
>--- a/modules/libpr0n/src/imgRequest.cpp
>+++ b/modules/libpr0n/src/imgRequest.cpp
>@@ -301,22 +301,16 @@ nsresult imgRequest::RemoveProxy(imgRequ
>   // Let the status tracker do its thing before we potentially call Cancel()
>   // below, because Cancel() may result in OnStopRequest being called back
>   // before Cancel() returns, leaving the image in a different state then the
>   // one it was in at this point.
> 
>   imgStatusTracker& statusTracker = GetStatusTracker();
>   statusTracker.EmulateRequestFinished(proxy, aStatus, !aNotify);
> 
>-  if (mImage && !HaveProxyWithObserver(nsnull)) {
>-    LOG_MSG(gImgLog, "imgRequest::RemoveProxy", "stopping animation");
>-
>-    mImage->StopAnimation();
>-  }

I'd like to be super-sure we're not screwing something up here. So I'd like to add a getter for mAnimationConsumers on Image, and then assert (HaveProxyWithObservers(nsnull) || !mImage || (mImage->GetAnimationConsumers() == 0)) here.


Overall, this patch is a big step forward for imagelib - I appreciate you working on it

2 general comments:
-We can't land this until we add code to register border-image and list-style image with the Document. It's probably worth making that a separate patch.
-This will need tests.
Attachment #469131 - Flags: review?(bobbyholley+bmo) → review-
Why do we need to register list-style image?
(In reply to comment #73)
> Comment on attachment 469131 [details] [diff] [review]
> Patch v9

> StopAnimationEnumerator?  And call the other StartAnimationEnumerator by
> analogy?

I'd be against this, because the whole change here is that it's not actually stopping animation. So it seems analogous to saying "delete" when we really mean "NS_RELEASE()".


> Why do we need separate deferred and nondeferred consumer counts?  Can't we
> just have a single count?  What's the benefit of having both?

We don't really need both, I don't think. The one upside is that it makes it more explicit what's going on, though I think it should go away. I'd still like to keep that part of the patch as-is though, because it mirrors what we do for locks (so it'll make it easier to change them both later).

> This patch breaks animating border-images but nothing else, right?  Is there a
> plan to fix that?

See my previous comment.
(In reply to comment #75)
> Why do we need to register list-style image?

Because you said we care about them animating? When we stop calling StartAnimation from nsImageLoader, all the CSS images will need to be registered with their documents in order to get animation. Unless I'm missing something...
> I'd be against this,

OK, I thought you might.  But Animate/Unanimate are pretty much meaningless...  (and the latter isn't very good English).  What's a better name that makes sense?

> I'd still like to keep that part of the patch as-is though

OK.  It's your module.  I just wasn't sure why we wanted to pay the memory cost....

Would it make sense to make those members a union?

> Because you said we care about them animating?

Yes, but nsBulletFrame makes sure of that for actual list bullets in the attached patch, and I trust that the XUL changes handle it for whatever XUL does with list-style-image.  list-style-image images do not go through nsImageLoader, iirc; the only things that do are border images and background images.  So the change to nsImageLoader doesn't affect them.
(In reply to comment #78)
> > I'd be against this,
> 
> OK, I thought you might.  But Animate/Unanimate are pretty much meaningless... 
> (and the latter isn't very good English).  What's a better name that makes
> sense?

{Start,Stop}AnimationConsumptionEnumerator? {Acquire/Release}AnimationEnumerator? Neither of them are great...

> 
> > I'd still like to keep that part of the patch as-is though
> 
> OK.  It's your module.  I just wasn't sure why we wanted to pay the memory
> cost....

Ok, you've successfully shamed me into fixing it. I just filed bug 590838 and posted a patch. Alon - can you base your patch on top of it, and follow the (much simpler) convention there instead? Sorry for all the trouble...’ 

> 
> > Because you said we care about them animating?
> 
> Yes, but nsBulletFrame makes sure of that for actual list bullets in the
> attached patch, and I trust that the XUL changes handle it for whatever XUL
> does with list-style-image.  list-style-image images do not go through
> nsImageLoader, iirc; the only things that do are border images and background
> images.  So the change to nsImageLoader doesn't affect them.

Ah I see - so nsBulletFrame takes care of those. In that case, all that needs doing is border-image.
Given the need to write good tests, let's move this out to beta 6.
blocking2.0: beta5+ → beta6+
Depends on: 590838
Attached patch Patch v10 (obsolete) — Splinter Review
Updated patch following comments.

> Further, a document already knows when its going into bfcache or coming out of
it.  Why does this have to go via the presshell and new nsIDocument API at all?

I can't find out how to do that in the Document - how would that be done?

>+    if (timeout <= 0) // -1 means display this frame forever
>
> I know it was wrong in the old code, but this check should be a strict
inequality (timeout < 0).

Changed, but are you certain the comment was right and not the code? Maybe there are gif files out there that depend on the current code...

> We can't land this until we add code to register border-image [..] with the Document. It's probably worth making that a separate patch.

A separate patch in this bug, or do we want a new bug for it?

> This will need tests.

What kind of tests are we thinking about here (C++, Mochitest, reftest, other)? And do we have a list of what should be tested?

> But Animate/Unanimate are pretty much meaningless... (and the latter isn't very good English)

How about Increment|DecrementAnimationEnumerator? (used in the latest patch)
Attachment #469131 - Attachment is obsolete: true
Attachment #469722 - Flags: superreview?(bzbarsky)
Attachment #469722 - Flags: review?(bzbarsky)
Attachment #469722 - Flags: review?(bobbyholley+bmo)
Attachment #469131 - Flags: superreview?(bzbarsky)
Attachment #469131 - Flags: review?(bzbarsky)
> I can't find out how to do that in the Document - how would that be done?

nsDocument::OnPageHide for going into the cache and OnPageShow for coming out of it.  You can use the first argument to determine whether this is a bfcache call or not.

> How about Increment|DecrementAnimationEnumerator?

Seems ok to me.
bholley: Turns out that assertion you asked for,

+  NS_ABORT_IF_FALSE(HaveProxyWithObserver(nsnull) || !mImage ||
+                    mImage->GetAnimationConsumers() == 0,
+                    "Patch in bug 359608 has failed");

fails - there is an mImage with positive AnimationConsumers. What do we want to do there?
(In reply to comment #83)
> bholley: Turns out that assertion you asked for,
> 
> +  NS_ABORT_IF_FALSE(HaveProxyWithObserver(nsnull) || !mImage ||
> +                    mImage->GetAnimationConsumers() == 0,
> +                    "Patch in bug 359608 has failed");
> 
> fails - there is an mImage with positive AnimationConsumers. What do we want to
> do there?

We should investigate it.

First, make sure that when that assertion is fired, we still do have at least one proxy (just not one with an observer). If we don't have any proxies, then something is messed up with the refcounting, because each proxy should remove its animationConsumers before dying.

Assume all is well with that, figure out who is calling IncrementAnimationConsumers() but passing a null observer. Whoever that is won't be notified of animation frame changes, so presumably they don't actually care about animation.

Ideally, we'd fix this by altering the culprit consumer and stopping them from asking for animation when they're not going to listen for it. However, if it turns out that's too tricky, I'd also accept a compromise of replacing HaveProxyWithObserver(nsnull) with "mObserver.Length() > 0" in the assert (ie, do we have any observers at all). But only once we figure out who the culprit is.
Attached patch Patch v11 (obsolete) — Splinter Review
The issues with that assertion are that

1. nsDocument::AddImage calls imgRequestProxy::IncrementAnimationConsumers, and this adds animation consumers on all proxies, even if they don't have an observer.
2. Aside from that, imgRequest::RemoveProxy removes the proxy while it is still active, then that assertion checks if there is |a proxy without an observer, and animation consumers > 0|, which should not occur. But if the just-removed proxy was the only one with an observer, the imgRequest now doesn't think it has a proxy with observer, but the animation consumer count still reflects the just-removed proxy. In other words the proxy has not yet cleaned up its animation consumers.

To fix this, proxies now ignore animation consumers if they do not have an observer, and if they lose their observer they lose them. Also their observer is nullified when the request removes them. This required moving NullOutListener from protected to public on proxy. I also added various NS_ABORT_IF_FALSE.

Other changes in the patch: The last issue bz mentioned, with OnPageShow/Hide.

I am still not sure what to do about border-image (are we addressing it in this bug? if so how?) and about tests (which do we want, and which kind?).
Attachment #469722 - Attachment is obsolete: true
Attachment #470667 - Flags: review?(bobbyholley+bmo)
Attachment #469722 - Flags: superreview?(bzbarsky)
Attachment #469722 - Flags: review?(bzbarsky)
Attachment #469722 - Flags: review?(bobbyholley+bmo)
Comment on attachment 470667 [details] [diff] [review]
Patch v11

>- * with StartAnimation().
>+ * with EnsureAnimation().

>- * StartAnimation() checks if animating is allowed, and creates a timer.  The
>- * timer calls Notify when the specified frame delay time is up.
>+ * EnsureAnimation() checks if animating is required, and if so calls
>+ * StartAnimation().

See comment 74.


>+printf("notify! %d\n", PR_Now());
>+

I'm guessing you didn't mean to include that.


>@@ -123,6 +124,11 @@ nsresult imgRequestProxy::Init(imgRequest* request, nsILoadGroup* aLoadGroup, Im
> 
>   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequestProxy::Init", "request", request);
> 
>+  // If we have animation consumers, but will not have an observer, then they don't matter
>+  if (!aObserver)
>+    while (mAnimationConsumers > 0)
>+      DecrementAnimationConsumers();

>+  if (!HasObserver()) {
>+    NS_ABORT_IF_FALSE(mAnimationConsumers > 0,
>+                      "How can we have animation consumers without an observer?");

Why does this need to be in Init? Do we ever call IncrementAnimationConsumers() before calling Init()? Seems to me that we should just replace this with:
NS_ABORT_IF_FALSE(mAnimationConsumers == 0, ...)
and fix anybody who violates it.


>+NS_IMETHODIMP
>+imgRequestProxy::DecrementAnimationConsumers()
>+{
>+  if (!HasObserver()) {
>+    NS_ABORT_IF_FALSE(mAnimationConsumers > 0,
>+                      "How can we have animation consumers without an observer?");

Isn't this check backwards? Seems to me it should be "mAnimationConsumers == 0"

>+  // If we have animation consumers, then they don't matter anymore
>+  if (mListener)
>+    while (mAnimationConsumers > 0)
>+      DecrementAnimationConsumers();

Add braces around the outer if() block.

This is very close. r- until we get the imgRequestProxy::Init issue sorted out.
Attachment #470667 - Flags: review?(bobbyholley+bmo) → review-
> I am still not sure what to do about border-image (are we addressing it in this
> bug? if so how?)

We should address it with a patch underneath yours, along the same lines as some of the ones in bug 512260. I might be able to do it today or tomorrow depending on how things go.

>and about tests (which do we want, and which kind?).

(for posterity - we talked about this IRL)
Why is the nsIDocument change still in the last patch?  That should just be an nsDocument protected function, no?
just filed and added a patch for bug 592493 to handle the border-image case.
Attached patch Tests (obsolete) — Splinter Review
Automated tests, using browser mochitest.

An interface for accessing the internal number of animated frames is used in debug builds (and not optimized ones, so this does not add any overhead).

The tests check that

1. Animation works in general - frame timers fire.
2. Going into the bfcache pauses us, and leaving it resumes us.
3. imgContainers are shared between tabs and on the same tab, saving animation timers.
Attachment #470990 - Flags: review?(bobbyholley+bmo)
Depends on: 592493
Comment on attachment 470990 [details] [diff] [review]
Tests

Joe loves tests. Given that he's also going to need to OK the imgIContainerDebug.idl stuff, I'll let him do the review.

These tests will, paradoxically, requires super-review. ;-)
Attachment #470990 - Flags: review?(bobbyholley+bmo) → review?(joe)
Attached patch Patch v12 (obsolete) — Splinter Review
Patch fixing the issues bholley and bz mentioned in the last few comments.

About:

> 
> 
> >+NS_IMETHODIMP
> >+imgRequestProxy::DecrementAnimationConsumers()
> >+{
> >+  if (!HasObserver()) {
> >+    NS_ABORT_IF_FALSE(mAnimationConsumers > 0,
> >+                      "How can we have animation consumers without an observer?");
> 
> Isn't this check backwards? Seems to me it should be "mAnimationConsumers == 0"

Yes, I had that backwards. I just noticed now that the wrong way made it fail the existing reftests - which is comforting to know ;)
Attachment #470667 - Attachment is obsolete: true
Attachment #471001 - Flags: review?(bobbyholley+bmo)
Comment on attachment 471001 [details] [diff] [review]
Patch v12

r=bholley \o/
Attachment #471001 - Flags: review?(bobbyholley+bmo) → review+
Attached patch Tests v2 (obsolete) — Splinter Review
Fixes for bustage in optimized builds.
Attachment #470990 - Attachment is obsolete: true
Attachment #471173 - Flags: review?(joe)
Attachment #470990 - Flags: review?(joe)
Attached patch Patch v13 (obsolete) — Splinter Review
It seems that calling NullOutListener() as I previously did was dangerous, it nulled out not just the animation consumers but also the reference. This led to it being dropped, and onload not being called, leading to a few seemingly unrelated test failures.

Fixed it with a new function, ClearAnimationConsumers. Also cleaned up the code by calling that from everywhere we used to do a loop on DecrementAnimationConsumers (since that's exactly what ClearAnimationConsumers does, plus it has a proper abort-if-false to boot).

Marked bz for sr on the changes to the IDLs.
Attachment #471001 - Attachment is obsolete: true
Attachment #471346 - Flags: superreview?(bzbarsky)
Attachment #471346 - Flags: review?(bobbyholley+bmo)
Comment on attachment 471173 [details] [diff] [review]
Tests v2


>diff --git a/modules/libpr0n/src/RasterImage.cpp b/modules/libpr0n/src/RasterImage.cpp

>+#ifndef DEBUG
> NS_IMPL_ISUPPORTS4(RasterImage, imgIContainer, nsITimerCallback, nsIProperties,
>                    nsISupportsWeakReference)
>+#else
>+NS_IMPL_ISUPPORTS5(RasterImage, imgIContainer, nsITimerCallback, nsIProperties,
>+                   imgIContainerDebug, nsISupportsWeakReference)
>+#endif

[rest of RasterImage elided]

There are a ton of these ifdefs - is there a way we could make RasterImage a nsIInterfaceRequestor instead, and call GetInterface on it to get an imgIContainerDebug? Then we could just put all the ifdefs in one place.


>diff --git a/modules/libpr0n/test/browser/browser_image.js b/modules/libpr0n/test/browser/browser_image.js

>+      // Let animation run for a bit
>+      gTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+      gTimer.initWithCallback(function() {
>+        is(gImage.framesNotified > 5, true, "Must have animated a few frames so far");

I'm quite concerned that load problems on any VM unittest machines we have could make this test fail intermittently. Can you make it requeue an event - say, 5 times - to try a poor man's busywait on this?


>+        gFrames = gImage.framesNotified;
>+
>+        // Browse elsewhere; push our animating page into the bfcache
>+        gBrowser.loadURI("about:blank");
>+
>+        // Wait a while and see no animation occurred
>+        gTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+        gTimer.initWithCallback(function() {
>+          // Might have a few stray frames, until other page totally loads
>+          is((gImage.framesNotified - gFrames) < 5, true, "Must have not animated in bfcache!");

Shouldn't this be framesNotified == gFrames?


>+          gFrames = gImage.framesNotified;
>+
>+          // Go back
>+          gBrowser.goBack();
>+
>+          // Animation should resume
>+          gTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+          gTimer.initWithCallback(function() {
>+            is((gImage.framesNotified - gFrames) > 5, true, "Must have animated once out of bfcache!");

Same comment as above regarding busywaiting.


>+              if (theFrames == null) {
>+                theFrames = frames;
>+              } else {
>+                is(theFrames, frames, "Sharing the same imgContainer means *exactly* the same frame counts!");
>+              }
>+            });
>+            is((theFrames - gFrames) > 10, true, "Must have been animating all this time");

Same comment regarding busywait.
Attachment #471173 - Flags: review?(joe) → review-
Comment on attachment 471346 [details] [diff] [review]
Patch v13

 nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBool aNotify)
 {
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::RemoveProxy", "proxy", proxy);
 
+  NS_ABORT_IF_FALSE(!mImage || HaveProxyWithObserver(nsnull) ||
+                    mImage->GetAnimationConsumers() == 0,
+    "How can we have an image with animation consumers, but no observer?");
+
+  if (proxy->HasObserver()) {
+    // This will remove our animation consumers, so after removing
+    // this proxy, we don't end up with no proxies with observers but with
+    // animation consumers.
+    proxy->ClearAnimationConsumers();
+  }
+

I don't think this should be conditional. Just do proxy->ClearAnimationConsumers(), which should be a no-op in the no-observer case.

-  if (mImage && !HaveProxyWithObserver(nsnull)) {
-    LOG_MSG(gImgLog, "imgRequest::RemoveProxy", "stopping animation");
-
-    mImage->StopAnimation();
-  }
+  NS_ABORT_IF_FALSE(!mImage || HaveProxyWithObserver(nsnull) ||
+                    mImage->GetAnimationConsumers() == 0,
+    "How can we have an image with animation consumers, but no observer?");

This is a duplicate of the above assert.

+  if (mListener) {
+    ClearAnimationConsumers();
+  }

No need for the {} here.

r=bholley on the imagelib bits with those changes.
Attachment #471346 - Flags: review?(bobbyholley+bmo) → review+
Attached patch Tests v3 (obsolete) — Splinter Review
Made the changes in the last comments and in irc chat.
Attachment #471173 - Attachment is obsolete: true
Attachment #471725 - Flags: review?(joe)
Attached patch Patch v14 (obsolete) — Splinter Review
Fixes for the comments.

Regarding:

> 
> -  if (mImage && !HaveProxyWithObserver(nsnull)) {
> -    LOG_MSG(gImgLog, "imgRequest::RemoveProxy", "stopping animation");
> -
> -    mImage->StopAnimation();
> -  }
> +  NS_ABORT_IF_FALSE(!mImage || HaveProxyWithObserver(nsnull) ||
> +                    mImage->GetAnimationConsumers() == 0,
> +    "How can we have an image with animation consumers, but no observer?");
> 
> This is a duplicate of the above assert.

It is, but it is on purpose. We change two things between them -
remove a proxy, and clear a proxy's consumers. I think it makes
sense to check for valid state after that as well. I added
a comment explaining why in the source (hopefully a convincing one ;)

--

One other change here: Fix a reftest crash in opt builds. Since we purposefully clear out the animation consumers in imgRequest::RemoveProxy (see previous comments), we may end up with a case where Decrement is legitimately called but there are no consumers. So now the patch checks for that.
Attachment #471346 - Attachment is obsolete: true
Attachment #471727 - Flags: review?(bobbyholley+bmo)
Attachment #471346 - Flags: superreview?(bzbarsky)
Comment on attachment 471727 [details] [diff] [review]
Patch v14

I wish we didn't have to remove that assertion, but I can't think of an easy way to fix it offhand.

r=bholley. You should probably push this with its dependencies (bug 592493, bug 590838, and the tests) for a full try run, because I didn't push either of the other two bugs to try.
Attachment #471727 - Flags: review?(bobbyholley+bmo) → review+
Attachment #471727 - Flags: superreview?(bzbarsky)
Comment on attachment 471727 [details] [diff] [review]
Patch v14

You need to rev the imgIRequest iid.  And put the new methods at the end of the interface?  Also, rev the imgIContainer iid.

sr=me with those changes.  Let's hope there are no binary extensions using this stuff...
Attachment #471727 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #101)
> Hmm it looks like we might be slowing down Talos, in particular the shutdown
> tests. Maybe the loop in ClearAnimationConsumers could be optimized into a
> single op, but aside from that I don't have any immediate ideas. Thoughts?

The noise margins for the startup/shutdown talos tests are pretty high. My experience of late has been that perceived improvements/regressions in those areas have not amounted to dev-tree-management emails when I land the same changes on trunk.

So I'd give it a try, especially over the weekend on a quiet tree if you're so inclined.
Try adding a few more base revisions to compare against so you can get a better idea of what the normal fluctuations are.
Attached patch Tests v4Splinter Review
Small fix to tests, to pass properly in optimized builds.
Attachment #471725 - Attachment is obsolete: true
Attachment #472779 - Flags: review?(joe)
Attachment #471725 - Flags: review?(joe)
Attached patch Patch v16Splinter Review
Undid the loop optimization as per bholley's request. Carried r+,sr+.
Attachment #472774 - Attachment is obsolete: true
Attachment #472787 - Flags: superreview+
Attachment #472787 - Flags: review+
Attachment #472774 - Flags: review?(bobbyholley+bmo)
Comment on attachment 472779 [details] [diff] [review]
Tests v4

I didn't look at these in any detail, but they appear to busywait, which was joe's only standing review concern. Assuming that pass with this patch and don't pass without it, r=bholley.
Attachment #472779 - Flags: review?(joe) → review+
Pushed to mozilla-central:
Patch: http://hg.mozilla.org/mozilla-central/rev/fd33aa85de93
Tests: http://hg.mozilla.org/mozilla-central/rev/7fda546db6a6

Resolving fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 595122
Depends on: 594771
Depends on: 595142
Depends on: 601723
Depends on: 641198
Depends on: 680022
Blocks: 674244
Depends on: 773203
You need to log in before you can comment on or make changes to this bug.