Last Comment Bug 780342 - Don't allow compositor-driven animation of frames that are not prerendered, provide diagnostics for when that happens
: Don't allow compositor-driven animation of frames that are not prerendered, p...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-03 19:59 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-08-20 22:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (8.83 KB, patch)
2012-08-04 18:35 PDT, David Zbarsky (:dzbarsky)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-03 19:59:04 PDT
Like we do for async animations.
Comment 1 David Zbarsky (:dzbarsky) 2012-08-04 18:35:34 PDT
Created attachment 649051 [details] [diff] [review]
Patch

Some animations aren't prerendered before the first frame, so they display the message once.  That is ok, but when you have a bunch of messages with the same frame pointer, that is an actual performance problem.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-06 12:17:41 PDT
Comment on attachment 649051 [details] [diff] [review]
Patch

>diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js

>+pref("layers.offmainthreadcomposition.log-animations", true);

This needs to be off by default.  Let's file a Boot2Gecko bugzilla bug
and a gaia issue for exposing a setting to enable this.

>diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp

>+AddAnimationsAndTransitionsToLayer(Layer* aLayer, nsDisplayListBuilder* aBuilder,
>+                                   nsDisplayItem* aItem, nsCSSProperty aProperty)
> {
 
>+  // If the frame is not prerendered, bail out.  Layout will still perform the
>+  // animation.
>+  if (!nsDisplayTransform::ShouldPrerenderTransformedContent(aBuilder, frame)) {
>+    if (nsLayoutUtils::IsAnimationLoggingEnabled()) {
>+      printf_stderr("Performance warning: Async animation disabled because frame (%p) is not prerendered\n", frame);

Can we pull something more identifying off the nsIContent for the
frame?  Like an element name and/or id attribute?

Can do that in a followup.

r=me with those changes.
Comment 3 David Zbarsky (:dzbarsky) 2012-08-06 12:57:05 PDT
Changed to use the tag name and id if there is one.
Comment 4 David Zbarsky (:dzbarsky) 2012-08-06 13:33:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbe36ab8964
Comment 5 Ed Morley [:emorley] 2012-08-07 07:38:17 PDT
https://hg.mozilla.org/mozilla-central/rev/0fbe36ab8964
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-20 20:58:49 PDT
Why isn't this using PR_LOG? You can follow nsGlobalWindow and force logging on for this module in opt builds. This is easy to fix and I think should be fixed before it becomes entrenched.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-20 21:06:33 PDT
It should be using the console service.  Now that it's off by default, that should be OK.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-20 21:06:44 PDT
it == this logging, I mean
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-20 22:53:27 PDT
OK. Whichever we replace it with, the code currently here is an anti-pattern and I'd like to see it removed before someone copies it.

Note You need to log in before you can comment on or make changes to this bug.