1228280 - (mask-image) Support multiple SVG mask elements as mask images

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

Firefox support one svg-mask. For example, this css works perfect:
#test {
  /* external svg doc with mask ID*/
  mask: url("xxx.svg#maskID");
  /* or */
  /* mask: url("#maskID"); */
};

But we don't support multiple svg-mask. For example, you get no mask at all by adding two svg-mask in mask property.
#test {
  mask: url("xxx.svg#maskID"), mask: url("xxx2.svg#maskID");;
};

To support this, the following modification is required.
1. nsSVGEffects need to carry more then one SVG mask.
2. Move nsStyleSVGReset::mMask to nsStyleImageLayers::Layer
3. Support multiple mask blend in nsSVGIntegrationUtils::PaintFramesWithEffects
4. Carry mask ref in nsStyleImage?

Prototype:
https://github.com/CJKu/gecko-dev/commit/f5d8fd16803aa8d67dc37da6fd40c818472d3c51

Comment 13

[u459114]

Hi mstange/heycom,
With patches here, FF will support
1. multiple SVG masks.
2. multiple SVG masks interleave with multiple image mask.

Part 1. and Part 2. are trying to reduce the number of functions parameters, which are not directly relative to this bug.
With Part 3., nsSVGIntegrationUtils::PaintFramesWithEffects, which is already a long function, will not get longer even we have Part 5/6/7. 
Part 4 is to provide functionally to fetch multiple SVG mask frames,
Part 6 and Part 7 are performance optimize base on Part 5

Comment 14

[u459114]

https://reviewboard.mozilla.org/r/51387/#review48849

::: layout/tables/nsTablePainter.cpp:578
(Diff revision 2)
> -      nsCSSRendering::PaintBackgroundWithSC(mPresContext, mRenderingContext,
> +                                                 mDirtyRect,
> -                                            mCols[colIndex].mColGroup.mFrame, mDirtyRect,
> -                                            mCols[colIndex].mColGroup.mRect + mRenderPt,
> +                                                 mCols[colIndex].mColGroup.mRect + mRenderPt,
> +                                                 mCols[colIndex].mColGroup.mFrame,
> +                                                 mBGPaintFlags);
> +

Miss 
params.BGClipRect = &aColBGRect;

which casue many test case failure

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8750361 [details]
MozReview Request: Bug 1228280 - Part 1. Change the parameter of nsCSSRendering::PaintBackground;

https://reviewboard.mozilla.org/r/51387/#review48943

::: layout/base/nsCSSRendering.h:600
(Diff revision 2)
> +    nsRenderingContext& renderingCtx;
> +    const nsRect& dirtyRect;
> +    const nsRect& borderArea;
> +    nsIFrame* frame;
> +    uint32_t paintFlags = 0;
> +    nsRect* BGClipRect = nullptr;

Let's call this "bgClipRect" instead.

::: layout/base/nsCSSRendering.h:635
(Diff revision 2)
>     * being painted.
>     * aCompositionOp is only respected if a single layer is specified (aLayer != -1).
>     * If all layers are painted, the image layer's blend mode (or the mask
>     * layer's composition mode) will be used.
>     */
> -  static DrawResult PaintBackgroundWithSC(nsPresContext* aPresContext,
> +  static DrawResult PaintBackgroundWithSC(PaintBackgroundParams& aParams,

Could / should aParams be a const reference?

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8751291 [details]
MozReview Request: Bug 1228280 - Part 5. Paint multiple SVG and image masks;

https://reviewboard.mozilla.org/r/51935/#review48949

::: layout/svg/nsSVGIntegrationUtils.cpp:503
(Diff revision 1)
> -                                                 rc, aParams.dirtyRect,
> +                                                   rc, aParams.dirtyRect,
> -                                                 aParams.borderArea,
> +                                                   aParams.borderArea,
> -                                                 aParams.frame,
> +                                                   aParams.frame,
> -                                                 aParams.builder->GetBackgroundPaintFlags() |
> +                                                   aParams.builder->GetBackgroundPaintFlags() |
> -                                                 nsCSSRendering::PAINTBG_MASK_IMAGE);
> +                                                   nsCSSRendering::PAINTBG_MASK_IMAGE);
> +      params.layer = i;

