u459114 Assignee
	Description • 9 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.

	u459114 Assignee
	Comment 1 • 8 years ago

I will start working on this topic form 2016 Q3

	Astley Chen (inactive)
	Comment 2 • 8 years ago

Work in progress. This is mainly a performance fine tuning for animated masking effect.

	u459114 Assignee
	Comment 3 • 8 years ago

Current plan
1. Paint css mask onto the mask layer created in ContainerState::CreateMaskLayer
2. Create mask layer before background/ mask animation.

	Jet Villegas (inactive)
	Comment 4 • 8 years ago

+cc: mstange who's working on clipping changes related to scrolling which may affect scrolling inside masked parent containers.

	Markus Stange [:mstange]
	Comment 5 • 8 years ago

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.

	u459114 Assignee
	Comment 6 • 8 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)

	Markus Stange [:mstange]
	Comment 7 • 8 years ago

(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.

	Astley Chen (inactive)
	Comment 8 • 8 years ago

Entering planning status and to be broken down into finer milestones and updated here.

	u459114 Assignee
	Comment 9 • 8 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

	u459114 Assignee
	Comment 10 • 8 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.

	u459114 Assignee
	Comment 18 • 8 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?

	u459114 Assignee
	Comment 19 • 8 years ago

TBD:
1. mask layer cache mechanism in nsDisplayMask.
2. Profiling data.

	Markus Stange [:mstange]
	Comment 20 • 8 years ago

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.

	Hiroyuki Ikezoe (:hiro)
	Comment 21 • 8 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.

	u459114 Assignee
	Comment 22 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0fb206d5799

	u459114 Assignee
	Comment 30 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b74c885c6088

	u459114 Assignee
	Comment 32 • 8 years ago

Verify attachment 8804997 [details]
After painting onto mask layer, frame rate goes up to 25FPS from 11FPS.

	u459114 Assignee
	Comment 34 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7e32278e7f

	u459114 Assignee
	Comment 35 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=618784daf8f7

	u459114 Assignee
	Comment 36 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3747b817f07

	Markus Stange [:mstange]
	Comment 37 • 8 years ago

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.)

	u459114 Assignee
	Comment 38 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c22f9053d7f

	u459114 Assignee
	Comment 39 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb0b6812977

	u459114 Assignee
	Comment 48 • 8 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

	u459114 Assignee
	Comment 49 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a012a6ceed

	u459114 Assignee
	Comment 58 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f65f6d156a67

	u459114 Assignee
	Comment 59 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f865b6dbd4a4

	Markus Stange [:mstange]
	Comment 70 • 8 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?

	Markus Stange [:mstange]
	Comment 71 • 8 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

	Markus Stange [:mstange]
	Comment 72 • 8 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.

	Markus Stange [:mstange]
	Comment 73 • 8 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).

	Markus Stange [:mstange]
	Comment 74 • 8 years ago
mozreview-review

Comment on attachment 8804962 [details]
Bug 1234485 - Part 5. Implement MaskImageData::CreateImageAndImageContainer.

https://reviewboard.mozilla.org/r/88784/#review89364

	Markus Stange [:mstange]
	Comment 75 • 8 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

	Markus Stange [:mstange]
	Comment 76 • 8 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.

	Markus Stange [:mstange]
	Comment 77 • 8 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>

	Markus Stange [:mstange]
	Comment 78 • 8 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?

	Markus Stange [:mstange]
	Comment 79 • 8 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

	Matt Woodrow (:mattwoodrow)
	Comment 80 • 8 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.

	u459114 Assignee
	Comment 81 • 8 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)

	Markus Stange [:mstange]
	Comment 82 • 8 years ago

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.

	u459114 Assignee
	Comment 83 • 8 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)

	u459114 Assignee
	Comment 84 • 8 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.

	u459114 Assignee
	Comment 85 • 8 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

	u459114 Assignee
	Comment 97 • 8 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.

	u459114 Assignee
	Comment 115 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58d35653723a

	Markus Stange [:mstange]
	Comment 129 • 8 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==.

	Markus Stange [:mstange]
	Comment 130 • 8 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.

	Markus Stange [:mstange]
	Comment 131 • 8 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.

	Markus Stange [:mstange]
	Comment 132 • 8 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.

	Markus Stange [:mstange]
	Comment 133 • 8 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(...).

	Markus Stange [:mstange]
	Comment 134 • 8 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.

	u459114 Assignee
	Comment 135 • 8 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.

	u459114 Assignee
	Comment 136 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=407195b738c6

	Markus Stange [:mstange]
	Comment 137 • 8 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

	u459114 Assignee
	Comment 138 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53884e9eef43

	u459114 Assignee
	Comment 139 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fae92b9a0cd

	Markus Stange [:mstange]
	Comment 151 • 8 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.

	u459114 Assignee
	Comment 152 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5165fbe640a6

	u459114 Assignee
	Comment 153 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0ca6b4b31b

	Markus Stange [:mstange]
	Comment 154 • 8 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?

	u459114 Assignee
	Comment 155 • 8 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

	Markus Stange [:mstange]
	Comment 170 • 8 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.

	Matt Woodrow (:mattwoodrow)
	Comment 171 • 8 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.

	Matt Woodrow (:mattwoodrow)
	Comment 172 • 8 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).

	Matt Woodrow (:mattwoodrow)
	Comment 173 • 8 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.

	Matt Woodrow (:mattwoodrow)
	Comment 174 • 8 years ago
mozreview-review

Comment on attachment 8804963 [details]
Bug 1234485 - Part 8. Implement ContainerState::SetupMaskLayerForCSSMask.

https://reviewboard.mozilla.org/r/88786/#review90956