1462672 - Flatten inactive nsDisplayTransform display items

Status: VERIFIED FIXED

(Core :: Web Painting, enhancement, P2)

Description

[Miko Mynttinen]

This bug tracks the work of implementing flattening, as described in bug 1439292, for nsDisplayTransform items.

Comment 1

[Miko Mynttinen]

Attachment: transform-flattening-v1.patch

Comment 2

[Miko Mynttinen]

I ran some tests with the WIP version of the inactive nsDisplayTransform flattening patch and they look very promising. In displaylist_mutate test on MBP, the time spent in FLB drops from 57-60ms to 12-13ms, and total FPS goes up from 9 to 18.

Comment 3

[Miko Mynttinen]

Attachment: flatten-inactive-transform-v2.diff

Comment 4

[Miko Mynttinen]

Most of the correctness issues are fixed with the latest WIP patch. The current talos test results are:

Without flattening:
| ------- Summary: start -------
| Number of tests: 2
|
| [#0] mutate.html  Cycles:5  Average:3170.63  Median:3168.52  stddev:31.44 (1.0%)  stddev-sans-first:26.67
| Values: 3132.5  3168.5  3154.4  3181.0  3216.7
|
| [#1] inactive_mutate.html  Cycles:5  Average:4310.83  Median:4273.36  stddev:83.67 (2.0%)  stddev-sans-first:79.32
| Values: 4225.4  4372.2  4424.2  4259.0  4273.4
| -------- Summary: end --------

With flattening:
| ------- Summary: start -------
| Number of tests: 2
|
| [#0] mutate.html  Cycles:5  Average:3090.58  Median:3098.42  stddev:86.28 (2.8%)  stddev-sans-first:86.22
| Values: 3013.2  3098.4  2992.3  3176.1  3172.8
|
| [#1] inactive_mutate.html  Cycles:5  Average:1905.86  Median:1896.84  stddev:21.26 (1.1%)  stddev-sans-first:20.13
| Values: 1884.1  1916.1  1894.5  1937.8  1896.8
| -------- Summary: end --------

Comment 5

[Miko Mynttinen]

Attachment: flatten-inactive-transform-v3.diff

Comment 6

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 1: Remove unused parameter aUseClipBounds

Review commit: https://reviewboard.mozilla.org/r/257330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257330/

Comment 7

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 2: Make nsDisplayTransform::GetTransformForRendering and DisplayItemClip::ApplyTo const

Review commit: https://reviewboard.mozilla.org/r/257332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257332/

Comment 8

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 3: Extract transform matrix selection into a separate function nsDisplayTransform::ShouldSkipTransform

Review commit: https://reviewboard.mozilla.org/r/257334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257334/

Comment 9

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 4: Add nsDisplayTransformGeometry

Review commit: https://reviewboard.mozilla.org/r/257336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257336/

Comment 10

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 5: Add a pref flag to allow flattening inactive nsDisplayTransform items

Review commit: https://reviewboard.mozilla.org/r/257338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257338/

Comment 11

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 6: Add MatrixStack and TransformClipNode

Review commit: https://reviewboard.mozilla.org/r/257340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257340/

Comment 12

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms

Review commit: https://reviewboard.mozilla.org/r/257342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257342/

Comment 13

[Miko Mynttinen]

Attachment: Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review commit: https://reviewboard.mozilla.org/r/257344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257344/

Comment 14

[Miko Mynttinen]

Comment on attachment 8992499 [details]
Bug 1462672 - Part 5: Add a pref flag to allow flattening inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257338/diff/1-2/

Comment 15

[Miko Mynttinen]

Comment on attachment 8992500 [details]
Bug 1462672 - Part 6: Add MatrixStack and TransformClipNode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257340/diff/1-2/

Comment 16

[Miko Mynttinen]

Comment on attachment 8992501 [details]
Bug 1462672 - Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257342/diff/1-2/

Comment 17

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/1-2/

Comment 18

[Code Review Bot [:reviewbot]]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

https://reviewboard.mozilla.org/r/257344/#review264234


Code analysis found 12 defects in this patch:
 - 12 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 1)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 1)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 1)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 1)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 1)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 1)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 1)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 1)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 1)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 1)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 1)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 1)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

https://reviewboard.mozilla.org/r/257344/#review264236


Code analysis found 12 defects in this patch:
 - 12 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 2)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 2)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 2)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:3793
