Closed Bug 1313898 Opened 8 years ago Closed 8 years ago

Draw non-trivial clip-path/svg mask onto mask layer

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

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

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(5 files, 4 obsolete files)

Bug 1313898 - Part 2. Paint non-trivial clip-path onto mask layer.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1313898 - Part 1. Re-implement CSSMaskLayerUserData invalidation detection.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1313898 - Part 4. Invalidation test for image-mask boxing model.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1313898 - Part 5. SVG-mask/clip-path invalidation reftest.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1313898 - Part 3. Enable svg-image on mask layer.
58 bytes, text/x-review-board-request
mstange
: review+
Details
No description provided.
Blocks: basic-shape-ship, 1303654
Priority: -- → P3
CJ: I think we can remove the isTrivialClip optimization for now, and make all clip paths go through a mask surface. The optimization is probably not helping much at the moment, because the drawing library underneath creates a mask surface anyway if you ask it to clip something to a path.
(In reply to Markus Stange [:mstange] from comment #1) > CJ: I think we can remove the isTrivialClip optimization for now, and make > all clip paths go through a mask surface. The optimization is probably not > helping much at the moment, because the drawing library underneath creates a > mask surface anyway if you ask it to clip something to a path. Understand. I will start a prototype soon.
test failure: http://searchfox.org/mozilla-central/source/layout/reftests/svg/smil/transform/translate-clipPath-1.svg Painting clip-path and svg-mask on mask layer has the same problem 1. Elements inside clipPath/mask element is animatable/restyle-able. We need to receive this render change and refresh mask layer. 2. we have a bug on supporting svg-mask animation(bug 1118710). Basically, in gecko, svg-mask is not animatable. Even though, we still need to handle elements restyle. So, this one is highly associated with bug 1313877
See Also: → 1313877
This one might be very simple. Add a render observer to receive rendering change of clip-path element and it's descendant by using nsSVGEffects::AddRenderingObserver, corrupt mask layer once notification receive.
(In reply to C.J. Ku[:cjku](UTC+8) from comment #15) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=a87653793798 Test result looks ok. Most failures are pixel not perfect. Some of them are fixed(bug 1234485 comment 119)
Summary: Draw clip-path onto mask layer → Draw clip-path/svg mask onto mask layer
In part 3, we have UDRenderObserver uses CSSMaskUserData. The life cycle of UDRenderObserver is managed by Frame. The life cycle of CSSMaskUserData is managed by Layer. I still can not find a way to make sure CSSMaskUserData live longer then UDRenderObserver
Matt, do you have an opinion on how we should invalidate SVG mask layers? IIRC modifying the mask invalidates the masked frame. So maybe we could just call maskItem->IsInvalid, which checks the frame's invalidation marker. But nsDisplayWrapList::IsInvalid does a little more magic than just that, and I don't really understand what it's doing at first glance.
Flags: needinfo?(matt.woodrow)
(In reply to Markus Stange [:mstange] from comment #25) > Matt, do you have an opinion on how we should invalidate SVG mask layers? > > IIRC modifying the mask invalidates the masked frame. So maybe we could just > call maskItem->IsInvalid, which checks the frame's invalidation marker. But > nsDisplayWrapList::IsInvalid does a little more magic than just that, and I > don't really understand what it's doing at first glance. nsDisplayWrapList items can sometimes be merged together, this happens often with nsDisplayOpacity. That implementation is just checking IsInvalid() on all the merged frames, not just the primary one. How does modifying the mask invalidate the masked frame? If we're sure that all changes within the mask content result in us invalidating the masked frame, then checking IsInvalid sounds fine.
Flags: needinfo?(matt.woodrow)
Keywords: perf
Priority: P3 → P2
It wokks and make whole thing much simpler. We can use nsIFrame::IsInvalidate for both image-mask and svg-mask/cip-path.
Attachment #8807474 - Attachment is obsolete: true
Attachment #8808727 - Attachment is obsolete: true
Attachment #8807452 - Attachment is obsolete: true
Blocks: mask-perf
Blocks: 1317636
Attachment #8810742 - Flags: review?(mstange)
Attachment #8807450 - Flags: review?(mstange)
Attachment #8807451 - Flags: review?(mstange)
Attachment #8810770 - Flags: review?(mstange)
Attachment #8810743 - Flags: review?(mstange)
Attachment #8810744 - Flags: review?(mstange)
Comment on attachment 8810742 [details] Bug 1313898 - Part 1. Re-implement CSSMaskLayerUserData invalidation detection. https://reviewboard.mozilla.org/r/93058/#review93242 ::: layout/base/FrameLayerBuilder.cpp:1590 (Diff revision 3) > > bool > operator==(const CSSMaskLayerUserData& aOther) const > { > - if (mHash != aOther.mHash) { > - return false; > + nsRect dirtyRect; > + if (mFrame != aOther.mFrame || mFrame->IsInvalid(dirtyRect)) { I think the mFrame->IsInvalid check should go into the caller. And actually I'd prefer if you called IsInvalid on the mask display item instead of on the frame.
Attachment #8810742 - Flags: review?(mstange) → review+
Comment on attachment 8807450 [details] Bug 1313898 - Part 2. Paint non-trivial clip-path onto mask layer. https://reviewboard.mozilla.org/r/90586/#review93246 ::: layout/svg/nsSVGIntegrationUtils.cpp:755 (Diff revision 6) > + matSR.Restore(); > + matSR.SetContext(&ctx); > + > + SetupContextMatrix(firstFrame, aParams, offsetToBoundingBox, > + offsetToUserSpace, false); > + Matrix clippedMaskTransform; rename to clipMaskTransform ::: layout/svg/nsSVGIntegrationUtils.cpp:778 (Diff revision 6) > + Rect drawRect = IntRectToRect(IntRect(IntPoint(0, 0), > + clipMaskSurface->GetSize())); This could be Rect drawRect(Point(0, 0), Size(clipMaskSurface->GetSize())); I think
Attachment #8807450 - Flags: review?(mstange) → review+
Comment on attachment 8807451 [details] Bug 1313898 - Part 3. Also paint trivial clip-path onto mask layer. https://reviewboard.mozilla.org/r/90588/#review93250
Attachment #8807451 - Flags: review?(mstange) → review+
Comment on attachment 8810770 [details] Bug 1313898 - Part 3. Enable svg-image on mask layer. https://reviewboard.mozilla.org/r/93102/#review93252
Attachment #8810770 - Flags: review?(mstange) → review+
Comment on attachment 8810743 [details] Bug 1313898 - Part 4. Invalidation test for image-mask boxing model. https://reviewboard.mozilla.org/r/93060/#review93254
Attachment #8810743 - Flags: review?(mstange) → review+
Comment on attachment 8810743 [details] Bug 1313898 - Part 4. Invalidation test for image-mask boxing model. https://reviewboard.mozilla.org/r/93060/#review93256 Actually, these need to go in a different directory. We can't use MozReftestInvalidate in w3c submitted reftests.
Attachment #8810743 - Flags: review+ → review-
Comment on attachment 8810744 [details] Bug 1313898 - Part 5. SVG-mask/clip-path invalidation reftest. https://reviewboard.mozilla.org/r/93062/#review93262 So these also need to be moved to a different directory. I'd prefer will-change:transform as a layer activator over 3d transforms.
Attachment #8810744 - Flags: review?(mstange)
Comment on attachment 8810743 [details] Bug 1313898 - Part 4. Invalidation test for image-mask boxing model. https://reviewboard.mozilla.org/r/93060/#review93510
Attachment #8810743 - Flags: review?(mstange) → review+
Comment on attachment 8810744 [details] Bug 1313898 - Part 5. SVG-mask/clip-path invalidation reftest. https://reviewboard.mozilla.org/r/93062/#review93512
Attachment #8810744 - Flags: review?(mstange) → review+
Comment on attachment 8810742 [details] Bug 1313898 - Part 1. Re-implement CSSMaskLayerUserData invalidation detection. https://reviewboard.mozilla.org/r/93058/#review93680 ::: layout/base/FrameLayerBuilder.cpp:3872 (Diff revision 4) > bool snap; > nsRect bounds = aMaskItem->GetBounds(mBuilder, &snap); > nsIntRect itemRect = ScaleToOutsidePixels(bounds, snap); > - CSSMaskLayerUserData newUserData(aMaskItem->Frame(), itemRect); > - if (*oldUserData == newUserData) { > + CSSMaskLayerUserData newUserData(aMaskItem->Frame(), itemRect.Size()); > + nsRect dirtyRect; > + if (aMaskItem->IsInvalid(dirtyRect) && *oldUserData == newUserData) { Isn't this missing a negation in front of the IsInvalid call?
Reftest is ok. Once assertion crash while running mochitest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee826fda6de1c97e812ef544398d57b026c78ab7&selectedJob=31298136 This crash is caused indirectly by Part 3: Bug 1313898 - Part 3. Also paint trivial clip-path onto mask layer. Need more time to figure out root cause. I will move "Part 3" into another follow-up bug.
Summary: Draw clip-path/svg mask onto mask layer → Draw non-trivial clip-path/svg mask onto mask layer
Blocks: 1318266
Attachment #8807451 - Attachment is obsolete: true
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0e237db8f61e Part 1. Re-implement CSSMaskLayerUserData invalidation detection. r=mstange https://hg.mozilla.org/integration/autoland/rev/5001a4e7f907 Part 2. Paint non-trivial clip-path onto mask layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/9ee1fec609d2 Part 3. Enable svg-image on mask layer. r=mstange https://hg.mozilla.org/integration/autoland/rev/cf2844a4d93c Part 4. Invalidation test for image-mask boxing model. r=mstange https://hg.mozilla.org/integration/autoland/rev/ba20391edbec Part 5. SVG-mask/clip-path invalidation reftest. r=mstange
Blocks: basic-shape
Depends on: 1328063
Depends on: 1362000
Depends on: 1363855
Depends on: 1378707
Depends on: 1378710
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: