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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Like we do for async animations.

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: 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]

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)]

Changed to use the tag name and id if there is one.

Comment 4

[David Zbarsky (:dzbarsky)]

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

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0fbe36ab8964

Comment 6

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

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]

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]

it == this logging, I mean

Comment 9

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

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.