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

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lsalzman, Assigned: lsalzman)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.'
(Assignee)

Comment 1

3 years ago
Created attachment 8708547 [details] [diff] [review]
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.
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-
(Assignee)

Comment 3

3 years ago
Created attachment 8710055 [details] [diff] [review]
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.
Attachment #8708547 - Attachment is obsolete: true
Attachment #8710055 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

3 years ago
Created attachment 8710056 [details] [diff] [review]
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.
Attachment #8710056 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

3 years ago
Created attachment 8710057 [details] [diff] [review]
part 3 - make DrawTargetSkia::OptimizeSourceSurface directly upload to GPU textures

Newer version of the original OptimizeSourceSurface patch that makes use of the cleanups.
Attachment #8710057 - Flags: review?(jmuizelaar)
(Assignee)

Comment 6

3 years ago
Created attachment 8710061 [details] [diff] [review]
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.
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+

Comment 9

3 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1220474
Depends on: 1244228
Blocks: 1256577
(Assignee)

Comment 11

2 years ago
Created attachment 8740266 [details] [diff] [review]
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.
Attachment #8740266 - Flags: review?(matt.woodrow)
Attachment #8740266 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 14

2 years ago
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.
status-firefox46: fixed → affected
status-firefox47: --- → affected
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.
status-firefox46: affected → wontfix
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.
status-firefox46: wontfix → affected
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.