Closed Bug 1002632 Opened 10 years ago Closed 9 years ago

SVG-as-an-image document containing animateTransform continues to spin its RefreshDriver after tab is closed

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- fixed

People

(Reporter: BenWa, Assigned: dholbert)

References

Details

(Keywords: power, Whiteboard: power)

Attachments

(9 files, 1 obsolete file)

Attached image throbber.svg
      No description provided.
Attached file throbber.html
STR:
0) Optional: Apply Diagnostic patch
1) Open https://bug1002632.bugzilla.mozilla.org/attachment.cgi?id=8413878
2) Close the tab
3) Observe 'Refresh https://bug1002632.bugzilla.mozilla.org/attachment.cgi?id=8413877' causing CPU wake ups

After ~35 seconds the refresh driver stops
Attachment #8413883 - Attachment is patch: true
Attachment #8413883 - Attachment mime type: text/x-patch → text/plain
Keywords: power
Whiteboard: power
dholbert, are you or know someone who is familiar with this?

My diagnostic based on the above backtrace is: If we have an SVG open we correctly call 'nsSMILAnimationController::Pause' but if we have an SVG in an image (sub document?) then 'nsSMILAnimationController::Pause' isn't called on OnPageHide.
Flags: needinfo?(dholbert)
So what's supposed to happen here is:
 (a) Clients of the image (i.e. tabs that have it open) send ticks to their images, when their own (external) refresh drivers fire, via imgIContainer::RequestRefresh().
 (b) The document inside of the image *ignores* (or suppresses) its own refresh driver, and instead responds to these "RequestRefresh" ticks, to drive its own internal SMIL/CSS animations.

This lets animation framerate be controlled by the clients, and drop to 0 when there are no clients.

Part (a) was done in bug 666446. But unfortunately, we've never gotten around to part (b). :(  There's a TODO for it, in VectorImage::RequestRefresh, here:
http://mxr.mozilla.org/mozilla-central/source/image/src/VectorImage.cpp?rev=6c1c7e45c902#491

So the behavior you're reporting is expected, with the current implementation, and we need to actually implement part (b) to fix it.

Out of curiosity, do we have any live web content (or gaia content) that's triggering this?  I don't have a good feeling for how common SMIL-animated SVG-as-an-image actually is on the web right now.
Flags: needinfo?(dholbert)
(Should've mentioned: we did the equivalent of part (b) for animated *raster* images (GIF/PNG) in bug 666446.  That was the main goal of that bug, to address a similar power/perf issue stemming from per-image timers that we were using. The equivalent SVG-image code has remained as a TODO since then, though.)
This is being triggered by Cleopatra.
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Out of curiosity, do we have any live web content (or gaia content) that's
> triggering this?  I don't have a good feeling for how common SMIL-animated
> SVG-as-an-image actually is on the web right now.

Cleopatra (the profiler UI) triggers this. I also hit this needless tick when the tab is focused but the SVG is hidden (display:none I think). Cleopatra also hit needless invalidation from a hidden static progressbar. So the platform handles the cleopatra page like a complete disaster.

This bug is particularly annoying because people will open a profile, switch away from the tab and still have activity from the tab because of this bug which will show up in their next profile they save.
Do we want to suppress the refresh driver in general for documents being used as images, or only for SMIL animation?  Because just having it not tick at all would be pretty trivial, I'd think.
Ideally, we should suppress its automatic refreshing entirely, and then make VectorImage::RequestRefresh forcibly trigger a refresh-driver tick...  Perhaps via nsRefreshDriver::DoTick()?
That seems reasonable, yes.
Summary: Sub SVG Document containing animateTransform triggers RefreshDriver after tab is closed → SVG-as-an-image document containing animateTransform continues to spin its RefreshDriver after tab is closed
Assignee: nobody → dholbert
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch fix v1 (obsolete) — Splinter Review
This patch does wht Comment 11 suggested, & it seems to fix this, according to breakpoints I put in nsSMILAnimationController::WillRefresh, and also according to BenWa's diagnostic patch.  The image still visibly animates when it's open in a foreground tab.

I'm suppressing the refresh driver's own timer by making image documents return early from nsRefreshDriver::EnsureTimerStarted().
Attachment #8414736 - Flags: review?(bzbarsky)
Actually, I guess we don't technically need the StopTimer() call in this case (in the early return from EnsureTimerStarted). I'll spin this off into a separate clause without that call.
Attached patch fix v2Splinter Review
Attachment #8414736 - Attachment is obsolete: true
Attachment #8414736 - Flags: review?(bzbarsky)
Attachment #8414745 - Flags: review?(bzbarsky)
So the patch fixes the STR from comment 2. However, it doesn't completely fix the profiler UI --  after I've created a profile & the throbber has been displayed, I get continuous refresh driver ticks in chrome://browser/content/devtools/profiler/cleopatra/images/throbber.svg [shown by BenWa's diagnostic patch], even after the throbber is no longer visible in the UI.  This continues until I close the profiler UI.

The backtrace of these ticks -- with the patch applied -- shows that they're actually being triggered by the *outer* document's refresh driver's clock, which is what's supposed to happen, per comment 6. So things are actually mostly working as they should.

The remaining problem there (after this patch is applied) is just that the image is remaining in its host document's refresh driver's list of images-to-refresh (nsRefreshDriver::mRequestTable), even though it's not visible. This issue isn't SVG-image-specific -- we'd have the same problem if this were a raster image (i.e. the image would keep the outer document's refresh driver spinning unnecessarily, and that refresh driver would trigger animation inside of the image, albeit with no invalidation).

