867758 - Coordinate start of animation for same-FPS images

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

When we have a bunch of animated images on the same page (as is the case on the newtab page when you install Whimsy[1]), we do a *lot* of invalidation at all different times. I still need to run a few tests to prove that this will help, but Jeff and I came up with the following idea:

Instead of basing animated image refresh driver callbacks on the timeout, which means that we can storm along redrawing at slightly different times for every image on a page -- even if they're all the same frame rate -- we should base it on their *desired frame rate*, with the understanding that any individual image's first refresh can be slightly delayed.

At that point, all images that are animated at the same rate can be redrawn at the same time instead of having many different draws. Further, images that get redrawn at different rates but that match up occasionally could also have their draws batched up.

1. https://github.com/bwinton/whimsy

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #0)
> Instead of basing animated image refresh driver callbacks on the timeout,
> which means that we can storm along redrawing at slightly different times
> for every image on a page -- even if they're all the same frame rate -- we
> should base it on their *desired frame rate*

I sort of get the idea, but this isn't very clear.

Comment 2

[Joe Drew (not getting mail)]

Pretend that we're rendering 10 10fps animated images that are started 10 ms apart. If my reading of the code is correct, the refresh driver will cause screen updates every 10 ms for as long as those images are animated. This is because we call RequestRefresh on every animated image registered with a refresh driver on every refresh driver tick, and since they were all started at different times, they advance frames at different times.

If, instead, we relax the restriction on when images start animating, we can make it so that every image of the same FPS renders at the same time. This wouldn't be much of a relaxing of the requirements, since for most images we're talking about sub-frame delays. But it could make a big difference in the amount of redrawing we do.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I get that part. I still don't understand exactly what you're proposing to do.

Comment 4

[Joe Drew (not getting mail)]

Oh! My current thumbnail sketch of a plan is to register animated images with their desired FPS in the refresh driver, and to let the refresh driver inform that image when it should start animation rather than having it driven by the image itself. Then images with similar FPSes can be explicitly coordinated.

Comment 5

[Joe Drew (not getting mail)]

As far as I can tell, HTML5 contains no requirements on when precisely an animated image starts animating. We already know that people rely on images restarting their animation at certain times (eg reload, setting .src, etc), but this won't impact that.

So I'm (perhaps optimistically) going to say that delaying the first frame of an animated image is OK w.r.t. the web.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> HTML5 contains no requirements on when precisely an animated image starts animating

In that case it needs to grow some, honestly.  Web sites do depend on it in various cases.

But yes, I suspect that the change proposed here is probably ok.

Comment 7

[Seth Fowler [:seth] [:s2h]]

It seems to me we can usefully bucket images according to whether their frame rates are divisible. This would save even more invalidation since e.g. 5 FPS and 10 FPS images will coincide every other time the 5 FPS images are painted.

Comment 8

[Joe Drew (not getting mail)]

100% for sure! I definitely want that to be follow-on work, though, perhaps even as a mentored bug to get others involved in the code! :)

Comment 9

[Joe Drew (not getting mail)]

Attachment: Add an imgIContainer getter for the first frame's delay time

Comment 10

[Joe Drew (not getting mail)]

Attachment: Add imgIContainer::SetAnimationStartTime, which lets us explicitly set the time animation starts (rather than using TimeStamp::Now())

Comment 11

[Joe Drew (not getting mail)]

Attachment: Synchronize the first time we call RequestRefresh() on an image

This does the work that is necessary (but not quite sufficient; that part is upcoming) to synchronize images with the same frame rate.

We write down the start time for the first image of a particular frame rate. Then, for every subsequent image, we wait until the time we're at has crossed a multiple of that image's frame rate, and start doing RequestRefresh on that image.

Comment 12

[Joe Drew (not getting mail)]

Attachment: don't StartAnimation() immediately when we know an image is animated

This patch makes us not start images' animation until the refresh driver calls RequestRefresh() on them. This makes the synchronization in the previous patch work.

Comment 13

[Joe Drew (not getting mail)]

A try run with these patches (and the patch from bug 873505, also required): https://tbpl.mozilla.org/?tree=Try&rev=51916e54ef5d

I'm in the middle of working up a test for the functionality in this patch.

Comment 14

[Joe Drew (not getting mail)]

Attachment: Add imgIContainer::SetAnimationStartTime, which lets us explicitly set the time animation starts (rather than using TimeStamp::Now())

Try, happily, found problems in the previous patch. Here it is fixed.

https://tbpl.mozilla.org/?tree=Try&rev=54fda2729bda

Comment 15

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 751142 [details] [diff] [review]
Add an imgIContainer getter for the first frame's delay time

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

::: image/public/imgIContainer.idl
@@ +298,5 @@
> +   * Returns the delay, in ms, between the first and second frame. If this
> +   * returns 0, there is no delay between first and second frame (i.e., this
> +   * image could render differently whenever it draws).
> +   *
> +   * If this image is not animated, returns -1.

It also returns -1 if the image hasn't finished loading, it seems.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 751144 [details] [diff] [review]
Synchronize the first time we call RequestRefresh() on an image

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

r+ once RemoveImageRequest is fixed.

::: layout/base/nsRefreshDriver.cpp
@@ +439,5 @@
> +
> +  // If this image isn't animated, there isn't a first frame delay.
> +  int32_t delay = container->GetFirstFrameDelay();
> +  if (delay < 0)
> +    return 0;

GetFirstFrameDelay also returns -1 if the image hasn't loaded enough that we know whether it's animated. This is probably OK since the image doesn't get registered with the refresh driver until we know it's animated, though, right? (Which I assume means that GetFirstFrameDelay will also work.)

