Closed Bug 1318266 Opened 8 years ago Closed 7 years ago

Draw trivial clip-path onto mask layer

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

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

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(8 files, 5 obsolete files)

LOG
5.00 MB, text/plain
Details
Bug 1318266 - Part 1. Paint trivial clip-path onto mask layer.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1318266 - Part 2. Remove unnecessary nsDisplayScrollInfoLayer.
58 bytes, text/x-review-board-request
mstange
: review+
Details
testcase for scroll info layer
950 bytes, text/html
Details
screenshot of squared-off tabs
9.98 KB, image/png
Details
Bug 1318266 - Part 3. Create css mask layer even if there are clips on the layer.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1318266 - Part 4. Update mask transform.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1318266 - Part 5. Test cases.
58 bytes, text/x-review-board-request
mstange
: review+
Details
A follow-up of bug 1313898.(bug 1313898 comment 71)
Status: NEW → ASSIGNED
Priority: -- → P3
I can 100% repro it locally on linux debug build too.
0x00007fffe4651439 in mozilla::layers::APZCTreeManager::PrepareNodeForLayer (this=0x7fffb62a7400, aLayer=..., aMetrics=..., aLayersId=1, aAncestorTransform=..., aParent=0x7fffb8f0b680, aNextSibling=0x0, aState=...) at /home/cjcool/repository/mozilla-central/gfx/layers/apz/src/APZCTreeManager.cpp:656 656 MOZ_ASSERT(aAncestorTransform.FuzzyEqualsMultiplicative(apzc->GetAncestorTransform())); (gdb) p aAncestorTransform $1 = (const mozilla::gfx::Matrix4x4 &) @0x7fffb8c0ef00: {{{_11 = 1, _12 = 0, _13 = 0, _14 = 0, _21 = 0, _22 = 1, _23 = 0, _24 = 0, _31 = 0, _32 = 0, _33 = 1, _34 = 0, _41 = 477.983398, _42 = 38, _43 = 0, _44 = 1}, components = {1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 477.983398, 38, 0, 1}}, static kTransformAndClipRectMaxVerts = <optimized out>} (gdb) p apzc->GetAncestorTransform() $2 = {{{_11 = -1, _12 = 0, _13 = 0, _14 = 0, _21 = 0, _22 = 1, _23 = 0, _24 = 0, _31 = 0, _32 = 0, _33 = 1, _34 = 0, _41 = 1249.80005, _42 = 38, _43 = 0, _44 = 1}, components = {-1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 1249.80005, 38, 0, 1}}, static kTransformAndClipRectMaxVerts = <optimized out>}
Oh! Is this related to bad nesting of ScrollMetaData? If you get a layers dump of the client layers (e.g. using layout.display-list.dump-content), is there a layer that has a ScrollMetaData that's also on one of its ancestor layers?
./mach mochitest toolkit/components/extensions/test/mochitest/test_ext_i18n_css.html
Attached file LOG —
crash at #15969
(In reply to Markus Stange [:mstange] from comment #3) > Oh! Is this related to bad nesting of ScrollMetaData? If you get a layers > dump of the client layers (e.g. using layout.display-list.dump-content), is > there a layer that has a ScrollMetaData that's also on one of its ancestor > layers? I didn't see this symptom. With this clip-path[1] and paint-clip-path on mask layer will hit this assertion This clip path is actually the one applied to the address bar, and is used in chrome process. [1] http://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#1121
Attachment #8818486 - Attachment is obsolete: true
There are several things that I do not quite understand 1. Even if not calling aBuilder->EnterSVGEffectsContents/ExitSVGEffectsContents in nsFrame::BuildDisplayListForStackingContext, all test cases are still pass. Not sure why we need it(nsDisplayScrollInfoLayer), and no idea how to create a test case to verify usage of nsDisplayScrollInfoLayer 2. I can not create a test case which will call AppendNewScrollInfoItemForHoisting in ScrollFrameHelper::BuildDisplayList. Add overflow:scroll to a masked element is not work.
Attachment #8818915 - Flags: review?(mstange)
Attachment #8818915 - Flags: review?(mstange)
Attachment #8818915 - Flags: review?(mstange)
Here's a testcase that's not scrollable with the mouse wheel if you remove the calls to EnterSVGEffectsContents / ExitSVGEffectsContents. I'm sorry we don't have a test for this :-(
Comment on attachment 8818915 [details] Bug 1318266 - Part 2. Remove unnecessary nsDisplayScrollInfoLayer. https://reviewboard.mozilla.org/r/98836/#review99542 Seems kinda hacky but ok. Once we know that all nsDisplayMask items can create layers, we should just be able to change BuildDisplayListForStackingContext to only create the scroll info layer items for filters.
Attachment #8818915 - Flags: review?(mstange) → review+
Comment on attachment 8818485 [details] Bug 1318266 - Part 1. Paint trivial clip-path onto mask layer. https://reviewboard.mozilla.org/r/98544/#review99544
Attachment #8818485 - Flags: review+
(In reply to Markus Stange [:mstange] from comment #15) > Comment on attachment 8818915 [details] > Bug 1318266 - Part 2. Remove unnecessary nsDisplayScrollInfoLayer. > > https://reviewboard.mozilla.org/r/98836/#review99542 > > Seems kinda hacky but ok. Once we know that all nsDisplayMask items can > create layers, we should just be able to change > BuildDisplayListForStackingContext to only create the scroll info layer > items for filters. We need to know which kind of layer manager to determine layer state, unfortunately that information can not be retrieved in BuildDisplayListForStackingContext. I will spend more time on understanding scroll relative things after land this bug. BuildDisplayListForStackingContext
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/55790893cab4 Part 1. Paint trivial clip-path onto mask layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/00bb4e2f5a3c Part 2. Remove unnecessary nsDisplayScrollInfoLayer. r=mstange
https://hg.mozilla.org/mozilla-central/rev/55790893cab4 https://hg.mozilla.org/mozilla-central/rev/00bb4e2f5a3c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Attached image screenshot of squared-off tabs —
Backed out for breaking tab drawing with a LWT applied (see the attached screenshot). We should probably come up with a reduced testcase for this regression and make sure that also lands whenever this is ready for re-landing. https://hg.mozilla.org/mozilla-central/rev/a7b0bf1ba2c17230e12af396c1b15cbdd4070018
Status: RESOLVED → REOPENED
status-firefox53: fixed → ---
Flags: in-testsuite?
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
(In reply to Ryan VanderMeulen [:RyanVM] from comment #22) > Created attachment 8819627 [details] > screenshot of squared-off tabs > > Backed out for breaking tab drawing with a LWT applied (see the attached > screenshot). We should probably come up with a reduced testcase for this > regression and make sure that also lands whenever this is ready for > re-landing. > > https://hg.mozilla.org/mozilla-central/rev/ > a7b0bf1ba2c17230e12af396c1b15cbdd4070018 Hi Ryan, May I know what LWT you applied and what platform you test on? I can not repro it on linux.
Flags: needinfo?(ryanvm)
Windows 10 here. Reproduced with any on AMO.
Flags: needinfo?(ryanvm)
OK, I can repro on my win10 too. Will figure it out.
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b3eaebda3d9d Part 1. Paint trivial clip-path onto mask layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/9e9d4b22a77f Part 2. Remove unnecessary nsDisplayScrollInfoLayer. r=mstange
Comment on attachment 8818485 [details] Bug 1318266 - Part 1. Paint trivial clip-path onto mask layer. https://reviewboard.mozilla.org/r/98544/#review99742 ::: layout/painting/nsDisplayList.cpp:7349 (Diff revision 4) > return LAYER_SVG_EFFECTS; > } > > bool nsDisplayMask::ShouldPaintOnMaskLayer(LayerManager* aManager) > { > - if (!aManager->IsCompositingCheap()) { > + if (!aManager->IsCompositingCheap() || aManager->IsWidgetLayerManager()) { Doesn't this change completely disable the use of mask layers?
Attachment #8818485 - Flags: review-
Y(In reply to Markus Stange [:mstange] from comment #29) > Comment on attachment 8818485 [details] > Bug 1318266 - Part 1. Paint trivial clip-path onto mask layer. > > https://reviewboard.mozilla.org/r/98544/#review99742 > > ::: layout/painting/nsDisplayList.cpp:7349 > (Diff revision 4) > > return LAYER_SVG_EFFECTS; > > } > > > > bool nsDisplayMask::ShouldPaintOnMaskLayer(LayerManager* aManager) > > { > > - if (!aManager->IsCompositingCheap()) { > > + if (!aManager->IsCompositingCheap() || aManager->IsWidgetLayerManager()) { > > Doesn't this change completely disable the use of mask layers? Yes, I press on land button on review-board by accident :(
Oops :)
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/56a81444793c Backed out changeset 9e9d4b22a77f for developer's request https://hg.mozilla.org/integration/autoland/rev/6ba1e8fcb6e1 Backed out changeset b3eaebda3d9d
Comment on attachment 8819828 [details] Bug 1318266 - Part 3. Create css mask layer even if there are clips on the layer. https://reviewboard.mozilla.org/r/99498/#review99782 Why is there a clip on the mask item? We call maskClipState.Clear(); before we create the nsDisplayMask.
Comment on attachment 8819828 [details] Bug 1318266 - Part 3. Create css mask layer even if there are clips on the layer. https://reviewboard.mozilla.org/r/99498/#review99782 Even we create a mask item with no clip, we can still setClip later, such as: http://searchfox.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp#4076
No test for the regression this caused last time? :(
Flags: needinfo?(cku)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #37) > Even we create a mask item with no clip, we can still setClip later, such as: > http://searchfox.org/mozilla-central/source/layout/painting/ > FrameLayerBuilder.cpp#4076 Oh, I see. Ok, that's a fine workaround for now. We do need a test though.
Yes, I am still thinking how to create a test to hit this condition...
Update current status: 1. Part 3 Even if layerClip.HasClip() return true, we still can create mask for CSS. All we care is layerClip.GetRoundedRectCount() must be equal to zero since we are not able to paint both round-clip and css mask onto single mask layer. 2. While testing, I notice the mask position is not correct in the following scenario a. Apply path 1~3 b. Hide menu bar c. Press Alt key to make menu bar appear. The position of clip-path's mask is not at the right place. That's why I create part 4. 3. Still have no clue of how to create test case to hit the regression in the first check-in. Simply create a xul reftest is not work. Keep working
Flags: needinfo?(cku)
To create a test, we need 1. A div with mask plus willchange 2. InnermostScrollClipApplicableToAGR at [1] return nullptr(or any pointer value other then itemScrollClip) 3. So that we will call SetClip at [2] 4. Then, layerClip.HasClip() at [3] return true, so the else-if statement at [4] is skip. clip-path mask is not created correctly. To return nullptr in #2 is most difficult part here. [1] https://hg.mozilla.org/mozilla-central/file/1e5621e43ac6/layout/painting/FrameLayerBuilder.cpp#l4063 [2] https://hg.mozilla.org/mozilla-central/file/1e5621e43ac6/layout/painting/FrameLayerBuilder.cpp#l4076 [3] https://hg.mozilla.org/mozilla-central/file/1e5621e43ac6/layout/painting/FrameLayerBuilder.cpp#l4320 [4] https://hg.mozilla.org/mozilla-central/file/1e5621e43ac6/layout/painting/FrameLayerBuilder.cpp#l4340
Attachment #8819887 - Flags: review?(mstange)
(In reply to Markus Stange [:mstange] from comment #48) > Does > http://searchfox.org/mozilla-central/source/layout/reftests/xul/inactive- > fixed-bg-bug1205630.xul manage to do #2? Yay, it works with some minor change. you are awesome, thanks.
Attachment #8820148 - Flags: review?(mstange)
Comment on attachment 8818485 [details] Bug 1318266 - Part 1. Paint trivial clip-path onto mask layer. https://reviewboard.mozilla.org/r/98544/#review100148
Attachment #8818485 - Flags: review?(mstange) → review+
Comment on attachment 8819828 [details] Bug 1318266 - Part 3. Create css mask layer even if there are clips on the layer. https://reviewboard.mozilla.org/r/99498/#review100150
Attachment #8819828 - Flags: review?(mstange) → review+
Comment on attachment 8819887 [details] Bug 1318266 - Part 4. Update mask transform. https://reviewboard.mozilla.org/r/99522/#review100152
Attachment #8819887 - Flags: review?(mstange) → review+
Comment on attachment 8820148 [details] Bug 1318266 - Part 5. Test cases. https://reviewboard.mozilla.org/r/99690/#review100154 ::: layout/reftests/svg/reftest.list:465 (Diff revision 1) > +fuzzy(255,304) == paint-on-maskLayer-1a.html paint-on-maskLayer-1-ref.html > +fuzzy(48,200) == paint-on-maskLayer-1b.html paint-on-maskLayer-1-ref.html This is a crazy amount of fuzz. Why is it so much?
Comment on attachment 8820148 [details] Bug 1318266 - Part 5. Test cases. https://reviewboard.mozilla.org/r/99690/#review100154 > This is a crazy amount of fuzz. Why is it so much? border: 1px solid transparent; Hm... that's becasue of this property. Let me try to reduce diff pixels
Oh, of course, it's not using box-sizing:border-box so the border width adds to the size of the element.
Attached file reftest.txt (obsolete) —
Attach reftest result after adding box-sizing:border-box;
Comment on attachment 8820148 [details] Bug 1318266 - Part 5. Test cases. https://reviewboard.mozilla.org/r/99690/#review100154 > border: 1px solid transparent; > Hm... that's becasue of this property. Let me try to reduce diff pixels From fuzzy(255,304) to fuzzy(48,200)
Hmm, not sure why the rectangle has a fuzzy bottom edge. Maybe because it's not pixel aligned. Can you try adding body { margin: 0; } and seeing whether that fixes it?
(In reply to Markus Stange [:mstange] from comment #64) > Hmm, not sure why the rectangle has a fuzzy bottom edge. Maybe because it's > not pixel aligned. Can you try adding > body { margin: 0; } > and seeing whether that fixes it? No, but if I set mask {height: 100px} and div { height: 200px; } result look good
Comment on attachment 8820148 [details] Bug 1318266 - Part 5. Test cases. https://reviewboard.mozilla.org/r/99690/#review100178 Hmm, not sure why that would make a difference. Anyway, let's land this. Thank you!
Attachment #8820148 - Flags: review?(mstange) → review+
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a4c80aed15d8 Part 1. Paint trivial clip-path onto mask layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/95d7547ad1c3 Part 2. Remove unnecessary nsDisplayScrollInfoLayer. r=mstange https://hg.mozilla.org/integration/autoland/rev/936cc01602c4 Part 3. Create css mask layer even if there are clips on the layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/47b22189955a Part 4. Update mask transform. r=mstange https://hg.mozilla.org/integration/autoland/rev/a6d9ee58afd9 Part 5. Test cases. r=mstange
Ah, I should have caught that during review.
Comment on attachment 8818915 [details] Bug 1318266 - Part 2. Remove unnecessary nsDisplayScrollInfoLayer. https://reviewboard.mozilla.org/r/98836/#review100324 ::: layout/painting/FrameLayerBuilder.cpp:4347 (Diff revision 6) > SetupMaskLayerForCSSMask(ownLayer, maskItem); > + > + nsDisplayItem* next = aList->GetBottom(); > + if (next && next->GetType() == nsDisplayItem::TYPE_SCROLL_INFO_LAYER) { > + // Since we do build a layer for mask, there is no need for this > + // scroll info layer anymore. You need to call the destructor on |next| here. The nsDisplayList destructor will only destroy items that are still in the list at the end (or re-added, see "savedItems"); every element that gets removed needs to be destroyed manually. See e.g. http://searchfox.org/mozilla-central/rev/46ded325e8f60b3d4c731b27c31d383911056a38/layout/painting/FrameLayerBuilder.cpp#3994-3995 . This is because the memory model of nsDisplayItems is that they get allocated into the nsDisplayListBuilder's arena and their memory is freed once the nsDisplayListBuilder is destroyed. But the destructors still need to run, so something needs to call item->~nsDisplayItem().
Attachment #8818915 - Flags: review+ → review-
Comment on attachment 8818915 [details] Bug 1318266 - Part 2. Remove unnecessary nsDisplayScrollInfoLayer. https://reviewboard.mozilla.org/r/98836/#review100352
Attachment #8818915 - Flags: review- → review+
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1d7029919771 Part 1. Paint trivial clip-path onto mask layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/cd13811e4697 Part 2. Remove unnecessary nsDisplayScrollInfoLayer. r=mstange https://hg.mozilla.org/integration/autoland/rev/60a32c7312ae Part 3. Create css mask layer even if there are clips on the layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/3a639e630b0f Part 4. Update mask transform. r=mstange https://hg.mozilla.org/integration/autoland/rev/ea340a9879aa Part 5. Test cases. r=mstange
Depends on: 1325550
Reopen because of Bug 1325550. Paint clip-path on mask layer is going to be disable before figure out performance regression
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This size of this clip-path[1] keep changing while running tresize talos test, so we keep regenerate mask, mask layer cache is always missed. And I notice when the width of URL in urlBar is longer then the urlBar container, RequiredLayerStateForChildren return LAYER_ACTIVE since there is one child has different AGR. [1] clip-path: url("chrome://browser/content/browser.xul#urlbar-back-button-clip-path-win10");
Can you find out why there is a child with a different AGR? I can see how, if you have a very long URL, the URL text field becomes scrollable, and once it's actively scrolled, the text will have a different AGR. But as long as it's not being scrolled, I'd hope it wouldn't get its own AGR. Unless... maybe it's being activated through nsLayoutUtils::MaybeCreateDisplayPort because it's the first scrollable frame in the document? That would be unfortunate. It's worth disabling that code and checking whether it improves tresize.
Depends on: 1382534
Profile this page: https://bug1364743.bmoattachments.org/attachment.cgi?id=8867529 FPS goes up to 60, from 40, if we land these patch....
Keywords: perf
Attachment #8898245 - Flags: review?(mstange)
Attachment #8898245 - Flags: review?(mstange)
Attachment #8898245 - Attachment is obsolete: true
Err... we don't apply clip-path only url address bar anymore, instead, now we have gradient mask on it. So whether paint clip-path onto mask layer won't impact tresize of talos anymore.
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1da62a47edd Enable paint clip-path on mask layer. r=me
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3376bebf1f11 Enable paint clip-path on mask layer. r=me.
https://hg.mozilla.org/mozilla-central/rev/3376bebf1f11
Status: REOPENED → RESOLVED
Closed: 8 years ago → 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: