Closed Bug 1462672 Opened 7 years ago Closed 6 years ago

Flatten inactive nsDisplayTransform display items

Categories

(Core :: Web Painting, enhancement, P2)

enhancement

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- verified

People

(Reporter: mikokm, Assigned: mikokm)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 3 obsolete files)

(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), text/x-review-board-request
mattwoodrow
: review+
Details
(deleted), video/mp4
Details
This bug tracks the work of implementing flattening, as described in bug 1439292, for nsDisplayTransform items.
Priority: -- → P2
Attached patch transform-flattening-v1.patch (obsolete) (deleted) — Splinter Review
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.
Attached patch flatten-inactive-transform-v2.diff (obsolete) (deleted) — Splinter Review
Attachment #8983968 - Attachment is obsolete: true
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 --------
Attached patch flatten-inactive-transform-v3.diff (obsolete) (deleted) — Splinter Review
Attachment #8987611 - Attachment is obsolete: true
Attachment #8989453 - Attachment is obsolete: true
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 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 on attachment 8992495 [details] Bug 1462672 - Part 1: Remove unused parameter aUseClipBounds https://reviewboard.mozilla.org/r/257330/#review264248
Attachment #8992495 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8992496 [details] Bug 1462672 - Part 2: Make nsDisplayTransform::GetTransformForRendering and DisplayItemClip::ApplyTo const https://reviewboard.mozilla.org/r/257332/#review264250
Attachment #8992496 - Flags: review?(matt.woodrow) → review+
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
Attachment #8992497 - Flags: review?(matt.woodrow) → review+
Attachment #8992498 - Flags: review?(matt.woodrow) → review+
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 on attachment 8992501 [details] Bug 1462672 - Part 7: Add FLBDisplayItemIterator support for flattening inactive transforms https://reviewboard.mozilla.org/r/257342/#review264258
Attachment #8992501 - Flags: review?(matt.woodrow) → review+
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 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>?
Attachment #8992500 - Flags: review?(matt.woodrow) → review+
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
Attachment #8992499 - Flags: review?(matt.woodrow) → review+
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?
Attachment #8992502 - Flags: review?(matt.woodrow) → review+
Pushed by mikokm@gmail.com: 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
Depends on: 1477260
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
Attached video blue_blob_regression.mp4 (deleted) —
Depends on: 1477693
Depends on: 1477847, 1477831
No longer depends on: 1477847
Depends on: 1477427
No longer depends on: 1477427
Depends on: 1483649
Depends on: 1490518
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1502870
Depends on: 1503463
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: