SkImage::NewFromYUVTexturesCopy

SkImage::NewFromYUVTexturesCopy

Committed: https://skia.googlesource.com/skia/+/993a4216a6014b9de8f4d8120360c94550dc6761

[bsalomon, 2015-05-27 04:19:14]

bsalomon@google.com changed reviewers:
+ dcastagna@chromium.org, reed@google.com

[bsalomon, 2015-05-27 04:20:52]

Mike, this also adds needed functionality for getting access to the
GrGLInterface in GMs in order to test external GL calls.

[Daniele Castagna, 2015-05-27 05:28:56]

Thank you for working on this!

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

https://codereview.chromium.org/1149553002/diff/40001/include/core/SkImage.h#...
include/core/SkImage.h:90: static SkImage* NewFromYUVTexturesCopy(GrContext*,
SkYUVColorSpace,
Can we also have NewFromYUVTexture that behaves similarly to NewFromTexture
(safe to delete textures after draw/flush when not drawing to a SkPicture)?

In that way we could avoid a copy in SkCanvasVideoRenderer::Paint.

https://codereview.chromium.org/1149553002/diff/40001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1149553002/diff/40001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:219: SkAutoTUnref<GrTexture>
dst(ctx->textureProvider()->createTexture(
Should this be refScratchTexture?
We'd be requesting a texture with the same SurfaceDescription more than once,
it'd be nice to avoid reallocating the storage.

[Daniele Castagna, 2015-05-27 05:28:58]

Thank you for working on this!

[bsalomon, 2015-05-27 13:29:59]

On 2015/05/27 05:28:58, Daniele Castagna wrote:
> Thank you for working on this!

Of course! This image constructor will always consume the yuv textures
immediately so you should be able to delete/recycle them as soon as it returns.

So given that the try jobs showed no recording canvases, it seems like at
present you wouldn't make use of a callback when a external ID GrTexure is
destroyed, right?

[Daniele Castagna, 2015-05-27 16:33:28]

On 2015/05/27 at 13:29:59, bsalomon wrote:
> On 2015/05/27 05:28:58, Daniele Castagna wrote:
> > Thank you for working on this!
> 
> Of course! This image constructor will always consume the yuv textures
immediately so you should be able to delete/recycle them as soon as it returns.
> 
> So given that the try jobs showed no recording canvases, it seems like at
present you wouldn't make use of a callback when a external ID GrTexure is
destroyed, right?

You're right.
So what I'm asking is: can we avoid copying to an intermediate texture knowing
that? Can we directly draw to the canvas?
I was thinking of something like:

SkImage* yuv = SkImage::NewFromYUVTextures(ctx, s, backendObjects, sizes,
origin);  // Doesn't create a new textures and doesn't draw on a context.
canvas->drawImage(yuv, 0, 0, &paint);  // Draw on the GPU canvas using using
GrTextures wrapping backendObjects.
SkSafeSetNull(yuv);
glDeleteTextures(3, texture_ids);  // Delete the textures backing yuv SkImage.


This makes sense assuming the try bots are exhaustively testing
SkCanvasVideoRenderer::Paint, it might not be the case. :/

[bsalomon, 2015-05-27 16:46:41]

So what you're asking for is indeed possible. I'd probably want to defer it to a
later CL.

In that case I think we'd want to add the callback to know when the yuvs are
safe to delete. One thing that's not clear: should we convert on every draw or
do it once to an rgba texture and then reuse that.

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

https://codereview.chromium.org/1149553002/diff/40001/include/core/SkImage.h#...
include/core/SkImage.h:90: static SkImage* NewFromYUVTexturesCopy(GrContext*,
SkYUVColorSpace,
On 2015/05/27 05:28:56, Daniele Castagna wrote:
> Can we also have NewFromYUVTexture that behaves similarly to NewFromTexture
> (safe to delete textures after draw/flush when not drawing to a SkPicture)?
> 
> In that way we could avoid a copy in SkCanvasVideoRenderer::Paint.

https://codereview.chromium.org/1149553002/diff/40001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1149553002/diff/40001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:219: SkAutoTUnref<GrTexture>
dst(ctx->textureProvider()->createTexture(
On 2015/05/27 05:28:56, Daniele Castagna wrote:
> Should this be refScratchTexture?
> We'd be requesting a texture with the same SurfaceDescription more than once,
> it'd be nice to avoid reallocating the storage.

Yes you're right. Will fix. I want createTexture to internally use scratch
textures, but that's a larger fix.

[Daniele Castagna, 2015-05-27 17:04:05]

On 2015/05/27 at 16:46:41, bsalomon wrote:
> So what you're asking for is indeed possible. I'd probably want to defer it to
a later CL.
> 
SGTM.

> In that case I think we'd want to add the callback to know when the yuvs are
safe to delete. One thing that's not clear: should we convert on every draw or
do it once to an rgba texture and then reuse that.

The compositor is converting on the GPU on every draw. YUV textures with
subsampling are smaller than RGB textures. On platforms where we're
bandwidth-constrained converting might be faster than drawing an already
converted texture.

> 
> https://codereview.chromium.org/1149553002/diff/40001/include/core/SkImage.h
> File include/core/SkImage.h (right):
> 
>
https://codereview.chromium.org/1149553002/diff/40001/include/core/SkImage.h#...
> include/core/SkImage.h:90: static SkImage* NewFromYUVTexturesCopy(GrContext*,
SkYUVColorSpace,
> On 2015/05/27 05:28:56, Daniele Castagna wrote:
> > Can we also have NewFromYUVTexture that behaves similarly to NewFromTexture
> > (safe to delete textures after draw/flush when not drawing to a SkPicture)?
> > 
> > In that way we could avoid a copy in SkCanvasVideoRenderer::Paint.
> 
>
https://codereview.chromium.org/1149553002/diff/40001/src/image/SkImage_Gpu.cpp
> File src/image/SkImage_Gpu.cpp (right):
> 
>
https://codereview.chromium.org/1149553002/diff/40001/src/image/SkImage_Gpu.c...
> src/image/SkImage_Gpu.cpp:219: SkAutoTUnref<GrTexture>
dst(ctx->textureProvider()->createTexture(
> On 2015/05/27 05:28:56, Daniele Castagna wrote:
> > Should this be refScratchTexture?
> > We'd be requesting a texture with the same SurfaceDescription more than
once,
> > it'd be nice to avoid reallocating the storage.
> 
> Yes you're right. Will fix. I want createTexture to internally use scratch
textures, but that's a larger fix.

[bsalomon, 2015-05-29 15:03:09]

Mike can you take a look?

[reed1, 2015-05-29 16:17:46]

lgtm

[bsalomon, 2015-05-29 17:15:59]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-05-29 17:16:15]

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

[commit-bot: I haz the power, 2015-05-29 17:18:02]

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

[commit-bot: I haz the power, 2015-05-29 17:18:03]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[bsalomon, 2015-05-29 18:01:44]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-05-29 18:01:45]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1149553002/#ps80001
(title: "fix unused var")

[commit-bot: I haz the power, 2015-05-29 18:02:03]

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

[commit-bot: I haz the power, 2015-05-29 18:03:54]

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

[commit-bot: I haz the power, 2015-05-29 18:03:55]

Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[bsalomon, 2015-05-29 18:24:00]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-05-29 18:24:00]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1149553002/#ps100001
(title: "fix warnings")

[commit-bot: I haz the power, 2015-05-29 18:24:14]

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

[commit-bot: I haz the power, 2015-05-29 18:37:32]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/993a4216a6014b9de8f4d8120360c94550dc6761