Closed Bug 933567 Opened 6 years ago Closed 6 years ago

Implement MaskSurface for DrawTargetSkia

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: gal, Assigned: kevin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(3 files, 8 obsolete files)

Attached patch Part 1: Implement MaskSurface (obsolete) — Splinter Review
No description provided.
Attachment #825668 - Attachment is patch: true
Attachment #825668 - Attachment mime type: text/x-patch → text/plain
Attachment #825668 - Flags: review?(matt.woodrow)
Attachment #825668 - Attachment description: patch → Part 1: Implement MaskSurface
Assignee: nobody → gal
Attachment #825670 - Flags: review?(matt.woodrow)
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
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.
Attachment #825668 - Attachment is obsolete: true
Attachment #825668 - Flags: review?(matt.woodrow)
Attachment #825681 - Flags: review?(matt.woodrow)
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.
Attachment #825670 - Attachment is obsolete: true
Attachment #825670 - Flags: review?(matt.woodrow)
Attachment #825688 - Flags: review?(matt.woodrow)
Attachment #825681 - Attachment is obsolete: true
Attachment #825681 - Flags: review?(matt.woodrow)
Attachment #825689 - Flags: review?(matt.woodrow)
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.
Attachment #825689 - Flags: review?(matt.woodrow) → review+
Yeah, new bug and patch coming up for that, but lets land this to get vlad moving.
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.
Attachment #825688 - Flags: review?(matt.woodrow) → review+
Blocks: 933577
Attachment #825688 - Attachment is obsolete: true
Attachment #825696 - Flags: review+
Attached image 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).
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.
(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?
Can you upload a patch? The fix looks good to me.
Assignee: gal → kevin
Attachment #825689 - Attachment is obsolete: true
Attachment #8336295 - Flags: review?(matt.woodrow)
Attachment #8336295 - Attachment is obsolete: true
Attachment #8336295 - Flags: review?(matt.woodrow)
Attachment #8336300 - Flags: review?(matt.woodrow)
Attachment #8336300 - Flags: review?(matt.woodrow) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch bug933567_masksurface_skia.diff (obsolete) — Splinter Review
Attachment #8336300 - Attachment is obsolete: true
Attachment #8336795 - Flags: review?(matt.woodrow)
Attachment #8336795 - Attachment is obsolete: true
Attachment #8336795 - Flags: review?(matt.woodrow)
Attachment #8336798 - Flags: review?(matt.woodrow)
Attachment #8336798 - Flags: review?(matt.woodrow) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e31cf975f22
https://hg.mozilla.org/mozilla-central/rev/f6a7521fd518
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.