887927 - SourceSurfaceSkia needs to copy the bitmap when DT is destroyed

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

If we have an outstanding SourceSurfaceSkia referencing a DT that gets destroyed, we will fail to do the readback later on if GetData() is called. SkBitmap is not really independent like it is with software. It is not surprising that it would need the device to be alive in order to do the readback, and I guess it does not hold a strong reference.

Comment 1

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

Attachment: Make a copy of the SkBitmap when DT is destroyed

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 768479 [details] [diff] [review]
Make a copy of the SkBitmap when DT is destroyed

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

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +110,5 @@
>  
>  void
>  SourceSurfaceSkia::DrawTargetDestroyed()
>  {
> +  DrawTargetWillChange();

Can we do this only for SkiaGL?

It works fine for software!

Comment 3

[Matt Woodrow (:mattwoodrow)]

Or even better, can't we just take ownership of the device in the SourceSurfaceSkia?

It's a fairly common pattern to create a draw target, draw into it, snapshot and let the DT die. Having to make a copy when that happens could be expensive.

Comment 4

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

Alright, so I made SourceSurfaceSkia hold a reference to the SkDevice. This made it possible for the SkBitmap to attempt to lock the pixels (where before I assume it just bailed immediately). This doesn't work, though, because the SkDevice relies on the GLContext which is only held in DrawTargetSkia (via a GenericRefPtr member). So once the DrawTargetSkia is gone, the SkDevice (and GrContext, etc) are all effectively busted. I think we'll have to live with a copy when the DT goes away, at least for SkiaGL.

Comment 5

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

I've also tried having SourceSurfaceSkia hold a reference to the GLContext. This would've worked before, but the code has changed such that GLContextSkia now uses the DT to retrieve the GLContext. So we absolutely must have the originating DT be alive for SourceSurfaceSkia to complete a readback (with current code). We can hold a reference to it in SourceSurfaceSkia, but I think the lifetime of the DT now becomes a little more ambiguous than we would like.

I think I would like to just copy the SkBitmap when the DT is destroyed but only when using GL. Unless you are going to just throw the SourceSurface away, you're going to end up doing a readback at some point anyway. For software we can keep it as it is now.

Comment 6

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

Attachment: Copy SkiaSourceSurface bitmap when DT is destroyed if using GL

Comment 7

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

Attachment: Copy SkiaSourceSurface bitmap when DT is destroyed if using GL

Comment 8

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

Matt and I talked some more and agreed that just refing the DT in the SourceSurfaceSkia is better since the performance remains the same regardless of whether or not the callerd destroys the DT (or rather, tries to, since it will remain alive while any outstanding SourceSurfaceSkias are alive).

Comment 9

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

Attachment: Hold a ref to DrawTargetSkia within SourceSurfaceSkia

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/295946bd016a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/295946bd016a