Closed Bug 880836 Opened 7 years ago Closed 7 years ago

Add MaskSurface() to Azure

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
This will also match well with our existing gfxContext::Mask(gfxASurface *surface, const gfxPoint& offset) call.
Attached patch Now with more changes (obsolete) — Splinter Review
Attachment #759944 - Attachment is obsolete: true
Attachment #759969 - Flags: feedback?(bas)
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.
(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.
Bas can you just review the D2D part for now?
Attachment #759969 - Attachment is obsolete: true
Attachment #759969 - Flags: feedback?(bas)
Attachment #761867 - Flags: review?
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()
Attachment #761869 - Flags: feedback?(gwright)
Attachment #761867 - Flags: review? → review?(bas)
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.
Attachment #761867 - Flags: review?(bas) → review+
This also adds CG version that may probably need some more work.
Attachment #761867 - Attachment is obsolete: true
Attachment #763946 - Flags: review?
Attachment #763946 - Flags: review? → review?(bas)
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.
Attachment #763946 - Flags: review?(bas) → review+
I needed to add an implementation for DrawTargetDual
https://hg.mozilla.org/integration/mozilla-inbound/rev/43516c3e17fb
https://hg.mozilla.org/mozilla-central/rev/43516c3e17fb
https://hg.mozilla.org/mozilla-central/rev/8f04327695ca
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 887916
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.
(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.
I've sketched this out in bug 889693
Attachment #761869 - Flags: feedback?(gwright)
You need to log in before you can comment on or make changes to this bug.