Don't allow compositor-driven animation of frames that are not prerendered, provide diagnostics for when that happens

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: dzbarsky)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Like we do for async animations.
(Assignee)

Updated

5 years ago
Summary: Provide diagnostics for if transformed frames hit the prerender optimization, and if not why not → Don't allow compositor-driven animation of frames that are not prerendered, provide diagnostics for when that happens
(Assignee)

Comment 1

5 years ago
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.
Attachment #649051 - Flags: review?(jones.chris.g)
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.
Attachment #649051 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 3

5 years ago
Changed to use the tag name and id if there is one.
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbe36ab8964

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/0fbe36ab8964
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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.
It should be using the console service.  Now that it's off by default, that should be OK.
it == this logging, I mean
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.
You need to log in before you can comment on or make changes to this bug.