(Diff revision 2)
>                               const DisplayItemClip& aClip,
>                               LayerState aLayerState,
>                               nsDisplayList* aList,
>                               DisplayItemEntryType aType,
> -                             nsTArray<size_t>& aOpacityIndices)
> +                             nsTArray<size_t>& aOpacityIndices,
> +                             RefPtr<TransformClipNode> aTransform)

Warning: The parameter 'aTransform' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                             RefPtr<TransformClipNode> aTransform)
                             ~~~~~~                    ^
                             const                    &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 2)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 2)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 2)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:4633
(Diff revision 2)
> -        static_cast<nsDisplayTransform*>(item)->MayBeAnimated(mBuilder);
> -    ParentLayerIntRect clipRect;
>      const DisplayItemClip& itemClip = item->GetClip();
> +    ParentLayerIntRect clipRect;
>      if (itemClip.HasClip()) {
> -      itemContent.IntersectRect(itemContent, itemClip.GetClipRect());
> +      nsRect itemClipRect = itemClip.GetClipRect();

Warning: The variable 'itemClipRect' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

      nsRect itemClipRect = itemClip.GetClipRect();
      ~~~~~~ ^
      const &

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 2)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 2)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 2)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

::: layout/painting/FrameLayerBuilder.cpp:5519
(Diff revision 2)
>    , mReused(aItem->IsReused())
>    , mMerged(aItem->HasMergedFrames())
>    , mHasOpacity(aHasOpacity)
> +  , mHasTransform(aHasTransform)
>    , mHasPaintRect(aItem->HasPaintRect())
> +  , mTransform(aTransform)

Warning: Parameter 'aTransform' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

  , mTransform(aTransform)
               ^~~~~~~~~~~
               std::move()

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992495 [details]
Bug 1462672 - Part 1: Remove unused parameter aUseClipBounds

https://reviewboard.mozilla.org/r/257330/#review264248

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992496 [details]
Bug 1462672 - Part 2: Make nsDisplayTransform::GetTransformForRendering and DisplayItemClip::ApplyTo const

https://reviewboard.mozilla.org/r/257332/#review264250

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992497 [details]
Bug 1462672 - Part 3: Extract transform matrix selection into a separate function nsDisplayTransform::ShouldSkipTransform

https://reviewboard.mozilla.org/r/257334/#review264252

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992498 [details]
Bug 1462672 - Part 4: Add nsDisplayTransformGeometry

https://reviewboard.mozilla.org/r/257336/#review264254

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992499 [details]
Bug 1462672 - Part 5: Add a pref flag to allow flattening inactive nsDisplayTransform items

https://reviewboard.mozilla.org/r/257338/#review264256

::: layout/painting/nsDisplayList.h:6383
(Diff revision 2)
>    virtual RetainedDisplayList* GetChildren() const override
>    {
>      return mStoredList.GetChildren();
>    }
>  
> +  virtual RetainedDisplayList* GetSameCoordinateSystemChildren() const override

I'm not a big fan of this, since the children are very much not in the same coordinate space (and FLB has to do a bunch of work to account for that).

Can we instead make the calling code use GetChildren(), and make it clearer that the caller is taking responsibility for dealing with the coordinate space difference?

::: layout/painting/nsDisplayList.cpp:7702
(Diff revision 2)
> +  if (gfxVars::UseWebRender() || !gfxPrefs::LayoutFlattenTransform()) {
> +    return false;
> +  }
> +
> +  MOZ_ASSERT(!mShouldFlatten);
> +  mShouldFlatten = GetTransform().Is2D();

What if we wanted an active layer for this 2d transform?

I think this will break async animations of 2d transforms as written.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992501 [details]
Bug 1462672 - Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms

https://reviewboard.mozilla.org/r/257342/#review264258

Comment 26

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992499 [details]
Bug 1462672 - Part 5: Add a pref flag to allow flattening inactive nsDisplayTransform items

https://reviewboard.mozilla.org/r/257338/#review264256

> What if we wanted an active layer for this 2d transform?
> 
> I think this will break async animations of 2d transforms as written.

I missed the FLBDisplayItemIterator::ShouldFlattenNextItem code handling this. Also explains why tests didn't fail :)

Comment 27

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992500 [details]
Bug 1462672 - Part 6: Add MatrixStack and TransformClipNode

https://reviewboard.mozilla.org/r/257340/#review264264

