Closed Bug 867758 Opened 7 years ago Closed 7 years ago

Coordinate start of animation for same-FPS images

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox23 + verified
firefox24 --- verified

People

(Reporter: joe, Assigned: joe)

References

Details

Attachments

(5 files, 1 obsolete file)

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
Assignee: nobody → joe
(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.
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.
I get that part. I still don't understand exactly what you're proposing to do.
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.
Summary: FPS-based refresh driver for animated images → Coordinate start of animation for same-FPS images
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.
> 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.
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.
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! :)
Depends on: 873505
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.
Attachment #751144 - Flags: review?(seth)
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.
Attachment #751148 - Flags: review?(seth)
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.
Try, happily, found problems in the previous patch. Here it is fixed.

https://tbpl.mozilla.org/?tree=Try&rev=54fda2729bda
Attachment #751143 - Attachment is obsolete: true
Attachment #751143 - Flags: review?(seth)
Attachment #751207 - Flags: review?(seth)
Depends on: 873683
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.
Attachment #751142 - Flags: review?(seth) → review+
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.
Attachment #751144 - Flags: review?(seth) → review+
Attachment #751148 - Flags: review?(seth) → review+
Attachment #751207 - Flags: review?(seth) → review+
Attached patch testSplinter Review
Attachment #752553 - Flags: review?(seth)
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.
Attachment #752553 - Flags: review?(seth) → review+
Depends on: 875173
(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.
(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.
Ah, no, you're 100% correct. I'll just try to unconditionally remove from both tables.
Depends on: 876332
Depends on: 876333
No longer depends on: 876332
Depends on: 876499
Blocks: 882050
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
Attachment #751148 - Flags: approval-mozilla-aurora?
(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?
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.
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
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!
Flags: needinfo?(release-mgmt)
Tracking so I can keep an eye on this during FF23 beta and hopefully there are no surprises. Will approve the uplift next.
Flags: needinfo?(release-mgmt)
Keywords: verifyme
Attachment #751148 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(joe)
Cornel, no. Thank you!
Flags: needinfo?(joe)
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
Depends on: 885277
You need to log in before you can comment on or make changes to this bug.