880836 - Add MaskSurface() to Azure

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

The current Mask api is very general, taking source and mask patterns. This is not a very good match for Direct2D and CoreGraphics.

Direct2D has FillOpacityMask(Bitmap mask, Brush brush) and CG has CGClipWithMask(Image)

Using both of these API's will avoid an extra allocation to rasterize the brush to temporary surface to mask through.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This will also match well with our existing gfxContext::Mask(gfxASurface *surface, const gfxPoint& offset) call.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: An example of what this could look like

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Now with more changes

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 759969 [details] [diff] [review]
Now with more changes

I sort of feel this is so easy to recognize with a normal Mask operation (I have detection and an optimized version locally for Direct2D and Direct2D 1.1 as well, that it probably doesn't really need a separate function? Can you convince me otherwise? I don't really mind doing it I just don't want to clutter the API too much. Also if we actually do this we should include the Recording functionality for it as well.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> Comment on attachment 759969 [details] [diff] [review]
> Now with more changes
> 
> I sort of feel this is so easy to recognize with a normal Mask operation (I
> have detection and an optimized version locally for Direct2D and Direct2D
> 1.1 as well, that it probably doesn't really need a separate function? Can
> you convince me otherwise? I don't really mind doing it I just don't want to
> clutter the API too much. Also if we actually do this we should include the
> Recording functionality for it as well.

The biggest reason I have is that it avoids having a performance cliff and exposes the performance characteristics to the user. I looked at all the users of Mask() and it looked like all of them were basically doing Mask with a surface pattern and extend none. This is the functionality that MaskSurface exposes as opposed to Mask() which requires the clipRect dance to get right.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: A working D2D MaskSurface implementation

Bas can you just review the D2D part for now?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: A sketch of the Skia implementation

I've no idea if this actually works, but I think the general idea is right. This should be more efficient than the current layer stuff we do in Mask()

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 761867 [details] [diff] [review]
A working D2D MaskSurface implementation

Review of attachment 761867 [details] [diff] [review]:
-----------------------------------------------------------------

There's some whitespace in here that needs fixing too! Other than that looks fine.

::: gfx/2d/2D.h
@@ +711,5 @@
> +   * onto the destination surface using the alpha channel of the mask source.
> +   * The operation is bound by the extents of the mask.
> +   *
> +   * aSource Source pattern
> +   * aMask Mask surface

Please document aOffset. I had to look at the coce to be sure.

::: gfx/2d/DrawTargetCairo.cpp
@@ +675,5 @@
>  
>    cairo_pattern_destroy(mask);
>    cairo_pattern_destroy(source);
>  }
> +#if 0

I'm guessing you didn't really want this?

::: gfx/2d/DrawTargetD2D.cpp
@@ +345,5 @@
> +  rt->SetAntialiasMode(D2D1_ANTIALIAS_MODE_ALIASED);
> +
> +  switch (aMask->GetType()) {
> +
> +  case SURFACE_D2D1_BITMAP:

Looks like this code was copy-pasted, it would be best to somehow factor this out into a separate function. In the D2D 1.1 backend I added GetImageForSourceSurface, for D2D 1.0 we could probably have GetBitmapForSourceSurface.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add MaskSurface API

This also adds CG version that may probably need some more work.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 763946 [details] [diff] [review]
Add MaskSurface API

Review of attachment 763946 [details] [diff] [review]:
-----------------------------------------------------------------

Little bit of whitespace here and there, looks fairly good otherwise.

::: gfx/2d/2D.h
@@ +733,5 @@
> +   */
> +  virtual void MaskSurface(const Pattern &aSource,
> +                           SourceSurface *aMask,
> +                           Point aOffset,
> +                           const DrawOptions &aOptions = DrawOptions()) {};

I'd prefer throwing an error in backends which don't support this over silently doing nothing.

::: gfx/2d/DrawTargetD2D.cpp
@@ +259,5 @@
>  
> +TemporaryRef<ID2D1Bitmap>
> +DrawTargetD2D::GetBitmapForSurface(ID2D1RenderTarget *aRT,
> +                                   SourceSurface *aSurface,
> +                                   Rect &aSource)

You don't really need to pass aRT, you can always use mRT since the two are always compatible.

@@ +333,5 @@
> +  Rect srcRect = aSource;
> +
> +  bitmap = GetBitmapForSurface(rt, aSurface, srcRect);
> +  if (!bitmap)
> +      return;

nit: { } even for single line ifs.

@@ +353,5 @@
> +
> +  PrepareForDrawing(rt);
> +
> +  // FillOpacityMask only works if the antialias mode is MODE_ALIASED
> +  rt->SetAntialiasMode(D2D1_ANTIALIAS_MODE_ALIASED);

Hrm, we better hope we don't forget to set the proper AA mode anywhere :-). Well, this will let us know at least.

@@ +357,5 @@
> +  rt->SetAntialiasMode(D2D1_ANTIALIAS_MODE_ALIASED);
> +
> +  IntSize size = aMask->GetSize();
> +  Rect maskRect = Rect(0.f, 0.f, size.width, size.height);
> +  bitmap = GetBitmapForSurface(rt, aMask, maskRect);

please bail if !bitmap.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I needed to add an implementation for DrawTargetDual
https://hg.mozilla.org/integration/mozilla-inbound/rev/43516c3e17fb

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43516c3e17fb
https://hg.mozilla.org/mozilla-central/rev/8f04327695ca

Comment 13

[Jacek Caban]

I landed a trivial mingw/GCC fixup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fda0046aa376

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fda0046aa376

Comment 15

[James Kolb]

Are there plans for implementing MaskSurface() for DrawTargetCairo anytime soon?

Without it, these patches break gfxContext::Mask(gfxASurface *surface, const gfxPoint& offset) for contexts based on DrawTargetCairo. That function is used in rendering blurred shadows directly to a canvas in linux, and is necessary for fixing bug 603488.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to James Kolb from comment #15)
> Are there plans for implementing MaskSurface() for DrawTargetCairo anytime
> soon?
> 
> Without it, these patches break gfxContext::Mask(gfxASurface *surface, const
> gfxPoint& offset) for contexts based on DrawTargetCairo. That function is
> used in rendering blurred shadows directly to a canvas in linux, and is
> necessary for fixing bug 603488.

One difficulty with supporting this in DrawTargetCairo is that there's no easy way
to support alpha with out using a temporary surface. There are currently no users that depend on that functionality so we could remove it. However, I think cairo is the only backend without this functionality so maybe just slow pathing it is the right thing to do.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I've sketched this out in bug 889693