1240177 - DrawTargetSkia::OptimizeSourceSurface does not directly upload data to GPU textures

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

We currently rely on the odd Skia caching behavior of SkBitmap to make OptimizeSourceSurface work for Skia GPU. It currently creates a new copy of the data to live in the SkBitmap, and eventually Skia will upload the data to the GPU when a canvas operation is done upon it, caching the resulting the texture. This makes the upload happen at unpredictable times as well as incurs the memory cost of always keeping a copy of the data around, even though we don't need it.

As per discussion with Bas on irc:
<Bas> There's 3 guarantees I want: 'After OptimizeSourceSurface, as soon as the last reference to the original surface is released there should be no more data held in RAM when GPU drawing is used.' 'Map can always either fail or make the pixel data available fast to a caller.' 'GetDataSurface always succeeds, if called on a surface whose pixels are only stored in VRAM it will incur the performance penalty of reading that data back. The actually blocking for this -can- be done in the first subsequent map call on the resulting data source surface to facilitate asynchronous readback.'

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: make DrawTargetSkia::OptimizeSourceSurface directly upload to GPU textures

This patch ensures that the upload of data to a GPU texture happens in OptimizeSourceSurface itself, so that the data does not need to be kept around.

It also fixes a texture leak that was occurring inside SourceSurfaceSkia::InitFromTexture where the GrTexture and SkPixelRef did not have their initial references properly managed.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8708547 [details] [diff] [review]
make DrawTargetSkia::OptimizeSourceSurface directly upload to GPU textures

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

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +93,5 @@
> +bool
> +SourceSurfaceSkia::InitFromGrTexture(DrawTargetSkia* aOwner,
> +                                     GrTexture* aTexture,
> +                                     const IntSize &aSize,
> +                                     SurfaceFormat aFormat)

I don't know why InitFromTexture takes a DrawTargetSkia as a parameter but I suspect you should just drop this parameter and leave mDrawTarget as null. This will avoid the const_cast that you do in the caller.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: part 1 - add helper function to make Skia image info

Some much-needed helper functionality to avoid having to do gfx-to-skia format conversions all over the place. This makes the code easier to read and also makes our alpha format checks consistent.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: part 2 - cleanups of DrawTargetSkia and SourceSurfaceSkia creation for GPU contexts

This cleans up and fixes a few bugs in the creation of GPU draw targets.

It avoids keeping the DrawTargetSkia::mTexture value around since Skia doesn't guarantee this is always unchanged and prefers we access it via the accessRenderTarget in GetNativeSurface.

It fixes CreateSimilarDrawTarget to preserve GPU-ness.

It fixes/cleans DrawTargetSkia::InitWithGrContext to ensure the texture is cleared, so it behaves more similarly to Init.

It fixes two leaks: both the GrTexture and SkGrPixelRef were being leaked when SourceSurfaceSkia was being created due to neither being properly managed/unref'd.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: part 3 - make DrawTargetSkia::OptimizeSourceSurface directly upload to GPU textures

Newer version of the original OptimizeSourceSurface patch that makes use of the cleanups.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: part 4 - avoid GPU readbacks in SourceSurfaceSkia::DrawTargetWillChange

Incidental finding - if the snapshot happens to be backed by a GPU texture, then calling copyTo on it will cause a readback/stall which we don't really want. This tries to use deepCopyTo first which will preserve GPU-ness, and then falls back to copyTo if that didn't succeed.

Comment 7

[Lee Salzman [:lsalzman]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75dc927ca397

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27b47b5ed8f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/166723c9c3e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/58f157683d4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/36bd96c1cd13

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/27b47b5ed8f3
https://hg.mozilla.org/mozilla-central/rev/166723c9c3e3
https://hg.mozilla.org/mozilla-central/rev/58f157683d4b
https://hg.mozilla.org/mozilla-central/rev/36bd96c1cd13

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: fix SourceSurfaceSkia::DrawTargetWillChange deepCopyTo usage

Follow-up fix for issue noticed by Matt Woodrow that it should only be doing the copyTo if the deepCopyTo fails.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12f6413904b1

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12f6413904b1

Comment 14

[Lee Salzman [:lsalzman]]

Comment on attachment 8740266 [details] [diff] [review]
fix SourceSurfaceSkia::DrawTargetWillChange deepCopyTo usage

Approval Request Comment
[Feature/regressing bug #]: 1240177
[User impact if declined]: Possible performance losses (moreso on OS X and Android) because of unintentional readbacks of rendering snapshots and double-copying where none was initially intended.
[Describe test coverage new/current, TreeHerder]: mochitests, reftests
[Risks and why]: Low risk, this code is not stressed in many places and this patch makes the code more correct by removing an unintentional error in the original patches for this bug.
[String/UUID change made/needed]: None

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Changing Fx46 status from fixed to affected otherwise this won't show up on Sheriffs-To-Land queries.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8740266 [details] [diff] [review]
fix SourceSurfaceSkia::DrawTargetWillChange deepCopyTo usage

Fixes a recent regression, seems safe, Aurora47+

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

This sounds good, but I don't want to land this so late in beta (heading into beta 11 Friday, with the RC build on Monday).  We would not have a lot of time for backouts (and not much time to detect regressions).  Let's keep this improvement with 47.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

After talking with Lee I realized most of this work already landed in 46 in January. So we did regress performance on 46 and the fix is in the one patch that needs to uplift.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8740266 [details] [diff] [review]
fix SourceSurfaceSkia::DrawTargetWillChange deepCopyTo usage

Reconsidered, let's uplift this so that we don't regress performance on mac and android for 46.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4d417a8b34c
 https://hg.mozilla.org/releases/mozilla-beta/rev/ba6b68e8b767