1239040 - Implement PushLayer/PopLayer for DrawTargetSkia

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: part 1 - allow usage of SkCanvas::getTopDevice in Skia

We need to use SkCanvas::getTopDevice() to access the topmost layer in the canvas for copying the background and masking. Skia considers this legacy so we need to explicitly enable it.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: part 2 - cleanup of DrawTargetSkia GetBitmapForSurface to use installPixels

The old TempBitmap setup was cumbersome and only really appropriate if used within a single function. However, we need bitmaps to last beyond the scope of PushLayer until they finally get used in PopLayer.

So instead this uses the existing release function override mechanism in SkPixelRef - we're already paying for it, so may as well use it. It allows the underlying source surface to be released when the pixelref's refcount zeroes out, which works better for our purposes here. It also cleans up the code a lot.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: part 3 - cleanup of DrawTargetSkia Mask and MaskSurface

These are some drive-by cleanups of Mask and MaskSurface that I did as I was investigating Skia for PushLayer.

We don't really need to be creating local matrix shaders for each pattern, since all shaders allow the specifying of one on creation.

SkRectCoveringWholeSurface() is just unnecessary because drawPaint already does it.

Using SkLayerRasterizer doesn't make much sense if we already know we're dealing with a source surface. So instead we can just use extractAlpha on the bitmap when it's not the correct mask format, and otherwise sense all MaskSurface requests through the same path. It will still incur a copy in the non-A8 case, but will reduce some allocations and branches taken for path rendering that we don't actually need there.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: part 4 - implement PushLayer for DrawTargetSkia

Patch does what it says on the tin.

Some trickery is required to deal with masks, since Skia does not have any simple support for that within saveLayer. So instead we're forced to steal a reference to the layer device before we restore the background. While restoring, we make sure the layer is not implicitly blended back by forcing the opacity to 0 temporarily. Finally, we draw the layer device to the background with our custom mask.

In the non-masked case this boils down to just simple usage of SkCanvas::saveLayer and SkCanvas::restore without the above trickery.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: part 4 - implement PushLayer for DrawTargetSkia

Small fix for setting the background copying xfer mode.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: part 5 - cleanup of DrawTargetSkia CopySurface to avoid accessing bottom layer directly

This mainly fixes CopySurface to avoid accessing properties of the bottom layer so that it is safe to use inside PushLayer.

There are some other drive-by cleanups in this patch for clearing and drawSprite removal.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: part 6 - fix DrawTargetCairo/DrawTargetSkia LockBits to work inside PushLayer

The widget gtk code was trying to use LockBits (and also BorrowedXlibDrawable) inside PushLayer when rendering native widgets under certain circumstances. This never worked and has basically been broken since Bas' implemented PushLayer.

This fixes it to do saner things in both Skia and Cairo.

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: part 6 - fix DrawTargetSkia LockBits to work inside PushLayer

Separated out the DrawTargetCairo bits so they can be fixed in the relevant bug report 1239106. This patch now only deals with DrawTargetSkia.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: part 6 - fix DrawTargetCairo/DrawTargetSkia LockBits and BorrowedXlibDrawable to work inside PushLayer

Okay, one more re-roll of this... Upon further consideration it just made more sense to let LockBits to optionally report an origin for the data, without disturbing any existing use-cases (since they don't request it). BorrowedXlibDrawable is also similarly easily modified.

This way we don't have any performance regressions vs. the non-native-pushlayer paths when rendering widgets and keep them rendering through the same backend bits in widget/gtk.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: part 5 - cleanup of DrawTargetSkia CopySurface to avoid accessing bottom layer directly

Revert the drawSprite -> drawBitmap conversion as that will cause a performance regression until we use Skia m49+.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8707031 [details] [diff] [review]
part 2 - cleanup of DrawTargetSkia GetBitmapForSurface to use installPixels

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

Nice.

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: part 6 - fix DrawTargetCairo/DrawTargetSkia LockBits and BorrowedXlibDrawable to work inside PushLayer

This adds in support for LockBits under PushLayer in DrawTargetCairo rather than failing.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 8707301 [details] [diff] [review]
part 4 - implement PushLayer for DrawTargetSkia

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +1069,5 @@
> +      SkShader* shader = SkShader::CreateBitmapShader(layerBitmap,
> +                                                      SkShader::kClamp_TileMode,
> +                                                      SkShader::kClamp_TileMode,
> +                                                      &layerMat);
> +      SkSafeUnref(paint.setShader(shader));

nit: Wow this is an ugly pattern.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8707907 [details] [diff] [review]
part 6 - fix DrawTargetCairo/DrawTargetSkia LockBits and BorrowedXlibDrawable to work inside PushLayer

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

Other than the aOrigin default parameter business this looks fine.

::: gfx/2d/2D.h
@@ +746,5 @@
>     * Release takes the original data pointer for safety.
>     */
>    virtual bool LockBits(uint8_t** aData, IntSize* aSize,
> +                        int32_t* aStride, SurfaceFormat* aFormat,
> +                        IntPoint* aOrigin = nullptr) { return false; }

I'd rather not have aOrigin have a default parameter. I don't think there are many cases where you want to ignore the result.

Comment 15

[Bas Schouten (:bas.schouten)]

Comment on attachment 8707907 [details] [diff] [review]
part 6 - fix DrawTargetCairo/DrawTargetSkia LockBits and BorrowedXlibDrawable to work inside PushLayer

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

::: gfx/2d/2D.h
@@ +746,5 @@
>     * Release takes the original data pointer for safety.
>     */
>    virtual bool LockBits(uint8_t** aData, IntSize* aSize,
> +                        int32_t* aStride, SurfaceFormat* aFormat,
> +                        IntPoint* aOrigin = nullptr) { return false; }

I wonder if just to make sure we're not having other unexpected behavior we want to specify the bounds the caller can draw to so they can optionally check if they're sufficient to do what they want.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8707907 [details] [diff] [review]
part 6 - fix DrawTargetCairo/DrawTargetSkia LockBits and BorrowedXlibDrawable to work inside PushLayer

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

I changed my mind about the default parameter business.

::: gfx/2d/2D.h
@@ +746,5 @@
>     * Release takes the original data pointer for safety.
>     */
>    virtual bool LockBits(uint8_t** aData, IntSize* aSize,
> +                        int32_t* aStride, SurfaceFormat* aFormat,
> +                        IntPoint* aOrigin = nullptr) { return false; }

I'd rather not have aOrigin have a default parameter. I don't think there are many cases where you want to ignore the result.

Comment 17

[Lee Salzman [:lsalzman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e035d0616a3f

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b9f8f265db
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52b5d9ac763
https://hg.mozilla.org/integration/mozilla-inbound/rev/0798566a8d00
https://hg.mozilla.org/integration/mozilla-inbound/rev/a32088113b26
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcdab1d4fda
https://hg.mozilla.org/integration/mozilla-inbound/rev/606793e62820

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4b9f8f265db
https://hg.mozilla.org/mozilla-central/rev/f52b5d9ac763
https://hg.mozilla.org/mozilla-central/rev/0798566a8d00
https://hg.mozilla.org/mozilla-central/rev/a32088113b26
https://hg.mozilla.org/mozilla-central/rev/9bcdab1d4fda
https://hg.mozilla.org/mozilla-central/rev/606793e62820