add getTextureHandle to SkSurface

add getTextureHandle to SkSurface

BUG=485243
Committed: https://skia.googlesource.com/skia/+/fa5e68e4b4a10227d3e2c0725b55260175903a80

[reed1, 2015-06-26 21:12:25]

reed@google.com changed reviewers:
+ bsalomon@google.com, fmalita@chromium.org, junov@google.com, kbr@google.com

[reed1, 2015-06-26 21:12:26]

will add tests, but wanted feed back on the api/contract soon.

[bsalomon, 2015-06-26 21:15:22]

otherwise api looks good.

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h#ne...
include/core/SkSurface.h:152: *  If flushPendingWrites then the GrContext will
issue to the backend API any deferred
comment looks incorrect

[reed1, 2015-06-26 21:21:13]

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h#ne...
include/core/SkSurface.h:152: *  If flushPendingWrites then the GrContext will
issue to the backend API any deferred
On 2015/06/26 21:15:22, bsalomon wrote:
> comment looks incorrect

Doh

[reed1, 2015-06-26 21:22:30]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-26 21:22:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210303003/20001

[Justin Novosad, 2015-06-26 21:33:14]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2015-06-26 21:33:15]

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h#ne...
include/core/SkSurface.h:144: *  CAN WE DEPRECATE THIS?
No we can't. This API drives an important optimization: it allows us to skip the
'copy' part of copy-on-write when we know the new content will overwrite the
surface's prior contents. It is a piece of magic in SkDeferredCanvas that has an
important impact on performance.

https://codereview.chromium.org/1210303003/diff/1/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/1210303003/diff/1/src/image/SkSurface_Gpu.cpp...
src/image/SkSurface_Gpu.cpp:29: GrBackendObject
SkSurface_Gpu::onGetTextureHandle(ContentChangeMode mode) {
Where in this method does the genID get bumped? API should allow the the caller
to specify whether the texture is accessed for writing to avoid unnecessarily
bumping the genID.

https://codereview.chromium.org/1210303003/diff/1/src/image/SkSurface_Gpu.cpp...
src/image/SkSurface_Gpu.cpp:33: rt->discard();
What is the use case for this?

[commit-bot: I haz the power, 2015-06-26 21:37:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-26 21:37:08]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-06-26 21:39:38]

fmalita@chromium.org changed reviewers:
- junov@chromium.org

[f(malita), 2015-06-26 21:39:38]

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h#ne...
include/core/SkSurface.h:155: GrBackendObject
getTextureHandle(ContentChangeMode);
Can you also document the lifetime & ownership for this texture/handle?

[reed1, 2015-06-27 17:56:12]

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h#ne...
include/core/SkSurface.h:144: *  CAN WE DEPRECATE THIS?
On 2015/06/26 21:33:14, Justin Novosad wrote:
> No we can't. This API drives an important optimization: it allows us to skip
the
> 'copy' part of copy-on-write when we know the new content will overwrite the
> surface's prior contents. It is a piece of magic in SkDeferredCanvas that has
an
> important impact on performance.
> 

Lets discuss as part of DeferredCanvas discussion

https://codereview.chromium.org/1210303003/diff/1/include/core/SkSurface.h#ne...
include/core/SkSurface.h:155: GrBackendObject
getTextureHandle(ContentChangeMode);
On 2015/06/26 21:39:38, f(malita) wrote:
> Can you also document the lifetime & ownership for this texture/handle?

Done.

https://codereview.chromium.org/1210303003/diff/1/src/image/SkSurface_Gpu.cpp
File src/image/SkSurface_Gpu.cpp (right):

https://codereview.chromium.org/1210303003/diff/1/src/image/SkSurface_Gpu.cpp...
src/image/SkSurface_Gpu.cpp:29: GrBackendObject
SkSurface_Gpu::onGetTextureHandle(ContentChangeMode mode) {
On 2015/06/26 21:33:15, Justin Novosad wrote:
> Where in this method does the genID get bumped? API should allow the the
caller
> to specify whether the texture is accessed for writing to avoid unnecessarily
> bumping the genID.

New enum supports read and write access.

We are not bumping a genID, since there is no "bitmap" behind a gpu-surface (or
there shouldn't be). The way to access a surface's contents is via its snapshot,
and that will be invalidated.

https://codereview.chromium.org/1210303003/diff/1/src/image/SkSurface_Gpu.cpp...
src/image/SkSurface_Gpu.cpp:33: rt->discard();
On 2015/06/26 21:33:15, Justin Novosad wrote:
> What is the use case for this?

Enum has changed, but the idea is to have 2 write access modes, with the option
for the caller to tell us they plan to overwrite the entire backing texture, in
which case we can discard rather than flush any pending writes.

[reed1, 2015-06-27 17:56:23]

ptal

[reed1, 2015-06-27 17:56:27]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-27 17:56:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210303003/40001

[reed1, 2015-06-27 17:57:44]

The CQ bit was unchecked by reed@google.com

[Justin Novosad, 2015-06-27 19:31:36]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2015-06-27 19:31:37]

lgtm with nit.

https://codereview.chromium.org/1210303003/diff/40001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/1210303003/diff/40001/include/core/SkSurface....
include/core/SkSurface.h:149: kDiscardWrite_TextureHandleAccess,  //!< caller
may over-write the entire texture
For discard: "may over-write" -> "will over-write"

[Justin Novosad, 2015-06-29 03:29:12]

Should this be associated with crbug.com/485243?

[reed1, 2015-06-29 14:29:51]

The CQ bit was checked by reed@google.com

[reed1, 2015-06-29 14:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1210303003/#ps60001
(title: "tweak dox")

[commit-bot: I haz the power, 2015-06-29 14:29:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1210303003/60001

[reed1, 2015-06-29 14:30:01]

https://codereview.chromium.org/1210303003/diff/40001/include/core/SkSurface.h
File include/core/SkSurface.h (right):

https://codereview.chromium.org/1210303003/diff/40001/include/core/SkSurface....
include/core/SkSurface.h:149: kDiscardWrite_TextureHandleAccess,  //!< caller
may over-write the entire texture
On 2015/06/27 19:31:37, Justin Novosad wrote:
> For discard: "may over-write" -> "will over-write"

Yes, will add more/stronger dox

[commit-bot: I haz the power, 2015-06-29 14:37:04]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/fa5e68e4b4a10227d3e2c0725b55260175903a80