@@ +657,5 @@
>  nsRefreshDriver::RemoveImageRequest(imgIRequest* aRequest)
>  {
> +  uint32_t delay = GetFirstFrameDelay(aRequest);
> +  if (delay == 0) {
> +    mRequests.RemoveEntry(aRequest);

Don't all images end up in mRequests once we decide to start their first frame? Seems like we want to unconditionally try to remove the image from mRequests, and if that fails we can try mStartTable.

Comment 17

[Joe Drew (not getting mail)]

Attachment: test

Comment 18

[Joe Drew (not getting mail)]

https://tbpl.mozilla.org/?tree=Try&rev=c1d7f68328bb

Comment 19

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 752553 [details] [diff] [review]
test

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

Looks OK, though it's not clear to me how reliably this would fail with the old code.

Comment 20

[Joe Drew (not getting mail)]

(In reply to Seth Fowler [:seth] from comment #16)
> Don't all images end up in mRequests once we decide to start their first
> frame? Seems like we want to unconditionally try to remove the image from
> mRequests, and if that fails we can try mStartTable.

Yes, but RemoveEntry returns void, so we can't tell whether it's already in there. I could do a GetEntry() first to see whether it's in there, but that doesn't feel better.

Comment 21

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #20)
> (In reply to Seth Fowler [:seth] from comment #16)
> Yes, but RemoveEntry returns void, so we can't tell whether it's already in
> there. I could do a GetEntry() first to see whether it's in there, but that
> doesn't feel better.

That API sucks, but I still think your existing code isn't actually correct. I looked again and it still looks like it fails once BeginRefreshingImage is called for an image, since at that point it is removed from mStartTable->mEntries and is only found in mRequests, but RemoveImageRequest won't look for it there because its GetFirstFrameDelay is non-zero. I may be missing something though.

Comment 22

[Joe Drew (not getting mail)]

Ah, no, you're 100% correct. I'll just try to unconditionally remove from both tables.

Comment 23

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b55bea4d72
https://hg.mozilla.org/integration/mozilla-inbound/rev/420fe3994edf
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e0ec1acdf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f787efe558a
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa399729ff4

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f5b55bea4d72
https://hg.mozilla.org/mozilla-central/rev/420fe3994edf
https://hg.mozilla.org/mozilla-central/rev/86e0ec1acdf2
https://hg.mozilla.org/mozilla-central/rev/9f787efe558a
https://hg.mozilla.org/mozilla-central/rev/aaa399729ff4

Comment 25

[Joe Drew (not getting mail)]

Comment on attachment 751148 [details] [diff] [review]
don't StartAnimation() immediately when we know an image is animated

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression, a fix for bug 882050
User impact if declined: animated SVG images dynamically added to a document don't animate
Testing completed (on m-c, etc.): on m-c for a while, as part of a larger piece of work
Risk to taking this patch (and alternatives if risky): Sort of risky. This has never existed on its own, and this bug caused some regressions (though I don't think that this patch caused any of the regressions). A possible alternative is to just port the VectorImage hunk, which might fix this on its own.
String or IDL/UUID changes made by this patch: none

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #25)
> Comment on attachment 751148 [details] [diff] [review]
> Risk to taking this patch (and alternatives if risky): Sort of risky. This
> has never existed on its own, and this bug caused some regressions (though I
> don't think that this patch caused any of the regressions). A possible
> alternative is to just port the VectorImage hunk, which might fix this on
> its own.

Sorry for the delay in checking in on this request.  Joe - would it be possible/feasible to run a try build with your alternate proposal and see if your hunch there is correct? Is the risk of regression here something that can be given thorough QA with steps?

Comment 27

[Joe Drew (not getting mail)]

https://tbpl.mozilla.org/?tree=Try&rev=f06eb72d4d98

Regressions would be related to animated images; problems we had before was from switching tabs to animated images after a long while, and scrolling an animated image off the screen for a long time and then scrolling it back. Other things could be whether or not images, added dynamically, get animated automatically.

If we can get those working, I'm not that worried about it being broken more subtly.

Comment 28

[Joe Drew (not getting mail)]

I really don't know if Try is being jerky on Aurora or what. Here's another try at Try: https://tbpl.mozilla.org/?tree=Try&rev=dcd9de3a77f5

Comment 29

[Joe Drew (not getting mail)]

OK, I'm reasonably happy with those results. We'd want QA on animated images as I mentioned above, including the throbber and the subtly-animated favicon on about:robots, but it's probably safe. Hopefully it fixes the bug on its own too.

Ball's in your court, release mgmt!

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking so I can keep an eye on this during FF23 beta and hopefully there are no surprises. Will approve the uplift next.

Comment 31

[Joe Drew (not getting mail)]

"sort of fixed" on aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/c569195c2993

Comment 32

[Cornel Ionce [:noni] [Hubs QA]]

No crashes or hangs if switching tabs containing animated images left for about 1 hour while continuous browsing in other tabs. 
Also tested on a webpage containing a throbber. Scrolled the throbber down (off the screen) and left it for the same period of time. After scrolling back, the throbber worked fine, continuing it's rotation.

Tested on Firefox 23 beta 2 (build ID: 20130701144430)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0

Is there anything else you would like me to test or I can mark this Verified for FF 23?

Comment 33

[Joe Drew (not getting mail)]

Cornel, no. Thank you!

Comment 34

[Cornel Ionce [:noni] [Hubs QA]]

Marking verified for FF 23 based on comment 32

Comment 35

[Cornel Ionce [:noni] [Hubs QA]]

Tested with the same scenarios from comment 32 with no crashes and hangs for more than one hour.

Tested on Firefox 24 beta 2 (build ID: 20130812173056)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0

Marking this issue as verified.