Closed Bug 1234485 Opened 9 years ago Closed 8 years ago

Draw image mask onto mask layer

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: tlt-wip)

Attachments

(15 files, 7 obsolete files)

Bug 1234485 - Part 10. Paint mask onto mask layer when possible.
58 bytes, text/x-review-board-request
mstange
: review+
mattwoodrow
: review+
Details
mask-anim.html
700 bytes, text/html
Details
Bug 1234485 - Part 1. Extract DetermineMaskUsage from PaintMaskAndClipPath.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 3. Implement nsSVGIntegrationUtils::PaintMask.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 4. Implement nsDisplayMask::ShouldPaintOnMaskLayer.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 5. Implement MaskImageData::CreateImageAndImageContainer.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask.
58 bytes, text/x-review-board-request
mstange
: review+
mattwoodrow
: review+
Details
background-position-anim.html
8.15 KB, text/html
Details
Bug 1234485 - Part 2. Implement nsSVGIntegrationUtils::IsMaskResourceReady.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 11. Paint SVG mask on PaintedLayer before bug 1313877 fixed.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 12. Extract PaintMaskSurface from GenerateMaskSurface.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 6. Implement nsStyleImageLayers::operator=.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1234485 - Part 9. Implement MaskLayerUserData::constructor & operator=.
58 bytes, text/x-review-board-request
mstange
: review+
Details
mask transform
662 bytes, text/html
Details
Current implementation in bug 686281 is composing mask with background image at client side, which is basically using CPU. We may try to create a mask layer and do the composition at compositor side. We should have smoother mask-size/ mask-position animation with this optimization.
Assignee: nobody → cku
I will start working on this topic form 2016 Q3
Work in progress. This is mainly a performance fine tuning for animated masking effect.
Blocks: mask-ship
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla51
Current plan 1. Paint css mask onto the mask layer created in ContainerState::CreateMaskLayer 2. Create mask layer before background/ mask animation.
+cc: mstange who's working on clipping changes related to scrolling which may affect scrolling inside masked parent containers.
Flags: needinfo?(mstange)
The patches I'm working on for bug 1214146 will make it easier to get APZ scrolling of compositor-side masks correct, especially if you have something like a position:fixed element inside a scrolled element with a clip-path. But there's lots of stuff that can be worked on for this bug before you get to the point of worrying about APZ correctness. CJ, have you already started working on this? I recommend starting by separating nsDisplaySVGEffects into nsDisplayMask and nsDisplayFilter. nsDisplayMask would be responsible for the combined rendering of both clip-path and mask, regardless of whether they're CSS or SVG clip-paths / masks. Then you can use LAYER_INACTIVE / LAYER_ACTIVE for nsDisplayMask, and LAYER_SVG_EFFECTS for nsDisplayFilter. If an element has both a clip-path / mask and a filter, the separation might conceivably hurt performance a bit, but I don't think that's worth worrying about much. There are two challenges that I anticipate in FrameLayerBuilder. The first one is layer transforms. If you create a ContainerLayer for a mask, and then have a PaintedLayer inside that layer, then the transform of the PaintedLayer will be computed relative to the reference frame. Masks don't cause a frame to become a reference frame, so the transform of the PaintedLayer will be computed across the mask to the nearest ancestor that has a CSS transform, or to the root frame. That means that there must not be a transform on the mask ContainerLayer. Instead, to get the mask shape to be in the right place, I think you'll need to set the right transform on the ContainerLayer's MaskLayer. The second challenge is the question of how to teach FrameLayerBuilder to combine rounded rects from a DisplayItemClip and the clip-path / mask. This is necessary if you have an element with clip-path / mask that's also inside an element with overflow:hidden and border-radius. I'm not quite sure what the best way to do that is, but it might become obvious as you start writing the code. It's also not quite clear to me whether the mask layer for the mask/clip-path should be created in nsDisplayMask::BuildLayer, or whether FrameLayerBuilder should do it in ProcessDisplayItems. The latter is probably a better idea, because that's where we know that we'll need to combine the mask with the DisplayItemClip.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #5) > The patches I'm working on for bug 1214146 will make it easier to get APZ > scrolling of compositor-side masks correct, especially if you have something > like a position:fixed element inside a scrolled element with a clip-path. > But there's lots of stuff that can be worked on for this bug before you get > to the point of worrying about APZ correctness. > > CJ, have you already started working on this? I recommend starting by I am prototyping now. Mostly just hacking FrameLayerBuilder, such as painting mask onto the mask layer of a container layer, how to hash msImageLayers::Layer, these sort of things. > separating nsDisplaySVGEffects into nsDisplayMask and nsDisplayFilter. nsDisplayMask is already in my plan already. And yes, if we put clip-path into nsDisplayMask, it makes sense to separate nsDisplaySVGEffects into these two new classes, will do. I will create another bug for this re-factory. > nsDisplayMask would be responsible for the combined rendering of both > clip-path and mask, regardless of whether they're CSS or SVG clip-paths / > masks. Then you can use LAYER_INACTIVE / LAYER_ACTIVE for nsDisplayMask, and > LAYER_SVG_EFFECTS for nsDisplayFilter. If an element has both a clip-path / > mask and a filter, the separation might conceivably hurt performance a bit, > but I don't think that's worth worrying about much. > > There are two challenges that I anticipate in FrameLayerBuilder. The first > one is layer transforms. If you create a ContainerLayer for a mask, and then > have a PaintedLayer inside that layer, then the transform of the > PaintedLayer will be computed relative to the reference frame. Masks don't > cause a frame to become a reference frame, so the transform of the > PaintedLayer will be computed across the mask to the nearest ancestor that > has a CSS transform, or to the root frame. That means that there must not be > a transform on the mask ContainerLayer. Instead, to get the mask shape to be > in the right place, I think you'll need to set the right transform on the > ContainerLayer's MaskLayer. Thanks for sharing. > The second challenge is the question of how to teach FrameLayerBuilder to > combine rounded rects from a DisplayItemClip and the clip-path / mask. This > is necessary if you have an element with clip-path / mask that's also inside > an element with overflow:hidden and border-radius. I'm not quite sure what > the best way to do that is, but it might become obvious as you start writing > the code. It's also not quite clear to me whether the mask layer for the > mask/clip-path should be created in nsDisplayMask::BuildLayer, or whether > FrameLayerBuilder should do it in ProcessDisplayItems. The latter is > probably a better idea, because that's where we know that we'll need to > combine the mask with the DisplayItemClip. It's a mystery for me too, will(and have to) figure it out soon. The reason I want to use mask layer is improve animation performance for both background and mask animation. Has border-radius on the mask layer is not a problem for background-image animation, but it does hurt the performance of mask animation, since we have to re-generate mask per frame during animation. So, paint mask/clip-path on an additional mask layer might be a choice, but we may need to write a new shader to handle multiple mask textures. In the first version, I probably will still using single mask layer, and do multiple-mask textures in the next. (Another further optimization to generate clip-path for basic-shape clip-path in shader)
Depends on: 1293535
(In reply to C.J. Ku[:cjku](UTC+8) from comment #6) > So, paint mask/clip-path on an additional mask layer might > be a choice, but we may need to write a new shader to handle multiple mask > textures. Our compositors already support multiple mask layers; a layer can have multiple ancestor mask layers (referenced by the scroll metadata and by the layer's ScrolledClip), and our compositors just do multiple passes with intermediate surfaces to combine them. So yeah, we could do something like that and it would be fairly easy to get working. > In the first version, I probably will still using single mask > layer, and do multiple-mask textures in the next. Sounds good. > (Another further optimization to generate clip-path for basic-shape > clip-path in shader) Well... maybe. The first optimization that I care about, once we ship basic shape clip-paths, is to not create mask layers for inset() clip-paths and just use the layer's clip rect for them.
Entering planning status and to be broken down into finer milestones and updated here.
Target Milestone: mozilla51 → ---
Blocks: mask-perf
Whiteboard: tlt-queue
Whiteboard: tlt-queue → tlt-wip
Copy and paste mstange's idea from IRC. This idea may make the work here simpler: mstange> 04:19 CJKu: Here's a crazy idea for nsDisplayMask: What if it contained two nsDisplayLists, not just one? One for the contents, and one for the mask images 04:19 CJKu: Then the mask image layers could go into separate display items inside that mask image display list <mstange> and then we'd call the nsCSSRendering code to render one item at a time, the way background images do 04:20 and nsDisplayMask would have some magic to apply mask blend modes and do the actual masking 04:21 (I'm just brain storming here) 04:22 so for image mask layers we'd have an nsDisplayBackgroundImage display item, and for SVG mask layers we'd have a new item type, e.g. nsDisplaySVGMaskSource 04:23 and we could even have nsDisplayBlendMode around the individual mask layers, if they have mask blend modes 04:23 the tricky part would be to convert things from RGBA to A8 at the right time, I think
Priority: -- → P2
Bug 1295094 1299715 1309804 1309646 are preceding work of this bug. Since all of them are fixed, it's right time to go back here.
Attached file test case (obsolete) —
Attachment #8802250 - Attachment is obsolete: true
Attached file mask-anim.html —
By prototype in Part 4, I know 1. We can set a mask layer to a container layer and mask child layer of that container layer correctly There are still two problem that I do not figure out yet: 1. How/Where we animate an image layer in nsDisplayBackgroundImage? 2. Can/How we animation an mask layer of a container layer?
TBD: 1. mask layer cache mechanism in nsDisplayMask. 2. Profiling data.
I think you should focus on a different part of the problem first. The first step is to get this working: <div with static mask-image> <div with OMTA CSS transform animation></div> </div> Animating the mask itself (and making that use OMTA) sounds much more complicated.
Blocks: 1313001
Comment on attachment 8802227 [details] Bug 1234485 - Part 1. Add ActiveLayerTracker::IsMaskPositionAnimated. https://reviewboard.mozilla.org/r/86694/#review87560 ::: layout/base/ActiveLayerTracker.cpp:552 (Diff revision 1) > + return IsStyleAnimated(aBuilder, aFrame, eCSSProperty_mask_position_x) || > + IsStyleAnimated(aBuilder, aFrame, eCSSProperty_mask_position_y); > +} Note that IsStyleAnimated() returns true for the properties (i.e. mask-position-(x|y)) even if the property is overridden by important rules. I am not sure this is desirable or not.
Attachment #8802227 - Attachment is obsolete: true
Attachment #8802228 - Attachment is obsolete: true
Attachment #8802229 - Attachment is obsolete: true
Attachment #8802230 - Attachment is obsolete: true
Attached file background-position-anim.html —
Verify attachment 8804997 [details] After painting onto mask layer, frame rate goes up to 25FPS from 11FPS.
Summary: (mask-image) Compose mask at compositor. → Draw image mask onto mask layer
Blocks: 1313877
Blocks: 1313898
Do we have reftests for image masks on transformed elements? We need to make sure that the transform of the mask ContainerLayer is also applied on that layer's mask layer. (A mask layer's transform is relative to the *parent* of the layer that holds the mask layer.)
Attachment #8804964 - Attachment is obsolete: true
(In reply to Markus Stange [:mstange] from comment #37) > Do we have reftests for image masks on transformed elements? We need to make > sure that the transform of the mask ContainerLayer is also applied on that > layer's mask layer. (A mask layer's transform is relative to the *parent* of > the layer that holds the mask layer.) Hmm, I can find two: mask-image-3e.html mask-image-3g.html
Attachment #8804959 - Flags: review?(mstange)
Attachment #8805849 - Flags: review?(mstange)
Attachment #8804960 - Flags: review?(mstange)
Attachment #8804961 - Flags: review?(mstange)
Attachment #8804962 - Flags: review?(mstange)
Attachment #8806316 - Flags: review?(mstange)
Attachment #8804963 - Flags: review?(mstange)
Attachment #8802231 - Flags: review?(mstange)
Attachment #8805850 - Flags: review?(mstange)
Attachment #8806317 - Flags: review?(mstange)
Comment on attachment 8804959 [details] Bug 1234485 - Part 1. Extract DetermineMaskUsage from PaintMaskAndClipPath. https://reviewboard.mozilla.org/r/88778/#review89332 ::: layout/svg/nsSVGIntegrationUtils.h:171 (Diff revision 4) > + bool shouldGenerateMaskLayer; > + bool shouldGenerateClipMaskLayer; > + bool shouldApplyClipPath; > + bool shouldApplyBasicShape; > + float opacity; > + explicit MaskUsage(nsIFrame* aFrame, bool aHandleOpacity) The "explicit" here probably doesn't do anything, because your constructor has more than one argument. I think you can remove it. ::: layout/svg/nsSVGIntegrationUtils.cpp:663 (Diff revision 4) > aFrame->PresContext()->AppUnitsPerDevPixel(), > *context.GetDrawTarget())); > } > } > > -DrawResult > +typedef nsSVGIntegrationUtils::MaskUsage MaskUsage; Is this needed? The code below is all inside methods of nsSVGIntegrationUtils, so using just "MaskUsage" should already work there. ::: layout/svg/nsSVGIntegrationUtils.cpp:665 (Diff revision 4) > - /* SVG defines the following rendering model: > - * > +void > +nsSVGIntegrationUtils::DetermineMaskUsage(MaskUsage& aUsage) Hmm, I don't really like in/out parameters. Could you have two arguments aFrame and aHandleOpacity, and make MaskUsage the return value of this function? (And then not have frame + handleOpacity properties in the MaskUsage struct.) Or would that cause too much hassle for other parts of the code you're adding?
Attachment #8804959 - Flags: review?(mstange) → review+
Comment on attachment 8805849 [details] Bug 1234485 - Part 2. Implement nsSVGIntegrationUtils::IsMaskResourceReady. https://reviewboard.mozilla.org/r/89468/#review89338 ::: layout/svg/nsSVGIntegrationUtils.cpp:737 (Diff revision 3) > +{ > + nsIFrame* firstFrame = > + nsLayoutUtils::FirstContinuationOrIBSplitSibling(aFrame); > + nsSVGEffects::EffectProperties effectProperties = > + nsSVGEffects::GetEffectProperties(firstFrame); > + nsTArray<nsSVGMaskFrame *> maskFrames = effectProperties.GetMaskFrames(); please remove the space between nsSVGMaskFrame and the asterisk ::: layout/svg/nsSVGIntegrationUtils.cpp:738 (Diff revision 3) > + nsIFrame* firstFrame = > + nsLayoutUtils::FirstContinuationOrIBSplitSibling(aFrame); > + nsSVGEffects::EffectProperties effectProperties = > + nsSVGEffects::GetEffectProperties(firstFrame); > + nsTArray<nsSVGMaskFrame *> maskFrames = effectProperties.GetMaskFrames(); > + const nsStyleSVGReset *svgReset = firstFrame->StyleSVGReset(); asterisk goes on the left
Attachment #8805849 - Flags: review?(mstange) → review+
Comment on attachment 8804960 [details] Bug 1234485 - Part 3. Implement nsSVGIntegrationUtils::PaintMask. https://reviewboard.mozilla.org/r/88780/#review89340 ::: layout/svg/nsSVGIntegrationUtils.h:165 (Diff revision 4) > static DrawResult > PaintMaskAndClipPath(const PaintFramesParams& aParams); > > + /** > + * Paint mask of non-SVG frame onto a given context, aParams.ctx. > + * aParams.ctx must contains a A8 surface. "aParams.ctx must contain an A8 surface." ::: layout/svg/nsSVGIntegrationUtils.cpp:803 (Diff revision 4) > + > + context.Multiply(ThebesMatrix(maskTransform)); > + > + DrawTarget* target = context.GetDrawTarget(); > + MOZ_ASSERT(target->GetFormat() == SurfaceFormat::A8); > + Rect drawingRect= IntRectToRect(IntRect(IntPoint(0, 0), target->GetSize())); Does this work? Rect drawingRect(Point(0, 0), Size(target->GetSize()); ::: layout/svg/nsSVGIntegrationUtils.cpp:804 (Diff revision 4) > + context.Multiply(ThebesMatrix(maskTransform)); > + > + DrawTarget* target = context.GetDrawTarget(); > + MOZ_ASSERT(target->GetFormat() == SurfaceFormat::A8); > + Rect drawingRect= IntRectToRect(IntRect(IntPoint(0, 0), target->GetSize())); > + target->DrawSurface(maskSurface, drawingRect, drawingRect); It would be nice to be able to avoid this copy, by making GenerateMaskSurface paint into the supplied target. But that can be done later. Or maybe your other patches already do it, I haven't read them yet.
Attachment #8804960 - Flags: review?(mstange) → review+
Comment on attachment 8804961 [details] Bug 1234485 - Part 4. Implement nsDisplayMask::ShouldPaintOnMaskLayer. https://reviewboard.mozilla.org/r/88782/#review89358 ::: layout/base/nsDisplayList.cpp:7063 (Diff revision 4) > - return LAYER_SVG_EFFECTS; > + return ShouldPaintOnMaskLayer(aManager) ? LAYER_ACTIVE : LAYER_SVG_EFFECTS; > +} > + > +bool nsDisplayMask::ShouldPaintOnMaskLayer(LayerManager* aManager) > +{ > + if (!aManager->IsCompositingCheap()) { Once we have eliminated the extra copies, we can probably remove this restriction. It doesn't make much of a difference where we do the mask operation. I think the biggest difference is the life time of the mask surface (and the memory used by it).
Attachment #8804961 - Flags: review?(mstange) → review+
Comment on attachment 8804962 [details] Bug 1234485 - Part 5. Implement MaskImageData::CreateImageAndImageContainer. https://reviewboard.mozilla.org/r/88784/#review89364
Attachment #8804962 - Flags: review?(mstange) → review+
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review89366
Attachment #8806316 - Flags: review?(mstange) → review+
Comment on attachment 8806317 [details] Bug 1234485 - Part 12. Lock MaskImageData::mTextureClient by OpenMode::OPEN_WRITE type. https://reviewboard.mozilla.org/r/89798/#review89380 Doesn't the DrawTarget expect to start out with a zeroed-out buffer? I don't know what difference passing OPEN_WRITE instead of OPEN_READ_WRITE makes.
Comment on attachment 8804963 [details] Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask. https://reviewboard.mozilla.org/r/88786/#review89376 ::: layout/base/FrameLayerBuilder.cpp:5560 (Diff revision 4) > + new CSSMaskLayerUserData(frame->StyleSVGReset()->mMask, > + aItem->GetBounds(aBuilder, &snap)); strange 3 space indent ::: layout/base/FrameLayerBuilder.cpp:6170 (Diff revision 4) > + MOZ_ASSERT(!aLayer->GetUserData(&gCSSMaskLayerUserData), > + "A layer contains round clips should not have css-mask on it."); So, how does this work then? What happens if you have a rounded clip applied to an element with a mask image? E.g. <div style="width:100px; height:100px; border-radius:100%; overflow:hidden"> <div style="mask-image: ..."> <div style="something with an animated transform"></div> </div> </div>
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review89378 ::: layout/base/nsDisplayList.cpp:7049 (Diff revision 5) > + maskLayer = > + aManager->GetLayerBuilder()->GetMaskLayerFor(aBuilder, aManager, this); Why not do this after you call BuildContainerLayerFor? Then you could just supply "container" to GetMaskLayerFor and it wouldn't need to look up the layer itself. ::: layout/base/nsDisplayList.cpp:7073 (Diff revision 5) > + > return container.forget(); > } > > +bool > +nsDisplayMask::PaintMaskLayer(nsDisplayListBuilder* aBuilder, I'm not convinced that dealing with TextureClients inside a display item's BuildLayer method is a good idea. Could we make FrameLayerBuilder responsible for setting the mask layer on the layer, and have it call containerItemAsDisplayMask->PaintMask on the mask layer's DrawTarget in the place where it's creating a mask layer for a container layer that was generated by an nsDisplayMask item?
Comment on attachment 8805850 [details] Bug 1234485 - Part 11. Paint SVG mask on PaintedLayer before bug 1313877 fixed. https://reviewboard.mozilla.org/r/89470/#review89384
Attachment #8805850 - Flags: review?(mstange) → review+
Attachment #8804963 - Flags: review?(matt.woodrow)
Attachment #8802231 - Flags: review?(matt.woodrow)
Attachment #8806317 - Flags: review?(matt.woodrow)
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review89412 ::: layout/base/nsDisplayList.cpp:7073 (Diff revision 5) > + > return container.forget(); > } > > +bool > +nsDisplayMask::PaintMaskLayer(nsDisplayListBuilder* aBuilder, I agree with Markus here. layers::Images (and SourceSurfaces) are meant to be immutable, so creating a blank image and then having the caller overwrite it via the TextureClient feels backwards. Doing the same as the existing MaskLayer creation code (with a callback to paint a custom mask, instead of the rounded rect code) seems much nicer.
Comment on attachment 8804963 [details] Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask. https://reviewboard.mozilla.org/r/88786/#review89376 > So, how does this work then? What happens if you have a rounded clip applied to an element with a mask image? > > E.g. > <div style="width:100px; height:100px; border-radius:100%; overflow:hidden"> > <div style="mask-image: ..."> > <div style="something with an animated transform"></div> > </div> > </div> The layer that associate with rounded clip mask is a painted layer, and the layer that associate with css-position-mask is a container layer created by nsDisplayMask::BuildLayer. These two kinds of mask will not be put on the same layer, and if this rule is broken in the future, we will hit this assertion, which means css-masking-mask is going to be replace by round-clip mask.(And, if that does happend, we need change Layer::SetMask to Layer::AddMask/RemoveMask)
Oh, right! Because at the display item level, the rounded corner clip is propagated into the mask's contents, and the mask item itself is not clipped. So it's an unexpected benefit from bug 1299715! Good.
Comment on attachment 8804960 [details] Bug 1234485 - Part 3. Implement nsSVGIntegrationUtils::PaintMask. https://reviewboard.mozilla.org/r/88780/#review89340 > It would be nice to be able to avoid this copy, by making GenerateMaskSurface paint into the supplied target. > But that can be done later. Or maybe your other patches already do it, I haven't read them yet. It takes some work. Will do it in a new created patch(Part 11)
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review89378 > Why not do this after you call BuildContainerLayerFor? Then you could just supply "container" to GetMaskLayerFor and it wouldn't need to look up the layer itself. This's possible, but I need to remove this line: https://hg.mozilla.org/mozilla-central/file/11dff62f0ef5/layout/base/FrameLayerBuilder.cpp#l5238 mask for round-clip will not be clear by this line, since it aways associate to a PaintedLayer. But css-position-mask mask layer will be clear since it binds to a container layer. And, that's the reason why I keep maskLayer reference before BuildContainerLayerFor.
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review89412 > I agree with Markus here. layers::Images (and SourceSurfaces) are meant to be immutable, so creating a blank image and then having the caller overwrite it via the TextureClient feels backwards. > > Doing the same as the existing MaskLayer creation code (with a callback to paint a custom mask, instead of the rounded rect code) seems much nicer. Now I know layers::Images (and SourceSurfaces) are meant to be immutable... Will rework this patch
Attachment #8806654 - Flags: review?(mstange)
Comment on attachment 8806317 [details] Bug 1234485 - Part 12. Lock MaskImageData::mTextureClient by OpenMode::OPEN_WRITE type. https://reviewboard.mozilla.org/r/89798/#review89380 Yes, it is. Lock texture client by OPEN_WRITE reflect the truth that we will only to write data into it and will not read pixel from it. *Supposely*, lock with OPEN_WRITE type will return a memmap with better writing throughput. This patch is pretty minor, we may discard it if you don't like.
Blocks: 1250490
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review90416 ::: layout/base/FrameLayerBuilder.cpp:1550 (Diff revision 4) > + } > + > + bool > + operator== (const CSSMaskLayerUserData& aOther) const > + { > + return mHash == aOther.mHash; Wait, can't this cause collisions? I think you need to store all the arguments in fields, not just the hash. You can then still use mHash != aOther.mHash as a quick way to return false from operator==.
Attachment #8806316 - Flags: review+ → review-
Comment on attachment 8804963 [details] Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask. https://reviewboard.mozilla.org/r/88786/#review90234 ::: layout/base/FrameLayerBuilder.cpp:5487 (Diff revision 8) > ResetLayerStateForRecycling(layer); > return layer; > } > > +already_AddRefed<ImageLayer> > +FrameLayerBuilder::GetMaskLayerFor(nsDisplayListBuilder* aBuilder, Can you call this GetExistingMaskLayerForCSSMask? ::: layout/base/FrameLayerBuilder.cpp:5490 (Diff revision 8) > > +already_AddRefed<ImageLayer> > +FrameLayerBuilder::GetMaskLayerFor(nsDisplayListBuilder* aBuilder, > + LayerManager* aManager, > + Layer* aContainer, > + nsDisplayItem* aItem) The type of this argument should be nsDisplayMask*. ::: layout/base/FrameLayerBuilder.cpp:5517 (Diff revision 8) > + ResetLayerStateForRecycling(aContainer); > + return maskLayer.forget(); > + } > + } > + > + ResetLayerStateForRecycling(aContainer); Calling this should not be necessary - BuildContainerLayerFor already does it. Doing this also feels like a very unexpected side effect of a function that's called "GetMaskLayerFor". ::: layout/base/FrameLayerBuilder.cpp:5522 (Diff revision 8) > + ResetLayerStateForRecycling(aContainer); > + return nullptr; > +} > + > +already_AddRefed<ImageLayer> > +FrameLayerBuilder::CreateMaskLayer(nsDisplayListBuilder* aBuilder, Can you call this CreateMaskLayerForCSSMask? Otherwise we have two different CreateMaskLayer methods in this file (on two different classes). ::: layout/base/FrameLayerBuilder.cpp:5524 (Diff revision 8) > +} > + > +already_AddRefed<ImageLayer> > +FrameLayerBuilder::CreateMaskLayer(nsDisplayListBuilder* aBuilder, > + LayerManager* aManager, > + nsDisplayItem* aItem) This argument should be of type nsDisplayMask*. ::: layout/base/FrameLayerBuilder.cpp:6171 (Diff revision 8) > ContainerState::CreateMaskLayer(Layer *aLayer, > const DisplayItemClip& aClip, > const Maybe<size_t>& aForAncestorMaskLayer, > uint32_t aRoundedRectClipCount) > { > + MOZ_ASSERT(!aLayer->GetUserData(&gCSSMaskLayerUserData), Can you add a comment here? Something like // aLayer will never be the container layer created by an nsDisplayMask // because nsDisplayMask propagates the DisplayItemClip to its contents // and is not clipped itself. // This assertion will fail if that ever stops being the case.
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review90426 ::: layout/base/nsDisplayList.cpp:7052 (Diff revision 10) > + if (ShouldPaintOnMaskLayer(aManager)) { > + RefPtr<ImageLayer> maskLayer = > + aManager->GetLayerBuilder()->GetMaskLayerFor(aBuilder, container, this); > + > + if (!maskLayer) { > + maskLayer = > + aManager->GetLayerBuilder()->CreateMaskLayer(aBuilder, aManager, this); > + } > + > + container->SetMaskLayer(maskLayer); > + } Can you move this whole section into ContainerState::ProcessDisplayItems? I think the best place to do this would be after the block with the "SetupMaskLayer(ownLayer, layerClip);" line. At the end of the "if (layerClip.HasClip()) {", add an "else if (item->GetType() == nsDisplayItem::TYPE_MASK) {" and then call GetMaskLayerForCSSMask / CreateMaskLayerForCSSMask / SetMaskLayer for ownLayer. Then we will have all the ways of setting a mask layer on a container layer in the same place. ::: layout/base/nsDisplayList.cpp:7073 (Diff revision 10) > + MOZ_ASSERT(aManager->IsCompositingCheap(), > + "PaintMaskLayer should not be used with BasicLayerManager."); Can we remove this assert? ::: layout/base/nsDisplayList.cpp:7090 (Diff revision 10) > + > + nsRect borderArea = nsRect(ToReferenceFrame(), mFrame->GetSize()); > + nsSVGIntegrationUtils::PaintFramesParams params(*maskCtx, > + mFrame, mVisibleRect, > + borderArea, aBuilder, > + aManager, Can we pass nullptr here? nsSVGIntegrationUtils::PaintMask shouldn't need the layerManager field. ::: layout/base/nsDisplayList.cpp:7098 (Diff revision 10) > + ComputeMaskGeometry(params); > + > + image::DrawResult result = nsSVGIntegrationUtils::PaintMask(params); > + > + // Setup mask layer offset > + Matrix4x4 matrix; Can you return this matrix as an outparam? Then you wouldn't need to pass the mask layer into this function.
Comment on attachment 8806317 [details] Bug 1234485 - Part 12. Lock MaskImageData::mTextureClient by OpenMode::OPEN_WRITE type. https://reviewboard.mozilla.org/r/89798/#review89380 Hmm, I'm not sure that's true. We're not using OP_SOURCE when we're painting into the DrawTarget, are we? If not, e.g. if we're using OP_OVER (the default), then each pixel is written by doing a calculation like pixelValue = op_over(pixelValue, sourceValue), where sourceValue is the value of the pixel of the thing you're drawing on top, and pixelValue is the destination. So I'd prefer not to land this patch.
Comment on attachment 8804961 [details] Bug 1234485 - Part 4. Implement nsDisplayMask::ShouldPaintOnMaskLayer. https://reviewboard.mozilla.org/r/88782/#review90446 ::: layout/base/nsDisplayList.cpp:7060 (Diff revision 7) > LayerState > nsDisplayMask::GetLayerState(nsDisplayListBuilder* aBuilder, > LayerManager* aManager, > const ContainerLayerParameters& aParameters) > { > - return LAYER_SVG_EFFECTS; > + return ShouldPaintOnMaskLayer(aManager) ? LAYER_ACTIVE : LAYER_SVG_EFFECTS; We don't want to turn all CSS masks into active layers - only those that require layers on the compositor. The others should be LAYER_INACTIVE (and use mask layers with BasicLayerManager). So instead of always returning LAYER_ACTIVE when ShouldPaintOnMaskLayer returns true, please return RequiredLayerStateForChildren(...).
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review90448 ::: layout/base/nsDisplayList.cpp:7098 (Diff revision 10) > + ComputeMaskGeometry(params); > + > + image::DrawResult result = nsSVGIntegrationUtils::PaintMask(params); > + > + // Setup mask layer offset > + Matrix4x4 matrix; So then FrameLayerBuilder would set the base transform on the mask layer. And I think it will also need to do matrix.PreTranslate(mParameters.mOffset.x, mParameters.mOffset.y, 0); like it does for rounded corner mask layers. This should fix the offset for inactive layers / BasicLayerManager.
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review90416 > Wait, can't this cause collisions? I think you need to store all the arguments in fields, not just the hash. You can then still use mHash != aOther.mHash as a quick way to return false from operator==. Hmm, I think I should also keep these boxing rectangles: nsIFrame::GetContentRectRelativeToSelf nsIFrame::GetPaddingRectRelativeToSelf nsIFrame::GetMarginRectRelativeToSelf A user may increase padding 1 pixel and decrease border 1 pixel, then we get the same visible rect, but mask content is out of date already.
Comment on attachment 8806654 [details] Bug 1234485 - Part 12. Extract PaintMaskSurface from GenerateMaskSurface. https://reviewboard.mozilla.org/r/90030/#review90460 ::: layout/svg/nsSVGIntegrationUtils.cpp:816 (Diff revision 5) > - > - DrawTarget* target = ctx.GetDrawTarget(); > - MOZ_ASSERT(target->GetFormat() == SurfaceFormat::A8); > + opacityApplied ? maskUsage.opacity : 1.0, > + firstFrame->StyleContext(), maskFrames, > + ctx.CurrentMatrix(), offsetToUserSpace); looks like the indent is wrong here
Attachment #8806654 - Flags: review?(mstange) → review+
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review90604 ::: layout/base/FrameLayerBuilder.cpp:4244 (Diff revision 11) > + RefPtr<ImageLayer> maskLayer = > + mLayerBuilder->GetExistingMaskLayerForCSSMask(mBuilder, mManager, > + ownLayer, maskItem); > + if (!maskLayer) { > + maskLayer = > + mLayerBuilder->CreateMaskLayerForCSSMask(mBuilder, mManager, > + maskItem, params); > + } Hmm, does it still make sense for these two functions to live on FrameLayerBuilder, and not on ContainerState? You could also merge them into one function, called ContainerState::SetupMaskLayerForCSSMask(Layer* aLayer, nsDisplayMask* aMaskItem). ::: layout/base/FrameLayerBuilder.cpp:4250 (Diff revision 11) > + mLayerBuilder->GetExistingMaskLayerForCSSMask(mBuilder, mManager, > + ownLayer, maskItem); > + if (!maskLayer) { > + maskLayer = > + mLayerBuilder->CreateMaskLayerForCSSMask(mBuilder, mManager, > + maskItem, params); This is actually the wrong ContainerLayerParameters. You need to pass mParameters, i.e. the parameters of the parent layer of ownLayer. ::: layout/base/FrameLayerBuilder.cpp:5342 (Diff revision 11) > + Layer* maskLayer = containerLayer->GetMaskLayer(); > + if (!maskLayer || !maskLayer->HasUserData(&gCSSMaskLayerUserData)) { Why is this necessary? We call ownLayer->SetMaskLayer(maskLayer) after we call BuildContainerLayerFor. So we'll overwrite the cleared mask layer with the actual mask layer we want. So clearing the mask layer first should be the right thing to do. ::: layout/base/nsDisplayList.cpp:7057 (Diff revision 11) > return container.forget(); > } > > +bool > +nsDisplayMask::PaintMaskLayer(nsDisplayListBuilder* aBuilder, > + LayerManager* aManager, This function no longer needs aManager. ::: layout/base/nsDisplayList.cpp:7081 (Diff revision 11) > + Matrix4x4 matrix; > + matrix.PreTranslate(offset.x, offset.y, 0); > + matrix.PreTranslate(aContainerParameters.mOffset.x, > + aContainerParameters.mOffset.y, 0); > + aMaskLayer->SetBaseTransform(matrix); I still think it would be better to apply the transform in CreateMaskLayerForCSSMask, not in PaintMaskLayer.
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review90604 > Why is this necessary? We call ownLayer->SetMaskLayer(maskLayer) after we call BuildContainerLayerFor. So we'll overwrite the cleared mask layer with the actual mask layer we want. So clearing the mask layer first should be the right thing to do. Oh, I see it now. When you recycle the mask layer, you get the old mask layer from containerLayer->GetMaskLayer(). Hmm. Could you do something like mRecycledMaskImageLayers, but for CSS mask mask layers?
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review90604 > Oh, I see it now. When you recycle the mask layer, you get the old mask layer from containerLayer->GetMaskLayer(). Hmm. > > Could you do something like mRecycledMaskImageLayers, but for CSS mask mask layers? Sure, done. Will patches update later
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review90890 ::: layout/base/nsDisplayList.cpp:7056 (Diff revision 13) > > return container.forget(); > } > > +bool > +nsDisplayMask::PaintMaskLayer(nsDisplayListBuilder* aBuilder, Should we call this just PaintMask? This function doesn't really need to know that the result goes into a "mask layer". ::: layout/base/nsDisplayList.cpp:7057 (Diff revision 13) > + ImageLayer* aMaskLayer, > + DrawTarget* aMaskDT, > + const ContainerLayerParameters& aContainerParameters) This function no longer needs aMaskLayer or aContainerParameters. ::: layout/base/nsDisplayList.cpp:7063 (Diff revision 13) > + bool snap; > + nsRect bounds = GetBounds(aBuilder, &snap); > + gfxPoint offset = > + nsLayoutUtils::PointToGfxPoint(bounds.TopLeft(), > + mFrame->PresContext()->AppUnitsPerDevPixel()); > + maskCtx->SetMatrix(gfxMatrix::Translation(-offset)); Actually, now that I see this again, how about we move this code to the caller as well? Then you'll only need to compute the offset once, and nsDisplayMask::PaintMaskLayer can assume that aMaskDT has its 0,0 at the reference frame position.
Comment on attachment 8804963 [details] Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask. https://reviewboard.mozilla.org/r/88786/#review90946 ::: layout/base/FrameLayerBuilder.cpp:1263 (Diff revision 11) > * index in the layer, if any. > */ > struct MaskLayerKey; > - already_AddRefed<ImageLayer> CreateOrRecycleMaskImageLayerFor(const MaskLayerKey& aKey); > + template <typename T> > + already_AddRefed<ImageLayer> > + CreateOrRecycleMaskImageLayerFor(const MaskLayerKey& aKey, T SetUserData); Rather than using templates, can you instead make the type: mozilla::function<void(Layer* aLayer)> 'a' prefix for SetUserData too.
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review90948 ::: layout/base/FrameLayerBuilder.cpp:3942 (Diff revision 13) > + return; > + } > + > + // Setup mask layer offset > + bool snap; > + nsRect bounds = aMaskItem->GetBounds(mBuilder, &snap); 'bounds' is shadowed from the outer scope isn't it? And should contain the same value. ::: layout/base/nsDisplayList.h:3918 (Diff revision 13) > #endif > > void PaintAsLayer(nsDisplayListBuilder* aBuilder, > nsRenderingContext* aCtx, > LayerManager* aManager); > + bool PaintMaskLayer(nsDisplayListBuilder* aBuilder, ImageLayer* aLayer, Please document this, especially the coordinate space that aMaskDT uses (I like Markus' suggestion below for what this is).
Attachment #8802231 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8806317 [details] Bug 1234485 - Part 12. Lock MaskImageData::mTextureClient by OpenMode::OPEN_WRITE type. https://reviewboard.mozilla.org/r/89798/#review90952 Unless we're using OPERATOR_SOURCE (and our API doesn't really let us enforce that the DT actually does this), then we will be reading from the locked pixels.
Attachment #8806317 - Flags: review?(matt.woodrow) → review-
Comment on attachment 8804963 [details] Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask. https://reviewboard.mozilla.org/r/88786/#review90956
Attachment #8804963 - Flags: review?(matt.woodrow) → review+
Attachment #8806317 - Attachment is obsolete: true
Attachment #8806317 - Flags: review?(mstange)
Attached file mask transform —
Translate/rotate/skew look good. Scale is meltfunction
(In reply to C.J. Ku[:cjku](UTC+8) from comment #187) > Created attachment 8808627 [details] > mask transform > > Translate/rotate/skew look good. Scale is meltfunction Fix by scaling base mask's base matrix
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review91296 ::: layout/base/FrameLayerBuilder.cpp:1550 (Diff revision 7) > + CSSMaskLayerUserData(const nsIFrame* aFrame, const nsRect& aBound) > + : mImageLayers(aFrame->StyleSVGReset()->mMask), > + mContentRect(aFrame->GetContentRectRelativeToSelf()), > + mPaddingRect(aFrame->GetPaddingRectRelativeToSelf()), > + mBorderRect(aFrame->GetRectRelativeToSelf()), > + mMarginRect(aFrame->GetMarginRectRelativeToSelf()), Do we need the margin rect? ::: layout/base/FrameLayerBuilder.cpp:1580 (Diff revision 7) > + if (!mContentRect.IsEqualEdges(aOther.mContentRect) || > + !mPaddingRect.IsEqualEdges(aOther.mPaddingRect) || > + !mBorderRect.IsEqualEdges(aOther.mBorderRect) || > + !mMarginRect.IsEqualEdges(aOther.mMarginRect)) { Can you add a few tests that fail if one of these checks is removed?
Attachment #8806316 - Flags: review?(mstange) → review+
Comment on attachment 8808252 [details] Bug 1234485 - Part 6. Implement nsStyleImageLayers::operator=. https://reviewboard.mozilla.org/r/91094/#review91298
Attachment #8808252 - Flags: review?(mstange) → review+
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review91296 > Do we need the margin rect? We will need it after landing Bug 1303623 and bug 1311270
Comment on attachment 8804963 [details] Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask. https://reviewboard.mozilla.org/r/88786/#review91302
Attachment #8804963 - Flags: review?(mstange) → review+
Comment on attachment 8802231 [details] Bug 1234485 - Part 10. Paint mask onto mask layer when possible. https://reviewboard.mozilla.org/r/86702/#review91304 This is looking very nice now.
Attachment #8802231 - Flags: review?(mstange) → review+
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review91296 > Can you add a few tests that fail if one of these checks is removed? Sure. To test this code path, we need nsDisplayMask::GetLayerState return LAYER_ACTIVE, except animation, is there anohter easy way to let RequiredLayerStateForChildren return true?
Comment on attachment 8808253 [details] Bug 1234485 - Part 9. Implement MaskLayerUserData::constructor & operator=. https://reviewboard.mozilla.org/r/91096/#review91308 ::: layout/base/FrameLayerBuilder.cpp:1539 (Diff revision 3) > + { > + mScaleX = aOther.mScaleX; > + mScaleY = aOther.mScaleY; > + mOffset = aOther.mOffset; > + mAppUnitsPerDevPixel = aOther.mAppUnitsPerDevPixel; > + mRoundedClipRects.SwapElements(aOther.mRoundedClipRects); You're consuming aOther, so you should make it an r-value reference. See e.g. http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/animation/KeyframeEffectReadOnly.h#101 ::: layout/base/FrameLayerBuilder.cpp:6340 (Diff revision 3) > Matrix4x4 matrix = Matrix4x4::From2D(maskTransform); > matrix.PreTranslate(mParameters.mOffset.x, mParameters.mOffset.y, 0); > maskLayer->SetBaseTransform(matrix); > > // save the details of the clip in user data > - userData->mScaleX = newData.mScaleX; > + *userData = newData; And then make this *userData = Move(newData);
Attachment #8808253 - Flags: review?(mstange) → review+
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review91296 > Sure. > To test this code path, we need nsDisplayMask::GetLayerState return LAYER_ACTIVE, except animation, is there anohter easy way to let RequiredLayerStateForChildren return true? I see. You can probably put an element with will-change:transform inside the mask.
Blocks: 1316270
Comment on attachment 8806316 [details] Bug 1234485 - Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. https://reviewboard.mozilla.org/r/89796/#review91296 > I see. You can probably put an element with will-change:transform inside the mask. bug 1316270 filed, I will fix it soon
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8245f11d4585 Part 1. Extract DetermineMaskUsage from PaintMaskAndClipPath. r=mstange https://hg.mozilla.org/integration/autoland/rev/d8d3d4789ab8 Part 2. Implement nsSVGIntegrationUtils::IsMaskResourceReady. r=mstange https://hg.mozilla.org/integration/autoland/rev/b110f0b3ff81 Part 3. Implement nsSVGIntegrationUtils::PaintMask. r=mstange https://hg.mozilla.org/integration/autoland/rev/42ed9b2fcf5b Part 4. Implement nsDisplayMask::ShouldPaintOnMaskLayer. r=mstange https://hg.mozilla.org/integration/autoland/rev/bf9e4704d569 Part 5. Implement MaskImageData::CreateImageAndImageContainer. r=mstange https://hg.mozilla.org/integration/autoland/rev/6e607ef327c8 Part 6. Implement nsStyleImageLayers::operator=. r=mstange https://hg.mozilla.org/integration/autoland/rev/02b45619e3f0 Part 7. Implement CSSMaskLayerUserData to store css positioned mask user data. r=mstange https://hg.mozilla.org/integration/autoland/rev/43b1c0654aa6 Part 8. Implement ContainerState::SetupMaskLayerForCSSMask. r=mattwoodrow,mstange https://hg.mozilla.org/integration/autoland/rev/18ffd5fcd194 Part 9. Implement MaskLayerUserData::constructor & operator=. r=mstange https://hg.mozilla.org/integration/autoland/rev/ca961d579a39 Part 10. Paint mask onto mask layer when possible. r=mattwoodrow,mstange https://hg.mozilla.org/integration/autoland/rev/15301d7d6162 Part 11. Paint SVG mask on PaintedLayer before bug 1313877 fixed. r=mstange https://hg.mozilla.org/integration/autoland/rev/41d66b1327ac Part 12. Extract PaintMaskSurface from GenerateMaskSurface. r=mstange
Depends on: 1316719
No longer depends on: 1316719
Depends on: 1316719
Depends on: 1322341
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: