Closed Bug 1321412 Opened 8 years ago Closed 7 years ago

Introduce a basic mechanism (not enabled by default yet) for partial pre-rendering of content with an animated transform

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(11 files, 1 obsolete file)

Bug 1321412 - Add support for partial prerendering to ShouldPrerenderTransformedContent().
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321412 - Fix a bug in the definition of SizeTyped.
58 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1321412 - Add Min() and Max() functions to BaseSize.
58 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1321412 - Add an operator<< to BaseSize.
58 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1321412 - Rename nsDisplayTransform::mIsFullyVisible to mAllowAsyncAnimations.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321412 - Feed the return value of ShouldPrerenderTransformedContent() directly into nsDisplayTransform::mAllowAsyncAnimation.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321412 - Change the return type of ShouldPrerenderTransformedContent() to an enum.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321412 - Allow ShouldPrerenderTransformedContent() to override the dirty rect.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1321412 - Allow controlling the size of the prerendered area via prefs.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
birtles
: review+
flod
: review+
Details
Test (uses scroll-driven animations)
3.21 KB, patch
Details | Diff | Splinter Review
Bug 1321412 - If the animated content is already fully rendered, enable async animation regardless of size.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
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.
Assignee: nobody → botond
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.
Blocks: 1100357
Comment on attachment 8815926 [details]
Bug 1321412 - Rename nsDisplayTransform::mIsFullyVisible to mAllowAsyncAnimations.

https://reviewboard.mozilla.org/r/96706/#review98038
Attachment #8815926 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8815928 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8815929 [details]
Bug 1321412 - Allow ShouldPrerenderTransformedContent() to override the dirty rect.

https://reviewboard.mozilla.org/r/96712/#review98044
Attachment #8815929 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8815930 [details]
Bug 1321412 - Allow controlling the size of the prerendered area via prefs.

https://reviewboard.mozilla.org/r/96714/#review98046
Attachment #8815930 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8815922 - Flags: review?(matt.woodrow) → review+
(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
Attachment #8817523 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8815927 [details]
Bug 1321412 - Feed the return value of ShouldPrerenderTransformedContent() directly into nsDisplayTransform::mAllowAsyncAnimation.

https://reviewboard.mozilla.org/r/96708/#review98134
Attachment #8815927 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8815923 [details]
Bug 1321412 - Fix a bug in the definition of SizeTyped.

https://reviewboard.mozilla.org/r/96700/#review98162
Attachment #8815923 - Flags: review?(bugmail) → review+
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.
Attachment #8815924 - Flags: review?(bugmail) → review-
Comment on attachment 8815925 [details]
Bug 1321412 - Add an operator<< to BaseSize.

https://reviewboard.mozilla.org/r/96704/#review98166
Attachment #8815925 - Flags: review?(bugmail) → review+
(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 :)
Attachment #8815924 - Flags: review- → review+
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)"
Flags: needinfo?(botond)
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.
Flags: needinfo?(botond)
(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.
(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?)
Attachment #8815930 - Flags: review?(bbirtles) → review+
Attachment #8815930 - Flags: review?(francesco.lodolo)
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.
Attachment #8815930 - Flags: review?(francesco.lodolo) → review+
(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()?
Attachment #8817659 - Attachment is obsolete: true
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
Blocks: 1324591
See Also: → 1834065
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: