Closed Bug 1228280 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 8 obsolete files)

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
heycam
: 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
Details
58 bytes, text/x-review-board-request
mstange
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
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
Summary: (mask-image) Support multiple SVG mask. → (mask-image) Support multiple SVG masks
Blocks: mask-ship
Assignee: cku → nobody
Summary: (mask-image) Support multiple SVG masks → (mask-image) Support multiple SVG mask elements as mask images
Assignee: nobody → cku
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla49
Attachment #8750361 - Flags: review?(mstange)
Attachment #8750362 - Flags: review?(mstange)
Attachment #8750363 - Flags: review?(mstange)
Attachment #8750364 - Flags: review?(cam)
Attachment #8751291 - Flags: review?(mstange)
Attachment #8751292 - Flags: review?(mstange)
Attachment #8751293 - Flags: review?(mstange)
Attachment #8751294 - Flags: review?(mstange)
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
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 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?
Attachment #8750361 - Flags: review?(mstange) → review+
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.
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 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?
Attachment #8751293 - Flags: review?(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
Attachment #8751294 - Flags: review?(mstange) → review+
Attachment #8750363 - Flags: review?(mstange) → review+
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 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.
Attachment #8750362 - Flags: review?(mstange) → review+
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 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.
Attachment #8751291 - Flags: review?(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.
Attachment #8751292 - Flags: review?(mstange) → review+
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.)
Attachment #8750364 - Flags: review?(cam)
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.
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 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.
Attachment #8751291 - Flags: review?(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.
Attachment #8751293 - Flags: review?(mstange)
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
Attachment #8750364 - Flags: review?(cam) → review+
Attachment #8752206 - Flags: review?(cam) → review+
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
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.
Attached file mask-non-zero-offset.html (obsolete) —
This HTML says mstange is right. the rendering result on mc looks good, after applying patches here, purple circle be squashed
(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.
Blocks: 1272859
https://reviewboard.mozilla.org/r/51935/#review49519

mask-image-3d.html in Part 7 is the reftest for mask with offset.
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*.
Attachment #8751291 - Flags: review?(mstange)
Attachment #8752978 - Attachment is obsolete: true
Attachment #8752980 - Attachment is obsolete: true
Attachment #8752985 - Attachment is obsolete: true
Attachment #8752984 - Attachment is obsolete: true
Attachment #8752981 - Attachment is obsolete: true
Attachment #8752982 - Attachment is obsolete: true
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 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/
Attachment #8751293 - Attachment is obsolete: false
Attachment #8751291 - Flags: review?(mstange)
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.
I think you still need to set aOutMaskTransform to ctx->CurrentMatrix(). I think that would solve your problem without fixing  bug 1272859.
(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.
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.
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?
Oops, I made a mistake in the code above. Instead of "maskContext->SetMatrix(svgMaskMatrix);", you'd need to call maskContext->Multiply(svgMaskMatrix).
Cool! thank you. Doing all painting in device space is a good idea. I will update a new version soon
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 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 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 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 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 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 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 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 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/
Attachment #8751291 - Flags: review?(mstange) → review+
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.
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.
Flags: needinfo?(cku)
Done. Added link rel="help"/ rel="match"/ rel="assert" for these 5 test cases
Flags: needinfo?(cku)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a89d0ddb218ec4a41061417589790a19ab04de9
Bug 1228280 - (follow up)Part 9. Remove trailing spec in test case title. r=me
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
Flags: needinfo?(cku)
This feature is enabled after bug 1294660 landed. 49.0b8 is too early for it.
Flags: needinfo?(cku)
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
Flags: needinfo?(gchang)
Per comment #118, remove the 49+ release notes.
Flags: needinfo?(gchang)
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/
Flags: needinfo?(gchang)
The release note is removed. Thanks for reminder.
Flags: needinfo?(gchang)
You need to log in before you can comment on or make changes to this bug.