Closed
Bug 1462672
Opened 7 years ago
Closed 6 years ago
Flatten inactive nsDisplayTransform display items
Categories
(Core :: Web Painting, enhancement, P2)
Core
Web Painting
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.
Assignee | ||
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8983968 -
Attachment is obsolete: true
Assignee | ||
Comment 4•6 years ago
|
||
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 --------
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #8987611 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8989453 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
mozreview-review |
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•6 years ago
|
||
mozreview-review |
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•6 years ago
|
||
mozreview-review |
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 21•6 years ago
|
||
mozreview-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 22•6 years ago
|
||
mozreview-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+
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8992498 [details]
Bug 1462672 - Part 4: Add nsDisplayTransformGeometry
https://reviewboard.mozilla.org/r/257336/#review264254
Attachment #8992498 -
Flags: review?(matt.woodrow) → review+
Comment 24•6 years ago
|
||
mozreview-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 25•6 years ago
|
||
mozreview-review |
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 26•6 years ago
|
||
mozreview-review-reply |
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•6 years ago
|
||
mozreview-review |
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 28•6 years ago
|
||
mozreview-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 29•6 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 50•6 years ago
|
||
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
Comment 51•6 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 52•6 years ago
|
||
Pushed by mikokm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4036bfc73565
Move #ifdef/#endif to right place r=me
Comment 53•6 years ago
|
||
bugherder |
Comment 54•6 years ago
|
||
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•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Flags: qe-verify+
Comment 57•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•