::: layout/painting/MatrixStack.h:65
(Diff revision 2)
> +typedef MatrixStack<gfx::Matrix4x4Flagged> MatrixStack4x4;
> +
> +class TransformClipNode {
> +  NS_INLINE_DECL_REFCOUNTING(TransformClipNode);
> +public:
> +  TransformClipNode(RefPtr<TransformClipNode> aParent,

Add a comment somewhere about the relative ordering of the clip/transform.

::: layout/painting/MatrixStack.h:75
(Diff revision 2)
> +  , mClip(aClip)
> +  {
> +    MOZ_COUNT_CTOR(TransformClipNode);
> +  }
> +
> +  RefPtr<TransformClipNode>& Parent()

Just return TransformClipNode\*, we don't want caller to think that they can mutate our parent.

::: layout/painting/MatrixStack.h:98
(Diff revision 2)
> +    MOZ_COUNT_DTOR(TransformClipNode);
> +  }
> +
> +  RefPtr<TransformClipNode> mParent;
> +  gfx::Matrix4x4Flagged mTransform;
> +  Maybe<nsRect> mClip;

A DisplayItemClip can have the base rectangle, plus a rectangle for each rounded rect.

We might want to clip to all of these, so maybe make this a DisplayItemClip instead of Maybe<nsRect>?

Comment 28

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992499 [details]
Bug 1462672 - Part 5: Add a pref flag to allow flattening inactive nsDisplayTransform items

https://reviewboard.mozilla.org/r/257338/#review264266

Comment 29

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

https://reviewboard.mozilla.org/r/257344/#review264262

::: layout/painting/FrameLayerBuilder.h:232
(Diff revision 2)
>                        DisplayItemData* aData,
>                        const nsRect& aContentRect,
>                        DisplayItemEntryType aType,
> -                      const bool aHasOpacity);
> +                      const bool aHasOpacity,
> +                      const bool aHasTransform,
> +                      RefPtr<TransformClipNode> aTransform);

clang-tidy complained about this already, but it's easier to just pass TransformClipNode\* instead of passing the RefPtr byval. Here, and in other places.

::: layout/painting/FrameLayerBuilder.h:254
(Diff revision 2)
>    bool mMerged;
>    bool mHasOpacity;
> +  bool mHasTransform;
>    bool mHasPaintRect;
> +
> +  RefPtr<TransformClipNode> mTransform;

Can we put this before all the bools (which could be :1) to try reduce the size of the struct?

