Closed Bug 1240177 Opened 4 years ago Closed 4 years ago

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


(Core :: Graphics, defect)

Not set



Tracking Status
firefox46 --- fixed
firefox47 --- fixed


(Reporter: lsalzman, Assigned: lsalzman)




(5 files, 1 obsolete file)

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.'
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.
Attachment #8708547 - Flags: review?(jmuizelaar)
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.
Attachment #8708547 - Flags: review?(jmuizelaar) → review-
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.
Attachment #8708547 - Attachment is obsolete: true
Attachment #8710055 - Flags: review?(jmuizelaar)
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.
Attachment #8710056 - Flags: review?(jmuizelaar)
Newer version of the original OptimizeSourceSurface patch that makes use of the cleanups.
Attachment #8710057 - Flags: review?(jmuizelaar)
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.
Attachment #8710061 - Flags: review?(jmuizelaar)
Attachment #8710055 - Flags: review?(jmuizelaar) → review+
Attachment #8710056 - Flags: review?(jmuizelaar) → review+
Attachment #8710057 - Flags: review?(jmuizelaar) → review+
Attachment #8710061 - Flags: review?(jmuizelaar) → review+
Duplicate of this bug: 1220474
Depends on: 1244228
Blocks: 1256577
Follow-up fix for issue noticed by Matt Woodrow that it should only be doing the copyTo if the deepCopyTo fails.
Attachment #8740266 - Flags: review?(matt.woodrow)
Attachment #8740266 - Flags: review?(matt.woodrow) → review+
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
Attachment #8740266 - Flags: approval-mozilla-beta?
Attachment #8740266 - Flags: approval-mozilla-aurora?
Changing Fx46 status from fixed to affected otherwise this won't show up on Sheriffs-To-Land queries.
Comment on attachment 8740266 [details] [diff] [review]
fix SourceSurfaceSkia::DrawTargetWillChange deepCopyTo usage

Fixes a recent regression, seems safe, Aurora47+
Attachment #8740266 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8740266 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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 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.
Attachment #8740266 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.