lots of animated gifs swamp us with paint events

RESOLVED FIXED in mozilla11

Status

()

RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jwir3)

Tracking

(Blocks: 5 bugs, {regression})

Trunk
mozilla11
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(15 attachments, 178 obsolete attachments)

701.12 KB, application/zip
Details
558.79 KB, application/zip
Details
253.66 KB, application/zip
Details
10.82 KB, patch
jwir3
: review+
Details | Diff | Splinter Review
36.69 KB, patch
jwir3
: review+
Details | Diff | Splinter Review
101.53 KB, image/jpeg
Details
18.51 KB, patch
joe
: review+
Details | Diff | Splinter Review
25.26 KB, patch
joe
: review+
Details | Diff | Splinter Review
6.73 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.99 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.59 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
16.77 KB, patch
roc
: review+
Details | Diff | Splinter Review
152.00 KB, text/html
Details
(Reporter)

Description

8 years ago
An example of this happening is here:

http://chat.vitebsk.ws/smile.php?name=kolobki2&jsclose=1&PHPSESSID=712fb5f997934bf52dbaf00e73fe81ea

It would be interesting to know if this a regression.
(Reporter)

Updated

8 years ago
Blocks: 595671
(Reporter)

Comment 1

8 years ago
The obvious solution is to coalesce paint events.
(Reporter)

Updated

8 years ago
Assignee: nobody → sjohnson
(Reporter)

Comment 2

8 years ago
(In reply to comment #0)
> An example of this happening is here:
> 
> http://chat.vitebsk.ws/smile.
> php?name=kolobki2&jsclose=1&PHPSESSID=712fb5f997934bf52dbaf00e73fe81ea
> 
> It would be interesting to know if this a regression.

Another side effect of not coalescing shows up on OS X. When Firefox is in the background the system throttles the number of paint events, this causes the animations to run slowly. I don't see the same problem in Chrome.
(Reporter)

Comment 3

8 years ago
We probably have a couple of different ways to solve this problem:

1. Animate GIFs using nsRefreshDriver instead of their own timers. This is probably the ideal solution, but might require a fair amount of work because I believe we'll need to be able to paint a GIF at particular time instead of just the current frame.

2. Trigger paints using nsRefreshDriver but animate the images using individual timers still. We still spin the event loop as often, but we don't try to paint really fast.

3. Coalesce invalidations. The basic idea here is to accumulate invalidation areas until a certain threshold before sending them to the OS. This is general solution to the problem, but I'm not sure it's the direction we want to go.

It would be interesting to know which solution WebKit uses.
(In reply to comment #3)
> 2. Trigger paints using nsRefreshDriver but animate the images using
> individual timers still.

This strikes me as the most straightforward.  I'm not sure we could do #1, because nsRefreshDrivers are tab-specific, whereas images aren't.
(Assignee)

Comment 5

8 years ago
Since I'm running Ubuntu right now, I added some printing code to the widget/src/gtk2/nsWindow.cpp file, in OnExposeEvent, which dholbert advised me might be the platform-dependent version of nsWindowGfx::OnPaint(). I had it print a counter to determine how much it's being called, with it printing every 10 counts. 

This was run on an intel Core i7-2820QM, with 8GB RAM. I found the following:

1) When using Nightly to view several news pages (some images, mostly text), retrieved from igoogle.com main site, over the period of ~1min resulted in about 1280 calls to OnExposeEvent().

2) When using the same version, but viewing the link in the Description, over about the same amount of time (~1min), I observed 7590 calls to OnExposeEvent(). Also, this was without any page reloads or modification of the address bar (aside from the first typing of the address in the bar). 

These are mostly just stats - I'm currently working on analysing this further and looking into a possible fix.
(Assignee)

Updated

8 years ago
Keywords: regression
(Assignee)

Comment 7

8 years ago
Posted image Timeline of the problematic situation (obsolete) —
(Assignee)

Comment 8

8 years ago
dbaron, dholbert, and I were talking this afternoon. As Daniel suggested in comment 4, we could trigger repaints using nsRefreshDriver instead of the image's Notify() routine. David pointed out a problem where we might have an out-of-sync issue with respect to some painting.

