Bug 82564 – [chromium] Remove SkCanvas::LayerIter use from OpaqueRegionSkia

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Remove SkCanvas::LayerIter use from OpaqueRegionSkia

[Dana Jansens]

This is required for per-tile-painting in the renderer.

[Dana Jansens]

I attempted to do this a few times before I found the LayerIter and felt like we got very solid with it. But I'm sorry to say that it doesn't work with SkPictureRecord, and it is going to  leave the SkCanvas public API so we can't rely on it. 

But I think I'm in a spot where I can do a better job of this now. :)

We have to change any code that does saveLayer() on the canvas underlying the GraphicsContext, to instead do it through the platformContext. And PlatformContextSkia implements imageClipping through the saveLayer()/restore(), so we need to track what it is doing explicitly as well.

[Dana Jansens]

Created attachment 134485 [details]
Patch

[Dana Jansens]

*** Bug 81723 has been marked as a duplicate of this bug. ***

[Stephen White]

Comment on attachment 134485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134485&action=review

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:284
>          &layerPaint,
>          static_cast<SkCanvas::SaveFlags>(SkCanvas::kHasAlphaLayer_SaveFlag |
>                                           SkCanvas::kFullColorLayer_SaveFlag));
> +    platformContext()->didSaveLayer(layerPaint);

Couldn't this call platformContext()->saveLayer() (and skip the "did" here)?

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:292
> +    platformContext()->didRestoreLayer();
>      platformContext()->canvas()->restore();

Same here:  couldn't this call platformContext()->restore()?

[Dana Jansens]

Comment on attachment 134485 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134485&action=review

>> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:284
>> +    platformContext()->didSaveLayer(layerPaint);
> 
> Couldn't this call platformContext()->saveLayer() (and skip the "did" here)?

yes! i added that later.. good idea, will do.

[Dana Jansens]

Created attachment 134676 [details]
Patch

[Dana Jansens]

Created attachment 134712 [details]
Patch

While it is not in practice now null, the SkPaint* for saveLayer() could be null, so fixing this patch to work correctly in the face of that.

FYI I'm working on a followup to use SKDrawLoopers more intelligently instead of just bailing. I say this cuz it's going to mess up the stack in this patch, but it's a similar idea so fair to do this first I think.

[Stephen White]

Comment on attachment 134712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134712&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:107
> +    SkRect m_previousImageBufferClipRect;
> +    SkRect m_totalImageBufferClipRect;
> +    SkRect m_previousImageBufferOpaqueRect;
> +    SkRect m_totalImageBufferOpaqueRect;

Since this seems to be specific to the opaque region tracker, perhaps it should be moved there?

[Dana Jansens]

Created attachment 134897 [details]
Patch

[Dana Jansens]

- Added the bounds to the PlatformContextSkia::saveLayer() method.
- Save the SkPaints in the stack in OpaqueRegionSkia.
- Moved the data for the image clip/mask into the OpaqueRegionSkia stack.
- Fixed the logic around preservesOpaque and added a unit test to verify it.

[Stephen White]

Comment on attachment 134897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134897&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:234
> +    m_opaqueRegion.pushCanvasLayer(paint);

Shouldn't this be guarded by if (m_trackOpaqueRegion)?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:242
> +    m_opaqueRegion.pushCanvasLayer(paint);

Same as above.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:248
> +    m_opaqueRegion.popCanvasLayer();

Same as above.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:277
> +    SkRect opaqueRect = bitmap->isOpaque() ? m_state->m_clip : SkRect::MakeEmpty();
> +    m_opaqueRegion.setImageMask(opaqueRect);

Same as above.

[Dana Jansens]

Created attachment 135239 [details]
Patch

done x4 !

[Stephen White]

Comment on attachment 135239 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Comment on attachment 135239 [details]
Patch

Clearing flags on attachment: 135239

Committed r112980: <http://trac.webkit.org/changeset/112980>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.