::: layout/painting/FrameLayerBuilder.cpp:580
(Diff revision 2)
> +                   NSAppUnitsToFloatPixels(aRect.y, aA2D),
> +                   NSAppUnitsToFloatPixels(aRect.width, aA2D),
> +                   NSAppUnitsToFloatPixels(aRect.height, aA2D));
> +
> +  while (aNode) {
> +    const Matrix4x4Flagged transform = aNode->Transform();

Make this a reference? Don't want to copy it.

::: layout/painting/FrameLayerBuilder.cpp:584
(Diff revision 2)
> +  while (aNode) {
> +    const Matrix4x4Flagged transform = aNode->Transform();
> +
> +    gfx::Rect maxBounds = gfx::Rect::MaxIntRect();
> +
> +    if (aNode->Clip()) {

Are you sure about the ordering here?

Looks like initialize TransformClipNode using the transform and clip both from nsDisplayTransform, and I believe the clip is post-transform in that case.

::: layout/painting/FrameLayerBuilder.cpp:593
(Diff revision 2)
> +                            NSAppUnitsToFloatPixels(clip.width, aA2D),
> +                            NSAppUnitsToFloatPixels(clip.height, aA2D));
> +    }
> +
> +    result = transform.TransformAndClipBounds(result, maxBounds);
> +    aNode = aNode->Parent().get();

Should be able to drop the .get() once Parent() returns a bare pointer.

::: layout/painting/FrameLayerBuilder.cpp:2168
(Diff revision 2)
>    // Convert the region from the coordinates of the container layer
>    // (relative to the snapped top-left of the display list reference frame)
>    // to the PaintedLayer's own coordinates
>    nsIntRegion rgn = aRegion;
> +  if (aTransform) {
> +    Matrix4x4Flagged matrix = GetAccumulatedTransform(aTransform);

Is there a specific reason we just use the accumulated transform here, and not TransformWithNode (clipped!)?

We might want to consider finding a way to cache the result of this, and/or accessing the transform by-reference when there's only one node in the stack.

No rush now, just something to think about.

::: layout/painting/FrameLayerBuilder.cpp:3808
(Diff revision 2)
> -      aItem, aLayerState, nullptr, aContentRect, aType, hasOpacity);
>  
> +  if (IsEffectEndMarker(aType)) {
> +    AssignedDisplayItem item(aItem, aLayerState, nullptr, aContentRect,
> +                             aType, hasOpacity, hasTransform, aTransform);
> +    mAssignedDisplayItems.push_back(std::move(item));

Why did you switch away from emplace_back here?

::: layout/painting/FrameLayerBuilder.cpp:3863
(Diff revision 2)
> -
>    if (aItem->MustPaintOnContentSide()) {
>       mShouldPaintOnContentSide = true;
>    }
>  
> +  if (hasTransform && aType == DisplayItemEntryType::ITEM) {

Comment here about skipping opaque region calculation. We could still do it for axis-aligned transforms too, worth mentioning that option.

::: layout/painting/FrameLayerBuilder.cpp:4657
(Diff revision 2)
> +      MOZ_ASSERT(transformNode);
> +
> +      itemContent =
> +        TransformWithNode(transformNode, itemContent, mAppUnitsPerDevPixel);
> +
> +      Matrix4x4Flagged transform = GetAccumulatedTransform(transformNode);

Why do we use TransformWithNode for one of these and GetAccumulatedTransform for the other? Comment on why at least.

::: layout/painting/FrameLayerBuilder.cpp:5145
(Diff revision 2)
> +
> +        const Matrix4x4Flagged& matrix =
> +          transform->GetTransformForRendering();
> +
> +        Maybe<nsRect> clip =
> +          itemClip.HasClip() ? Some(itemClip.GetClipRect()) : Nothing();

As mentioned in the other patch, this should probably copy the whole clip, not just the base rect.

Alternatively comment and mention that we're not clipping as precisely as we potentially could.

::: layout/painting/FrameLayerBuilder.cpp:5223
(Diff revision 2)
>  
>    PaintedDisplayItemLayerUserData* layerData =
>      static_cast<PaintedDisplayItemLayerUserData*>(aData->mLayer->GetUserData(&gPaintedDisplayItemLayerUserData));
>    nsPoint shift = layerData->mAnimatedGeometryRootOrigin - layerData->mLastAnimatedGeometryRootOrigin;
>  
> +  if (aData->mTransform) {

Comment!

::: layout/painting/FrameLayerBuilder.cpp:6763
(Diff revision 2)
> -    const nsRect& visibleRect = item->GetPaintRect();
> +    nsRect visibleRect = item->GetPaintRect();
> +
> +    if (matrixStack.HasTransform()) {
> +      MOZ_ASSERT(transformLevel > 0);
> +      const Matrix4x4Flagged& matrix =
> +        SelectTransformForEntryType(matrixStack, cdi.mType);

Having this code to select a matrix, and then doing the visibleRect transformation on the pop marker seems wasteful.

It would be nice if we could know if we applied the push marker, and base our decision on that.

::: layout/painting/FrameLayerBuilder.cpp:6828
(Diff revision 2)
>        continue;
>      }
> +#endif
>  
> -    // If the new desired clip state is different from the current state,
> -    // update the clip.
> +    const DisplayItemClip* itemClip = GetItemClip(item, temporaryClip);
> +    const bool itemPaintsOwnClip = itemClip ? item->CanPaintWithClip(*itemClip)

If we've already pushed this clip to the context, then we should just leave it applied and ignore CanPaintWithClip. Otherwise we'll pop it, and maybe have to push it again for the next item.

Moving this logic into ChangeClipIfNeeded might be the easiest?

Comment 30

[Miko Mynttinen]

Comment on attachment 8992500 [details]
Bug 1462672 - Part 6: Add MatrixStack and TransformClipNode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257340/diff/2-3/

Comment 31

[Miko Mynttinen]

Comment on attachment 8992501 [details]
Bug 1462672 - Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257342/diff/2-3/

Comment 32

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/2-3/

Comment 33

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/3-4/

Comment 34

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/4-5/

Comment 35

[Miko Mynttinen]

Comment on attachment 8992500 [details]
Bug 1462672 - Part 6: Add MatrixStack and TransformClipNode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257340/diff/3-4/

Comment 36

[Miko Mynttinen]

Comment on attachment 8992501 [details]
Bug 1462672 - Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257342/diff/3-4/

Comment 37

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/5-6/

Comment 38

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/6-7/

Comment 39

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/7-8/

Comment 40

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/8-9/

Comment 41

[Miko Mynttinen]

Comment on attachment 8992495 [details]
Bug 1462672 - Part 1: Remove unused parameter aUseClipBounds

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257330/diff/1-2/

Comment 42

[Miko Mynttinen]

Comment on attachment 8992496 [details]
Bug 1462672 - Part 2: Make nsDisplayTransform::GetTransformForRendering and DisplayItemClip::ApplyTo const

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257332/diff/1-2/

Comment 43

[Miko Mynttinen]

Comment on attachment 8992497 [details]
Bug 1462672 - Part 3: Extract transform matrix selection into a separate function nsDisplayTransform::ShouldSkipTransform

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257334/diff/1-2/

Comment 44

[Miko Mynttinen]

Comment on attachment 8992498 [details]
Bug 1462672 - Part 4: Add nsDisplayTransformGeometry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257336/diff/1-2/

Comment 45

[Miko Mynttinen]

Comment on attachment 8992499 [details]
Bug 1462672 - Part 5: Add a pref flag to allow flattening inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257338/diff/2-3/

Comment 46

[Miko Mynttinen]

Comment on attachment 8992500 [details]
Bug 1462672 - Part 6: Add MatrixStack and TransformClipNode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257340/diff/4-5/

Comment 47

[Miko Mynttinen]

Comment on attachment 8992501 [details]
Bug 1462672 - Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257342/diff/4-5/

Comment 48

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/9-10/

Comment 49

[Miko Mynttinen]

Comment on attachment 8992502 [details]
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257344/diff/10-11/

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd6df5e62e50
Part 1: Remove unused parameter aUseClipBounds r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/560013cac4ff
Part 2: Make nsDisplayTransform::GetTransformForRendering and DisplayItemClip::ApplyTo const r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/32dde9bafb1c
Part 3: Extract transform matrix selection into a separate function nsDisplayTransform::ShouldSkipTransform r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/acbaec381c31
Part 4: Add nsDisplayTransformGeometry r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/4aeb47e02f20
Part 5: Add a pref flag to allow flattening inactive nsDisplayTransform items r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/4ddb7c20e61a
Part 6: Add MatrixStack and TransformClipNode r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/84ecaa7074f6
Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/92bd3b217862
Part 8: Flatten inactive nsDisplayTransform items r=mattwoodrow

Comment 51

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/bd6df5e62e50
https://hg.mozilla.org/mozilla-central/rev/560013cac4ff
https://hg.mozilla.org/mozilla-central/rev/32dde9bafb1c
https://hg.mozilla.org/mozilla-central/rev/acbaec381c31
https://hg.mozilla.org/mozilla-central/rev/4aeb47e02f20
https://hg.mozilla.org/mozilla-central/rev/4ddb7c20e61a
https://hg.mozilla.org/mozilla-central/rev/84ecaa7074f6
https://hg.mozilla.org/mozilla-central/rev/92bd3b217862

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4036bfc73565
Move #ifdef/#endif to right place r=me

Comment 53

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4036bfc73565

Comment 54

[get_logan]

Regression - Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items r=mattwoodrow

Causes abnormality on https://www.fynedc.com/
The blue blob thing doesn't refresh properly/quick enough when scrolling around page and looks crumpled with jagged edges instead of smooth curves (like in stable, beta and earlier nightlies).

2018-07-21T13:43:07: INFO : Narrowed inbound regression window from [85f1c6e5, 522e7b9b] (3 builds) to [85f1c6e5, 92bd3b21] (2 builds) (~1 steps left)
2018-07-21T13:43:07: DEBUG : Starting merge handling...
2018-07-21T13:43:07: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=92bd3b2178623a036ea91990aa5570e039890ff4&full=1
2018-07-21T13:43:08: DEBUG : Found commit message:
Bug 1462672 - Part 8: Flatten inactive nsDisplayTransform items r=mattwoodrow

MozReview-Commit-ID: 5ushAgOec9U

2018-07-21T13:43:08: DEBUG : Did not find a branch, checking all integration branches
2018-07-21T13:43:08: INFO : The bisection is done.
2018-07-21T13:43:08: INFO : Stopped

Comment 55

[get_logan]

Attachment: blue_blob_regression.mp4

Comment 57

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 63.0a1 (2018-07-20) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 63.0b14, on Windows 10 x64, Ubuntu 16.04 LTS and macOS 10.13.6.