In this bug, I'd like to land some work in progress towards bug 1100357. The patches here introduce a simple mechanism for partial pre-rendering of content with an animated transform, to be used in cases where the content is too large to be fully pre-rendered. The partial pre-rendering is behind a pref, and will not be enabled by default until some additional pieces of bug 1100357 (which will done in other bugs) are in place: - Changes to AsyncCompositionManager to jank animations instead of checkerboarding them if we hit the edge of the pre-rendered area. - Improvements to how we choose the size of the pre-rendered area.

A couple of notes for Matt: - Due to a bug in MozReview, the patches are in the wrong order in the "Attachments" table. Please see the "MozReview Requests" table, or the MozReview UI, for the right order. - I have a test for this (attached), but it uses scroll-driven animations (because that allows testing this sort of thing with a garden-variety async-scrolling reftest). Would you be OK with landing this without the test, with the understanding that the test will land once scroll-driven animations land? I hope to land enough of scroll-driven animations to run this test, in the coming weeks. Alternatively, should we wait with landing the entire patch series until scroll-driven animations land? (The other alternative is to extend the test infrastructure to allow testing this with time-driven animations. I'm not super keen on spending time on that right now, although if you have a simple approach in mind I'd be open to it.)

(In reply to Botond Ballo [:botond] from comment #11) > I hope to land enough of scroll-driven animations to run this > test, in the coming weeks. Specifically, the test runs (and passes) on top of the patches I intend to post (and land) in bug 1321428.

Comment on attachment 8815926 [details] Bug 1321412 - Rename nsDisplayTransform::mIsFullyVisible to mAllowAsyncAnimations. https://reviewboard.mozilla.org/r/96706/#review98038

Comment on attachment 8815927 [details] Bug 1321412 - Feed the return value of ShouldPrerenderTransformedContent() directly into nsDisplayTransform::mAllowAsyncAnimation. https://reviewboard.mozilla.org/r/96708/#review98040 ShouldPrerenderTransformedContent only returns true if the element is roughly the size of the viewport or smaller. It's possible for an element to be bigger than this, and also entirely rendered already without needing prerendering (thanks to the displayport). This change will disable async animations for these elements, even though we have them fully rendered. That sounds non-ideal, but maybe it doesn't matter.

Comment on attachment 8815928 [details] Bug 1321412 - Change the return type of ShouldPrerenderTransformedContent() to an enum. https://reviewboard.mozilla.org/r/96710/#review98042 ::: layout/painting/nsDisplayList.cpp:6159 (Diff revision 1) > return mAllowAsyncAnimation; > } > > -/* static */ bool > +/* static */ auto > nsDisplayTransform::ShouldPrerenderTransformedContent(nsDisplayListBuilder* aBuilder, > - nsIFrame* aFrame) > + nsIFrame* aFrame) -> PrerenderDecision Is there any reason to prefer the 'auto func -> rettype' syntax? I'd prefer the old syntax for consistency if not.

Comment on attachment 8815929 [details] Bug 1321412 - Allow ShouldPrerenderTransformedContent() to override the dirty rect. https://reviewboard.mozilla.org/r/96712/#review98044

Comment on attachment 8815930 [details] Bug 1321412 - Allow controlling the size of the prerendered area via prefs. https://reviewboard.mozilla.org/r/96714/#review98046

Comment on attachment 8815922 [details] Bug 1321412 - Add support for partial prerendering to ShouldPrerenderTransformedContent(). https://reviewboard.mozilla.org/r/96698/#review98050 ::: layout/painting/nsDisplayList.cpp:6169 (Diff revision 1) > + nscoord xExcess = aPrerenderSize.width - aDirtyRect.width; > + nscoord yExcess = aPrerenderSize.height - aDirtyRect.height; > + return nsRect(aDirtyRect.x - (xExcess / 2), > + aDirtyRect.y - (yExcess / 2), > + aPrerenderSize.width, > + aPrerenderSize.height).MoveInsideAndClamp(aOverflow); You could use Inflate().Intersect() on aDirtyRect, that might be slightly simpler.

(In reply to Matt Woodrow (:mattwoodrow) from comment #14) > ShouldPrerenderTransformedContent only returns true if the element is > roughly the size of the viewport or smaller. It's possible for an element to > be bigger than this, and also entirely rendered already without needing > prerendering (thanks to the displayport). > > This change will disable async animations for these elements, even though we > have them fully rendered. > > That sounds non-ideal, but maybe it doesn't matter. Good catch! I'll fix this. (In reply to Matt Woodrow (:mattwoodrow) from comment #15) > > -/* static */ bool > > +/* static */ auto > > nsDisplayTransform::ShouldPrerenderTransformedContent(nsDisplayListBuilder* aBuilder, > > - nsIFrame* aFrame) > > + nsIFrame* aFrame) -> PrerenderDecision > > Is there any reason to prefer the 'auto func -> rettype' syntax? > > I'd prefer the old syntax for consistency if not. With the old syntax, the class's scope hasn't started yet in the return type, so the return type would need to be spelt nsDisplayTranform::PrerenderDecision. I can do that if you'd like.

(In reply to Botond Ballo [:botond] from comment #19) > (In reply to Matt Woodrow (:mattwoodrow) from comment #14) > > ShouldPrerenderTransformedContent only returns true if the element is > > roughly the size of the viewport or smaller. It's possible for an element to > > be bigger than this, and also entirely rendered already without needing > > prerendering (thanks to the displayport). > > > > This change will disable async animations for these elements, even though we > > have them fully rendered. > > > > That sounds non-ideal, but maybe it doesn't matter. > > Good catch! I'll fix this. This is addressed in a new commit I appended to the series.

(In reply to Botond Ballo [:botond] from comment #19) > > With the old syntax, the class's scope hasn't started yet in the return > type, so the return type would need to be spelt > nsDisplayTranform::PrerenderDecision. I can do that if you'd like. Nah, this is enough of a compelling reason for me.

Comment on attachment 8817523 [details] Bug 1321412 - If the animated content is already fully rendered, enable async animation regardless of size. https://reviewboard.mozilla.org/r/97768/#review98132

Comment on attachment 8815927 [details] Bug 1321412 - Feed the return value of ShouldPrerenderTransformedContent() directly into nsDisplayTransform::mAllowAsyncAnimation. https://reviewboard.mozilla.org/r/96708/#review98134

Comment on attachment 8815923 [details] Bug 1321412 - Fix a bug in the definition of SizeTyped. https://reviewboard.mozilla.org/r/96700/#review98162

Comment on attachment 8815924 [details] Bug 1321412 - Add Min() and Max() functions to BaseSize. https://reviewboard.mozilla.org/r/96702/#review98164 ::: gfx/2d/BaseSize.h:97 (Diff revision 2) > + friend Sub Min(const Sub& aA, const Sub& aB) { > + return Sub(std::min(aA.width, aB.width), > + std::min(aA.height, aB.height)); > + } > + > + friend Sub Max(const Sub& aA, const Sub& aB) { > + return Sub(std::max(aA.width, aB.width), > + std::max(aA.height, aB.height)); Can we pull this out of the class? If I'm reading this right, this is non-static so somebody could write code like so: size4 = size1.Min(size2, size3); which is really annoying. We had similar badness with nsRegion.Or and friends. I think just a templated function outside the struct would be preferable.

Comment on attachment 8815925 [details] Bug 1321412 - Add an operator<< to BaseSize. https://reviewboard.mozilla.org/r/96704/#review98166

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36) > ::: gfx/2d/BaseSize.h:97 > (Diff revision 2) > > + friend Sub Min(const Sub& aA, const Sub& aB) { > > + return Sub(std::min(aA.width, aB.width), > > + std::min(aA.height, aB.height)); > > + } > > + > > + friend Sub Max(const Sub& aA, const Sub& aB) { > > + return Sub(std::max(aA.width, aB.width), > > + std::max(aA.height, aB.height)); > > Can we pull this out of the class? If I'm reading this right, this is > non-static so somebody could write code like so: > > size4 = size1.Min(size2, size3); > > which is really annoying. We had similar badness with nsRegion.Or and > friends. I think just a templated function outside the struct would be > preferable. Nope, these are friend functions, which are non-member functions (there is no "this" inside them, and you can't call them on an object). Defining them inside the class as friends is just a convenient way of defining a different function for every class "Sub" that derives from a BaseSize.

Comment on attachment 8815924 [details] Bug 1321412 - Add Min() and Max() functions to BaseSize. https://reviewboard.mozilla.org/r/96702/#review98198 ::: gfx/2d/BaseSize.h:97 (Diff revision 2) > + friend Sub Min(const Sub& aA, const Sub& aB) { > + return Sub(std::min(aA.width, aB.width), > + std::min(aA.height, aB.height)); > + } > + > + friend Sub Max(const Sub& aA, const Sub& aB) { > + return Sub(std::max(aA.width, aB.width), > + std::max(aA.height, aB.height)); Thanks, that's my new thing I learned today :)

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/34a1640010d6 Fix a bug in the definition of SizeTyped. r=kats https://hg.mozilla.org/integration/autoland/rev/31f15a3b5ce0 Add Min() and Max() functions to BaseSize. r=kats https://hg.mozilla.org/integration/autoland/rev/2f88a62520a4 Add an operator<< to BaseSize. r=kats https://hg.mozilla.org/integration/autoland/rev/c2b8107d8468 Rename nsDisplayTransform::mIsFullyVisible to mAllowAsyncAnimations. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/dbd699a36844 Feed the return value of ShouldPrerenderTransformedContent() directly into nsDisplayTransform::mAllowAsyncAnimation. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/964ff414c560 Change the return type of ShouldPrerenderTransformedContent() to an enum. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/94302209286e Allow ShouldPrerenderTransformedContent() to override the dirty rect. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/9587048cd2ee Allow controlling the size of the prerendered area via prefs. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/b2508bbc5ef9 Add support for partial prerendering to ShouldPrerenderTransformedContent(). r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/a8e8ba250d62 If the animated content is already fully rendered, enable async animation regardless of size. r=mattwoodrow

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/fd5cb70c0286 Follow-up to remove an unused variable. r=bustage on a CLOSED TREE

Backed out for failing test_animation_performance_warning.html at least on Linux and Android: https://hg.mozilla.org/integration/autoland/rev/284e73444c60dfcff0dcfcd4bd45a3c248a7b8ec https://hg.mozilla.org/integration/autoland/rev/7121735f6c766a3bcc3169c83681074b8005b763 https://hg.mozilla.org/integration/autoland/rev/3141d9e9e6349d4db3929f06e1373c42fa97d937 https://hg.mozilla.org/integration/autoland/rev/4ccfb1be82560257cca8491985d79ddd1738380e https://hg.mozilla.org/integration/autoland/rev/d3054b3af748a20ceffa8475490c0ddcf495f4ee https://hg.mozilla.org/integration/autoland/rev/daf9dcc44caeca034c5e521f74744dc0ac13dc3b https://hg.mozilla.org/integration/autoland/rev/2391b4ab6f371cbd1881caa65bc75260e577c163 https://hg.mozilla.org/integration/autoland/rev/e56d0e8cf1d13d5c4324e8c3e8ed9edb68d35ecd https://hg.mozilla.org/integration/autoland/rev/f8a8676709152e98773ed5803a2a785a50d6e3c8 https://hg.mozilla.org/integration/autoland/rev/d008de7ea68cde2e59be1fd78b09e655a8aa5d9e https://hg.mozilla.org/integration/autoland/rev/d112c18816637fdb5e23a70a96e4647bc5075f5f Follow-up with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fd5cb70c0286e9792420f3a3506d160591d33058 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7772312&repo=autoland [task 2016-12-09T20:55:01.933970Z] 20:55:01 INFO - TEST-START | dom/animation/test/chrome/test_animation_performance_warning.html [task 2016-12-09T20:55:06.747406Z] 20:55:06 INFO - TEST-INFO | started process screentopng [task 2016-12-09T20:55:07.572914Z] 20:55:07 INFO - TEST-INFO | screentopng: exit 0 [task 2016-12-09T20:55:07.574359Z] 20:55:07 INFO - Buffered messages logged at 20:55:06 [task 2016-12-09T20:55:07.575071Z] 20:55:07 INFO - TEST-PASS | dom/animation/test/chrome/test_animation_performance_warning.html | Bug 1196114 - Test metadata related to which animation properties are running on the compositor - Bug 1196114 - Test metadata related to which animation properties are running on the compositor: Elided 45 passes or known failures. [task 2016-12-09T20:55:07.575678Z] 20:55:07 INFO - Buffered messages finished [task 2016-12-09T20:55:07.576412Z] 20:55:07 INFO - TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_animation_performance_warning.html | transform on too big element - transform on too big element: assert_regexp_match: warning message should match expected object "/Animation cannot be run on the compositor because the fr..." but got "Animation cannot be run on the compositor because the frame size (10000, 10000) is too large relative to the viewport (larger than (1440, 1170)) or larger than the maximum allowed value (4096, 4096)"

Ah. One of the patches changed the wording of a devtools warning message; I didn't realize there was a test testing for the example wording! I'll fix and reland. Apologies for the trouble, I should have just pushed the patch series to Try first.

Try push with fix applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02472078e2c34026ac3467b6f944230c7e5710b3

(In reply to Botond Ballo [:botond] from comment #44) > Ah. One of the patches changed the wording of a devtools warning message; I > didn't realize there was a test testing for the example wording! Which patch? We're not supposed to change those messages without giving them a new key (unless the change is something you wouldn't update translations for).[1] [1] https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

(In reply to Brian Birtles (:birtles) from comment #49) > Which patch? We're not supposed to change those messages without giving them > a new key (unless the change is something you wouldn't update translations > for).[1] Thanks for pointing that out. I guess I'll have to change the key.

Trying again, also with the key changed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58442484f6e08cbbc13bca33c04ac38696abe543

(In reply to Botond Ballo [:botond] from comment #51) > Trying again, also with the key changed: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=58442484f6e08cbbc13bca33c04ac38696abe543 Looking better. Flagged Brian for additional review on the relevant patch, to make sure the string change is handled properly.

Comment on attachment 8815930 [details] Bug 1321412 - Allow controlling the size of the prerendered area via prefs. https://reviewboard.mozilla.org/r/96714/#review98316 It seems fine to me but Francesco can advise if this is the right way to do things (e.g. do we drop the old string? Is adding a '2' the usual pattern?)

Comment on attachment 8815930 [details] Bug 1321412 - Allow controlling the size of the prerendered area via prefs. https://reviewboard.mozilla.org/r/96714/#review98342 To answer the questions: we drop unused strings, and it's common to "version" the string ID (i.e. add 2) in lack of a better new ID to use.

(In reply to Matt Woodrow (:mattwoodrow) from comment #18) > ::: layout/painting/nsDisplayList.cpp:6169 > (Diff revision 1) > > + nscoord xExcess = aPrerenderSize.width - aDirtyRect.width; > > + nscoord yExcess = aPrerenderSize.height - aDirtyRect.height; > > + return nsRect(aDirtyRect.x - (xExcess / 2), > > + aDirtyRect.y - (yExcess / 2), > > + aPrerenderSize.width, > > + aPrerenderSize.height).MoveInsideAndClamp(aOverflow); > > You could use Inflate().Intersect() on aDirtyRect, that might be slightly > simpler. I understand the Inflate() part of this suggestion: aDirtyRect.Inflate(xExcess / 2, yExcess / 2) is a simpler way to express the same rectangle as: nsRect(aDirtyRect.x - (xExcess / 2), aDirtyRect.y - (yExcess / 2), aPrerenderSize.width, aPrerenderSize.height) but wouldn't I still want to MoveInsideAndClamp() rather than Intersect()?

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/68aaa196ae5a Fix a bug in the definition of SizeTyped. r=kats https://hg.mozilla.org/integration/autoland/rev/313fe04267d6 Add Min() and Max() functions to BaseSize. r=kats https://hg.mozilla.org/integration/autoland/rev/592f556e7644 Add an operator<< to BaseSize. r=kats https://hg.mozilla.org/integration/autoland/rev/b1c62ccec829 Rename nsDisplayTransform::mIsFullyVisible to mAllowAsyncAnimations. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/e40dd90cd728 Feed the return value of ShouldPrerenderTransformedContent() directly into nsDisplayTransform::mAllowAsyncAnimation. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/f9b9c558f02a Change the return type of ShouldPrerenderTransformedContent() to an enum. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/2f289f029b73 Allow ShouldPrerenderTransformedContent() to override the dirty rect. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/925e1dc46a40 Allow controlling the size of the prerendered area via prefs. r=birtles,flod,mattwoodrow https://hg.mozilla.org/integration/autoland/rev/5b6482c160d5 Add support for partial prerendering to ShouldPrerenderTransformedContent(). r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/bc4528a8cc23 If the animated content is already fully rendered, enable async animation regardless of size. r=mattwoodrow

https://hg.mozilla.org/mozilla-central/rev/68aaa196ae5a https://hg.mozilla.org/mozilla-central/rev/313fe04267d6 https://hg.mozilla.org/mozilla-central/rev/592f556e7644 https://hg.mozilla.org/mozilla-central/rev/b1c62ccec829 https://hg.mozilla.org/mozilla-central/rev/e40dd90cd728 https://hg.mozilla.org/mozilla-central/rev/f9b9c558f02a https://hg.mozilla.org/mozilla-central/rev/2f289f029b73 https://hg.mozilla.org/mozilla-central/rev/925e1dc46a40 https://hg.mozilla.org/mozilla-central/rev/5b6482c160d5 https://hg.mozilla.org/mozilla-central/rev/bc4528a8cc23