1046918 - [Skia] Remove Moz2D requirement for SK_SUPPORT_LEGACY_GETDEVICE

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[George Wright (:gw280) (needinfo me!)]

getDevice() on SkCanvas is no longer supported.

Comment 1

[Walter Litwinczyk [:walter]]

Attachment: skia_bug_legacy_get_device

Try: https://tbpl.mozilla.org/?tree=Try&rev=2f9999a8860a

Comment 2

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8474636 [details] [diff] [review]
skia_bug_legacy_get_device

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

I don't think this is the direction we want to go in.

Snorp - correct me if I'm wrong, but if I remember correctly the reason we accessBitmap on this is to allow for us to lazily have access to the pixel data associated with the DrawTarget in the case of a GPU target, right? We get to store the SkBitmap from the device, and then when we lockPixels or copyTo (triggered by GetData() or DrawTargetWillChange()), that causes a readPixels to occur so we don't needlessly cause a GPU readback.

I *think* therefore that the correct thing to do here is to:

- Store the SkCanvas object as mCanvas on the SourceSurfaceSkia
- Modify GetPixels/DrawTargetWillChange to use peekPixels and readPixels instead
- Explicitly document this behaviour because it's not obvious what's currently going on here

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to George Wright (:gw280) from comment #2)
> Comment on attachment 8474636 [details] [diff] [review]
> skia_bug_legacy_get_device
> 
> Review of attachment 8474636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this is the direction we want to go in.
> 
> Snorp - correct me if I'm wrong, but if I remember correctly the reason we
> accessBitmap on this is to allow for us to lazily have access to the pixel
> data associated with the DrawTarget in the case of a GPU target, right? We
> get to store the SkBitmap from the device, and then when we lockPixels or
> copyTo (triggered by GetData() or DrawTargetWillChange()), that causes a
> readPixels to occur so we don't needlessly cause a GPU readback.
> 
> I *think* therefore that the correct thing to do here is to:
> 
> - Store the SkCanvas object as mCanvas on the SourceSurfaceSkia
> - Modify GetPixels/DrawTargetWillChange to use peekPixels and readPixels
> instead
> - Explicitly document this behaviour because it's not obvious what's
> currently going on here

As I recall, yes, this is what is going on. Your plan there sounds fine.

Comment 4

[Walter Litwinczyk [:walter]]

Attachment: skia_bug_legacy_get_device

So I have no idea what I'm doing. I imagine the SkCanvas needs to be ref counted, it does implement SkRefCnt, but it appears there's another RefPtr in the code, should I be using that or the skia stuff?

Is this what you had in mind, I really am not sure how this impacts things.

Comment 5

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8490462 [details] [diff] [review]
skia_bug_legacy_get_device

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

You may want to check DrawTargetSkia to see how the refcounting for SkCanvas is handled there.

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +53,4 @@
>    mFormat = aFormat;
>  
>    mSize = IntSize(size.fWidth, size.fHeight);
>    mStride = mBitmap.rowBytes();

mBitmap was just reset, and I suspect we'll need to set it up to have an SkImageInfo that matches aCanvas' size and aFormat

@@ +112,2 @@
>      mBitmap.reset();
> +    if (!mCanvas->readPixels(&mBitmap, 0, 0)) {

This shouldn't fail unless something we can't really handle has happened, I think. I think a simple warning message would suffice.

::: gfx/2d/SourceSurfaceSkia.h
@@ +47,5 @@
>  
>    void DrawTargetWillChange();
>    void MaybeUnlock();
>  
> +  SkCanvas* mCanvas;

RefPtrSkia<SkCanvas>

Comment 6

[Walter Litwinczyk [:walter]]

Attachment: skia_bug_legacy_get_device

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.