For example, consider the first attachment ( https://bugzilla.mozilla.org/attachment.cgi?id=541840 ). It is an image of two frames, one being an image animation, the other being a partially transparent frame that partially overlays the image animation. 

In the second image ( https://bugzilla.mozilla.org/attachment.cgi?id=541841 ), I've attached a possible timeline for refresh behavior. 'IA' indicates where an image's Notify() routine would normally have been called to invalidate the RasterImage, and (currently) also paint the new image in the window, and 'RD' indicates a location where the nsRefreshDriver would paint the image to the window. If we move the invalidate call into the refresh driver, then we may have a situation where the RasterImage has already been updated, thus (assuming we were previously at frame x) memory now has the frame x+1 of the image already loaded. Since the invalidate call has been delayed until the refresh driver is next triggered, the image painted on the screen is still frame x. If, however, an invalidation happens to part of the image, as could happen if an invalidate call was made to the transparent overlaying area, then we have a situation where the pink area of the image above is redrawn, but because the RasterImage has frame x+1 in it, it is redrawn as x+1, whereas the rest of the image is still at frame x until the next call to the refresh driver.

So, the question is how to overcome this. One suggestion that was in the bug is to animate images using the refresh driver instead of their own timers, thus moving all of the code into the refresh driver for repainting. As mentioned in comment 3 above, though, this could be quite difficult.

If we decide that this might be too much work for the time being, then I could look into implementing the solution mentioned where we utilize an image's own timers to control animation, but do the repainting in the refresh driver, and then try to gauge how much of a problem this will turn out to be.

Other thoughts?
Animating images off the refresh driver sounds good, but one thing that'll make it hard is that the image cache is global and the refresh driver is best per-window, so the same image can be animating in multiple windows and animating it in one window could make it animate "too early" in another window. That's probably why Jeff said "we'll need to be able to paint a GIF at particular time instead of just the current frame". Then again, we have this bug currently where an animated image loaded in two windows is forced to use the same animation timeline, which is wrong.

I'll think about it.
Maybe we could make imgRequestProxy detect animated images and forcefully create a new imgIRequest for each instance of an animated image, with the requests sharing the underlying image data but with separate decoders, so each animated image instance gets its own timeline. Then we could control the animation from the refresh driver.
(Assignee)

Comment 11

8 years ago
Here's a proposed fix that dholbert and I have been working on. It does require changes to the imgIContainer API.

Current Ownership Model:
            nsDocument
             /       \
nsRefreshDriver    nsImageFrame
                      |
                      |
  (another proxy)  imgRequestProxy   (another proxy)
          \________   |   _________________/
                   \  |  /
                   imgRequest
                      |
                   imgIContainer (RasterImage, VectorImage)
Goal: 

Have imgIContainers register with a RefreshDriver, so they can get rid of their internal timers, and so we don't get a large number of sequential invalidates & repaints on pages with a very large number of animated GIFs
NOTE: This proposed fix will require changes to the imgIContainer API.

SETUP:
At some point, when we know we're an animated image but before animation starts (maybe the first time EvaluateAnimation succeeds -- this is just after our second frame decodes, for raster images), our imgIContainer fires a callback that makes imgRequest do the following:

For each imgRequestProxy:

    Clone the imgRequest & imgIContainer, and call imgRequestProxy::ChangeOwner to make the proxy point to its own personal imgRequest-clone.
      * We need information about which instance of an animation is at which frame, so the clones keep track of this for us. Each clone would have a member variable indicating which frame that instance is currently displaying.

      * Ideally, clones should share decoded image data. Maybe they're "RasterImageWrapper" or some other lightweight friend helper class that implements imgIContainer?

    If that succeeds, the imgIContainer or imgRequest then calls some new method like imgIContainerObserver::ReadyToStartAnimating()

    This makes it to the nsImageFrame (or some similar layout object), which then registers itself (or a helper class) as a refresh observer.

SAMPLING:

    nsRefreshDriver fires a tick, triggering ::WillRefresh(timestamp) on nsImageFrame (or an analogous layout object)

    This calls ::RefreshTick(timestamp) on the imgIContainer. 

    RasterImage::RefreshTick implementation:

      * Examines timeStamp and figures out what the correct frame for that timestamp is.

      * If that's different from the current frame, we call OnFrameChanged. (this is what RasterImage::Notify does now.)

    VectorImage::RefreshTick impl:

      * Seeks internal document to the correct time.

      * That will automatically trigger invalidations if appropriate, via VectorImage::InvalidateObserver()

      * The internal SVG document can now be Pause()d (which prevents ticks from *its* RefreshDriver from affecting animations)
(In reply to comment #11)
> Goal: 
> 
> Have imgIContainers register with a RefreshDriver

Oops, I forgot to update this part -- "register" should say "register (indirectly)" there. (since it's technically the layout object that is the refresh observer, and it passes ticks down to its imgIContainer)
(In reply to comment #11)
>     Clone the imgRequest & imgIContainer, and call
> imgRequestProxy::ChangeOwner to make the proxy point to its own personal
> imgRequest-clone.

I understand that you want to be able to seek to arbitrary timestamps, but this is really a lot of extra code and complexity for something of a niche requirement.

>     This makes it to the nsImageFrame (or some similar layout object), which
> then registers itself (or a helper class) as a refresh observer.

Do refresh drivers still need to be associated with a docshell? A while ago, Boris mentioned to me that nsRefreshDriver doesn't work with image documents; is this still the case?

(For image documents, my understanding is that we have no layout objects at all.)

>     RasterImage::RefreshTick implementation:
> 
>       * Examines timeStamp and figures out what the correct frame for that
> timestamp is.
> 
>       * If that's different from the current frame, we call OnFrameChanged.
> (this is what RasterImage::Notify does now.)

As mentioned above, I think a better option would be to evaluate animation globally. This should probably be a lot simpler.
(In reply to comment #13)
> (In reply to comment #11)
> >     Clone the imgRequest & imgIContainer, and call
> > imgRequestProxy::ChangeOwner to make the proxy point to its own personal
> > imgRequest-clone.
> 
> I understand that you want to be able to seek to arbitrary timestamps, but
> this is really a lot of extra code and complexity for something of a niche
> requirement.

You mean the requirement of having different instances of an animated image have their own timelines?

> >     This makes it to the nsImageFrame (or some similar layout object), which
> > then registers itself (or a helper class) as a refresh observer.
> 
> Do refresh drivers still need to be associated with a docshell? A while ago,
> Boris mentioned to me that nsRefreshDriver doesn't work with image
> documents; is this still the case?

I'm not sure what he meant. An image document belongs to a docshell, but more importantly it has a prescontext/presshell, and I'm pretty sure it has a refresh driver.

> (For image documents, my understanding is that we have no layout objects at
> all.)

This is not true. It builds a normal HTML document containing an <img> element, and has a regular frame tree.

> >     RasterImage::RefreshTick implementation:
> > 
> >       * Examines timeStamp and figures out what the correct frame for that
> > timestamp is.
> > 
> >       * If that's different from the current frame, we call OnFrameChanged.
> > (this is what RasterImage::Notify does now.)
> 
> As mentioned above, I think a better option would be to evaluate animation
> globally. This should probably be a lot simpler.

Can you be more specific? I'm not sure what option you're referring to.
(In reply to comment #14)
> (In reply to comment #13)
> > As mentioned above, I think a better option would be to evaluate animation
> > globally. This should probably be a lot simpler.
> 
> Can you be more specific? I'm not sure what option you're referring to.

I'm guessing that joe's suggesting that we replace the per-RasterImage timers with a single global timer, whose callback synchronously calls RasterImage::Notify on all the animated images in the image-cache.

That would batch invalidations, which could indeed be a simpler & more targeted solution for this bug's specific perf issues...
(In reply to comment #14)
> > I understand that you want to be able to seek to arbitrary timestamps, but
> > this is really a lot of extra code and complexity for something of a niche
> > requirement.
> 
> You mean the requirement of having different instances of an animated image
> have their own timelines?

joe: note also that having per-instance (or per-document) image timelines would have the benefit of fixing things like bug 663800, too.
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > >     Clone the imgRequest & imgIContainer, and call
> > > imgRequestProxy::ChangeOwner to make the proxy point to its own personal
> > > imgRequest-clone.
> > 
> > I understand that you want to be able to seek to arbitrary timestamps, but
> > this is really a lot of extra code and complexity for something of a niche
> > requirement.
> 
> You mean the requirement of having different instances of an animated image
> have their own timelines?

Right. (That's what I meant by "global" too.) That requirement seems completely orthogonal to paint events.

I don't think we should work towards different timelines for different instances of images right now, and possibly not at all.  (The simplest and maybe best way to do this would be to duplicate animated images, rather than sharing the image data, imo.)

> I'm not sure what he meant. An image document belongs to a docshell, but
> more importantly it has a prescontext/presshell, and I'm pretty sure it has
> a refresh driver.
> 
> > (For image documents, my understanding is that we have no layout objects at
> > all.)
> 
> This is not true. It builds a normal HTML document containing an <img>
> element, and has a regular frame tree.

Good. I'm glad to be wrong here! :)
> A while ago, Boris mentioned to me that nsRefreshDriver doesn't work with
> image documents; is this still the case?

I have no idea what that's about... I can only assume that either I misspoke or you misunderstood or both....

One issue we've had with refresh driver for animations in the past that hasn't been brought up here is that if an image is supposed to change frames every 250ms then running that off the 60Hz refresh driver timer is pretty wasteful.  Not sure what we can do about it or whether we should worry about it, but that's why Alon hadn't tried to hook up animations to refresh driver in the past.  Another issue is that weird things could happen in background tabs (e.g. we might need to suddenly run though a lot of the animation when switching to that tab.

There's some related discussion in bug 571394, by the way.
And in particular, see the discussion there about image animations and grouping their timers.
(In reply to comment #17)
> I don't think we should work towards different timelines for different
> instances of images right now, and possibly not at all.  (The simplest and
> maybe best way to do this would be to duplicate animated images, rather than
> sharing the image data, imo.)

Sharing timelines is a bug, e.g. bug 663800.

A single global timer would fix this particular bug more easily, but it wouldn't fix bug 663800, and wouldn't give us other benefits of refresh driver control such as throttling image animation in hidden tabs.

(In reply to comment #18)
> One issue we've had with refresh driver for animations in the past that
> hasn't been brought up here is that if an image is supposed to change frames
> every 250ms then running that off the 60Hz refresh driver timer is pretty
> wasteful.  Not sure what we can do about it or whether we should worry about
> it, but that's why Alon hadn't tried to hook up animations to refresh driver
> in the past.

I think we could extend nsRefreshDriver::AddRefreshObserver to take a delay parameter, and allow up to that much time to elapse before the refresh driver timer fires.

> Another issue is that weird things could happen in background
> tabs (e.g. we might need to suddenly run though a lot of the animation when
> switching to that tab.

We could add another parameter to AddRefreshObserver (or a new method) to let an observer be called at low frequency (1Hz?) for background tabs.
(In reply to comment #20)
> (In reply to comment #17)
> > I don't think we should work towards different timelines for different
> > instances of images right now, and possibly not at all.  (The simplest and
> > maybe best way to do this would be to duplicate animated images, rather than
> > sharing the image data, imo.)
> 
> Sharing timelines is a bug, e.g. bug 663800.

I see no reason why fixing that bug should require us to separate timelines.

> A single global timer would fix this particular bug more easily, but it
> wouldn't fix bug 663800, and wouldn't give us other benefits of refresh
> driver control such as throttling image animation in hidden tabs.

Sharing animated images across tabs is an edge case, and adding a huge amount of new code just to make that work more efficiently seems like the wrong trade-off to me.
> Sharing animated images across tabs is an edge case

It's the "hey, I have a few tabs open on this forum where all these people are using animated avatars and smilies" use case....
(In reply to comment #22)
> > Sharing animated images across tabs is an edge case
> 
> It's the "hey, I have a few tabs open on this forum where all these people
> are using animated avatars and smilies" use case....

True, but I still think it's pretty niche. I might be wrong.

Keep in mind that, aside from drawing (which we can/do hopefully throttle on background tabs with nsRefreshDriver), sharing animation state is _less_ work.

You just register for all your nsRefreshDrivers, then update to the time given by the callback. In the common case, the background tab won't do anything other than trigger invalidation.
(In reply to comment #21)
> > Sharing timelines is a bug, e.g. bug 663800.
> 
> I see no reason why fixing that bug should require us to separate timelines.

AFAICT the simplest approach is to separate timelines. If you have a better suggestion, I'm open to it.

> > A single global timer would fix this particular bug more easily, but it
> > wouldn't fix bug 663800, and wouldn't give us other benefits of refresh
> > driver control such as throttling image animation in hidden tabs.
> 
> Sharing animated images across tabs is an edge case, and adding a huge
> amount of new code just to make that work more efficiently seems like the
> wrong trade-off to me.

I don't think we need a huge amount of new code to do this separation.

I wasn't thinking about "sharing animated images across tabs". Suppose we have a single animated image in a hidden tab. How are you proposing we throttle its animation?
(In reply to comment #24)
> Suppose we have a single animated image in a hidden tab.
> How are you proposing we throttle its animation?

We already do something related for *closed* tabs -- we keep count of "mAnimationConsumers" on Image (RasterImage's superclass), and that count gets decremented in OnPageHide, via nsDocument::SetImagesNeedAnimating().  (Then we stop animation altogether when the count hits 0.)

So, we could potentially use a similar strategy to throttle in hidden tabs -- add a new member-var "mForegroundAnimationConsumers", and throttle (but not stop) animations whenever that hits 0.

(I'm not necessarily advocating that we do this -- just pointing it out as one way to throttle in background tabs without switching to per-document timelines.)
Yeah, we can do that.

I just feel that we're spending energy trying to come up with short-cuts to avoid fixing the problem right, for no valid reason I can see.
(In reply to comment #24)
> (In reply to comment #21)
> > > Sharing timelines is a bug, e.g. bug 663800.
> > 
> > I see no reason why fixing that bug should require us to separate timelines.
> 
> AFAICT the simplest approach is to separate timelines. If you have a better
> suggestion, I'm open to it.

Significantly simpler than that is to register for every nsRefreshDriver in every tab you're in (happens automatically via nsImageFrame or whatever), then listening for the current time (i.e., the refresh driver callback). When the time changes such that you need to change your frame, RasterImage does its compositing, then sends invalidations as normal.

All that code needs to be written anyways. The difference between this and comment 11 is that this doesn't involve writing new imgRequest implementations, doesn't require changing RasterImage to be able to share imgFrames, etc.

The only downside to this is that we don't throttle invalidations on background tabs. That's not an enormous problem; we probably need to throttle painting on background tabs anyways, because js can paint at whatever time it wants.

If, at some later time (or even now) we decide we desperately want separate timelines on separate tabs, we should cache animated images per-document instead of writing all that complicated code.
To be clear, I read roc's "the simplest approach is to separate timelines" as "the simplest approach to fix _this bug_ is to separate timelines". If that's not what roc was saying, then I was responding to something else.

But comment 27 is still how I think the simplest way to solve this bug would go.
> we probably need to throttle painting on background tabs anyways

We don't paint them at all.  But right now just the actual Invalidate() call can be pretty expensive....
When I load the URL from comment 0 in a foreground tab and let it load fully, I get 99% Firefox CPU usage and 15-20% Xorg CPU usage.  When I switch tabs, both drop to 3-5%.  (And when I close the tab, Firefox drops to 0-1%.)

So, the invalidates aren't free, but relative to painting, they're pretty cheap...
As a tangent, why do we do invalidations in background tabs? Why not just ignore them and invalidate the whole page when we bring the tab to the front?
(In reply to comment #27)
> Significantly simpler than that is to register for every nsRefreshDriver in
> every tab you're in (happens automatically via nsImageFrame or whatever),
> then listening for the current time (i.e., the refresh driver callback).
> When the time changes such that you need to change your frame, RasterImage
> does its compositing, then sends invalidations as normal.

OK. This does require imagelib changes to not use a timer and instead require explicit time advance via some new API; I assume you're OK with that?

There is also the complicating issue that we'll need to track, for each use of an image, the time offset between the refresh driver's time and the image's timeline. That code would go away if/when we switch to one animated image timeline per image instance or document.

I still don't think that plan is optimal, but it is satisfactory, so go for it.

> If, at some later time (or even now) we decide we desperately want separate
> timelines on separate tabs, we should cache animated images per-document
> instead of writing all that complicated code.

We can't do that since we don't know an image is animated until we've loaded it, right?
(In reply to comment #31)
> As a tangent, why do we do invalidations in background tabs? Why not just
> ignore them and invalidate the whole page when we bring the tab to the front?

We could. Checking whether content is in a background tab has a cost, though, so we wouldn't want to do that all time.
(In reply to comment #32)
> OK. This does require imagelib changes to not use a timer and instead
> require explicit time advance via some new API; I assume you're OK with that?

Yes, of course. That's why I suggested it :)

> > If, at some later time (or even now) we decide we desperately want separate
> > timelines on separate tabs, we should cache animated images per-document
> > instead of writing all that complicated code.
> 
> We can't do that since we don't know an image is animated until we've loaded
> it, right?

That's a complicating factor, but it's pretty easily worked around without any new classes.
(In reply to comment #32)
> OK. This does require imagelib changes to not use a timer and instead
> require explicit time advance via some new API; I assume you're OK with that?

(This is like "::RefreshTick" from Comment 11)

> There is also the complicating issue that we'll need to track, for each use
> of an image, the time offset between the refresh driver's time and the
> image's timeline. That code would go away if/when we switch to one animated
> image timeline per image instance or document.

Alternately, we could make RasterImage completely ignore the timestamp passed down from RefreshDriver, and instead use PR_Now() to find out how much time has passed since its last tick -- right?
We could, but I would rather not do that. If we use the refresh driver's time, we get the added benefit of keeping animated images in sync with other animations on the page, at least if it's not shared with other pages. A small benefit, to be sure, but worth having if we can get it easily.
Actually, from looking at the RefreshDriver impl, it looks like the timestamps it passes in are from Timestamp::Now() (that is -- not tied to any document).  

So the image should be able to directly use the time that's passed into ::RefreshTick (or whatever it's called), without caring about which document it came from.
(In reply to comment #33)
> (In reply to comment #31)
> > As a tangent, why do we do invalidations in background tabs? Why not just
> > ignore them and invalidate the whole page when we bring the tab to the front?
> 
> We could. Checking whether content is in a background tab has a cost,
> though, so we wouldn't want to do that all time.

We do this check in the root frame of every document right now. So the invalidation does get dropped, but it first has to make its way through the frame tree of one document first.

We of course have to invalidate the whole visible part of the page when it becomes visible.
(Assignee)

Comment 39

8 years ago
First in a series of patches to fix this bug. This patch is an API change for imgIContainer - probably needs super review.
Attachment #545294 - Flags: superreview?(bzbarsky)
Attachment #545294 - Flags: review?(joe)
Attachment #545294 - Flags: review?(dholbert)
Attachment #545294 - Attachment is patch: true
Comment on attachment 545294 [details] [diff] [review]
Patch 0 - Interface changes for b666446

>diff --git a/modules/libpr0n/public/imgIContainer.idl b/modules/libpr0n/public/imgIContainer.idl
>+[ref] native timestamp(mozilla::TimeStamp);

Seems like it'd be clearest if we made the IDL typename match the native typename here.  So, s/timestamp/Timestamp/

>+  /** 
>+    * Sends a signal indicating that this imgIContainer has been triggered by
>+    * a layout frame to refresh itself. Likely this should only be called from
>+    * within nsImageFrame or objects of similar type.
>+    */
>+  [notxpcom] void refreshTick([const] in timestamp t);
>+    

Nix whitespace-on-blank-line at the end there.  (there are some stray space characters)

Also, the header comment should probably mention animation.  Perhaps replace "to refresh iteself" with "to update its internal animation state".

Also, prefix function-arguments with "a", and use a slightly-more-descriptive arg name, e.g. "aTime".

>+// [notxpcom] void refreshTick([const] in timestamp t);
>+void RasterImage::RefreshTick(const mozilla::TimeStamp& t)
>+{
>+  // TODO: Implement me
>+}
>+

Add newline after "void", to match the prevailing mozilla style. Same with in VectorImage.cpp

>diff --git a/modules/libpr0n/src/VectorImage.cpp b/modules/libpr0n/src/VectorImage.cpp
>@@ -166,16 +166,17 @@ SVGDrawingCallback::operator()(gfxContex
>   // Clip to aFillRect so that we don't paint outside.
>   aContext->NewPath();
>   aContext->Rectangle(aFillRect);
>   aContext->Clip();
> 
>   gfxContextMatrixAutoSaveRestore contextMatrixRestorer(aContext);
>   aContext->Multiply(gfxMatrix(aTransform).Invert());
> 
>+  // [notxpcom] void refreshTick([const] in timestamp t);
> 
>   nsPresContext* presContext = presShell->GetPresContext();

I think that added line (inside of SVGDrawingCallback::operator()) isn't supposed to be there.
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
For historical reference on performance and background-tab animation, see bug 125025, bug 125137, and especially bug 120154 (which is still open, and fairly directly relevant if out-of-date).
(Assignee)

Comment 42

8 years ago
Fixed issues described by dholbert in review yesterday.
Attachment #545294 - Attachment is obsolete: true
Attachment #545431 - Flags: superreview?(bzbarsky)
Attachment #545431 - Flags: review?(joe)
Attachment #545431 - Flags: review?(dholbert)
Attachment #545294 - Flags: superreview?(bzbarsky)
Attachment #545294 - Flags: review?(joe)
Attachment #545294 - Flags: review?(dholbert)
(Assignee)

Comment 43

8 years ago
Attachment #545432 - Flags: review?(joe)
Attachment #545432 - Flags: review?(dholbert)
(Assignee)

Comment 44

8 years ago
Sorry about the review spam. I accidentally posted only the changes to the .idl file. This file is now the patch that actually fixes the review issues presented by dholbert.
Attachment #545431 - Attachment is obsolete: true
Attachment #545432 - Attachment is obsolete: true
Attachment #545433 - Flags: review?(dholbert)
Attachment #545431 - Flags: superreview?(bzbarsky)
Attachment #545431 - Flags: review?(joe)
Attachment #545431 - Flags: review?(dholbert)
Attachment #545432 - Flags: review?(joe)
Attachment #545432 - Flags: review?(dholbert)
Comment on attachment 545433 [details] [diff] [review]
Patch 0v4 - Interface changes for b666446

>+++ b/modules/libpr0n/src/RasterImage.cpp
>+// [notxpcom] void refreshTick([const] in timestamp t);
>+void
>+RasterImage::RefreshTick(const mozilla::TimeStamp& aTime)
>+{
>+  // TODO: Implement me
>+}

Nit: The comment there is now out of date ("timestamp t" --> "Timestamp aTime") Applies to the same spot in VectorImage.cpp, too.

Actually -- I think most of the impl-header-comments in these files are just directly copy/pasted from $OBJDIR/dist/include/imgIContainer.h, from the corresponding chunks underneath the 
  "Use the code below as a template for the implementation class for this interface"
comment.  (note that this .h file is auto-generated from the .idl file)

Probably best to do that, to be sure to match the style of the other header-comments in this file. (switching out // for /**/, too)  (super-picky, sorry :))

>+++ b/modules/libpr0n/src/VectorImage.cpp
>   gfxContextMatrixAutoSaveRestore contextMatrixRestorer(aContext);
>   aContext->Multiply(gfxMatrix(aTransform).Invert());
> 
>-
>   nsPresContext* presContext = presShell->GetPresContext();
>   NS_ABORT_IF_FALSE(presContext, "pres shell w/out pres context");
> 

Nit: This newline-deletion looks unrelated... However, it's a good change (and doesn't break blame), so I won't complain if you keep it in. :)

So, r=dholbert with the cpp-file comment-tweaks.
Attachment #545433 - Flags: review?(dholbert) → review+
(In reply to comment #45)
> Actually -- I think most of the impl-header-comments in these files are just
> directly copy/pasted from $OBJDIR/dist/include/imgIContainer.h

(also, per IRL conversation, you'll also need to use the NS_IMETHODIMP(void) or whatever from imgIContainer.h, at the beginning of the function impls)
(Assignee)

Comment 47

8 years ago
Posting patch as reviewed by dholbert, requesting second r in a second comment.
Attachment #545516 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #545516 - Flags: superreview?(bzbarsky)
Attachment #545516 - Flags: review?(joe)
(Assignee)

Updated

8 years ago
Attachment #545433 - Attachment is obsolete: true
Comment on attachment 545516 [details] [diff] [review]
Patch 0v4 (v5) - Interface changes for b666446 [r=dholbert]

Was there a reason to not call the API willRefresh, since this is basically forwarding on notifications from the refresh driver?

Other than that, looks fine.
Attachment #545516 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #48)
> Was there a reason to not call the API willRefresh, since this is basically
> forwarding on notifications from the refresh driver?

I'd recommended that we use a different function name, to prevent confusion over whether imgIContainer impls are expected to implement nsARefreshObserver (and also because these calls will be coming from multiple refresh drivers simultaneously).  That was possibly a silly concern on my part, though.  I don't have strong feelings on it at this point.
OK, I buy that argument.  How about refreshRequested then?  My issue with "refreshTick" is that it sounds like an order to refresh a tick, whatever that means....
Comment on attachment 545516 [details] [diff] [review]
Patch 0v4 (v5) - Interface changes for b666446 [r=dholbert]

Review of attachment 545516 [details] [diff] [review]:
-----------------------------------------------------------------

I'd also love to see your in-progress patches for implementing this interface.

::: modules/libpr0n/public/imgIContainer.idl
@@ +71,5 @@
>  native gfxGraphicsFilter(gfxPattern::GraphicsFilter);
>  [ref] native nsIntRect(nsIntRect);
>  [ref] native nsIntSize(nsIntSize);
>  [ptr] native nsIFrame(nsIFrame);
> +[ref] native Timestamp(mozilla::TimeStamp);

nit: TimeStamp

@@ +271,5 @@
> +    * a layout frame to update its internal animation state. Likely this
> +    * should only be called from within nsImageFrame or objects of similar
> +    * type.
> +    */
> +  [notxpcom] void refreshTick([const] in Timestamp aTime);

I'm not choosy about the name, but I prefer it be in the active tense; requestRefresh? I'm also ok with willRefresh, timerTick, updateAnimation, etc.
Attachment #545516 - Flags: review?(joe) → review+
(Assignee)

Comment 52

8 years ago
> I'd also love to see your in-progress patches for implementing this
> interface.
Sure.
I have patches available at:
http://hg.mozilla.org/users/sjohnson_mozilla.com/patches
The one I'm currently working on, b666446-nsImageFrame-impl, I believe includes implementations for RefreshTick() (or whatever we're going to call it) in RasterImage and VectorImage (although VectorImage is still just a stub).

> nit: TimeStamp
Sure, I'll make that modification.

> I'm not choosy about the name, but I prefer it be in the active tense;
> requestRefresh? I'm also ok with willRefresh, timerTick, updateAnimation,
> etc.

I agree. How about requestRefresh()? Does that make sense to everyone?
(Assignee)

Comment 53

8 years ago
> I'd also love to see your in-progress patches for implementing this
> interface.

Sorry, I re-read this and realized it might not be entirely clear. I just gave a link to my patch queue so that you can take a look at my work as I'm doing it. I will post work-in-progress patches once I get to a state where they are working somewhat well. 

Also, I just reviewed that patch and realized the implementations for RasterImage and VectorImage are not in that patch, so I'll post a patch once the implementations move from my head to source files. :)
requestRefresh is fine by me.
(Assignee)

Comment 55

8 years ago
Patch with the interface conforming to review comments by dholbert, JOEDREW!, and bz. The RefreshTick() function has also been changed to RequestRefresh().
Attachment #545516 - Attachment is obsolete: true
Attachment #545740 - Flags: superreview+
(Assignee)

Updated

8 years ago
Attachment #545740 - Flags: review+
Attachment #545740 - Attachment description: Patch 0 (v6) - Interface changes for b666446 [r=dholbert,bz,JOEDREW!] → Patch 0 (v6) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]
(In reply to comment #52)

> http://hg.mozilla.org/users/sjohnson_mozilla.com/patches
> The one I'm currently working on, b666446-nsImageFrame-impl, I believe
> includes implementations for RefreshTick() (or whatever we're going to call
> it) in RasterImage and VectorImage (although VectorImage is still just a
> stub).

You probably haven't committed/pushed that change yet, because I couldn't see any imagelib stuff in that patch :(
(In reply to comment #56)
> You probably haven't committed/pushed that change yet, because I couldn't
> see any imagelib stuff in that patch :(

(yup, see second half of comment 53 :))
Er, yes.
(Assignee)

Comment 59

8 years ago
This is work in progress for patch 1, if you'd like to look it over. Not requesting review just yet, because it will probably change.
(Assignee)

Comment 60

8 years ago
This is work in progress for patch 2, if you'd like to look it over. Not requesting review just yet, because it will probably change.
(Assignee)

Comment 61

8 years ago
I've posted a couple of patches for work in progress. I'm currently in the process of debugging what appears to be an issue loading the GIF images with the RasterImage implementation.
(Assignee)

Comment 62

8 years ago
So it appears that the test case described in comment 1 is giving me troubles now (appears to be network troubles - it's unavailable even if I attempt to Save as complete' to the page). As such, I'm posting another test case from bug 595671, downloaded and wrapped up as a .zip file.
(Assignee)

Comment 63

8 years ago
This is a test case for this bug.
(Assignee)

Comment 64

8 years ago
Original test case from comment 1.
(Assignee)

Comment 65

8 years ago
Disregard comment 62. I was able to get the web page to load now, and performed a 'save as complete' on it. It's attached in attachment 545998 [details].
(Assignee)

Comment 66

8 years ago
Modified a comment that had an incorrect additional word at the end of the line.
Attachment #545968 - Attachment is obsolete: true
(Assignee)

Comment 67

8 years ago
Moved some stuff over from Patch 2 to make Patch 1 more cohesive and self-contained.
Attachment #546004 - Attachment is obsolete: true
(Assignee)

Comment 68

8 years ago
Moved some items from Patch 2 to Patch 1 to make Patch 1 more self-contained and cohesive.
Attachment #545969 - Attachment is obsolete: true
(Assignee)

Comment 69

8 years ago
I'm going to be updating the RasterImage patch to add a couple of details that I missed the first time around, also to rebase against the patch for bug 672225.

Also, I thought I would share some basic performance testing I did for this bug, after the patch was applied.  I noticed a sharp increase in the number of calls to RasterImage::Draw() on 7/14/11, after applying the patch: 

Chat site without patch (~2min):
Draw Frame Count: 17873

Chat site with patch (~2min):
Draw Frame Count: 86227

After speaking with dholbert about this, we agreed that this might not necessarily be bad, because if the number of actual expose events is reduced, it might indicate that we're just getting more frames through the pipeline, thus increasing the frame rate (which is the goal to begin with anyway). To verify this, I put another set of print statements inside of the OnExposeEvent() callback, as before, to produce these results:

Chat Site w/ 1 minute run time:
Expose Counter: 1470
Draw Frame Count: 4300

Chat Site w/ 2 minutes run time:
Expose Counter: 5040
Draw Frame Count: 23200

Finally, we agreed that if this were the case, then the counts for a single image should be about the same number of calls to Draw() and OnExposeEvent (relative to one another). I used one of the images from the chat site and simply loaded JUST that image).

Chat Site single image icon without patch (~2min):
Draw Frame Count: 7200
Expose Counter: 1210

Chat Site single image icon with patch (~2min):
Draw Frame Count: 6900
Expose Counter: 1160

Additionally, the performance with the patch has visibly increased, and the CPU load has gone down from 100% usage of at least 1 core to roughly 67% usage of a least one core. So, it seems clear that this has given us about a 33% speedup, at least in RasterImage.
Sounds like you're on the right track, but I think the best comparison would be expose event rate before and after your patch.
(Assignee)

Comment 71

8 years ago
Made some minor adjustments to the nsImageFrame implementation for this bug from the WiP version.
Attachment #546020 - Attachment is obsolete: true
Attachment #547521 - Flags: review?(joe)
Attachment #547521 - Flags: review?(dholbert)
(Assignee)

Comment 72

8 years ago
Patch that fixes the RasterImage.h/.cpp files for animated gif performance issues.
Attachment #546021 - Attachment is obsolete: true
Attachment #547522 - Flags: review?(joe)
Attachment #547522 - Flags: review?(dholbert)
Comment on attachment 547521 [details] [diff] [review]
Patch 1 (v4) - Implementation for nsImageFrame

>+nsImageRefreshObserver::nsImageRefreshObserver(nsImageFrame* aParent)
>+{
>+  Register(aParent);
>+}

This would be nicer as:
  : mParent(aParent)
  {
    NS_ABORT_IF_FALSE(mParent, "nsImageRefreshObserver requires an image frame");
    Register();
  }

>+void
>+nsImageRefreshObserver::WillRefresh(mozilla::TimeStamp aTime)
>+{
>+  if (!mFrame) {
>+    // we don't want to dereference a null ptr
>+    return;
>+  }

I don't think it's worth null-checking mFrame here, though it'd be worth adding an NS_ABORT_IF_FALSE just to make clear the assumption that it's non-null. There's no sane way that we'd get here with a null value (since we unregister from refresh-driver & null out mFrame at the same spot), so the check isn't really necessary.  Moreover, if something breaks horribly and mFrame ends up being null somehow, then we'll just crash on a null-deref, which is relatively safe.

>+void
>+nsImageRefreshObserver::Register(nsImageFrame* aFrame)
>+{
>+  if (!aFrame) {
>+    // we can't register a null frame
>+    return;
>+  }
>+  mFrame = aFrame;

As noted above, it'd be simpler to set |mFrame| in the constructor init list.  Also as noted above, no need for the null-check.

>+void
>+nsImageRefreshObserver::Deregister()
>+{
>+  if (!mFrame) {
>+    return;
>+  }

As noted above, no need for the null-check.

>+  nsPresContext* presContext = mFrame->PresContext();
>+  presContext->PresShell()->RemoveRefreshObserver(this,
>+                                                  mozFlushType::Flush_Display);
>+  mFrame = nsnull;

Maybe add a comment explaining why we care about nulling this out, e.g.:
  // mFrame is being destroyed -- drop our reference to it, so we don't deref
  // a deleted pointer if we somehow outlive it & get callbacks after it's deleted
  // (highly unexpected)

>+  NS_ABORT_IF_FALSE(mRefCnt != 1, "Multiple references exist to an image refresh observer.");

Nit: no period at end of assertion messages.
(to prevent ".:" grossness in "this is a message.: path/to/file.cpp")

>+++ b/layout/generic/nsImageFrame.h
>+class nsImageRefreshObserver: public nsARefreshObserver
>+{

Add a short comment above this helper-class to explain what it's used for.

>+  // Handle to the refresh driver observation helper class
>+  nsRefPtr<nsImageRefreshObserver> mRefreshObs;
>+

Add a comment explaining why this is a nsRefPtr (rather than a nsAutoPtr, which we would normally use for one-off helper-objects like this).
e.g. 
 // Refcounted because the nsRefreshDriver temporarily AddRef's this object
 // during each refresh ping.
(In reply to comment #73)
> I don't think it's worth null-checking mFrame here, though it'd be worth
> adding an NS_ABORT_IF_FALSE just to make clear the assumption that it's
> non-null.

(but if you'd like to keep the null-check in WillRefresh, I wouldn't object as long as you add an NS_ERROR or NS_ABORT_IF_FALSE to accompany it.)
Comment on attachment 547522 [details] [diff] [review]
Patch 2 (v3) - Implementation for RasterImage

>+++ b/layout/generic/nsImageFrame.cpp
>@@ -213,17 +213,17 @@ nsImageRefreshObserver::Deregister()
>-  NS_ABORT_IF_FALSE(mRefCnt != 1, "Multiple references exist to an image refresh observer.");
>+  NS_ABORT_IF_FALSE(mRefCnt == 1, "Multiple references exist to an image refresh observer.");
> }

Looks like this chunk belongs in the previous patch.

>+nsresult
>+RasterImage::AdvanceFrame(imgIContainerObserver* aObserver, TimeStamp aTime)
>+{

This probably wants to return void, rather than nsresult.  The only value it returns right now is NS_OK.

>+  if (mFrames.IsEmpty()) {
>+    return NS_OK;
>+  }

I'm not sure it's makes sense to check for this.  Note that when we get here, we've already evaluated:
   mFrames[mAnim->currentAnimationFrameIndex];
in the caller (RequestRefresh), so it looks like we're already assuming that mFrames is nonempty.
I'm not sure if that's an valid assumption, but either way, it seems like we shouldn't be getting as far as AdvanceFrame if we have no frames.
So perhaps convert this into an ASSERTION or ABORT_IF_FALSE?

>+  imgFrame* nextFrame = nsnull;
>+  PRUint32 previousFrameIndex = mAnim->currentAnimationFrameIndex;
>+  PRUint32 nextFrameIndex = mAnim->currentAnimationFrameIndex + 1;

It looks like this only supports going forward 1 frame at a time, regardless of how long it's been since the last one.  Looks like this still needs fixing to support jumps of multiple frames at a time, right?

Also -- I think this would be clearer if we removed the "previous" / "prev" terminology, and instead dealt only with "current frame" and "next frame".  (particularly given that you wait until the last second to do the "current = next" assignment, which is great)  I'd suggest s/previous/current/ here, and also a chunk later on in this function "imgFrame *prevFrame = mFrames[previousFrameIndex]" would want s/prevFrame/curFrame/.

>+  PRInt32 timeout = 0;
>+
>+  // Figure out if we have the next full frame. This is more complicated than
>+  // just checking for mFrames.Length() because decoders append their frames
>+  // before they're filled in.
>+  NS_ABORT_IF_FALSE(mDecoder || nextFrameIndex <= mFrames.Length(),
>+                    "How did we get 2 indicies too far by incrementing?");

s/indicies/indices/

>+  bool haveFullNextFrame = !mDecoder || nextFrameIndex < mDecoder->GetCompleteFrameCount();
>+
>+  // If we're done decoding the next frame, go ahead and display it now and
>+  // reinit the timer with the next frame's delay time.
>+  if (haveFullNextFrame) {
>+    if (mFrames.Length() == nextFrameIndex) {
>+      // End of Animation
>+
>+      // If animation mode is "loop once", it's time to stop animating
>+      if (mAnimationMode == kLoopOnceAnimMode || mLoopCount == 0) {
>+        mAnimationFinished = PR_TRUE;
>+        EvaluateAnimation();
>+        return NS_OK;
>+      } else {

Drop the "else" here - unnecessary after a return statement (and it confuses some compilers, IIRC).

>+    // Change frame and announce it
>+    if (NS_FAILED(DoComposite(&frameToUse, &dirtyRect, prevFrame,
>+                              nextFrame, nextFrameIndex))) {
>+      // something went wrong, move on to next
>+      NS_WARNING("RasterImage::AdvanceFrame(): Composing Frame Failed\n");

No "\n" needed in warning messages

>+      nextFrame->SetCompositingFailed(PR_TRUE);
>+      mAnim->currentAnimationFrameIndex = nextFrameIndex;
>+      return NS_OK;
>+    } else {
>+      nextFrame->SetCompositingFailed(PR_FALSE);

Drop else after return here, too.

> RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)
> {
>+  this->ensureAnimExists();
>+  if (!mAnimating || !ShouldAnimate()) {
>+    return;
>+  }
A few things on ensureAnimExists:
 (a) no need for |this->|
 (b) We only want to call it after we've determined that we're animating.
 (c) It looks like ensureAnimExists thinks it can fail (that is, we have failure-checks for it in other places) -- however, infallible-new means it really can't fail.  To avoid confusion about where/whether we need to failure-check it, perhaps we should just make ensureAnimExists() return void? (and capitalize it to follow convention? I think it's the only non-capitalized method in RasterImage.)

Maybe (c) could happen in an separate patch (or separate bug).

>+  NS_ABORT_IF_FALSE(mAnim,
>+                    "RequestRefresh(): Animation object cannot be null");

This ABORT_IF_FALSE doesn't really make sense, given that we just called ensureAnimExists().  If you want to keep it, maybe change message to "ensureAnimExists lied!", to be clearer about why we expect the asserted condition to be true?

>+  // only advance the frame if the current time is greater than or
>+  // equal to the last frame time + the frame's offset
>+  imgFrame* currentFrame = mFrames[mAnim->currentAnimationFrameIndex];
>+  TimeStamp lastFrameTime = mAnim->currentAnimationFrameTime;
>+  PRInt64 timeout = currentFrame->GetTimeout();
>+  TimeDuration durationOfTimeout = TimeDuration::FromMilliseconds(timeout);
>+  TimeStamp timePlusTimeout = lastFrameTime + durationOfTimeout;

As noted for "prev" in AdvanceFrame:  the "current" vs. "last" distinction is a little confusing here, too - I think this would be clearer if it were just in terms of "current frame" and "next frame", with no mention of "last".

So, maybe replace the final 4 lines quoted above with:
    TimeStamp nextAnimationFrameTime = mAnim->currentAnimationFrameTime +
      TimeDuration::FromMilliseconds(currentFrame->GetTimeout());

>+    nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
>+    if (!observer) {
>+      // the imgRequest that owns us is dead, we should die now too.
>+      NS_ABORT_IF_FALSE(mAnimationConsumers == 0,
>+          "If no observer, should have no consumers");
>+      if (mAnimating) {
>+        StopAnimation();
>+      }
>+  
>+    this->AdvanceFrame(observer, aTime);

This chunk is problematic for a few reasons (though I think a good chunk of the weirdness predates this patch, and comes from ::Notify()).

Issue #1: We assert that mAnimationConsumers is 0 -- but if that were true, then Image::ShouldAnimate() would have returned false at the beginning of this method, and we never would have gotten here.  So the assertion is bogus.
Issue #2: We check 'mAnimating' -- but that's guaranteed to be true, because we already checked it at the beginning of this method.
Issue #3: Our clients (e.g. nsImageFrames) presumably had to use the imgRequest in order to look up the imgIContainer in order to call this method in the first place -- so if that were dead, I don't think it'd be possible for us to even get here. (?)
Issue #4: We currently we go ahead and call AdvanceFrame even though |observer| is null, which will crash.

I think this chunk can be reduced to:
   if (!observer) {
     NS_ERROR("Refreshing image after its imgRequest is gone");
     StopAnimation();
     return;
   }
(I believe that addresses all the issues I brought up above.)

>   /**
>+   * Advances the animation. Typically, this will advance a single frame, but it
>+   * may advance multiple frames in the case where a frame's length is <= 1
>+   * refresh driver "tick" - ~ 1/60th of a second

Better not to be this specific -- the 60hz figure is only true for foreground tabs, and even there it's not set in stone. (It's configurable in about:config, actually.)

Probably best not to mention any particular frequency here - instead, say something like:
   This may happen if we have infrequently-"ticking" refresh drivers (e.g. in background
   tabs), or extremely short-lived animation frames.

>+   *
>+   * @param aObserver the imgIContainerObserver to notify after the frame has
>+   *                 been changed.

Indentation issue there -- add space before 'been'.

>+   * @param aTime the time that the animation should advance to. this will
>+   *              typically be <= TimeStamp::Now().

The <= Now() statement there is probably worth asserting in the implementation, actually.  Also, capitalize "this"
>+    this->AdvanceFrame(observer, aTime);

Also: No need for |this->| there.
(Assignee)

Comment 77

8 years ago
Hi Daniel:

Thanks for the feedback - I'll make these changes and then repost.
(Assignee)

Comment 78

8 years ago
> >+  imgFrame* nextFrame = nsnull;
> >+  PRUint32 previousFrameIndex = mAnim->currentAnimationFrameIndex;
> >+  PRUint32 nextFrameIndex = mAnim->currentAnimationFrameIndex + 1;
> 
> It looks like this only supports going forward 1 frame at a time, regardless
> of how long it's been since the last one.  Looks like this still needs
> fixing to support jumps of multiple frames at a time, right?

Yes, I think you're right on this. After playing with this a bit, I noticed that we already have timing calculations in RequestRefresh(), and calculating how many frames we should advance in AdvanceFrame() is going to duplicate this timing calculation. Additionally, it depends on mAnim->currentAnimationFrameTime, which is only set in AdvanceFrame. So, it seems like a bit of a chicken-and-egg problem. 

I think I'm going to tackle this problem by implementing AdvanceFrame() to ONLY advance a single frame, and then in RequestRefresh force it to potentially call AdvanceFrame() multiple times if necessary to advance more than one frame. This is more overhead, although I'm not convinced it will be a significant amount of additional overhead, and I think the case where we're advancing more than one frame in a single refresh driver tick will be somewhat less common than the single-frame-advancement case. 

Thoughts?
(Assignee)

Comment 79

8 years ago
> which is only set in AdvanceFrame. So, it
> seems like a bit of a chicken-and-egg problem. 

Also, by this I mean that when I added the timing calculation into AdvanceFrame, I'm getting issues with mAnim->currentAnimationFrameTime not being initialized prior to the first frame advancement, which causes a number of problems.
(In reply to comment #78)
> After playing with this a bit, I noticed
> that we already have timing calculations in RequestRefresh() and
> calculating how many frames we should advance in AdvanceFrame() is going to
> duplicate this timing calculation.

Not necessarily -- RequestRefresh() only checks "Are we past the end of the current frame", whereas AdvanceFrame could be checking "Ok, what frame _are_ we in?"  You could even pass in "aTimePastEndOfCurrentFrame" instead of "aTime" as an argument to AdvanceFrame, if you like.  But your suggestion works too, per below.

(In reply to comment #78)
> I think I'm going to tackle this problem by implementing AdvanceFrame() to
> ONLY advance a single frame, and then in RequestRefresh force it to
> potentially call AdvanceFrame() multiple times if necessary to advance more
> than one frame.

Yeah, that's probably the simplest way forward for now.  If you're going to do that, though, I think we should only send the FrameChanged notifications once.  If we have a bunch of listeners, I think the duplicate invalidations could potentially be expensive (and either way they're definitely definitely redundant/silly).  So I think we need to move that FrameChanged call out to RequestRefresh().  Perhaps AdvanceFrame() can return a boolean indicating whether an invalidation is required or not (and RequestRefresh would honor the boolean returned by its final call to AdvanceFrame()).

(In reply to comment #79)
> I'm getting issues with mAnim->currentAnimationFrameTime not
> being initialized prior to the first frame advancement, which causes a
> number of problems.

Hmm -- it seems like that could cause issues in your attached patch, too -- I don't see where that ever gets initialized.  I think you'll need to start that out with some sentinel value (e.g. 0, #defined as FRAME_TIME_UNINITIALIZED or something?), and have a check for that to handle the first-frame case.
(Assignee)

Comment 81

8 years ago
> If you're going to
> do that, though, I think we should only send the FrameChanged notifications
> once.  If we have a bunch of listeners, I think the duplicate invalidations
> could potentially be expensive (and either way they're definitely definitely
> redundant/silly).  So I think we need to move that FrameChanged call out to
> RequestRefresh().  Perhaps AdvanceFrame() can return a boolean indicating
> whether an invalidation is required or not (and RequestRefresh would honor
> the boolean returned by its final call to AdvanceFrame()).

Hm, yes, I hadn't anticipated this initially. It does seem like a waste to send the FrameChanged() notifications multiple times. And it seems logical to have AdvanceFrame() advance a single frame. On the other hand, it seems equally logical to have the function AdvanceFrame() being fairly cohesive - which entails that it should send the notification, not the RequestRefresh() function. (Am I the only one that thinks this is more mentally cohesive to have the FrameChanged notification happen in AdvanceFrame()?)
 
> Hmm -- it seems like that could cause issues in your attached patch, too --
> I don't see where that ever gets initialized.  I think you'll need to start
> that out with some sentinel value (e.g. 0, #defined as
> FRAME_TIME_UNINITIALIZED or something?), and have a check for that to handle
> the first-frame case.

Yeah, I'm not sure why I didn't hit it before. I definitely thought this should have been a problem, but I wasn't seeing the same segfault I am seeing after I duplicated the timing calculations in the AdvanceFrame() call. It seems that mAnim->currentAnimationFrameTime is null at some point, which is causing a call to TimeStamp.operator+ (TimeDuration&) to fail an assertion. I *did* notice these assertion failures in the previous patch, but I didn't connect that they were coming from the TimeStamp::operator+ function until now. However, now it's also throwing a segfault, so I'm looking into why this is causing a problem like this. Perhaps when I have a bit more information, I can give a better suggestion as to how to proceed.
>+      // If animation mode is "loop once", it's time to stop animating
>+      if (mAnimationMode == kLoopOnceAnimMode || mLoopCount == 0) {
>+        mAnimationFinished = PR_TRUE;
>+        EvaluateAnimation();
>+        return NS_OK;
>+      } else {
>
>Drop the "else" here - unnecessary after a return statement (and it confuses some compilers, IIRC).

Any compiler that confuses should be recycled into random bits.  :-) Seriously, that's simply fundamental correctness; we shouldn't use any compiler that fubars that (and I can't imagine any non-toy compiler does).

If that's a style issue, I'd say "what's the prevailing style in the file/module?"  I do stuff like that when it (IMO) aids readability; I don't have a strong opinion pro or con to that - I personally wouldn't flag it in a review, but I wasn't the reviewer here.
(Assignee)

Updated

8 years ago
Attachment #547521 - Flags: review?(joe)
Attachment #547521 - Flags: review?(dholbert)
(Assignee)

Updated

8 years ago
Attachment #547522 - Flags: review?(joe)
Attachment #547522 - Flags: review?(dholbert)
(Assignee)

Comment 83

8 years ago
dholbert and I were speaking on IRC (in order to minimize a giant slough of comments back and forth), and we came to the agreement that the following is a good procedure to handle the AdvanceFrame/RequestRefresh problem:

  1) Implement AdvanceFrame to advance a _single_ frame, but without the notification.
  2) Extend the implementation of RequestRefresh() to calculate if a frame advancement is necessary, and then advance frames incrementally using AdvanceFrame() until it either a) fails or b) reaches the desired time. If at least one frame has been successfully advanced, then it will trigger an invalidate notification.

This saves us from having to do the timing calculations multiple times. AdvanceFrame is pretty cheap without the FrameChanged() call, so I think we're pretty safe in that respect. It does, however, calculate whether or not the next frame is currently decoding (which is when it would fail if the next frame can't be shown). In that case, we still want to trigger an update notification if we have had at least one frame changed since the start of RequestRefresh().

Just wanted to give you folks a heads-up as to what we were discussing, in the event that you have comments/suggestions.
(Assignee)

Updated

8 years ago
No longer blocks: 666352
(In reply to comment #82)
> >Drop the "else" here - unnecessary after a return statement (and it confuses some compilers, IIRC).
> 
> Any compiler that confuses should be recycled into random bits.  :-)
> Seriously, that's simply fundamental correctness; we shouldn't use any
> compiler that fubars that (and I can't imagine any non-toy compiler does).

Yeah, compiler-friendliness wasn't my primary concern -- it's more of a style & clarity thing.  (It's been mentioned to me multiple times by other reviewers, and it's part of the JST Reviewer simulacrum, so I've internalized it as a general mozilla-coding-style thing.)

(I can imagine cases where else-after-return would be helpful readability-wise, but I don't think that was the case in the chunks that I flagged.)
(Assignee)

Updated

8 years ago
Depends on: 673535
(Assignee)

Comment 85

8 years ago
Sending up a more polished version. Not spamming everyone with this yet, just getting a second look over by dholbert. If he gives the ok, then I will send it up for review to module owners.
Attachment #547521 - Attachment is obsolete: true
Attachment #548275 - Flags: review?(dholbert)
(Assignee)

Comment 86

8 years ago
Sending up a more polished version. Not spamming everyone with this yet, just getting a second look over by dholbert. If he gives the ok, then I will send it up for review to module owners.
Attachment #547522 - Attachment is obsolete: true
Attachment #548277 - Flags: review?(dholbert)
Comment on attachment 548275 [details] [diff] [review]
Patch 1 (v5) - Implementation for nsImageFrame

Looks good! Just 2 nits:

>+nsImageRefreshObserver::Deregister()
>+{
[...]
>+  // mFrame is being destroyed, so we want to drop our reference to it so we
>+  // don't accidentally dereference a pointer that's been deleted. this will
>+  // only happen in the (very extreme) case where we somehow outlive the owning
>+  // nsImageFrame & get callbacks after it's been deleted.
>+  mFrame = nsnull;

s/extreme/unexpected/ or something. ("extreme" gives me the impression that it's something that *can* happen, by design -- when really, that's not the case as far as we know)

>+  // Handle to the refresh driver observation helper class
>+  // Refcounted because the nsRefreshDriver temporarily AddRef's this object
>+  // during each refresh ping. we don't want to use an nsAutoPtr because this
>+  // object will temporarily have multiple references to it, but it should
>+  // only be deleted when its nsImageFrame object is destroyed, not before.
>+  nsRefPtr<nsImageRefreshObserver> mRefreshObs;

capitalize "we don't want" (new sentence in middle of paragraph)
Attachment #548275 - Flags: review?(dholbert) → review+
(Assignee)

Comment 88

8 years ago
Made modifications as suggested by dholbert, submitting for final review.
Attachment #548275 - Attachment is obsolete: true
(Assignee)

Comment 89

8 years ago
Comment on attachment 548284 [details] [diff] [review]
Patch 2 (v6) - Implementation for nsImageFrame

Requesting review for layout portion of this bug. Since Roc is on holiday, I'm also requesting review from dbaron.
Attachment #548284 - Flags: review?(roc)
Attachment #548284 - Flags: review?(dbaron)
Comment on attachment 548277 [details] [diff] [review]
Patch 2 (v4) - Implementation for RasterImage

>+bool
>+RasterImage::AdvanceFrame(nsIntRect* aDirtyRect, TimeStamp aTime)

>+  bool haveFullNextFrame = !mDecoder || nextFrameIndex
>+      < mDecoder->GetCompleteFrameCount();

Not enough indentation on second line there.

Also, best to avoid splitting a condition (nextFrameIndex < mDeco...) across multiple lines if you don't have to.  Maybe drop nextFrameIndex down to the next line?

>+  if (!(timeout > 0)) {
>+    mAnimationFinished = PR_TRUE;
>+    EvaluateAnimation();
>+  }

This should just change to "if (timeout <= 0)" (BUT maybe not exactly, read on....)

Also, this condition doesn't make sense to me.  It looks like this is checking for the "show this frame forever" condition, but IIRC (based on a comment elsewhere in the file) that's specifically for timeouts of -1.  So I'd think this should really be < instead of <=, so we actually want this:
  "if (timeout < 0) { //  -1 means show this frame forever"

Do you know if there was a reason for the existing behavior? (making 0-timeout trigger "animation finished" behavior)

>+    // Change frame and announce it

s/and announce it//  (This function no longer handles notification)

>+    if (NS_FAILED(DoComposite(&frameToUse, aDirtyRect, prevFrame,
>+            nextFrame, nextFrameIndex))) {

Fix indentation there.

>+      // something went wrong, move on to next
>+      NS_WARNING("RasterImage::AdvanceFrame(): Compositing of frame failed");
>+      nextFrame->SetCompositingFailed(PR_TRUE);
>+      mAnim->currentAnimationFrameIndex = nextFrameIndex;
>+      return false;

We probably want to set currentAnimationFrameTime there, too, right?  Otherwise that will be a stale/bogus value, the next time we hit RefreshRequested.

> RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)
> {
>-  // TODO: Implement me as part of b666446
>+  EnsureAnimExists();
>+  if (!mAnimating || !ShouldAnimate()) {
>+    return;
>+  }

As noted in in comment 75, I'm pretty sure we only want to call EnsureAnimExists() after we've determined that we're animating.  So, drop that EnsureAnimExists call down a few lines, to be after the early-return.

>+  // only advance the frame if the current time is greater than or
>+  // equal to the last frame time + the frame's offset

s/offset/timeout/ (this is the only place you use the term "offset")

>+  TimeStamp timePlusTimeout = currentFrameTime + durationOfTimeout;
[...]
>+  while (timePlusTimeout <= aTime) {
[...]
>+    timePlusTimeout = currentFrameTime + durationOfTimeout;

The significance of "timePlusTimeout" isn't immediately clear from its name.  Maybe rename to "frameEndTime" or "nextFrameStartTime" or something? That would make the loop condition a little easier to grock at a glance.

Also, RE the loop structure -- so it currently works like this:
 SetTimeVariablesForCurrentFrame;
 while (condition) {
   Advance Frame;
   SetTimeVariablesForCurrentFrame;
 }

That has some duplicated code (the "set time variables" stuff), and that creates possibility for bugs if we tweak something in the first duplicate chunk but not the second.

To avoid the duplicate code, could we instead structure it like this (or something equivalent/better):

 while (true) {
   SetABunchOfVariablesForCurrentFrame;
   if (!condition) {
     break;
   }
   Advance Frame;
 } 

>+  nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
>+
>+  while (timePlusTimeout <= aTime) {
>+    if (!observer) {
>+      NS_ERROR("Refreshing image after its imgRequest is gone");
>+      StopAnimation();
>+      return;
>+    }

Move the |observer| declaration & null-check down to where we actually use |observer|, inside the "if (frameAdvanced)" clause.  There's no need to null-check it on every loop iteration, and there's no need to create it at all for RefreshRequested() calls that don't trigger notification.

>+    frameAdvanced = frameAdvanced || this->AdvanceFrame(&dirtyRect, aTime);

s/this->//

>@@ -1117,53 +1244,46 @@ RasterImage::StartAnimation()
[...]
>+  // Default timeout to 100
>   PRInt32 timeout = 100;
[...]
>   if (currentFrame) {
>     timeout = currentFrame->GetTimeout();
>     if (timeout < 0) { // -1 means display this frame forever

We can ditch the variable |timeout| and the magic number 100 now -- looks like now we only care whether it's -1 or not. Replace with this:
      if (currentFrame->GetTimeout() < 0) { // -1 means show this frame forever

>+    // we need to set the time that this first frame was displayed
>+    // this is important for the new refresh driver-based animation
>+    // timing
>+    mAnim->currentAnimationFrameTime = TimeStamp::Now();

This multi-sentence comment is hard to parse without any capitalization/periods - add either or both of those features. :)

>+   * @param aDirtyRect a pointed to an nsIntRect which encapsulates the area to
>+   *        be repainted after the frame is advanced.

Looks like an extra "an" snuck into that comment.

Also, put [outparam] after "aDirtyRect" (see e.g. nsStyleAnimation.h for sample syntax)
Also, the general mozilla style is to make outparams be the final argument(s).  Could you switch the argument-order so that aDirtyRect is last?

>+   * @returns true, if the frame was successfully advanced, false if it was not
>+   *          able to be advanced (e.g. the frame to which we want to advance is
>+   *          still decoding).

It'd be good to also mention somewhere that if we return false, aDirtyRect will be untouched. (right?)

r=me with the above.
Attachment #548277 - Flags: review?(dholbert) → review+
(In reply to comment #90)
> >+  bool haveFullNextFrame = !mDecoder || nextFrameIndex
> >+      < mDecoder->GetCompleteFrameCount();
> 
> Not enough indentation on second line there.

(er sorry, that's technically too _much_ indentation, by 2 spaces - I was imagining a layer of parenthesis that weren't there. Still would be best to have nextFrameIndex on same line as its "<".)
(Assignee)

Comment 92

8 years ago
> Do you know if there was a reason for the existing behavior? (making 0-timeout > trigger "animation finished" behavior)

No, I am not sure why this is. Someone else might be able to enlighten us further, but I think that in RasterImage::StartAnimation() we only check to see if it's > 0, so perhaps this is what we should do here as well.
(Assignee)

Comment 93

8 years ago
Whoops, hit enter too soon. Anyway, what I meant to add was that I will submit another bug to clarify/fix that issue.
(In reply to comment #91)
> (In reply to comment #90)
> > >+  bool haveFullNextFrame = !mDecoder || nextFrameIndex
> > >+      < mDecoder->GetCompleteFrameCount();
> > 
> > Not enough indentation on second line there.
> 
> (er sorry, that's technically too _much_ indentation, by 2 spaces - I was
> imagining a layer of parenthesis that weren't there. Still would be best to
> have nextFrameIndex on same line as its "<".)

Imagelib standard (aka, "My personal preference" :) ) is to vertically align either with the start of the bracket, the conditional clause, or failing all of that the beginning of the value, i.e.

bool foo = !bar && something ||
           somethingElse;
(In reply to comment #92)
> > Do you know if there was a reason for the existing behavior? (making 0-timeout > trigger "animation finished" behavior)
> 
> No, I am not sure why this is. Someone else might be able to enlighten us
> further, but I think that in RasterImage::StartAnimation() we only check to
> see if it's > 0, so perhaps this is what we should do here as well.

This code was introduced as "timeout >= 0" in CVS revision 1.5 of the file:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/modules/libpr0n/src&command=DIFF_FRAMESET&file=imgContainer.cpp&rev2=1.5&rev1=1.4

and then modified in revision 1.8 to be "> 0":

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/modules/libpr0n/src&command=DIFF_FRAMESET&file=imgContainer.cpp&rev2=1.8&rev1=1.7

Unfortunately neither of these revisions were linked to bugs, and my bugzilla searching has not come up with anything, which makes me think it was a sort of over-the-shoulder development that was common in the project at the time.
I can add some historical context.

Animated GIFs were animating too fast in some cases, and IE (and maybe NS4) had put a minimum on the timeouts for animgifs.  That was the purpose of the 1.8 checkin. 

I *suspect* the reason for changing how 0 is handled was to a) avoid it sucking 100% of the CPU if there wasn't a a lower limit and it repeats, and b) if an anim is a run-once anim, the effect of a 0 timeout was to jump to the end.  But that's fuzzy.

I have a fuzzy memory of the discussions around this change; I was involved.  Not sure if they were in newsgroups or a bug.  Look for bugs closed by pav or hyatt in that timeframe.
See bug 125137 for a related issue; it notes that IE of the era had a minimum anim timeout of 100ms.

After much searching I haven't found a bug associated with hyatt's checkin; perhaps searching on the sr= in the text will find it.
(Assignee)

Comment 98

8 years ago
Added bug 674329 to track this issue further.
(Assignee)

Comment 99

8 years ago
Sorry, meant bug 674239. Apparently a bit dyslexic today. ;)
(Assignee)

Comment 100

8 years ago
r+ dholbert, submitting for final review with JOEDREW!. Please note that the issue with the timeout has not been fixed, as it was spun off into a different bug.
Attachment #548277 - Attachment is obsolete: true
Attachment #548507 - Flags: review?(joe)
(Assignee)

Updated

8 years ago
Attachment #548507 - Attachment is patch: true
(Assignee)

Comment 101

8 years ago
dholbert found a subtle bug in an edge case for the GetCurrentImgFrameEndTime() function I wrote. It has been fixed to return the max TimeStamp in the event of a negative timeout value. I added a macro that will probably be removed from RasterImage.h once prtypes.h is updated for bug 674277.
Attachment #548507 - Attachment is obsolete: true
Attachment #548524 - Flags: review?(joe)
Attachment #548507 - Flags: review?(joe)
(Assignee)

Comment 102

8 years ago
Comment on attachment 548524 [details] [diff] [review]
Patch 2 (v6) - Implementation for RasterImage [r=dholbert]

Cancelling review because I found another bug in it. :)
Attachment #548524 - Flags: review?(joe)
(Assignee)

Comment 103

8 years ago
I cancelled the review because I found a problem with non-looping GIF images in RasterImage::GetCurrentImgFrameEndTime(). I'm fixing this problem right now, so you can feel free to review the rest, and I'll post an updated patch as soon as I fix the issue.
(Assignee)

Comment 104

8 years ago
Fixed the infinite loop as described in the previous revision of this patch.
Attachment #548524 - Attachment is obsolete: true
(Assignee)

Comment 105

8 years ago
dholbert mentioned that he had a concern about the GetCurrentImgFrameEndTime() implementation, because he was worried that it might not respond correctly if the value of timeout was negative. 

I implemented a work-around to this where GetCurrentImgFrameEndTime() returns a time equivalent to positive infinity when this happens, but I wasn't sure exactly how to test it, so I looked through the code to determine where this is set & how it is used. It looks like the GIF spec says that the delay time between frames is unsigned, and imgFrame::GetTimeout() returns 100 if the timeout that is passed in is anywhere between 0 and 10. Thus, it looks like it can't return a negative value.

dholbert said it warranted more investigation, as he didn't know how the RasterImage would know to stay forever on the last frame in the event of a non-looping animation. I investigated it and found that mLoopCount within the RasterImage object is decremented for each loop through the image. The number of loops to run is specified by the GIF decoder, or is -1 if it should loop forever. This is checked in RasterImage::AdvanceFrame() (was previously called Notify()). Thus, if it's currently at 0, and we're advancing to the last frame, the animation terminates, and the last frame is shown forever after that. 

So, now the question remains - why are there checks at all for timeout < 0? For example, in RasterImage::StartAnimation():

>  if (currentFrame) {
>    if (currentFrame->GetTimeout() < 0) {
>      // -1 means display this frame forever
>      mAnimationFinished = PR_TRUE;
>      return NS_ERROR_ABORT;
>    } 

It seems like the _only_ way that we would have a negative timeout is if someone explicitly called imgFrame::SetTimeout() with a negative value. So, I'm wondering if this is used in a non-gif animation (e.g. an APNG animation) or whether there is a risk of this timeout being negative. If not, I think it might make sense to remove these checks. My gut tells me that they were put in for a reason, though, so I'm wondering if anyone might know the reasoning behind having a possible negative value here, and even why imgFrame::GetTimeout returns a PRInt32 instead of a PRUint32.
Comment on attachment 548284 [details] [diff] [review]
Patch 2 (v6) - Implementation for nsImageFrame

Review of attachment 548284 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +216,5 @@
> +  mFrame = nsnull;
> +
> +  // sanity check to verify our reference is only held by one other class
> +  NS_ABORT_IF_FALSE(mRefCnt == 1,
> +                    "Multiple references exist to an image refresh observer");

I don't think we should do this. It's not fatal for something else to hang onto a reference longer than necessary. At best this should be an NS_WARNING.

@@ +311,5 @@
>    if (mDisplayingIcon)
>      gIconLoad->RemoveIconObserver(this);
>  
> +  // deregister our helper object with the refresh driver to complete tear-down
> +  mRefreshObs->Deregister();

Null check, this could be called after Init failed. If you don't want the null check, create the observer in the constructor instead of Init.

Hmm, it would be nice if we could only create and add the observer for animated images. Can we do that? Would save memory.

::: layout/generic/nsImageFrame.h
@@ +97,5 @@
> +/**
> + * A refresh observer for an nsImageFrame. It's necessary to have nsImageFrame
> + * respond to refresh driver events, but we don't want the refresh driver to
> + * AddRef/Release the nsImageFrame object, as this causes havoc with the frame
> + * management hierarchy. This class functions as a helper object that is ref-

The real reason is that frames aren't refcounted so AddRef/Release simply doesn't work.

@@ +109,5 @@
> +    nsImageRefreshObserver(nsImageFrame* aParent);
> +    ~nsImageRefreshObserver() {}
> +
> +    NS_IMETHOD_(nsrefcnt) AddRef(void);
> +    NS_IMETHOD_(nsrefcnt) Release(void);

Use NS_INLINE_DECL_REFCOUNTING

@@ +348,5 @@
> +  // Refcounted because the nsRefreshDriver temporarily AddRef's this object
> +  // during each refresh ping. We don't want to use an nsAutoPtr because this
> +  // object will temporarily have multiple references to it, but it should
> +  // only be deleted when its nsImageFrame object is destroyed, not before.
> +  nsRefPtr<nsImageRefreshObserver> mRefreshObs;

I think we can go for mRefreshObserver :-).

The comment is misleading. nsImageRefreshObserver is refcounted so nsAutoPtr is an absolute no-no, you can't use it on refcounted objects, period.
(Assignee)

Comment 107

8 years ago
Hi Roc:

Some responses to your comments on review -

> I don't think we should do this. It's not fatal for something else to hang 
> onto a reference longer than necessary. At best this should be an NS_WARNING.

Agreed. I changed it to NS_WARN_IF_FALSE.

> Null check, this could be called after Init failed. If you don't want the
> null check, create the observer in the constructor instead of Init.

I added a null check.

> Hmm, it would be nice if we could only create and add the observer for 
> animated images. Can we do that? Would save memory.

Sure, I think this makes sense. What's the best way to determine if an image is animated during construction? It seems like it only thinks it's animated if it has seen at least two frames. So, perhaps once the image recognizes there is more than one frame (e.g. StartAnimation()), then have it activate the refresh driver in the nsImageFrame (e.g. have StartAnimation() call mFrame->ActivateAnimation(), which would init the refresh observer)?
 
> Use NS_INLINE_DECL_REFCOUNTING

I originally did use this, but when I added this in place of the above code, I received the following compile errors:

error: conflicting return type specified for ‘virtual void nsImageRefreshObserver::AddRef()’
../../dist/include/nsRefreshDriver.h:71:60: error:   overriding ‘virtual nsrefcnt nsARefreshObserver::AddRef()’
../generic/nsImageFrame.h:110:966: error: conflicting return type specified for ‘virtual void nsImageRefreshObserver::Release()’
../../dist/include/nsRefreshDriver.h:72:60: error:   overriding ‘virtual nsrefcnt nsARefreshObserver::Release()’

So, I took the above code from an example implementation of nsARefreshObserver - NotificationController. I can make the appropriate change to NS_INLINE_DECL_REFCOUNTING, but I think I will need to modify nsARefreshObserver, which would in turn require numerous changes to child classes. (Unless I'm doing something incorrectly.) For reference, here's how I specified the macro:

    ~nsImageRefreshObserver() {}

    NS_INLINE_DECL_REFCOUNTING(nsARefreshObserver)

    void WillRefresh(mozilla::TimeStamp aTime);

> The real reason is that frames aren't refcounted so AddRef/Release simply doesn't work.

Made a change to the comment so it should be more accurate now.

> I think we can go for mRefreshObserver :-).

Yep, change made. :)

> The comment is misleading. nsImageRefreshObserver is refcounted so nsAutoPtr 
> is an absolute no-no, you can't use it on refcounted objects, period.

I made the change to the comment so it isn't so misleading.
(In reply to comment #107)
> > Hmm, it would be nice if we could only create and add the observer for 
> > animated images. Can we do that? Would save memory.
> 
> Sure, I think this makes sense. What's the best way to determine if an image
> is animated during construction? It seems like it only thinks it's animated
> if it has seen at least two frames. So, perhaps once the image recognizes
> there is more than one frame (e.g. StartAnimation()), then have it activate
> the refresh driver in the nsImageFrame

Alternately, we could have the nsImageFrame's imgIDecoderObserver (nsImageListener) listen for the second call to "OnStartFrame" or "OnStopFrame".  That tells us we've got 2 frames & hence we're animated.
(In reply to comment #107)
> Sure, I think this makes sense. What's the best way to determine if an image
> is animated during construction? It seems like it only thinks it's animated
> if it has seen at least two frames. So, perhaps once the image recognizes
> there is more than one frame (e.g. StartAnimation()), then have it activate
> the refresh driver in the nsImageFrame (e.g. have StartAnimation() call
> mFrame->ActivateAnimation(), which would init the refresh observer)?

That sounds good.

> > Use NS_INLINE_DECL_REFCOUNTING
> 
> I originally did use this, but when I added this in place of the above code,
> I received the following compile errors:
> 
> error: conflicting return type specified for ‘virtual void
> nsImageRefreshObserver::AddRef()’
> ../../dist/include/nsRefreshDriver.h:71:60: error:   overriding ‘virtual
> nsrefcnt nsARefreshObserver::AddRef()’
> ../generic/nsImageFrame.h:110:966: error: conflicting return type specified
> for ‘virtual void nsImageRefreshObserver::Release()’
> ../../dist/include/nsRefreshDriver.h:72:60: error:   overriding ‘virtual
> nsrefcnt nsARefreshObserver::Release()’

I think we can make nsARefreshObserver use NS_INLINE_DECL_REFCOUNTING without much trouble.
(In reply to comment #108)
> (In reply to comment #107)
> > > Hmm, it would be nice if we could only create and add the observer for 
> > > animated images. Can we do that? Would save memory.
> > 
> > Sure, I think this makes sense. What's the best way to determine if an image
> > is animated during construction? It seems like it only thinks it's animated
> > if it has seen at least two frames. So, perhaps once the image recognizes
> > there is more than one frame (e.g. StartAnimation()), then have it activate
> > the refresh driver in the nsImageFrame
> 
> Alternately, we could have the nsImageFrame's imgIDecoderObserver
> (nsImageListener) listen for the second call to "OnStartFrame" or
> "OnStopFrame".  That tells us we've got 2 frames & hence we're animated.

Do those calls happen in the absence of any WillRefresh calls? If so, that sounds good too.
(Assignee)

Comment 111

8 years ago
roc, dholbert, and I came up with a plan to add a list of imgIRequests to the nsRefreshDriver, instead of using the nsARefreshObserver child classes as I am doing in the current patch. This eliminates the need for a large number of these child classes, most with very similar functionality, and saves on memoery, as we don't have to allocate memory for these helper objects. As part of this, though, there needs to be some way to register these imgIRequests with the refresh driver. Most likely this will be through PresShell, which will need a new set of functions to add/remove these requests from the nsRefreshDriver. Now, given that these imgIRequests that are registering with the nsRefreshDriver really are observers, would it make more sense to add AddRefreshObserver(imgIRequest* aRequest) or AddImageRequest(imgIRequest* aRequest) to the nsPresContext class?
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(In reply to comment #111)
> Most likely this will be through PresShell, which will need a new set of 
> functions to add/remove these requests from the nsRefreshDriver.
> Now, given that these imgIRequests that
> are registering with the nsRefreshDriver really are observers, would it make
> more sense to add AddRefreshObserver(imgIRequest* aRequest) or
> AddImageRequest(imgIRequest* aRequest) to the nsPresContext class?

I think you'll want to add the methods directly to the RefreshDriver, not the PresContext or PresShell.  (Note that anyone who has a handle on the PresContext or PresShell can ask for the RefreshDriver through them, and then communicate directly with it.)

I don't have strong feelings about the method-name. I've been imagining that it'd be something image-specific like "AddImageRequest()", but I don't think it matters too much as long as it's sensible.
(Assignee)

Updated

8 years ago
Attachment #548284 - Flags: review?(dbaron)
(Assignee)

Comment 113

8 years ago
(In reply to comment #112)
 
> I think you'll want to add the methods directly to the RefreshDriver, not
> the PresContext or PresShell.  (Note that anyone who has a handle on the
> PresContext or PresShell can ask for the RefreshDriver through them, and
> then communicate directly with it.)

I agree, but I was previously using the PresShell::AddRefreshObserver(nsARefreshObserver* aObserver). I wasn't sure whether I should expose the function in the PresShell API; if this were the case, then for consistency I was going to name the function 'AddRefreshObserver' (i.e. an overloaded function), and just use an imgIRequest* as a parameter.

I'm somewhat interested to know why the Add/RemoveRefreshObserver functions defined in PresShell are defined in such a way as to (optionally) allow overriding, with the Add/RemoveRefreshObserverExternal() versions. It seems that bug 563327 addresses this, but I don't see a place anywhere in our code where Add/RemoveRefreshObserverExternal() is actually defined...

> I don't have strong feelings about the method-name. I've been imagining that
> it'd be something image-specific like "AddImageRequest()", but I don't think
> it matters too much as long as it's sensible.

Since I think the correct approach for now is to access the nsRefreshDriver directly through PresShell::RefreshDriver(), I will name the method pair 'Add/RemoveImageRequest()', and we can modify it later if desired. I originally thought that since it's actually functioning as a refresh observer, that might be made clear by using the 'AddRefreshObserver' method name, but I can see where that would get confusing in a different way. What do others think?
(Assignee)

Comment 114

8 years ago
In the above comment, I actually meant PresContext::RefreshDriver(), not PresShell()::RefreshDriver().
(In reply to comment #113)
> I'm somewhat interested to know why the Add/RemoveRefreshObserver functions
> defined in PresShell are defined in such a way as to (optionally) allow
> overriding, with the Add/RemoveRefreshObserverExternal() versions. It seems
> that bug 563327 addresses this, but I don't see a place anywhere in our code
> where Add/RemoveRefreshObserverExternal() is actually defined...

They're defined in nsPresShell.cpp.

The External versions were created so that code not linked into the same library as layout could call them. However, now that we always use libxul, that is probably no longer an issue --- we don't want extensions to be calling these. So we should probably just remove them.

> Since I think the correct approach for now is to access the nsRefreshDriver
> directly through PresShell::RefreshDriver(), I will name the method pair
> 'Add/RemoveImageRequest()', and we can modify it later if desired.

Sounds good.
(Assignee)

Updated

8 years ago
Attachment #548284 - Attachment description: Patch 1 (v6) - Implementation for nsImageFrame → Patch 2 (v6) - Implementation for nsImageFrame
(Assignee)

Updated

8 years ago
Attachment #548584 - Attachment description: Patch 2 (v7) - Implementation for RasterImage [r=dholbert] → Patch 3 (v7) - Implementation for RasterImage [r=dholbert]
(Assignee)

Comment 116

8 years ago
This is the implementation for the nsRefreshDriver class. It was modified as part of work done to minimize memory impact of this patch (see comment 111).
(Assignee)

Comment 117

8 years ago
Reworked nsImageFrame so that it no longer requires an nsARefreshObserver to use the new GIF animation architecture.
Attachment #548284 - Attachment is obsolete: true
Attachment #552012 - Flags: review?(roc)
Attachment #548284 - Flags: review?(roc)
(Assignee)

Comment 118

8 years ago
Attachment #548584 - Attachment is obsolete: true
Comment on attachment 552012 [details] [diff] [review]
Patch 2 (v7) - Implementation for nsImageFrame

Review of attachment 552012 [details] [diff] [review]:
-----------------------------------------------------------------

We should try to make this only add an observer for animated images.

::: layout/base/nsIPresShell.h
@@ +103,5 @@
>  struct nsIntPoint;
>  struct nsIntRect;
>  class nsRefreshDriver;
>  class nsARefreshObserver;
> +class imgIRequest;

Why did you need to add this here?

::: layout/generic/nsImageFrame.cpp
@@ +164,5 @@
>  {
>    return new (aPresShell) nsImageFrame(aContext);
>  }
>  
> +NS_IMPL_FRAMEARENA_HELPERS(nsImageFrame);

Seems unneeded/wrong.
(Assignee)

Comment 120

8 years ago
Fixed issues found during review and modified the nsImageFrame to only register imgIRequest objects with the refresh driver if its an animation or during decoding.
Attachment #552012 - Attachment is obsolete: true
Attachment #552012 - Flags: review?(roc)
(Assignee)

Comment 121

8 years ago
Simply rebased the patch to the tip of mozilla-central.
Attachment #545740 - Attachment is obsolete: true
Attachment #552277 - Flags: superreview+
Attachment #552277 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #552015 - Attachment description: Patch 3 (v8) - Implementation for RasterImage [r=dholbert] → Patch 4 (v8) - Implementation for RasterImage [r=dholbert]
(Assignee)

Comment 122

8 years ago
Implementation for tab icon and favicon animations.
(Assignee)

Comment 123

8 years ago
Comment on attachment 552276 [details] [diff] [review]
Patch 2 (v8) - Implementation for nsImageFrame

I'm re-requesting review after fixing issues described during previous review. I also added some code that will remove the imgIRequest as an observer from the nsRefreshDriver once decoding has stopped and the image isn't an animation. The imgIRequest is re-added to the nsRefreshDriver in OnStartDecode again, as it's necessary for other work that is being built on top of this for the nsRefreshDriver to notify images while they are being decoded.
Attachment #552276 - Flags: review?(roc)
(In reply to Scott Johnson (:jwir3) from comment #122)
> Created attachment 552288 [details] [diff] [review]
> Patch 3 - Implementation for nsImageBoxFrame
> 
> Implementation for tab icon and favicon animations.

(Note: we're shifting away from allowing animated favicons -- see bug 111373 comment 49 & after -- but I imagine we'll still need this patch for the throbber animation during pageload.)
(Assignee)

Comment 125

8 years ago
Changed from nsTArray<nsRefPtr<imgIRequest> > to nsTArray<nsCOMPtr<imgIRequest> > (see bug 676603).
Attachment #551977 - Attachment is obsolete: true
(Assignee)

Comment 126

8 years ago
> (Note: we're shifting away from allowing animated favicons -- see bug 111373
> comment 49 & after -- but I imagine we'll still need this patch for the
> throbber animation during pageload.)

Yeah, actually that's where I noticed there was a problem. ;) I didn't really notice that when you load an animated gif directly, a smaller version of the gif appears as the favicon. But, I did notice that the progress indicator didn't move, which made me suspicious something was wrong. :) At any rate, the nsImageBoxFrame falls into the category of 'nsImageFrame-like' classes, so it needed to be fixed anyway.
Two things:
1) I think we should make the imgIRequest table in the refresh driver be a hash-set. Right now if you have a large number of images removing all the imgIRequests could easily be O(N^2).
2) I think we should add an optimization to track in nsImageFrame whether its imgIRequest is in the refresh driver. If it isn't, we shouldn't try to remove it. This will be more efficient and stop warning spam.
(Assignee)

Comment 128

8 years ago
Changed commit message to include review notation and more descriptive comments.
Attachment #552277 - Attachment is obsolete: true
(Assignee)

Comment 129

8 years ago
Minor changes to commit message to add more descriptive commentary.
Attachment #552289 - Attachment is obsolete: true
(Assignee)

Comment 130

8 years ago
Modified according to roc's review comments and changed commit message to include more descriptive commentary.
Attachment #552276 - Attachment is obsolete: true
Attachment #552276 - Flags: review?(roc)
(Assignee)

Comment 131

8 years ago
Modified commit message to be more informative.
Attachment #552288 - Attachment is obsolete: true
(Assignee)

Comment 132

8 years ago
Modified commit message to indicate reviewer and be more informative.
Attachment #552015 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #553051 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #553052 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #553055 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #553058 - Flags: review?(joe)
Comment on attachment 553051 [details] [diff] [review]
Patch 1 (v3) - Implementation for nsRefreshDriver

Review of attachment 553051 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +188,5 @@
> +nsRefreshDriver::AddImageRequest(imgIRequest* aRequest)
> +{
> +  nsPtrHashKey<imgIRequest>* hashKey = mRequests.PutEntry(aRequest);
> +  if (!hashKey) {
> +    return PR_FALSE;

Can just write
  if (!mRequests.PutEntry(aRequest))
    return PR_FALSE;

@@ +200,5 @@
> +PRBool
> +nsRefreshDriver::RemoveImageRequest(imgIRequest* aRequest)
> +{
> +  if (!mRequests.GetEntry(aRequest)) {
> +    return PR_FALSE;

Why do this check? This doubles the time taken in this function. Just call RemoveEntry and return void from this function.

@@ +274,5 @@
>    return sum;
>  }
>  
> +PRUint32
> +nsRefreshDriver::RequestCount() const

ImageRequestCount

@@ +479,5 @@
> +    static_cast<ImageRequestParameters*> (aUserArg);
> +  mozilla::TimeStamp* mostRecentRefresh = parms->ts;
> +  bool* didRefresh = parms->didRefresh;
> +  imgIRequest* req = aEntry->GetKey();
> +  NS_WARN_IF_FALSE(req, "Unable to retrieve the image request");

Might as well be NS_ASSERTION since we'll crash if it's null

@@ +482,5 @@
> +  imgIRequest* req = aEntry->GetKey();
> +  NS_WARN_IF_FALSE(req, "Unable to retrieve the image request");
> +  nsCOMPtr<imgIContainer> image;
> +  req->GetImage(getter_AddRefs(image));
> +  if (image.get()) {

if (image) {

@@ +486,5 @@
> +  if (image.get()) {
> +    image->RequestRefresh(*mostRecentRefresh);
> +
> +    if (didRefresh) {
> +      *didRefresh = true;

So didRefresh always gets set to true if the image request table is non-empty? Then you might as well remove the boolean and just test that instead.

::: layout/base/nsRefreshDriver.h
@@ +276,5 @@
>    bool mTimerIsPrecise;
>  
>    // separate arrays for each flush type we support
>    ObserverArray mObservers[3];
> +  RequestArray mRequests;

Call it RequestTable, since it's not an array.

@@ +292,5 @@
> +
> +  // Helper struct for processing image requests
> +  struct ImageRequestParameters {
> +      mozilla::TimeStamp* ts;
> +      bool* didRefresh;

Instead of using pointers, just use values; copy the timestamp in here, and the boolean can live in here directly.
Comment on attachment 553052 [details] [diff] [review]
Patch 2 (v8) - Implementation for nsImageFrame

Review of attachment 553052 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +280,5 @@
>    // Set the animation mode here
>    if (currentRequest) {
> +
> +    // register the imgIRequest with the refresh driver
> +    presContext->RefreshDriver()->AddImageRequest(currentRequest);

Should probably only set mRequestRegistered if this returns true.

@@ +621,5 @@
> +{
> +  nsPresContext* presContext = PresContext();
> +  if (!(presContext->RefreshDriver()->AddImageRequest(aRequest))) {
> +    return NS_ERROR_FAILURE;
> +  }

Shouldn't you be setting mRequestRegistered here?

@@ +703,5 @@
> +                      "Unable to deregister imgIRequest "
> +                      "with a null presContext");
> +    nsCOMPtr<imgIRequest> currentRequest;
> +    aImageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
> +                          getter_AddRefs(currentRequest));

Fix indent
(Assignee)

Comment 135

8 years ago
> Can just write
>   if (!mRequests.PutEntry(aRequest))
>     return PR_FALSE;

Ok, made this change.

> Why do this check? This doubles the time taken in this function. Just call 
> RemoveEntry and return void from this function.

Modified to reflect this.

> ImageRequestCount

Made this change.

> Might as well be NS_ASSERTION since we'll crash if it's null

Agreed. I changed this as well.

> if (image) {

I originally had this this way, but I was finding sometimes that the smart pointer that was returned was nonnull, but that the actual mRawPtr within it was 0x0, causing it to get past this check, but still segfault. I thought that checking the actual raw pointer was hackish, but perhaps there is another method for verifying that GetImage() returns a valid raw pointer?

> So didRefresh always gets set to true if the image request table is 
> non-empty? Then you might as well remove the boolean and just test that 
> instead.

Well, didRefresh is supposed to be true if at least one of the imgIRequests subscribed for notification successfully performed a refresh. This will likely amount to the table being non-empty, but I didn't know if it was possible that perhaps there might be an image request that was subscribed for notification, but for whatever reason, we failed in the GetImage() call, thus resulting in the RequestRefresh() method not being invoked.

I could probably simplify this, if you think we can ignore this strange corner case.

> Call it RequestTable, since it's not an array.
Done.

> Instead of using pointers, just use values; copy the timestamp in here, and 
> the boolean can live in here directly.
Sounds good. I'll make that change.
(Assignee)

Comment 136

8 years ago
> Should probably only set mRequestRegistered if this returns true.
Added an if statement to check for this.

> Shouldn't you be setting mRequestRegistered here?
Yes, thanks for the catch.

> Fix indent
Done.
(Assignee)

Comment 137

8 years ago
More review findings fixed.
Attachment #553052 - Attachment is obsolete: true
Attachment #553090 - Flags: review?(roc)
Attachment #553052 - Flags: review?(roc)
(Assignee)

Comment 138

8 years ago
Minor fix to propagate some changes made to patch 2.
Attachment #553055 - Attachment is obsolete: true
Attachment #553094 - Flags: review+
(In reply to Scott Johnson (:jwir3) from comment #135)
> > if (image) {
> 
> I originally had this this way, but I was finding sometimes that the smart
> pointer that was returned was nonnull, but that the actual mRawPtr within it
> was 0x0, causing it to get past this check, but still segfault.

I don't know what you mean by this. I believe "if (image)" does an implicit coercion to T*, effectively the same as a get().

> > So didRefresh always gets set to true if the image request table is 
> > non-empty? Then you might as well remove the boolean and just test that 
> > instead.
> 
> Well, didRefresh is supposed to be true if at least one of the imgIRequests
> subscribed for notification successfully performed a refresh. This will
> likely amount to the table being non-empty, but I didn't know if it was
> possible that perhaps there might be an image request that was subscribed
> for notification, but for whatever reason, we failed in the GetImage() call,
> thus resulting in the RequestRefresh() method not being invoked.

GetImage should never fail. Maybe it does in exceptional circumstances (OOM or something) but it's OK to do an unnecessary refresh in such cases.
(Assignee)

Comment 140

8 years ago
> I don't know what you mean by this. I believe "if (image)" does an implicit 
> coercion to T*, effectively the same as a get().

Hm... yes, I think I was mistaken about the location I previously mentioned that there was a segfault.

> GetImage should never fail. Maybe it does in exceptional circumstances (OOM or 
> something) but it's OK to do an unnecessary refresh in such cases.

Ok, I'll make that change then.
(Assignee)

Comment 141

8 years ago
Shifted a couple of things around that belonged in a different patch.
Attachment #553090 - Attachment is obsolete: true
Attachment #553106 - Flags: review?(roc)
Attachment #553090 - Flags: review?(roc)
(Assignee)

Comment 142

8 years ago
Shifted a couple of things around that belonged in a different patch.
Attachment #553051 - Attachment is obsolete: true
Attachment #553107 - Flags: review?(roc)
Attachment #553051 - Flags: review?(roc)
Comment on attachment 553107 [details] [diff] [review]
Patch 1 (v4) - Implementation for nsRefreshDriver

Review of attachment 553107 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't think didRefresh is needed. We can just check if mRequests is empty.
(Assignee)

Comment 144

8 years ago
> I still don't think didRefresh is needed. We can just check if mRequests is 
> empty.

Agreed, I removed it from this latest patch.
(Assignee)

Comment 145

8 years ago
Modifications to overcome a few review comments.
Also, I changed the hash key type to nsISupportsHashKey, as I was getting Segfaults without this where the parent container of an imgIRequest (the nsISupports pointer in the vtable) was becoming messed up. This seems to have fixed that issue.
Attachment #553107 - Attachment is obsolete: true
Attachment #553341 - Flags: review?(roc)
Attachment #553107 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #553050 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #553050 - Flags: superreview+
(Assignee)

Updated

8 years ago
Attachment #553058 - Attachment description: Patch 4 (v9) Implementation for RasterImage [r=dholbert] → Patch 5 (v9) Implementation for RasterImage [r=dholbert]
(Assignee)

Comment 146

8 years ago
Implementation for the next of the 'nsImageFrame-like' classes: nsBulletFrame. I have a (non-automated test) I will post if you want to run it, as well.
Attachment #553350 - Flags: review?(roc)
(Assignee)

Comment 147

8 years ago
A test case for nsBulletFrame.
Comment on attachment 553350 [details] [diff] [review]
Patch 4 Implementation for nsBulletFrame

Review of attachment 553350 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK but I think we should do CSS background images first and then figure out how to share as much code as possible.
Comment on attachment 553058 [details] [diff] [review]
Patch 5 (v9) Implementation for RasterImage [r=dholbert]

Review of attachment 553058 [details] [diff] [review]:
-----------------------------------------------------------------

r=me as long as you add tests and correct the below.

::: modules/libpr0n/src/RasterImage.cpp
@@ +324,5 @@
> +  // Figure out if we have the next full frame. This is more complicated than
> +  // just checking for mFrames.Length() because decoders append their frames
> +  // before they're filled in.
> +  NS_ABORT_IF_FALSE(mDecoder || nextFrameIndex <= mFrames.Length(),
> +      "How did we get 2 indices too far by incrementing?");

Correctly indent this second line.

@@ +425,5 @@
> +  nsIntRect dirtyRect;
> +
> +  while (currentFrameEndTime <= aTime) {
> +    TimeStamp oldFrameEndTime = currentFrameEndTime;
> +    frameAdvanced = frameAdvanced || AdvanceFrame(aTime, &dirtyRect);

Am I misreading this, or will this short-circuit and never advance past the first frame?

(Either way, you should add a test to make sure we can advance multiple frames via the refresh driver.)

@@ +1308,5 @@
>      }
> +
> +    // We need to set the time that this initial frame was first displayed.
> +    // This is important for the new refresh driver-based animation
> +    // timing.

No need to call it "new" here, because time-based references in code always get hilariously out of date. :)

Just say the first line in this sentence, and maybe explain that it's used in AdvanceFrame.
Attachment #553058 - Flags: review?(joe) → review+
(Assignee)

Comment 150

8 years ago
> as long as you add tests
Definitely. I have some tests that I am going to add as separate patches. I haven't posted them yet, because they aren't working completely yet. Once they are working, I will post these. They include a basic animated gif mochitest, a crash test for multi-frame animated gifs, and a test for bullet animations. I will likely have others, but I haven't finished them yet. 

> Correctly indent this second line.
Will do.

> Am I misreading this, or will this short-circuit and never advance past the 
> first frame?
Hm, yes, I think you're correct. If the frameAdvanced variable is true, it won't actually call AdvanceFrame(). I'll fix this so that it calls AdvanceFrame() in succession, regardless of whether it previously advanced a frame.

> (Either way, you should add a test to make sure we can advance multiple 
> frames via the refresh driver.)
Sure, I'll add a test for this as soon as I finish up changing the nsImageFrame-like classes.

> No need to call it "new" here, because time-based references in code always 
> get hilariously out of date. :)
Sure. I'll remove this time-based comment. I started off calling it 'new' for a while, but realized this would get out of date very quickly, so haven't done that in the newest patches I posted.
(Assignee)

Comment 151

8 years ago
Implementation of RasterImage with JOEDREW!'s review findings addressed.
Attachment #553058 - Attachment is obsolete: true
Attachment #553658 - Flags: review+
(Assignee)

Comment 152

8 years ago
Implementation of nsImageLoader (background images) using refresh driver.
Attachment #553659 - Flags: review?(roc)
(Assignee)

Comment 153

8 years ago
Coalesced a bunch of the duplicate code from other frames to utilize the refresh driver into nsLayoutUtils and added calls to these functions.
Attachment #553350 - Attachment is obsolete: true
Attachment #553660 - Flags: review?(roc)
Attachment #553350 - Flags: review?(roc)
(Assignee)

Comment 154

8 years ago
Coalesced a bunch of the duplicate code from other frames to utilize the refresh driver into nsLayoutUtils and added calls to these functions.
Attachment #553094 - Attachment is obsolete: true
Attachment #553661 - Flags: review+
(Assignee)

Comment 155

8 years ago
Coalesced a bunch of the duplicate code from other frames to utilize the refresh driver into nsLayoutUtils and added calls to these functions.
Attachment #553106 - Attachment is obsolete: true
Attachment #553662 - Flags: review+
(Assignee)

Comment 156

8 years ago
Added reviewer note in commit message.
Attachment #553662 - Attachment is obsolete: true
Attachment #553663 - Flags: review+
(Assignee)

Comment 157

8 years ago
Implementation of nsLayoutUtils' coalesced code to avoid duplication in multiple frames.
Attachment #553665 - Flags: review?(roc)
(Assignee)

Comment 158

8 years ago
Attachment #553341 - Attachment is obsolete: true
Attachment #553666 - Flags: review+
(Assignee)

Comment 159

8 years ago
No changes, just reposting with a different patch # to make things consistent and clear which patches to apply first.
Attachment #553050 - Attachment is obsolete: true
Attachment #553667 - Flags: superreview+
Attachment #553667 - Flags: review+
Comment on attachment 553665 [details] [diff] [review]
Patch 3 - Implementation for nsLayoutUtils

Review of attachment 553665 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed

::: layout/base/nsLayoutUtils.cpp
@@ +4338,5 @@
> +                                      bool* aRegistered)
> +{
> +  // Deregister our imgIRequest with the refresh driver to
> +  // complete tear-down, but only if it has been registered
> +  if (*aRegistered) {

Less indentation to do an early exit:
  if (!*aRegistered || !aFrame)
    return;

@@ +4346,5 @@
> +                        "Unable to deregister imgIRequest "
> +                        "with a null presContext");
> +      presContext->RefreshDriver()->RemoveImageRequest(aRequest);
> +
> +      if (aRegistered) {

Don't test aRegistered, it should never be null (and if it was, you'd have crashed already)

::: layout/base/nsLayoutUtils.h
@@ +1436,5 @@
> +   *        register with the refresh driver of the frame.
> +   * @param aRegistered A pointer to a boolean value indicating whether this
> +   *        imgIRequest object has already been registered with the refresh
> +   *        driver for this frame. It is the responsibility of the caller to
> +   *        maintain this boolean value, but the function will utilize it

It's not really the caller's responsibility. RegisterImageRequest will set it. Better to say that this value tracks whether the image is registered, and so of course it must initially be false, and these three APIs all use and maintain the value. Fix all three versions of this comment.
Attachment #553665 - Flags: review?(roc) → review+
Comment on attachment 553663 [details] [diff] [review]
Patch 4 (v9) - Implementation for nsImageFrame [r=roc]

Review of attachment 553663 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.h
@@ +265,5 @@
> +   *
> +   * @param aImageLoader An nsIImageLoadingContent object used to retrieve the
> +   *        current imgIRequest.
> +   */
> +  void DeregisterFromRefreshDriver(nsCOMPtr<nsIImageLoadingContent> aImageLoader);

This should go away now?
(Assignee)

Comment 162

8 years ago
> This should go away now?
Yep. Fixed in this version of the patch.
Attachment #553663 - Attachment is obsolete: true
Attachment #553672 - Flags: review+
(Assignee)

Comment 163

8 years ago
> Less indentation to do an early exit:
>   if (!*aRegistered || !aFrame)
>     return;
Fixed.

> Don't test aRegistered, it should never be null (and if it was, you'd have 
> crashed already)
Ok. Fixed.

> It's not really the caller's responsibility. RegisterImageRequest will set 
> it. Better to say that this value tracks whether the image is registered, and 
> so of course it must initially be false, and these three APIs all use and 
> maintain the value. Fix all three versions of this comment.
Reworded to make this a bit more clear.
Attachment #553665 - Attachment is obsolete: true
Attachment #553674 - Flags: review+
(Assignee)

Comment 164

8 years ago
Implementation for animated GIFs referenced within <image> elements embedded in svg documents (ok, so that was a mouthful).
Attachment #553686 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #553658 - Attachment description: Patch 8 (v9) - Implementation for RasterImage [r=dholbert,JOEDREW!] → Patch 9 (v9) - Implementation for RasterImage [r=dholbert,JOEDREW!]
(Assignee)

Comment 165

8 years ago
This is a test to verify that animated gif images work as expected. It loads two images (reference frames), and a two-frame animated gif. It currently only checks to verify that the last frame matches, but I've included a second frame so that in the future it will be easier to extend to check each frame individually.
Attachment #554990 - Flags: review?(joe)
(Assignee)

Comment 166

8 years ago
Fixed a bug found by attachment 554990 [details] [diff] [review] where a non-looping animated gif would reset to frame 0 after it had completed.
Attachment #553658 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #554991 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #554991 - Attachment description: Patch 9 (v10) - Implementation for RasterImage [r=dholbert,JOEDREW!] → Patch 10 (v10) - Implementation for RasterImage [r=dholbert,JOEDREW!]
(Assignee)

Comment 167

8 years ago
This is the implementation for the XUL tree image listener (mostly nsTreeBodyFrame and nsTreeImageListener). Just to jump ahead of an issue likely to come up in review, you'll see that I removed the inclusion of nsTreeImageListener.h from nsTreeBodyFrame.h, and added it to nsTreeBodyFrame.cpp, replacing the occurrence in the .h file with a forward declaration. This is so that I can include nsTreeBodyFrame.h in nsTreeImageListener.h, as I was getting a forward declaration compile error with just the class defined at the top of the file. Including the entire .h file actually helped resolve this issue, and I'm just being explicit about this in case you have questions about that stuff.
Attachment #555439 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #554991 - Attachment description: Patch 10 (v10) - Implementation for RasterImage [r=dholbert,JOEDREW!] → Patch 11 (v10) - Implementation for RasterImage [r=dholbert,JOEDREW!]
(Assignee)

Comment 168

8 years ago
This should be the last layout component that needs to change with this patch, the SVG filter image. I will shortly be uploading tests for as many of these components as I am able to write reasonable tests for.
Attachment #555496 - Flags: review?(roc)
Comment on attachment 555439 [details] [diff] [review]
Patch 9 - Implementation for xul tree

Review of attachment 555439 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +119,5 @@
> +  // because we might have multiple imgIRequests registered from the frame,
> +  // and we don't want to mark all of the imgIRequests as deregistered until
> +  // they actually are.
> +  bool requestRegistered = true;
> +  nsTreeBodyFrame* frame = static_cast<nsTreeBodyFrame*> (aData);

No space between > and (

@@ +4485,5 @@
>    }
>  }
>  
> +void
> +nsTreeBodyFrame::ClearCreatedListeners()

Call this DetachImageListeners, it's clearer

@@ +4670,5 @@
> +
> +nsresult
> +nsTreeBodyFrame::OnStartDecode(imgIRequest* aRequest)
> +{
> +  nsLayoutUtils::RegisterImageRequest(this, aRequest, &mRequestRegistered);

If there are multiple requests at a time (which there are, right?) then this won't work because for the second and subsequent requests mRequestRegistered will be true already.

It seems to me we should just not have an mRequestRegistered on nsTreeBodyFrame.

Probably instead of having a dummy variable we should let the bool pointer be null, and if it's null, then we'd just make pessimistic assumptions in those helper methods.

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.h
@@ +617,5 @@
>    PRPackedBool mReflowCallbackPosted;
> +
> +  // Array to keep track of which listeners we created and thus
> +  // have pointers to us.
> +  nsTArray<nsTreeImageListener*> mCreatedListeners;

Nothing keeps these alive, it seems like you'll have dangling pointers here if the listener is released and goes away?

I think the listeners' destructor should call back to the frame to remove the listener from the array.

Also, if we have a lot of listeners, removing from the array might get expensive, so we probably should use a hash-set here.

::: layout/xul/base/src/tree/src/nsTreeImageListener.h
@@ +97,5 @@
> +  /**
> +   * Clear the internal frame pointer to prevent dereferencing an object
> +   * that no longer exists.
> +   */
> +  void ClearFrame();

This can just be declared inline here.
Comment on attachment 555496 [details] [diff] [review]
Patch 10 - Implementation of nsSVGFEImageElement

Review of attachment 555496 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGFilters.cpp
@@ +5420,5 @@
>    AddStatesSilently(NS_EVENT_STATE_BROKEN);
> +
> +  // Register our image request with the refresh driver.
> +  nsLayoutUtils::RegisterImageRequest(GetPrimaryFrame(), mCurrentRequest,
> +                                      &mRequestRegistered);

Is mCurrentRequest ever non-null here? If so, how?

@@ +5428,5 @@
>  {
> +  if (mCurrentRequest) {
> +    nsIFrame *frame = GetPrimaryFrame();
> +    nsLayoutUtils::DeregisterImageRequest(frame, mCurrentRequest,
> +                                          &mRequestRegistered);

"frame" could be null here. For example, the element could be display:none.

@@ +5652,5 @@
> +nsSVGFEImageElement::OnStartDecode(imgIRequest *aRequest)
> +{
> +  // Register with the refresh driver.
> +  nsIFrame *frame = GetPrimaryFrame();
> +  nsLayoutUtils::RegisterImageRequest(frame, aRequest, &mRequestRegistered);

"frame" could be null here.

@@ +5665,5 @@
>  {
> +  // If we're not animated, then we want to deregister from the refresh driver.
> +  nsIFrame *frame = GetPrimaryFrame();
> +  nsLayoutUtils::DeregisterImageRequestIfNotAnimated(frame, aRequest,
> +                                                     &mRequestRegistered);

"frame" could be null here too.
(Assignee)

Updated

8 years ago
Depends on: 682077
(Assignee)

Comment 171

8 years ago
> No space between > and (
Fixed.

> Call this DetachImageListeners, it's clearer
Fixed.

> If there are multiple requests at a time (which there are, right?) then this won't work because for the second and subsequent
> requests mRequestRegistered will be true already.
> It seems to me we should just not have an mRequestRegistered on nsTreeBodyFrame.
> Probably instead of having a dummy variable we should let the bool pointer be null, and if it's null, then we'd just make 
> pessimistic assumptions in those helper methods.

True. I missed this occurrence of the issue, but it's been fixed now as you suggested. The handling of the null boolean parameter is included in the patch I will post next, in nsLayoutUtils.

> Nothing keeps these alive, it seems like you'll have dangling pointers here if the listener is released and goes away?
> I think the listeners' destructor should call back to the frame to remove the listener from the array.
Done.

> Also, if we have a lot of listeners, removing from the array might get expensive, so we probably should use a hash-set here.
I implemented this. Because of the inheritance structure of nsTreeImageListener, it took me a long time to figure out exactly how this should be done. khuey thinks we should remove the nsITreeImageListener altogether (see bug 682077), but I'm not sure this is possible because of the way we query the interface here: http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2146

I had to separate out this interface into a separate header file, though because of circular problems. It breaks blame, unfortunately, but I didn't know how else to handle this.

> This can just be declared inline here.
I originally did this, but then I had to pull it out into the interface in order to utilize QueryInterface() on the nsITreeImageListener but then actually utilize this function in DetachImageListener() (the hashtable's enumerator function), so it's no longer declared inline, but there is a reason for it now. :)
Attachment #555439 - Attachment is obsolete: true
Attachment #555860 - Flags: review?(roc)
Attachment #555439 - Flags: review?(roc)
(Assignee)

Comment 172

8 years ago
> "frame" could be null here. For example, the element could be display:none.
> "frame" could be null here.
> "frame" could be null here too.

I'm not exactly sure what you mean about these comments... I see what you are saying about frame possibly being null, but it's ok even if it is null, because Register/Deregister in nsLayoutUtils checks to make sure the frame passed in is non-null before doing anything - if the frame passed in is null, then it simply returns without performing any action.
(Assignee)

Comment 173

8 years ago
Just re-submitting for review so you can verify that I handled the situation where the boolean pointer argument might be null.
Attachment #553674 - Attachment is obsolete: true
Attachment #555863 - Flags: review?(roc)
(Assignee)

Comment 174

8 years ago
> Is mCurrentRequest ever non-null here? If so, how?

No, I don't think it is. The odd thing is that if I remove that statement from the constructor, the animation never runs (i.e. it simply stays on the first frame). But, if I add that call into the constructor, from what I can tell in gdb, it's not doing anything (just returning early), but the animation runs. 

I'll have to look into this further.
(In reply to Scott Johnson (:jwir3) from comment #172)
> > "frame" could be null here. For example, the element could be display:none.
> > "frame" could be null here.
> > "frame" could be null here too.
> 
> I'm not exactly sure what you mean about these comments... I see what you
> are saying about frame possibly being null, but it's ok even if it is null,
> because Register/Deregister in nsLayoutUtils checks to make sure the frame
> passed in is non-null before doing anything - if the frame passed in is
> null, then it simply returns without performing any action.

OK, then what happens if the image starts loading while the element has a frame, but then while the image is loading it gets made display:none and the frame goes away? It seems the request will never be unregistered.
Comment on attachment 555863 [details] [diff] [review]
Patch 3 (v3) - Implementation for nsLayoutUtils [r=roc]

Review of attachment 555863 [details] [diff] [review]:
-----------------------------------------------------------------

The code here is fine but it has the potential for misuse like I mentioned in my previous comment. Maybe it's better to not handle null frames here so the caller knows it has to clean up if a frame goes away.
(The handling of null aRegistered is fine.)
Comment on attachment 555860 [details] [diff] [review]
Patch 9 (v2) - Implementation for xul tree

Review of attachment 555860 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.h
@@ +627,5 @@
>    PRPackedBool mReflowCallbackPosted;
> +
> +  // Hash table to keep track of which listeners we created and thus
> +  // have pointers to us.
> +  nsTHashtable<nsISupportsHashKey> mCreatedListeners;

I think you can just use nsTHashtable<nsPtrHashKey<nsTreeImageListener>> here. You don't need to hold a strong reference since any listener in the table will remove itself via RemoveTreeImageListener before it dies.

In fact, I think you have a quasi-leak right now, at least as long as the frame stays alive. This hashtable keeps the listeners alive. The listeners won't remove themselves because they only remove themselves in their destructor, which won't be called as long as the frame is keeping them alive!
(Assignee)

Comment 179

8 years ago
> Is mCurrentRequest ever non-null here? If so, how?
Posting new patch that removes this dead code.
Attachment #555496 - Attachment is obsolete: true
Attachment #556068 - Flags: review?(roc)
Attachment #555496 - Flags: review?(roc)
(Assignee)

Comment 180

8 years ago
Another mochitest for this bug. More tests are coming as soon as I get them finished.
Attachment #556123 - Flags: review?(roc)
(Assignee)

Comment 181

8 years ago
Changed the timeout to 2 minutes.
Attachment #554990 - Attachment is obsolete: true
Attachment #554990 - Flags: review?(joe)
(In reply to Scott Johnson (:jwir3) from comment #179)
> Created attachment 556068 [details] [diff] [review]
> Patch 10 (v2) - Implementation of nsSVGFEImageElement
> 
> > Is mCurrentRequest ever non-null here? If so, how?
> Posting new patch that removes this dead code.

What about comment #175?
Comment on attachment 556123 [details] [diff] [review]
Mochitest 2 - Test for functionality of nsSVGImageFrame component

Review of attachment 556123 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that.

::: modules/libpr0n/test/mochitest/Makefile.in
@@ +98,5 @@
>                  test_animation.html \
>                  animated-gif-frame1.gif \
>                  animated-gif-frame2.gif \
>                  animated-gif.gif \
> +				test_svg_animatedGIF.html \

Fix indent

::: modules/libpr0n/test/mochitest/test_svg_animatedGIF.html
@@ +102,5 @@
> +}
> +
> +function doMyOnStopFrame() {
> +  myOnStopFrame();
> +  setTimeout(doMyOnStopFrame, 1000);

I think we should do this every 10ms. No reason not to and it'll make the test finish faster.
Attachment #556123 - Flags: review?(roc) → review+
(Assignee)

Comment 184

8 years ago
> What about comment #175?

I'm looking at that right now. It looks like all of the other functions in nsLayoutUtils that require a frame do an NS_PRECONDITION, which, iiuc, is the same as an NS_ASSERTION. MDN says not to use NS_ASSERTION, but instead use NS_ABORT_IF_FALSE. 

Should I go with the standard of the file and use NS_PRECONDITION, or use NS_ABORT_IF_FALSE if aFrame is null?
(Assignee)

Comment 185

8 years ago
(Assignee)

Comment 186

8 years ago
Only doesn't work if viewed directly with all patches up to patch 11 applied.
(Assignee)

Comment 187

8 years ago
I just noticed that one of my test cases for SVG filters works fine (attachment 556141 [details]) when the SVG file is placed into the HTML document using the <embed> tag. If I view the svg file directly, though, the animation doesn't play (attachment 556142 [details]). It does play in the current nightly, so I'm wondering if this is a different class that I haven't actually modified yet.

Strangely enough, if I add that dead code back into the nsSVGFEImageElement patch (the call to RegisterImageRequest), it will work if I view the SVG file directly, even though it really shouldn't do anything. 

After running this through gdb, it looks like mAnimating isn't getting set correctly for the imgIRequest used in the nsSVGFEImageElement - but only sometimes. If I keep hitting f5 over and over, it will eventually load the animation and start playing.
(Assignee)

Comment 188

8 years ago
Accidentally didn't use data uri with this attachment.
Attachment #556141 - Attachment is obsolete: true
Attachment #556142 - Attachment is patch: false
Attachment #556142 - Attachment mime type: text/plain → image/svg+xml
(Assignee)

Comment 189

8 years ago
dholbert has been assisting me with the problem I describe in comment 187. We believe this to be caused by a race condition. It seems that in ShouldAnimate(), the number of animation consumers is not getting appropriately incremented. ShouldAnimate() is returning false due to mAnimationConsumers being 0. 

However, imgRequestProxy::IncrementAnimationConsumers() is getting called for the imgIRequest representing the animated image from nsDocument::AddImage. Stepping through it with gdb causes the image to being animating, which dholbert and I think is probably more evidence that it's a race condition. 

I will keep investigating it and hopefully have an answer early next week.

Updated

8 years ago
Blocks: 682638
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #170)
> @@ +5652,5 @@
> > +nsSVGFEImageElement::OnStartDecode(imgIRequest *aRequest)
> > +{
> > +  // Register with the refresh driver.
> > +  nsIFrame *frame = GetPrimaryFrame();
> > +  nsLayoutUtils::RegisterImageRequest(frame, aRequest, &mRequestRegistered);
> 
> "frame" could be null here.

From a bit of debugging, I believe Comment 187 - 189 are due to this chunk that roc highlighted -- we don't have a frame yet, at the point where we're trying to register for callbacks from the refresh driver.  (and no frame --> no registration)  So there's basically a race condition between frame creation & OnStartDecode being called.  (and if OnStartDecode wins the race, we never get registered for refresh callbacks)

While jwir3's existing testcase sometimes loads correctly (e.g. with <embed> per comment 187) & sometimes fails, here's a variant with an initial "display:none" for 1 second, which delays frame-creation and makes us always lose the race (even in a testcase with <embed>)
(Assignee)

Comment 191

8 years ago
(In reply to Daniel Holbert [:dholbert] from comment #190)

> From a bit of debugging, I believe Comment 187 - 189 are due to this chunk
> that roc highlighted -- we don't have a frame yet, at the point where we're
> trying to register for callbacks from the refresh driver.  (and no frame -->
> no registration)  So there's basically a race condition between frame
> creation & OnStartDecode being called.  (and if OnStartDecode wins the race,
> we never get registered for refresh callbacks)

Thanks for assisting me with this. I will fix these functions. 
 
> While jwir3's existing testcase sometimes loads correctly (e.g. with <embed>
> per comment 187) & sometimes fails, here's a variant with an initial
> "display:none" for 1 second, which delays frame-creation and makes us always
> lose the race (even in a testcase with <embed>)

Great. I'll add a mochitest for this specific problem. Hopefully, I'll have it finished today & I'll post it to the bug.
Comment on attachment 556068 [details] [diff] [review]
Patch 10 (v2) - Implementation of nsSVGFEImageElement

One nit on the nsSVGFEImageElement patch:

>+++ b/content/svg/content/src/nsSVGFilters.cpp
> private:
>+  // Flag to indicate whether or not our imgIRequest was registered with the
>+  // refresh driver.
>+  bool mRequestRegistered;
>+
>   // Invalidate users of the filter containing this element.
>   void Invalidate();
> 
>   nsresult LoadSVGImage(PRBool aForce, PRBool aNotify);
> 
> protected:
>   virtual PRBool OperatesOnSRGB(nsSVGFilterInstance*,
>                                 PRInt32, Image*) { return PR_TRUE; }

(1) generally, moz style is to put member var declarations after function declarations.

(2) I'd suggest sticking 'mRequestRegistered' in the protected block, with nsSVGFEImageElement's other member variables, to keep that class somewhat organized (member variables all together) and to make your new variable easier to find.  (The private-vs.-protected distinction doesn't really matter anyway, since it has no subclasses.   And even if there were subclasses (which there never will be), those hypothetical subclasses could override ::Init, which would mean they might want to set mRequestRegistered themselves in their own Init method.)
No longer blocks: 682638
(Assignee)

Comment 193

8 years ago
There is a slight problem with the mRequestRegistered boolean value. It seems that some frames, such as nsImageBoxFrame, reuse the same frame with multiple image requests. So, the nsImageBoxFrame that is created to display the 'connecting' throbber is used again for the 'loading' throbber. Basically, when OnStartDecode is called again, the image request is different, but the nsImageBoxFrame is the same for both image requests - thus, the mRequestRegistered boolean value is already true. Thus, the second image doesn't get notifications for animations from the refresh driver (as it isn't registered).

I'm not sure how common this situation is, as I would think that this is a special case of reuse of the frame, as otherwise we might have different positioning information.
> I'm not sure how common this situation is

In general, pretty common.  Pages set a different src on images all the time.

> as otherwise we might have different positioning information

Not sure what you mean.
(Assignee)

Comment 195

8 years ago
> In general, pretty common.  Pages set a different src on images all the time.

Sure, but does this reuse an existing frame object, or create a new one?
It reuses the frame, if there's nothing else weird going on.  It does do relayout, but doesn't modify the object identity.
(Assignee)

Comment 197

8 years ago
Attachment #553667 - Attachment is obsolete: true
Attachment #557530 - Flags: superreview+
Attachment #557530 - Flags: review+
(Assignee)

Comment 198

8 years ago
Attachment #553666 - Attachment is obsolete: true
Attachment #557531 - Flags: review+
(Assignee)

Comment 200

8 years ago
Attachment #553672 - Attachment is obsolete: true
Attachment #557535 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #557533 - Flags: review+ → review?(roc)
(Assignee)

Comment 201

8 years ago
Attachment #553661 - Attachment is obsolete: true
Attachment #557537 - Flags: review?(roc)
(Assignee)

Comment 202

8 years ago
Attachment #553660 - Attachment is obsolete: true
Attachment #557539 - Flags: review?(roc)
(Assignee)

Comment 203

8 years ago
Attachment #553659 - Attachment is obsolete: true
Attachment #557540 - Flags: review?(roc)
(Assignee)

Comment 204

8 years ago
Attachment #553686 - Attachment is obsolete: true
Attachment #557541 - Flags: review?(roc)
(Assignee)

Comment 205

8 years ago
Attachment #555860 - Attachment is obsolete: true
Attachment #557542 - Flags: review?(roc)
Attachment #555860 - Flags: review?(roc)
(Assignee)

Comment 206

8 years ago
Attachment #556068 - Attachment is obsolete: true
Attachment #557544 - Flags: review?(roc)
Attachment #556068 - Flags: review?(roc)
(Assignee)

Comment 207

8 years ago
Attachment #554991 - Attachment is obsolete: true
Attachment #557545 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #555863 - Attachment is obsolete: true
Attachment #555863 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #541841 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #541840 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #556142 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #556143 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #556901 - Attachment is obsolete: true