1272859 - Reduce the size of the mask surface in GenerateMaskSurface

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

According to bug 1228280 comment 46

mstange wrote:
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 4

[Markus Stange [:mstange]]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

https://reviewboard.mozilla.org/r/54836/#review51566

::: layout/svg/nsSVGIntegrationUtils.cpp:462
(Diff revision 1)
> +  // Get the clip extents in device space.
>    ctx.Save();
> +  uint32_t appUnitsPerDevPixel =
> +    aParams.frame->PresContext()->AppUnitsPerDevPixel();
> +  gfxRect maskArea =
> +    nsLayoutUtils::RectToGfxRect(nsRect(nsPoint(0, 0), aParams.frame->GetSize()),

For frames that have overflow (e.g. box-shadows), this will clip too much. SVG masks can apply outside the frame's border-box. I'll attach a testcase.
Let's use the frame's visual overflow rect instead.

If we want to get more fancy, we could do a first pass over the list of masks just to compute the needed rect. For SVG masks, that would be the mask frame's mask rect, and for image masks, it would be the mask layer's clip rect as defined by the mask-clip value.

::: layout/svg/nsSVGIntegrationUtils.cpp:464
(Diff revision 1)
> +  uint32_t appUnitsPerDevPixel =
> +    aParams.frame->PresContext()->AppUnitsPerDevPixel();
> +  gfxRect maskArea =
> +    nsLayoutUtils::RectToGfxRect(nsRect(nsPoint(0, 0), aParams.frame->GetSize()),
> +                                 appUnitsPerDevPixel);
> +  maskArea.RoundOut();

This RoundOut shouldn't be necessary.

Comment 5

[Markus Stange [:mstange]]

Attachment: testcase that shows an SVG mask that shows stuff outside the masked frame's border box

The masked div has a box-shadow, which is drawn outside the frame's border box. The mask has a circle whose bounding box has its top left corner at -100, -100, which is the top left edge of the box shadow.
The mask region (as defined by the x/y/width/height attributes on the <mask> element) is extended to -50 on the left side and -100 on the top side. This causes the mask to be cut off on the left side.

I used "userSpaceOnUse" for the units because using "objectBoundingBox" would hit bug 1067355 and distract from what I'm trying to show.

Comment 6

[u459114]

(In reply to Markus Stange [:mstange] from comment #4)
> For frames that have overflow (e.g. box-shadows), this will clip too much.
> SVG masks can apply outside the frame's border-box. I'll attach a testcase.
> Let's use the frame's visual overflow rect instead.
Will do
> If we want to get more fancy, we could do a first pass over the list of
> masks just to compute the needed rect. For SVG masks, that would be the mask
> frame's mask rect, and for image masks, it would be the mask layer's clip
> rect as defined by the mask-clip value.
It's possible. Will do.

Comment 7

[Markus Stange [:mstange]]

Thanks. And, as always, please add a reftest that would have caught the bug.

Comment 8

[u459114]

Attachment: Bug 1272859: Part 2. SVG mask on border area reftest

Review commit: https://reviewboard.mozilla.org/r/55394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55394/

Comment 9

[u459114]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54836/diff/1-2/

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8756799 [details]
Bug 1272859: Part 2. SVG mask on border area reftest

https://reviewboard.mozilla.org/r/55394/#review52214

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

https://reviewboard.mozilla.org/r/54836/#review52218

Could you compute the rects in user space instead? I think that would make the code a little shorter and hopefully easier to understand.

And I think you should clip to the frame's overflow rect, too. Otherwise, if you have a single SVG mask with a big mask region applied to a small element, you'll get an unnecessarily large surface. (Actually, are masks applied before or after filters, if a frame has both a mask and a filter? If they are applied before the filter, then you'll need the pre filter visual overflow rect instead of the regular visual overflow rect.)

Comment 12

[u459114]

(In reply to Markus Stange [:mstange] from comment #11)
> element, you'll get an unnecessarily large surface. (Actually, are masks
> applied before or after filters, if a frame has both a mask and a filter? If
> they are applied before the filter, then you'll need the pre filter visual
> overflow rect instead of the regular visual overflow rect.)
In nsSVGIntegrationUtils::PaintFramesWithEffects
1. generate mask surface if need
2. generate clip surface if need
3. Push clip surface if we have; otherwise, push mask surface if we have
4. Paint filter.
5. Blend color buffer by the surface we push in #3
Although mask applied(#5) after filter paint(#4), it was generated in #1. So there are two choices, one is by using pre filter visual overflow(no idea how to get this rect) like you said, or move mask surface generation after $4. 

Logic in #3 is not quite right. If you define a clip path, mask will always be discard, that's not what spec said[1]:
In a last step the following effects are applied to the element in order: filter effects [FILTER-EFFECTS], clipping, masking and opacity.

[1] https://drafts.fxtf.org/css-masking-1/#placement

Comment 13

[u459114]

Attachment: test case for drop-shadow filter

Comment 14

[u459114]

By using test case in attachment 8757472 [details], I noticed that nsIFrame::GetVisualOverflowRect() contains the area extending by drop-show filter

Comment 21

[u459114]

1. add meta that w3c-css reftest need.
2. add one extra test case, mask-image-3g.html.

Comment 22

[u459114]

(In reply to C.J. Ku[:cjku] from comment #12)
> Logic in #3 is not quite right. If you define a clip path, mask will always
> be discard, that's not what spec said[1]:
I was wrong here. Please ignore this statement.

Comment 23

[u459114]

https://reviewboard.mozilla.org/r/54836/#review52880

::: layout/svg/nsSVGIntegrationUtils.cpp:531
(Diff revision 4)
> +                                                      aMaskFrames,
> +                                                      cssPxToDevPxMatrix);
> +
> +  gfxRect maskSurfaceRect= imageMaskSurfaceRect
> +                             .Union(svgMaskSurfaceRect)
> +                             .Intersect(clippedFrameSurfaceRect);

According to the change in Bug 1275450, change is requrired here.

gfxRect maskSurfaceRect= imageMaskSurfaceRect
                         .Union(svgMaskSurfaceRect);
// maskSurfaceRect might be empty if all mask
// references are not resolvable.
maskSurfaceRect = maskSurfaceRect.isEmpty()   
? clippedFrameSurfaceRect
: maskSurfaceRect.Intersect(clippedFrameSurfaceRect);

Comment 24

[Markus Stange [:mstange]]

(In reply to C.J. Ku[:cjku] from comment #12)
> Although mask applied(#5) after filter paint(#4), it was generated in #1.

OK. So, in the end result, the mask applies "outside" the filter.

> So
> there are two choices, one is by using pre filter visual overflow

Oh, I see where the confusion is coming from. "Pre" does not mean "before the filter is computed during painting", it means "inside the filter". Both rectangles have been computed by layout long before the paint started. The rects do not change during painting.

(In reply to C.J. Ku[:cjku] from comment #14)
> By using test case in attachment 8757472 [details], I noticed that
> nsIFrame::GetVisualOverflowRect() contains the area extending by drop-show
> filter

Correct.

Comment 25

[Markus Stange [:mstange]]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

https://reviewboard.mozilla.org/r/54836/#review52938

This is not really what I had in mind. I'd prefer nsSVGMaskFrame::ComputeMaskGeometry to compute the mask rect in user space, and the user-to-device space conversion would be done in the callers. And for computing the rect of all the masks, you'd do:

gfxRect unionInUserSpace;
loop over all mask layers:
  if image, union unionInUserSpace with image layer clip in user space
  if svg, union unionInUserSpace with nsSVGMaskFrame::ComputeMaskGeometry (which will now be in user space)

Now, convert to device space by clipping the context to both the visual overflow rect and to the union, unsetting the transform and getting the clip extents.

Comment 28

[Markus Stange [:mstange]]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

https://reviewboard.mozilla.org/r/54836/#review53216

::: layout/svg/nsSVGIntegrationUtils.cpp:437
(Diff revision 5)
> +  nsRect userSpaceBorderArea =
> +    nsRect(nsPoint(0, 0), aParams.borderArea.Size());
> +  nsRect userSpaceDirtyRect = nsRect(nsPoint(0, 0), aParams.dirtyRect.Size());

This seems wrong. Do you just need to move the rectangle's by the position of "frame"?

::: layout/svg/nsSVGIntegrationUtils.cpp:469
(Diff revision 5)
> +
> +  return maskSurfaceRect;
> +}
> +
> +static bool
> +ComputeMaskGeometry(const nsSVGIntegrationUtils::PaintFramesParams& aParams,

Can you make this return an IntRect? An empty IntRect can mean "no drawing".

::: layout/svg/nsSVGIntegrationUtils.cpp:478
(Diff revision 5)
> +                    gfxPoint& aMaskSurfacePosition, IntSize& aMaskSurfaceSize)
> +{
> +  gfxContext& ctx = aParams.ctx;
> +  nsIFrame* frame = aParams.frame;
> +
> +  // Compute mask geomerty in user space.

typo in geometry

::: layout/svg/nsSVGIntegrationUtils.cpp:479
(Diff revision 5)
> +{
> +  gfxContext& ctx = aParams.ctx;
> +  nsIFrame* frame = aParams.frame;
> +
> +  // Compute mask geomerty in user space.
> +  gfxRect maskInUserSpace = ComputeMaskGeometry(aParams, svgReset, aMaskFrames);

I think it would be better to just move that function's code into this function. Calling a function of the same name is confusing.

::: layout/svg/nsSVGIntegrationUtils.cpp:484
(Diff revision 5)
> +  gfxRect frameVisualOverflowRect =
> +    nsLayoutUtils::RectToGfxRect(frame->GetVisualOverflowRectRelativeToSelf(),
> +                                 appUnitsPerDevPixel);
> +  gfxMatrix cssPxToDevPxMatrix =
> +    nsSVGIntegrationUtils::GetCSSPxToDevPxMatrix(frame);
> +  nsSVGUtils::SetClipRect(&ctx, cssPxToDevPxMatrix,
> +                          frameVisualOverflowRect);

You're applying the CSS->DevPixel conversion twice. appUnitsPerDevPixel already contains that conversion.
Let's skip the nsSVGUtils::SetClipRect call and just clip to frameVisualOverflowRect.

::: layout/svg/nsSVGIntegrationUtils.cpp:509
(Diff revision 5)
> +                                     &resultOverflows);
> +  if (resultOverflows || maskSurfaceSize.IsEmpty()) {
> +    return false;
> +  }
> +
> +  aMaskSurfacePosition = clippedFrameSurfaceRect.TopLeft();

This also needs to use the rounded-out position. Let's just roundOut the whole rectangle. I don't think there's a reason to call nsSVGUtils::ConvertToSurfaceSize as long as you handle the overflow case when you call Rect::ToIntRect.

::: layout/svg/nsSVGMaskFrame.cpp:228
(Diff revision 5)
>    // Minimizing the mask surface extents (using both the current clip extents
>    // and maskArea) is important for performance.
>    aContext->Save();
>    nsSVGUtils::SetClipRect(aContext, aMatrix, maskArea);
>    aContext->SetMatrix(gfxMatrix());
> -  gfxRect maskSurfaceRect = aContext->GetClipExtents();
> +  gfxRect maskSurfaceRect  = aContext->GetClipExtents();

bad whitespace change

::: layout/svg/nsSVGMaskFrame.cpp:337
(Diff revision 5)
>    *aMaskTransform = ToMatrix(maskSurfaceMatrix);
>    return destMaskSurface.forget();
>  }
>  
> +gfxRect
> +nsSVGMaskFrame::ComputeMaskGeometry(nsIFrame* aMaskedFrame)

Let's call this GetMaskArea.

Comment 29

[u459114]

https://reviewboard.mozilla.org/r/54836/#review53216

> This seems wrong. Do you just need to move the rectangle's by the position of "frame"?

Just want to double confirm - Do you means "Why not just move the rectangle by the position of "frame"?"
PS: if that's what you ask, that's because of [1]
  gfxPoint devPixelOffsetToUserSpace =
    nsLayoutUtils::PointToGfxPoint(offsetToUserSpace,
                                   frame->PresContext()->AppUnitsPerDevPixel());
  context.SetMatrix(context.CurrentMatrix().Translate(devPixelOffsetToUserSpace));
    
Since context is already being translated to user space before calling ComputeMaskGeometry.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#627

Comment 30

[Markus Stange [:mstange]]

Okay, so do you have to translate it at all? No matter what you do, I don't think ignoring the position entirely is the right answer. You'll have to find out what you need to translate by.

Comment 31

[Markus Stange [:mstange]]

(In comment 30 I was referring to moving the position of the rectangles, not about applying a translation to a context.)

Comment 32

[u459114]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

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

Comment 33

[u459114]

Comment on attachment 8756799 [details]
Bug 1272859: Part 2. SVG mask on border area reftest

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

Comment 34

[Markus Stange [:mstange]]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

https://reviewboard.mozilla.org/r/54836/#review55778

::: layout/svg/nsSVGIntegrationUtils.cpp:425
(Diff revision 6)
> +  uint32_t appUnitsPerDevPixel = frame->PresContext()->AppUnitsPerDevPixel();
> +
> +  // Convert boaderArea and dirtyRect to user space.
> +  nsPoint toReferenceFrame(aToReferenceFrame.x * appUnitsPerDevPixel,
> +                           aToReferenceFrame.y * appUnitsPerDevPixel);
> +  nsRect userSpaceBorderArea = aParams.borderArea;

nsRect is used for values in app units. Is borderArea in app units? If it is, you can't move it by a value in layout device pixels.

And I'm not sure that this offset is actually the offset to the reference frame. Let's keep calling it "offset", or "offsetToUserSpace".

Comment 35

[u459114]

(In reply to Markus Stange [:mstange] from comment #34)
> Comment on attachment 8755830 [details]
> MozReview Request: Bug 1272859: Part 1. Reduce mask surface's size
> 
> https://reviewboard.mozilla.org/r/54836/#review55778
> 
> ::: layout/svg/nsSVGIntegrationUtils.cpp:425
> (Diff revision 6)
> > +  uint32_t appUnitsPerDevPixel = frame->PresContext()->AppUnitsPerDevPixel();
> > +
> > +  // Convert boaderArea and dirtyRect to user space.
> > +  nsPoint toReferenceFrame(aToReferenceFrame.x * appUnitsPerDevPixel,
> > +                           aToReferenceFrame.y * appUnitsPerDevPixel);
> > +  nsRect userSpaceBorderArea = aParams.borderArea;
> 
> nsRect is used for values in app units. Is borderArea in app units? If it
> is, you can't move it by a value in layout device pixels.
toReferenceFrame is actually app unit too.
> And I'm not sure that this offset is actually the offset to the reference
> frame. Let's keep calling it "offset", or "offsetToUserSpace".

Comment 38

[u459114]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

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

Comment 39

[u459114]

Comment on attachment 8756799 [details]
Bug 1272859: Part 2. SVG mask on border area reftest

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

Comment 40

[Markus Stange [:mstange]]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

https://reviewboard.mozilla.org/r/54836/#review56576

With those two comments applied I think this is ready to go. Thanks for the patience!

::: layout/svg/nsSVGIntegrationUtils.cpp:441
(Diff revision 8)
> +      nsCSSRendering::GetImageLayerClip(svgReset->mMask.mLayers[i],
> +                                       frame,
> +                                       *frame->StyleBorder(),
> +                                       userSpaceBorderArea,
> +                                       userSpaceDirtyRect,
> +                                       true, /* aWillPaintBorder */

I think this actually needs to be false. If it's true, opaque borders will be subtracted from the image clip, but we want masks to be applied to the whole element including the border regardless of the border's opacity.

::: layout/svg/nsSVGIntegrationUtils.cpp:530
(Diff revision 8)
>    RefPtr<gfxContext> maskContext = gfxContext::CreateOrNull(maskDT);
>    MOZ_ASSERT(maskContext);
>  
> +  gfxPoint devPixelOffsetToUserSpace =
> +    nsLayoutUtils::PointToGfxPoint(aOffsetToUserSpace,
> +                                   aParams.frame->PresContext()->AppUnitsPerDevPixel());

We use aParams.frame->PresContext() a second time further down in this function. Let's put it into its own variable.

Comment 41

[u459114]

Comment on attachment 8755830 [details]
Bug 1272859: Part 1. Reduce mask surface's size

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54836/diff/8-9/

Comment 42

[u459114]

Comment on attachment 8756799 [details]
Bug 1272859: Part 2. SVG mask on border area reftest

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55394/diff/8-9/

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/657006fadf7f
Part 1. Reduce mask surface's size r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c40761cbc60
Part 2. SVG mask on border area reftest r=mstange

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/657006fadf7f
https://hg.mozilla.org/mozilla-central/rev/7c40761cbc60