To fix this remaining issue, we need to get nsLayoutUtils::DeregisterImageRequest() called on the image. I suspect that clearing its "src" attribute or (more severe) dropping its node from the DOM would trigger that. It's possible that "display:none" might trigger that, too, but I'm not as sure there.

We should probably spin off a separate bug for this part, anyway, probably as a profiler bug. (since I think it's something that we'd be fixing in the profiler code)
However the spinner is hidden we should make it so that gecko doesn't tick the refresh driver so that web content can benefit without having to change.
Local testing shows that we do the right thing (unregister the image) when I dynamically set "display:none" on the <img>. So we're doing comment 17, at least for that form of "hidden".

However, we don't stop notifying the images (though arguably we probably should) if you instead set "visibility:hidden".

In practice, I have no idea how cleopatra hides these spinners, but it should be able to benefit from this bug's patch if it uses "display:none", I'd expect.
Here's a testcase to illustrate my previous comment.

With the diagnostic patch & "fix v2" applied, this testcase (using 'display:none' to hide the image) shows that the refresh driver *stops sampling* when the image disappears.

(With only the diagnostic patch & no bugfix, the refresh driver keeps spinning even after the image disappears.)
Yeah, we should de-register when the frame is destroy.

Bug 987212 should cover visibility hidden, but we should find out what cleopatra does, it's likely to be used in more places in the wild.
Comment on attachment 8414745 [details] [diff] [review]
fix v2

Hmm.  So if two different documents RequestRefresh on the same image we'll tick the refresh driver twice?  That seems a bit annoying.  Is there any way to avoid that?

The rest of this look fine.
Attachment #8414745 - Flags: review?(bzbarsky) → review+
Yeah, that's a flaw in the design of bug 666446 (hooking images up to their clients' refresh drivers). It affects raster animations, too, though probably less so, since it will only trigger bonus invalidations if the raster image has a really fast frame rate (> 60 FPS).

In practice, it's only a problem when we have two foreground documents with the same animation (i.e. two windows open, or perhaps a window and an iframe), since background tabs' refresh drivers will be throttled and hence won't cause many extra ticks.  So, probably an edge case.

I think the easiest way to avoid the problem (in the current design) is to have RequestRefresh check the current time, and return early if it's within N ms from our last RequestRefresh call. (where N should probably should be based on the refresh driver's normal framerate).
I filed Bug 1003683 to cover the issue discussed in Comment 21 / Comment 22. (Patch already attached there, just awaiting review.)