Don't you need to set params.compositionOp here? You're now passing a layer index, so PaintBackgroundWithSC will no longer look up the right mask-composite value.

Comment 17

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/51387/#review48971

::: layout/base/nsCSSRendering.h:606
(Diff revision 2)
> +    int32_t layer = -1;             // -1 means painting all layers; other
> +                                    // value means painting one specific
> +                                    // layer only.
> +    CompositionOp compositionOp = CompositionOp::OP_OVER;
> +
> +    PaintBackgroundParams(nsPresContext& aPresCtx,

Instead of a public constructor, how do you feel about adding two static creation functions? We could have PaintBackgroundParams::ForAllLayers() and PaintBackgroundParams::ForSingleLayer(), and the latter would require the composition op to be specified. That would have avoided the bug in your other patch.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8751293 [details]
MozReview Request: Bug 1228280 - Dummy patch. To keep patch order on review board

https://reviewboard.mozilla.org/r/51939/#review49161

How does this patch improve performance?

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 8751294 [details]
MozReview Request: Bug 1228280 - Part 7. reftest of SVG mask and image mask interleaving;

https://reviewboard.mozilla.org/r/51941/#review49167

Comment 28

[Markus Stange [:mstange]]

Comment on attachment 8750363 [details]
MozReview Request: Bug 1228280 - Part 3. Move mask painting out of nsSVGIntegrationUtils::PaintFramesWithEffects;

https://reviewboard.mozilla.org/r/51391/#review49169

::: layout/svg/nsSVGIntegrationUtils.cpp:456
(Diff revision 3)
> +  if (svgMaskFrame) {
> +    gfxMatrix cssPxToDevPxMatrix =
> +      nsSVGIntegrationUtils::GetCSSPxToDevPxMatrix(aParams.frame);
> +
> +    // Generate maskSurface from a SVG mask.
> +    maskSurface = svgMaskFrame->GetMaskForMaskedFrame(&aParams.ctx,

It seems like it would make sense to pull out aParams.ctx into a local "context" variable.

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 8750362 [details]
MozReview Request: Bug 1228280 - Part 2. Change the parameters of nsSVGIntegrationUtils::PaintFramesWithEffects;

https://reviewboard.mozilla.org/r/51389/#review49171

::: layout/svg/nsSVGIntegrationUtils.h:148
(Diff revision 3)
> +
>    /**
>     * Paint non-SVG frame with SVG effects.
>     */
>    static void
> -  PaintFramesWithEffects(gfxContext& aCtx,
> +  PaintFramesWithEffects(PaintFramesParams& aParams);

Same comment as in the other patch: Please try making this a const reference.

Comment 30

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/51391/#review49177

::: layout/svg/nsSVGIntegrationUtils.cpp:440
(Diff revision 3)
> +                    gfxMatrix aOriginMatrix, Matrix& maskTransform,
> +                    RefPtr<SourceSurface>& maskSurface)

Please call these aMaskTransform and aMaskSurface. If you want to mark them as outparameters, you could call them aOutMaskTransform and aOutMaskSurface instead.

Or you could create a struct MaskResult { RefPtr<SourceSurface> surface; Matrix transform; }; and return MaskResult{ maskSurface, maskTransform } from this function.

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8751291 [details]
MozReview Request: Bug 1228280 - Part 5. Paint multiple SVG and image masks;

https://reviewboard.mozilla.org/r/51935/#review49179

::: layout/svg/nsSVGIntegrationUtils.cpp:488
(Diff revision 2)
> +    // maskFrame == nullptr means we get an image mask.
> +    if (maskFrame) {
> +      RefPtr<SourceSurface> svgMask =
> +        maskFrame->GetMaskForMaskedFrame(&aParams.ctx, aParams.frame,
> +                                         cssPxToDevPxMatrix, aOpacity,
> +                                         &maskTransform,

If you have multiple masks, are they overwriting each other's maskTransform? This seems sketchy. Don't you somehow need to respect the maskTransform in the FillRect call below? Please add a reftest that triggers the bug if you find that there is a bug with this.
Oh, you're completely ignoring the value of maskTransform even if there's only one mask, because you're overwriting maskTransform at the end of this function. So something's definitely wrong.

::: layout/svg/nsSVGIntegrationUtils.cpp:492
(Diff revision 2)
> +                                         cssPxToDevPxMatrix, aOpacity,
> +                                         &maskTransform,
> +                                         svgReset->mMask.mLayers[i].mMaskMode);
> +      maskContext->SetMatrix(gfxMatrix());
> +      SurfacePattern pattern(svgMask, ExtendMode::CLAMP);
> +      maskDT->FillRect(IntRectToRect(IntRect(IntPoint(0, 0), drawRect.Size())), pattern);

Once you've figured out the transform thing, you could try using DrawSurface instead of FillRect here. Might be a bit more effecient, but probably doesn't matter much. You can ignore this request if it seems too hard to be worth it.

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8751292 [details]
MozReview Request: Bug 1228280 - Part 6. Single SVG mask optimization;

https://reviewboard.mozilla.org/r/51937/#review49181

::: layout/svg/nsSVGIntegrationUtils.cpp:454
(Diff revision 2)
>  
>    gfxMatrix cssPxToDevPxMatrix =
>      nsSVGIntegrationUtils::GetCSSPxToDevPxMatrix(aParams.frame);
>  
> +  // There is only one mask. And that mask is a SVG mask.
> +  if (1 == svgMaskFrames.Length() && svgMaskFrames[0]) {

Please make this "svgMaskFrames.Length() == 1". I'm not a fan of yoda conditions.

Comment 33

[Cameron McCormack (:heycam)]

Comment on attachment 8750364 [details]
MozReview Request: Bug 1228280 - Part 4. Create nsSVGMaskProperty to carry multiple mask info;

https://reviewboard.mozilla.org/r/51393/#review49273

::: layout/svg/nsSVGEffects.h:325
(Diff revision 3)
>  
>  protected:
>    virtual void DoUpdate() override;
>  };
>  
> +class nsSVGMaskProperty final: public nsISupports {

Nit: space before ":" and "{" on new line.  (I know the rest of the file isn't consistent.)

::: layout/svg/nsSVGEffects.h:327
(Diff revision 3)
>    virtual void DoUpdate() override;
>  };
>  
> +class nsSVGMaskProperty final: public nsISupports {
> +public:
> +  nsSVGMaskProperty(nsIFrame *aFrame);

Nit: "*" next to type.

::: layout/svg/nsSVGEffects.h:329
(Diff revision 3)
> +  // nsISupports
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS(nsSVGMaskProperty)

Why does this need to be cycle collected?

::: layout/svg/nsSVGEffects.h:464
(Diff revision 3)
>       * @return the mask frame, or null if there is no mask frame
>       * @param aOK if a mask was specified and the designated element
>       * exists but is an element of the wrong type, *aOK is set to false.
>       * Otherwise *aOK is untouched.
>       */
>      nsSVGMaskFrame *GetMaskFrame(bool *aOK);

Can this function be removed in a later patch?  If not, can you rename it to "GetFirstMaskFrame" and update the comment to say that is what it gets?

::: layout/svg/nsSVGEffects.h:469
(Diff revision 3)
>      nsSVGMaskFrame *GetMaskFrame(bool *aOK);
>  
> +    /**
> +     * @return an array which contains all SVG mask frames.
> +     */
> +    nsTArray<nsSVGMaskFrame *> GetMaskFrames();

Nit: no space before "*".

::: layout/svg/nsSVGEffects.cpp:376
(Diff revision 3)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_CYCLE_COLLECTION(nsSVGMaskProperty, mProperties)
> +
> +nsSVGMaskProperty::nsSVGMaskProperty(nsIFrame *aFrame)

Nit: "*" next to type, and in a few more places in the rest of the patch.

::: layout/svg/nsSVGEffects.cpp:520
(Diff revision 3)
> +  if (!prop)
> +    return nullptr;

Now that |operator new| is infallible, we don't need to check and return null here.

::: layout/svg/nsSVGEffects.cpp:661
(Diff revision 3)
>  nsSVGMaskFrame *
>  nsSVGEffects::EffectProperties::GetMaskFrame(bool *aOK)
>  {
> -  if (!mMask)
> +  const nsTArray<RefPtr<nsSVGPaintingProperty>>& props = mMask->GetProps();
> +
> +  if (0 == props.Length()) {

props.IsEmpty()

But I notice that we will now always create an nsSVGMaskProperty with at least one nsSVGPaintingProperty inside it.  This is because we unconditionally call GetOrCreateMaskProperty, whereas in the old code we used to check whether mSourceURI is null before calling GetPaintingProperty.

Should we return to not creating an nsSVGMaskProperty if we don't have any mask images?  (I think so, just to avoid unnecessary creation of frame properties.)

Comment 34

[u459114]

https://reviewboard.mozilla.org/r/51939/#review49161

Code between 2870 and 3012 execution times from O(N),, to O(1)
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2870
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3012

Actually, not much performance gain. We can skip this one.

Comment 35

[u459114]

https://reviewboard.mozilla.org/r/51935/#review49179

> If you have multiple masks, are they overwriting each other's maskTransform? This seems sketchy. Don't you somehow need to respect the maskTransform in the FillRect call below? Please add a reftest that triggers the bug if you find that there is a bug with this.
> Oh, you're completely ignoring the value of maskTransform even if there's only one mask, because you're overwriting maskTransform at the end of this function. So something's definitely wrong.

1. The mask transform matrix we get from GetMaskForMaskedFrame is totally the same with the transform overwrite in the end of this function(GenerateMaskSurface). Actually, we can skip maskTransform evaluation in the end of this function if we have one SVG mask, I will do this check in the next update.

Q: Don't you somehow need to respect the maskTransform in the FillRect call below?
A: No, we don't. In coordinate of aParams.ctx, the size and postion of maskDT and svgMask are exactly the same. We have to respect maskTransform when we draw maskDT onto aParams.ctx or draw svgMask onto aParams.ctx, but drawing svgMask to maskDT is simply indentical transform.

Comment 36

[u459114]

https://reviewboard.mozilla.org/r/51393/#review49273

> Can this function be removed in a later patch?  If not, can you rename it to "GetFirstMaskFrame" and update the comment to say that is what it gets?

In nsSVGUtils.cpp, we still need this function for convinience. Will rename to  GetFirstMaskFrame.

Comment 46

[Markus Stange [:mstange]]

Comment on attachment 8751291 [details]
MozReview Request: Bug 1228280 - Part 5. Paint multiple SVG and image masks;

https://reviewboard.mozilla.org/r/51935/#review49519

No, you need to respect the maskTransform. Look at the code in nsSVGMaskFrame::GetMaskForMaskedFrame again: maskSurfaceMatrix has a translation by the origin of maskSurfaceRect, which is computed by intersecting the context's current clip extents with maskArea, which depends on attributes set on the <mask> element.
So please add a reftest where you use two SVG <mask>s with different x/y/width/height attributes in a way that reproduces the bug with the current patch.

Also, I noticed that drawRect (in GenerateMaskSurface) is only based on the context's current clip extents and not on the size of the masks. This may result in a mask surface being much larger than necessary. It's an existing bug, so there's no need to fix it here in this bug, but I think it should be fixed before shipping the mask shorthand.

Comment 47

[Markus Stange [:mstange]]

Comment on attachment 8751293 [details]
MozReview Request: Bug 1228280 - Dummy patch. To keep patch order on review board

https://reviewboard.mozilla.org/r/51939/#review49521

Yes, let's drop this part.

Comment 48

[Cameron McCormack (:heycam)]

Comment on attachment 8750364 [details]
MozReview Request: Bug 1228280 - Part 4. Create nsSVGMaskProperty to carry multiple mask info;

https://reviewboard.mozilla.org/r/51393/#review49553

Comment 49

[Cameron McCormack (:heycam)]

Comment on attachment 8752206 [details]
MozReview Request: Bug 1228280 - Part 8. Keep style consistent and stop doing null check for return value of operator new

https://reviewboard.mozilla.org/r/52469/#review49555

Comment 50

[u459114]

https://reviewboard.mozilla.org/r/51935/#review49519

OK, thanks. I had tried. There is a bug here. By giving a non-zero value to x or y attribute of mask, GetMaskForMaskedFrame will not return a mask. I will look into this problem first.

Comment 51

[u459114]

Attachment: mask-non-zero-offset.html

This HTML says mstange is right. the rendering result on mc looks good, after applying patches here, purple circle be squashed

Comment 52

[u459114]

(In reply to Markus Stange [:mstange] from comment #46)
> Also, I noticed that drawRect (in GenerateMaskSurface) is only based on the
> context's current clip extents and not on the size of the masks. This may
> result in a mask surface being much larger than necessary. It's an existing
> bug, so there's no need to fix it here in this bug, but I think it should be
> fixed before shipping the mask shorthand.
Filed bug 1272859 for this issue.

Comment 62

[u459114]

https://reviewboard.mozilla.org/r/51935/#review49519

mask-image-3d.html in Part 7 is the reftest for mask with offset.

Comment 63

[Markus Stange [:mstange]]

Comment on attachment 8751291 [details]
MozReview Request: Bug 1228280 - Part 5. Paint multiple SVG and image masks;

https://reviewboard.mozilla.org/r/51935/#review49780

Almost there now. I'd like to see the final patch before r+ing it, though.

::: layout/svg/nsSVGIntegrationUtils.cpp:446
(Diff revision 4)
> -    // Create maskSuface.
> -    gfxRect clipRect = ctx.GetClipExtents();
> -    {
> -      gfxContextMatrixAutoSaveRestore matRestore(&ctx);
>  
> +  gfxRect clipRect = ctx.GetClipExtents();

This line seems unnecessary, since you are overwriting clipRect below.

::: layout/svg/nsSVGIntegrationUtils.cpp:479
(Diff revision 4)
> +    // maskFrame != nullptr means we get a SVG mask.
> +    // maskFrame == nullptr means we get an image mask.
> +    if (maskFrame) {
> +      Matrix svgMaskMatrix;
> +      RefPtr<SourceSurface> svgMask =
> +        maskFrame->GetMaskForMaskedFrame(&ctx, aParams.frame,

Please pass in maskContext instead of ctx here. maskContext doesn't have a transform on it, so svgMaskTransform will only have the translation that you're interested in, and then you won't have to do any complicated calculations below. That's really how this function is supposed to be used: You give it the context that you're going to draw the surface into, and the mask transform will be the transform you need *for that context*.

Comment 72

[u459114]

https://reviewboard.mozilla.org/r/51935/#review49780

> Please pass in maskContext instead of ctx here. maskContext doesn't have a transform on it, so svgMaskTransform will only have the translation that you're interested in, and then you won't have to do any complicated calculations below. That's really how this function is supposed to be used: You give it the context that you're going to draw the surface into, and the mask transform will be the transform you need *for that context*.

If we pass maskContext, instead of ctx, and ctx.CurrentMatrix is not an identify matrix(carry RTS), we need to add those vectors into maskContext before DrawSurface.
So the update version will look like
if (svgMask) {
  maskContext->SetMatrix(ctx.CurrentMatrix() *
                         gfxMatrix::Translation(-drawRect.TopLeft()) * << to maskDT space
                         ThebesMatrix(svgMaskMatrix)); << to svgMask space
           
  Rect drawRect = IntRectToRect(IntRect(IntPoint(0, 0), svgMask->GetSize()));
  maskDT->DrawSurface(svgMask, drawRect, drawRect);
}

Comment 73

[u459114]

Comment on attachment 8750361 [details]
MozReview Request: Bug 1228280 - Part 1. Change the parameter of nsCSSRendering::PaintBackground;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51387/diff/5-6/

Comment 82

[u459114]

https://reviewboard.mozilla.org/r/51935/#review49780

Eventually, I need to fix bug 1272859 right here. Otherwise, pass maskCtxt to GetMaskForMaskedFrame will get wrong rendering result if source context has scale or skew transfrom.

Comment 83

[Markus Stange [:mstange]]

I think you still need to set aOutMaskTransform to ctx->CurrentMatrix(). I think that would solve your problem without fixing  bug 1272859.

Comment 84

[u459114]

(In reply to Markus Stange [:mstange] from comment #83)
> I think you still need to set aOutMaskTransform to ctx->CurrentMatrix(). I
> think that would solve your problem without fixing  bug 1272859.
Problem happens in maskDT.
Let's say we have a div with this css attribute
div {
  width: 100px;
  height: 100px;
  trasnform: scale(0.5, 0.5);
}

Under this condition, in GenerateMaskSurface, ctx.GetClipExtents() returns (100 *0.5, 100 * 0.5) = (50, 50). So the size of maskDT become (50, 50).
Since maskContext is not scaled(maskContext.CurrentMatrix() is identity matrix) and we pass it to GetMaskForMaskedFrame, GetMaskForMaskedFrame will draw (100, 100) content onto (50, 50) surface. So the image in maskDT is clipped. That why I think I should set the size maskDT depend on mask size.

Comment 85

[u459114]

Before this update, we draw maskDT with transform in ctx.
That means if you have a rotation transform in ctx, you will see a rotated mask image in maskDT.
We need to set aOutMaskTransform as ctx.CurrentContext().invert() to recover back the rotation in maskDT.

In this update, we draw maskDT without transform in ctx, so mask image in maskDT is not rotated or scaled at all. So I think it make sense to keep aOutMaskTransform as identity matrix.

Comment 86

[Markus Stange [:mstange]]

Ok, I looked into this a little more and I think I understand what's happening now, and what needs to happen. Sorry for giving confusing / wrong advice before.

So we have the choice between combining masks in device space or in user space. Your current patch combines them in user space, which means that if your HTML element has a scale(2) transform, the mask will be blurry.
Our existing SVG mask code draws the mask in device space. So I think we should keep doing that.

For that, GenerateMaskSurface needs to do something very similar to what nsSVGMaskFrame::GetMaskForMaskedFrame is doing at the beginning and at the end:

  ctx.Save();
  ctx.Clip(yourMaskRectInDevPixels); // this line could be done in bug 1272859. yourMaskRectInDevPixels is actually in the context's "user" space.
  ctx.SetMatrix(gfxMatrix());
  gfxRect maskSurfaceRect = aContext->GetClipExtents(); // this gets the intersected clip extents in device space
  maskSurfaceRect.RoundOut();
  ctx.Restore();

  // create maskDT / maskContext
  // ...

  // Set ctx's matrix on maskContext, offset by the maskSurfaceRect's position.
  // This makes sure that we combine the masks in device space.
  gfxMatrix maskSurfaceMatrix =
    aContext->CurrentMatrix() * gfxMatrix::Translation(-maskSurfaceRect.TopLeft());
  maskContext->SetMatrix(maskSurfaceMatrix);

  // Now loop over the masks. 
  for (...) {
    if (svg mask) {
      don't make any changes to the maskContext's transform here
      Matrix svgMaskMatrix;
      call GetMaskForMaskedFrame with maskContext

      save maskContext state
      maskContext->SetMatrix(svgMaskMatrix); // the resulting matrix on maskContext will just be a translation (which is the combination of the translation from this function and the translation from inside GetMaskForMaskedFrame).
      maskDT->DrawSurface(...);
      restore maskContext state
    } else {
      ...
    }
  }

  Now set aOutMaskTransform to the inverse of maskSurfaceMatrix.

Does that make sense?

Comment 87

[Markus Stange [:mstange]]

Oops, I made a mistake in the code above. Instead of "maskContext->SetMatrix(svgMaskMatrix);", you'd need to call maskContext->Multiply(svgMaskMatrix).

Comment 88

[u459114]

Cool! thank you. Doing all painting in device space is a good idea. I will update a new version soon

Comment 89

[u459114]

Comment on attachment 8750361 [details]
MozReview Request: Bug 1228280 - Part 1. Change the parameter of nsCSSRendering::PaintBackground;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51387/diff/6-7/

Comment 90

[u459114]

Comment on attachment 8750362 [details]
MozReview Request: Bug 1228280 - Part 2. Change the parameters of nsSVGIntegrationUtils::PaintFramesWithEffects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51389/diff/6-7/

Comment 91

[u459114]

Comment on attachment 8750363 [details]
MozReview Request: Bug 1228280 - Part 3. Move mask painting out of nsSVGIntegrationUtils::PaintFramesWithEffects;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51391/diff/6-7/

Comment 92

[u459114]

Comment on attachment 8750364 [details]
MozReview Request: Bug 1228280 - Part 4. Create nsSVGMaskProperty to carry multiple mask info;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51393/diff/6-7/

Comment 93

[u459114]

Comment on attachment 8751291 [details]
MozReview Request: Bug 1228280 - Part 5. Paint multiple SVG and image masks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51935/diff/5-6/

Comment 94

[u459114]

Comment on attachment 8751292 [details]
MozReview Request: Bug 1228280 - Part 6. Single SVG mask optimization;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51937/diff/5-6/

Comment 95

[u459114]

Comment on attachment 8751293 [details]
MozReview Request: Bug 1228280 - Dummy patch. To keep patch order on review board

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51939/diff/5-6/

Comment 96

[u459114]

Comment on attachment 8751294 [details]
MozReview Request: Bug 1228280 - Part 7. reftest of SVG mask and image mask interleaving;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51941/diff/5-6/

Comment 97

[u459114]

Comment on attachment 8752206 [details]
MozReview Request: Bug 1228280 - Part 8. Keep style consistent and stop doing null check for return value of operator new

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52469/diff/3-4/

Comment 98

[Markus Stange [:mstange]]

Comment on attachment 8751291 [details]
MozReview Request: Bug 1228280 - Part 5. Paint multiple SVG and image masks;

https://reviewboard.mozilla.org/r/51935/#review50340

Once you've addressed these comments this is good to go. Thanks!

::: layout/svg/nsSVGIntegrationUtils.cpp:430
(Diff revision 6)
>  
> -  return hasMaskToDraw;
> +  return false;
>  }
>  
>  static void
> -GenerateMaskSurface(const nsSVGIntegrationUtils::PaintFramesParams& aParams,
> + GenerateMaskSurface(const nsSVGIntegrationUtils::PaintFramesParams& aParams,

An unnecessary space crept in at the start of this line.

::: layout/svg/nsSVGIntegrationUtils.cpp:466
(Diff revision 6)
> +  RefPtr<gfxContext> maskContext = gfxContext::ForDrawTarget(maskDT);
>  
> -    if (!targetDT || !targetDT->IsValid()) {
> -      return;
> +  // Set ctx's matrix on maskContext, offset by the maskSurfaceRect's position.
> +  // This makes sure that we combine the masks in device space.
> +  gfxMatrix maskSurfaceMatrix =
> +    ctx.CurrentMatrix() * gfxMatrix::Translation(-maskSurfaceRect.TopLeft());

You need to use maskSurfaceIntRect here. The translation should be by integer pixels.

(You could rename maskSurfaceRect to clipExtents and maskSurfaceIntRect to maskSurfaceRect.)

::: layout/svg/nsSVGIntegrationUtils.cpp:512
(Diff revision 6)
> -    Unused << nsCSSRendering::PaintBackgroundWithSC(params, aSC,
> +      Unused << nsCSSRendering::PaintBackgroundWithSC(params, aSC,
> -                                                    *aParams.frame->StyleBorder());
> +                                                      *aParams.frame->StyleBorder());
> -    aOutMaskSurface = targetDT->Snapshot();
> +    }
> +  }
>  
> -    // Compute mask transform.
> +  aOutMaskTransform = ToMatrix(ctx.CurrentMatrix());

Don't use ctx.CurrentMatrix() here. Just invert maskSurfaceMatrix and return that. Then you won't need to add the translation below.

Comment 108

[u459114]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42f1a5c8e5dd7978e42e362c5b667cc5bb94e290
Bug 1228280 - Part 1. Change the parameter of nsCSSRendering::PaintBackground;

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f187d5954801a8f2c0470e87db30814f79345b5
Bug 1228280 - Part 2. Change the parameters of nsSVGIntegrationUtils::PaintFramesWithEffects;

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3ebdaea7adcec432e4f6cf85c6626885f5df69
Bug 1228280 - Part 3. Move mask painting out of nsSVGIntegrationUtils::PaintFramesWithEffects;

https://hg.mozilla.org/integration/mozilla-inbound/rev/838f42a621fcfcf2f21b2630c7a791109903b5f3
Bug 1228280 - Part 4. Create nsSVGMaskProperty to carry multiple mask info;

https://hg.mozilla.org/integration/mozilla-inbound/rev/27914ad6e245f9d4485b1e23842039274de2290d
Bug 1228280 - Part 5. Paint multiple SVG and image masks;

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a6d836e51e6aec74442b62254f0beec61367cf
Bug 1228280 - Part 6. Single SVG mask optimization;

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7119f0122c278ea3f9038439594035ebdd3e95
Bug 1228280 - Part 7. reftest of SVG mask and image mask interleaving;

https://hg.mozilla.org/integration/mozilla-inbound/rev/58bedfd369f3346dde185d73486056324984dc3b
Bug 1228280 - Part 8. Keep style consistent and stop doing null check for return value of operator new

Comment 109

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42f1a5c8e5dd
https://hg.mozilla.org/mozilla-central/rev/1f187d595480
https://hg.mozilla.org/mozilla-central/rev/4e3ebdaea7ad
https://hg.mozilla.org/mozilla-central/rev/838f42a621fc
https://hg.mozilla.org/mozilla-central/rev/27914ad6e245
https://hg.mozilla.org/mozilla-central/rev/91a6d836e51e
https://hg.mozilla.org/mozilla-central/rev/7b7119f0122c
https://hg.mozilla.org/mozilla-central/rev/58bedfd369f3

Comment 110

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few issues with the reftests:

from check-for-references.sh (which checks that the tests have the reference links expected by the W3C format):

Missing link for ./masking/mask-image-3a.html
Missing link for ./masking/mask-image-3b.html
Missing link for ./masking/mask-image-3c.html
Missing link for ./masking/mask-image-3d.html
Missing link for ./masking/mask-image-3e.html


From the submission process:

remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-image-3a.html status changed to 'Needs Work' due to error:
remote: Not linked to a specification.
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-image-3b.html status changed to 'Needs Work' due to error:
remote: Not linked to a specification.
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-image-3c.html status changed to 'Needs Work' due to error:
remote: Not linked to a specification.
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-image-3d.html status changed to 'Needs Work' due to error:
remote: Not linked to a specification.
remote: vendor-imports/mozilla/mozilla-central-reftests/masking/mask-image-3e.html status changed to 'Needs Work' due to error:
remote: Not linked to a specification.

Comment 111

[u459114]

https://hg.mozilla.org/integration/mozilla-inbound/rev/055283f9ffed31d6b63d2ac6c25f59228679c21b
Bug 1228280: (followup) Update w3c-css reftest link. r=me

Comment 112

[u459114]

Done. Added link rel="help"/ rel="match"/ rel="assert" for these 5 test cases

Comment 113

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/055283f9ffed

Comment 114

[Jean-Yves Perrier [:teoli]]

Added a comment in: https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS

Comment 115

[u459114]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a89d0ddb218ec4a41061417589790a19ab04de9
Bug 1228280 - (follow up)Part 9. Remove trailing spec in test case title. r=me

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a89d0ddb218

Comment 117

[Sebastian Zartner [:sebo]]

Tested this right now in 49.0b8 and it doesn't actually support multiple masks within the 'mask' property.
In my test case I had 'mask: url(foo), url(bar);' defined.

What's the reason for that?

Sebastian

Comment 118

[u459114]

This feature is enabled after bug 1294660 landed. 49.0b8 is too early for it.

Comment 119

[Sebastian Zartner [:sebo]]

Ok, so it's obviously too early for a release note. Gerry, can you please remove the note again?

I've removed the related note at https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS again.

Sebastian

Comment 120

[Gerry Chang [:gchang]]

Per comment #118, remove the 49+ release notes.

Comment 121

[Ting-Yu Lin [:TYLin] (PST, UTC-8) (Away, back on Mar 24)]

Gerry, I still see "Support multiple SVG mask elements as mask images" in the release note.
https://www.mozilla.org/en-US/firefox/49.0beta/releasenotes/

Comment 122

[Gerry Chang [:gchang]]

The release note is removed. Thanks for reminder.