Draw image mask onto mask layer

RESOLVED FIXED in Firefox 52

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

(Depends on 2 bugs, Blocks 3 bugs)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: tlt-wip)

Attachments

(15 attachments, 7 obsolete attachments)

58 bytes, text/x-review-board-request
mstange
: review+
mattwoodrow
: review+
Details
700 bytes, text/html
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
mattwoodrow
: review+
Details
8.15 KB, text/html
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
662 bytes, text/html
Details
Assignee

Description

4 years ago
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

Updated

3 years ago
Assignee: nobody → cku
Assignee

Comment 1

3 years ago
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
Assignee

Comment 3

3 years ago
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)
Assignee

Comment 6

3 years ago
(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)
Assignee

Updated

3 years ago
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 → ---
Assignee

Updated

3 years ago
Blocks: mask-perf
Whiteboard: tlt-queue
Whiteboard: tlt-queue → tlt-wip
Assignee

Comment 9

3 years ago
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
Assignee

Comment 10

3 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 16

3 years ago
Posted file test case (obsolete) —
Assignee

Updated

3 years ago
Attachment #8802250 - Attachment is obsolete: true
Assignee

Comment 17

3 years ago
Posted file mask-anim.html
Assignee

Comment 18

3 years ago
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?
Assignee

Comment 19

3 years ago
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.
Assignee

Updated

3 years ago
Blocks: 1313001

Comment 21

3 years ago
mozreview-review
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.
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

3 years ago
Attachment #8802227 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8802228 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8802229 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8802230 - Attachment is obsolete: true
Assignee

Comment 31

3 years ago
Assignee

Comment 32

3 years ago
Verify attachment 8804997 [details]
After painting onto mask layer, frame rate goes up to 25FPS from 11FPS.
Duplicate of this bug: 1313628
Assignee

Updated

3 years ago
Summary: (mask-image) Compose mask at compositor. → Draw image mask onto mask layer
Assignee

Updated

3 years ago
Blocks: 1313877
Assignee

Updated

3 years ago
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.)
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

3 years ago
Attachment #8804964 - Attachment is obsolete: true
Assignee

Comment 48

3 years ago
(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
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)
Assignee

Updated

3 years ago
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 70

3 years ago
mozreview-review
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 71

3 years ago
mozreview-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 72

3 years ago
mozreview-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 73

3 years ago
mozreview-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 74

3 years ago
mozreview-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 75

3 years ago
mozreview-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 76

3 years ago
mozreview-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 77

3 years ago
mozreview-review
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 78

3 years ago
mozreview-review
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 79

3 years ago
mozreview-review
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 80

3 years ago
mozreview-review
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.
Assignee

Comment 81

3 years ago
mozreview-review-reply
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.
Assignee

Comment 83

3 years ago
mozreview-review-reply
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)
Assignee

Comment 84

3 years ago
mozreview-review-reply
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.
Assignee

Comment 85

3 years ago
mozreview-review-reply
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
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)
Assignee

Updated

3 years ago
Attachment #8806654 - Flags: review?(mstange)
Assignee

Comment 97

3 years ago
mozreview-review-reply
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.
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)
Assignee

Updated

3 years ago
Blocks: 1250490
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 129

3 years ago
mozreview-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/#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 130

3 years ago
mozreview-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 131

3 years ago
mozreview-review
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 132

3 years ago
mozreview-review-reply
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 133

3 years ago
mozreview-review
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 134

3 years ago
mozreview-review
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.
Assignee

Comment 135

3 years ago
mozreview-review-reply
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 137

3 years ago
mozreview-review
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 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 151

3 years ago
mozreview-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 154

3 years ago
mozreview-review-reply
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?
Assignee

Comment 155

3 years ago
mozreview-review-reply
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 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 170

3 years ago
mozreview-review
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 171

3 years ago
mozreview-review
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 172

3 years ago
mozreview-review
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 173

3 years ago
mozreview-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 174

3 years ago
mozreview-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+
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)
Assignee

Updated

3 years ago
Attachment #8806317 - Attachment is obsolete: true
Attachment #8806317 - Flags: review?(mstange)
Assignee

Comment 187

3 years ago
Posted file mask transform
Translate/rotate/skew look good. Scale is meltfunction
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 191

3 years ago
(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 192

3 years ago
mozreview-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

::: 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 193

3 years ago
mozreview-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+
Assignee

Comment 194

3 years ago
mozreview-review-reply
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 195

3 years ago
mozreview-review
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 196

3 years ago
mozreview-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+
Assignee

Comment 197

3 years ago
mozreview-review-reply
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 198

3 years ago
mozreview-review
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 199

3 years ago
mozreview-review-reply
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.
Assignee

Updated

3 years ago
Blocks: 1316270
Assignee

Comment 201

3 years ago
mozreview-review-reply
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
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 214

3 years ago
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
Assignee

Updated

3 years ago
No longer depends on: 1316719
Assignee

Updated

3 years ago
Depends on: 1316719

Updated

3 years ago
Depends on: 1322341
You need to log in before you can comment on or make changes to this bug.