933567 - Implement MaskSurface for DrawTargetSkia

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andreas Gal :gal]

Attachment: Part 1: Implement MaskSurface

Comment 1

[Andreas Gal :gal]

Attachment: Part 2: Clean up Mask implementation a bit

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This works in simple cases, but it doesn't work on the Firefox home page 3x3 page grid, hovering over each is supposed to paint a shadow (which animates outward).  I get something that looks like it's scaled up too much and offset.  I only see it for the top left one too, nothing renders for the others.

http://i.imgur.com/AkUMR34.jpg

Comment 3

[Matt Woodrow (:mattwoodrow)]

I think the best way to implement MaskSurface will be to use the secret features of drawBitmap().

drawBitmap normally draws the passed bitmap as a source, unless the bitmap is a8, in which case it will use the bitmap as a mask and use the SkPaint to decide the source.

Most of the time we should already have a8 bitmap as the mask surface, but we can call SkBitmap::extractAlpha() if we get some other sort of source.

Then the implementation should be more or less identical to FillRect() except passing mask, aOffset.x, aOffset.y instead of aRect.

Comment 4

[Andreas Gal :gal]

Attachment: Part 1: Implement MaskSurface for Skia.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 825681 [details] [diff] [review]
Part 1: Implement MaskSurface for Skia.

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +618,5 @@
> +
> +  IntSize size = aMask->GetSize();
> +  Rect rect = Rect(aOffset.x, aOffset.y, size.width, size.height);
> +
> +  mCanvas->drawRect(RectToSkRect(mTransform.TransformBounds(rect)), paint.mPaint);

Don't transform rect here I believe. We want the userspace rect.

Comment 6

[Andreas Gal :gal]

Attachment: Part 2: Clean up Mask implementation a bit

Comment 7

[Andreas Gal :gal]

Attachment: Part 1: Implement MaskSurface for Skia.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 825689 [details] [diff] [review]
Part 1: Implement MaskSurface for Skia.

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

Looks good, assuming it works.

I'd still rather replace this with what I said in comment 3 though.

Comment 9

[Andreas Gal :gal]

Yeah, new bug and patch coming up for that, but lets land this to get vlad moving.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 825688 [details] [diff] [review]
Part 2: Clean up Mask implementation a bit

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +850,5 @@
> +SkRect
> +DrawTargetSkia::SkRectCoveringWholeSurface() const
> +{
> +  Rect rect = Rect(0, 0, mSize.width, mSize.height);
> +  return RectToSkRect(Rect(mTransform * rect.TopLeft(), mTransform * rect.Size()));

Just use TransformBounds. Expanding outwards for non-axis aligned transforms should be ok, and transforming the point/size individually isn't necessarily correct.

Comment 11

[Andreas Gal :gal]

Attachment: Part 2: Clean up Mask implementation a bit (r=mattwoodrow).

Comment 12

[Andreas Gal :gal]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ccea8a7861f9

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: broken rendering

Still no bueno.  Left is with this patch + sw skia, right is what it looks like on d2d trunk (the shadow around when you hover).

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

*poke* Any progress on this?

Comment 15

[Kevin Simons [:kevsim] (Telenor)]

Keep in mind I have no idea what I'm doing, but I poked into this code today and made it work so that the drop shadows on the home page are rendered correctly. Basically I made this change to Andreas' code:

SkPaint maskPaint;
SetPaintPattern(maskPaint, SurfacePattern(aMask, EXTEND_CLAMP));

SkMatrix transform = maskPaint.getShader()->getLocalMatrix();
transform.setTranslate(SkFloatToScalar(aOffset.x), SkFloatToScalar(aOffset.y));
maskPaint.getShader()->setLocalMatrix(transform);

SkLayerRasterizer *raster = new SkLayerRasterizer();
raster->addLayer(maskPaint);

Basically it looks like the layer itself was being translated which clipped off the mask instead of the mask being translate to the appropriate place in the layer.

Comment 16

[Kevin Simons [:kevsim] (Telenor)]

(In reply to Kevin Simons [:kevsim] from comment #15)
> SkMatrix transform = maskPaint.getShader()->getLocalMatrix();
> transform.setTranslate(SkFloatToScalar(aOffset.x), SkFloatToScalar(aOffset.y));

I guess that should be postTranslate in case there's already some translation set?

Comment 17

[Andreas Gal :gal]

Can you upload a patch? The fix looks good to me.

Comment 18

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Implement MaskSurface for Skia.

Comment 19

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Implement MaskSurface for Skia.

Comment 20

[Kevin Simons [:kevsim] (Telenor)]

Attachment: bug933567_masksurface_skia.diff

Comment 21

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Part 1: Implement MaskSurface for Skia (r=mattwoodrow).

Comment 22

[Andreas Gal :gal]

I think we can skip the try server here. This code is currently unused by any of our tests. We will start covering this once we turn on skia for content. Lets land it until then.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e31cf975f22
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a7521fd518

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e31cf975f22
https://hg.mozilla.org/mozilla-central/rev/f6a7521fd518