I've landed the patch for this bug here:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c5aae1b3dc3f

I thought about holding off on landing this until Bug 1003683 was ready, but I opted to push since that Bug 1003683 is pretty edge-casey and already affects raster images, & I'd rather have this getting nightly testing sooner rather than later. (and Bug 1003683 should presumably be reviewed & landing soon anyway)
Flags: in-testsuite-
(In reply to Daniel Holbert [:dholbert] from comment #16)
> So the patch fixes the STR from comment 2. However, it doesn't completely
> fix the profiler UI --  after I've created a profile & the throbber has been
> displayed, I get continuous refresh driver ticks in
> chrome://browser/content/devtools/profiler/cleopatra/images/throbber.svg
> [shown by BenWa's diagnostic patch], even after the throbber is no longer
> visible in the UI.  This continues until I close the profiler UI.

I filed bug 1004541 on this.
Blocks: 1004541
(In reply to Benoit Girard (:BenWa) from comment #9)
> Cleopatra (the profiler UI) triggers this.
[...]
> This bug is particularly annoying because people will open a profile, switch
> away from the tab and still have activity from the tab

Also, FWIW: per comment 16, I still see this sort of thing (activity for the profiler UI's throbber even when it's in a background tab), with this bug's patch applied.

I suspect that's because the profiler UI is in chrome, instead of in the background-tab's content, so it's driven by the chrome's refresh driver, which doesn't throttle when it's in a background tab. Or something like that.  Bug 1004541 would limit the impact of this (by silencing the throbber when it's removed from from the profiler UI), but there may be more that we should do here as well...
https://hg.mozilla.org/mozilla-central/rev/c5aae1b3dc3f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(In reply to Daniel Holbert [:dholbert] from comment #25)
> I suspect that's because the profiler UI is in chrome, instead of in the
> background-tab's content

If it's open via the 'Analyze' button it's possible, however I would of assumed the add-on API wouldn't do this. Should we file a bug for that? But if you open a profile via a file or a ?report=HASH url then it can't be.
I haven't hit the "analyze" button.

My STR are as follows, for the thing I was talking about in comment 16 & comment 25:
 - Start a build with your diagnostic patch (and with this bug's fix)
 - hit Shift + F5 to open profiler UI
 - click the clock icon twice (to start & end a profile)
 - switch tabs

I see refresh driver ticks for the profiler UI ( chrome://browser/content/devtools/profiler.xul ), at what looks like ~60fps, which means we're spinning the chrome://.../profiler.xul refresh driver at full speed in the background tab.  When the throbber appears (in the background tab's profiler UI), some of those ticks also trigger ticks in the throbber. Those are the easiest to see in the diagnostic output, because they're usually persistent (for now), due to bug 1004541.
(In reply to Benoit Girard (:BenWa) from comment #27)
> however I would of
> assumed the add-on API wouldn't do this

(I assume you're talking about the add-on API because that's what the profiler uses to create its footer?)

> Should we file a bug for that?

I don't know enough about how chrome documents are managed to be sure.

bz, does it strike you as unexpected that the refresh driver for the profiler footer-bar (chrome://browser/content/devtools/profiler.xul) seems to tick at full speed, even when it's in a background tab? Or is that expected behavior for some reason? (maybe as a result of it being part of the chrome)
Flags: needinfo?(bzbarsky)
Where is this profiler.xul file loaded?

Normally, when we switch tabs the tabbrowser marks the docshell inside the tab (and hence all its descendants) as inactive, but if there are other docshells around they wouldn't know anything about being "in a background tab"...
Flags: needinfo?(bzbarsky)
Ohh you're using the profiler UI from the devtools panel (profiler.xul). I noticed the bug with the profiler addon which uses (http://people.mozilla.org/~bgirard/cleopatra/). That's probably where the confusion is coming from.
Depends on: 1008598
Depends on: 1058634
This broke animations in SVG-in-opentype fonts, since those use SVG-as-an-image documents, but they don't (currently) pass RequestRefresh() signals in from the host document(s). See bug 1058634 comment 3 for more.

I think we should back this out until we've got a solution there. roc agrees that we should back it out for Firefox 33 (currently Aurora, going to Beta next week), per bug 1058634 comment 8, and I'd feel more comfortable backing out there if we backout on Nightly first.  Then, we can re-land this once we've got a RequestRefresh communication channel hooked up for SVG-in-opentype.

(In the meantime, we'll take a bit of a hit on perf/power usage -- reverting to the state described in comment 2 here.)
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b88f451d3ecc
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch backout patchSplinter Review
Requesting approval to back this out on Aurora, to fix bug 1058634.

Approval Request Comment
[Feature/regressing bug #]: This bug (which I'd like to back out) caused bug 1058634.

[User impact if declined]: Animations in "SVG in OpenType" fonts won't run. These fonts are a newish feature that we support, and probably aren't widely used, but it'd be a shame to break them. We have a plan to fix them (see bug 1058634 comment 5 and bug 1058634 comment 6), but I'm not sure how soon it'll be ready, and it's probably not backport material.

[Describe test coverage new/current, TBPL]: This patch was just an optimization (not expected to impact behavior), so this is N/A. (There's some reftest coverage for animated SVG-as-an-image, though we're sadly lacking tests that would've caught bug 1058634.)

[Risks and why]: Low risk, aside from the known perf impact when this bug is triggered (which this optimization was intended to address).  In particular, we'll return to the state described in comment 2 -- animations in SVG-as-an-image in closed tabs consume CPU for ~35 seconds after the tab is closed (until they're purged from the image cache).

[String/UUID change made/needed]: None.
Attachment #8481516 - Flags: approval-mozilla-aurora?
(After we back out on Aurora, we'll be in a state where Firefox 32 [about to ship] is affected by bug 1058634, but Firefox 33 and beyond will not be.  And as noted in comment 32, I intend to re-land this  optimization once we've fixed things so that this doesn't cause bug 1058634.)
Attachment #8481516 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Daniel Holbert [:dholbert] from comment #34)
> Backed out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b88f451d3ecc

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/b88f451d3ecc
Target Milestone: mozilla32 → ---
It seems unlikely we'd backport a fix for this to Beta33 at this point? I think we should just wontfix firefox33.
Sounds right to me. (given that this causes bug 1058634, and the hypothetical fix for that bug would probably not be backport-to-beta material.)
Here's a followup to exclude SVG-in-opentype fonts from getting this optimization, to let us land this without breaking them as described in comment 32.

(I filed bug 1107252 on creating a way to send refresh-driver ticks from the host document to drive the animations in SVG-in-opentype glyphs -- when that's fixed, we can remove this special-case exclusion.)
Attachment #8531731 - Flags: review?(edwin)
(Note that if the image doc has a null URI, this followup patch makes us take the "greedily use your own internal refresh driver" path.  I was just re-thinking that & considering reposting the patch with the logic flipped for that case, but I don't think it really matters, because I don't think we'd ever actually hit a null URI here, in practice, for a doc with IsBeingUsedAsImage()=true, since: I think GetDocumentURI can only be null during setup/teardown & if we seek to a different URI, none of which should be happening while we're setting up the refresh driver for an image/font internal-doc.)
Comment on attachment 8531731 [details] [diff] [review]
fix, part 2: exclude SVG-in-opentype from this optimization, for now (with "IsFontTableURI()" check)

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

:(
Attachment #8531731 - Flags: review?(edwin) → review+
Re-landed, now with "part 2" to avoid breaking svg-in-opentype fonts:
 part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1a15f16c65
 part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/100f8bfb6de2
https://hg.mozilla.org/mozilla-central/rev/0f1a15f16c65
https://hg.mozilla.org/mozilla-central/rev/100f8bfb6de2
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.