gpu: Add CopyTexImage function to GLImage API.

gpu: Add CopyTexImage function to GLImage API.

This provides a mechanism for GLImage implementations to optimize
blit image to texture.

In the case of GLImageMemory, this allows us to upload directly
to the destination texture instead of first uploading to a temporary
texture copying pixels from this texture to the destination texture.

BUG=374962
Committed: https://crrev.com/ce8fbe800ffe1478c43459e2e9341ceb4b7a7001
Cr-Commit-Position: refs/heads/master@{#293404}

[reveman, 2014-09-04 17:05:38]

reveman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2014-09-04 17:32:54]

https://codereview.chromium.org/536353002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/536353002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:137: need_do_bind_tex_image_ = false;
nit: move this to DoBindTexImage and save a line. (conceptually, it makes sense
that DoBindTexImage resets the state).

https://codereview.chromium.org/536353002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:157: memory_);
If the data was already uploaded to the source texture, a GPU->GPU copy will be
more efficient than a new upload. Is there a way to detect this? E.g. if
need_do_bind_tex_image_ is false, fail the copy, so that the calling code will
fallback.

BTW, why do we need the copy in the first place?

[reveman, 2014-09-04 18:57:43]

FYI, my plan is to make some minor modifications to the one-copy rasterizer in
cc following this that will make us stream copy operations at a similar rate as
we perform raster. This should work much better than async uploads on ChromeOS
as we don't have to worry about idle uploads being starved.

I think it's possible that this might also be an improvement on Android even
without using SurfaceTextures. That would allow us to remove all async upload
related code in favor of the much simpler zero-copy and one-copy systems.

https://codereview.chromium.org/536353002/diff/1/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/536353002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:137: need_do_bind_tex_image_ = false;
On 2014/09/04 17:32:54, piman (OOO) wrote:
> nit: move this to DoBindTexImage and save a line. (conceptually, it makes
sense
> that DoBindTexImage resets the state).

Done.

https://codereview.chromium.org/536353002/diff/1/ui/gl/gl_image_memory.cc#new...
ui/gl/gl_image_memory.cc:157: memory_);
On 2014/09/04 17:32:54, piman (OOO) wrote:
> If the data was already uploaded to the source texture, a GPU->GPU copy will
be
> more efficient than a new upload. Is there a way to detect this? E.g. if
> need_do_bind_tex_image_ is false, fail the copy, so that the calling code will
> fallback.

Good point. I made this work by tracking if the image is bound to a texture
using |target_| and in that case only upload directly to destination texture if
need_do_bind_tex_image_ is set.

> 
> BTW, why do we need the copy in the first place?

You mean instead of just using the image for zero-copy? This alternative
one-copy approach is useful for 2 reasons:

1. Easier synchronization if images are used in a fixed sequence in a staging
pool as we only have to worry about the copy operations completing on the GPU.
Doesn't matter in the software case but useful with SurfaceTextures where the
synchronization mechanism is not exactly what we want.

2. Gives us control over the amount of staging resources and zero-copy buffers
that are used. On some systems it might be too expensive to use one zero-copy
buffer for each tile resource. ie. file descriptor limits on Android. In the
software case, it's shared memory objects and having a shared memory backing for
each tile texture is not really an option. This allows us to use a number of
images as staging buffers for uploads, similar to async uploads but this gives
us much better control over the amount of staging memory.

[piman, 2014-09-04 19:11:10]

https://codereview.chromium.org/536353002/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/536353002/diff/20001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:128: DCHECK(!target_);
I don't think we (currently) raise an error if the client binds the image to 2
textures. Should we return false instead if target_ is set?
Do we need consistency between the image types?

[reveman, 2014-09-04 20:12:44]

PTAL

https://codereview.chromium.org/536353002/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/536353002/diff/20001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:128: DCHECK(!target_);
On 2014/09/04 19:11:10, piman (OOO) wrote:
> I don't think we (currently) raise an error if the client binds the image to 2
> textures. Should we return false instead if target_ is set?
> Do we need consistency between the image types?

Ah, right. We should probably add some unit tests for binding the image to
multiple textures if we want to keep supporting that. I'll do that in a separate
patch.

I made this return false if the target is different. Similar to EGL version.
That means binding of image to multiple textures is limited to the same target
for this implementation. Maybe we can improve that if necessary by changing the
Will/DidUseTexImage API.

https://codereview.chromium.org/536353002/diff/20001/ui/gl/gl_image_memory.cc...
ui/gl/gl_image_memory.cc:156: return false;
I prefer to remove this as there's not really a way we can know from here that
the caller is interested in copying the data from a texture that this image is
currently bound to. We can revisit this later if it becomes a use case we need
to support efficiently. For now we assume that the client is not interested in
using the image both for sampling and and copying out of.

[piman, 2014-09-04 20:58:05]

OK, LGTM

[reveman, 2014-09-04 21:03:24]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-04 21:07:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/536353002/40001

[commit-bot: I haz the power, 2014-09-05 02:21:06]

Committed patchset #3 (id:40001) as 6c6b5970fa2d19b7a7d10e0f2586ce75c1ecd78a

[no sievers, 2014-09-05 21:06:08]

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

[no sievers, 2014-09-05 21:06:09]

https://codereview.chromium.org/536353002/diff/40001/content/common/gpu/strea...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/536353002/diff/40001/content/common/gpu/strea...
content/common/gpu/stream_texture_android.cc:206: NOTREACHED();
Don't we trigger this from
WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture()?

https://codereview.chromium.org/536353002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/stream_texture_manager_in_process_android.cc
(right):

https://codereview.chromium.org/536353002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/stream_texture_manager_in_process_android.cc:77:
NOTREACHED();
same here for webview

[reveman, 2014-09-06 04:02:57]

https://codereview.chromium.org/536353002/diff/40001/content/common/gpu/strea...
File content/common/gpu/stream_texture_android.cc (right):

https://codereview.chromium.org/536353002/diff/40001/content/common/gpu/strea...
content/common/gpu/stream_texture_android.cc:206: NOTREACHED();
On 2014/09/05 21:06:09, sievers wrote:
> Don't we trigger this from
> WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture()?

Ah, maybe. I just assumed it would not based on BindTexImage never being called.
Maybe we should allow Bind/ReleaseTexImage to be called too for consistency? And
actually call them if these images are being bound to textures as other images.

[no sievers, 2014-09-08 17:19:01]

https://codereview.chromium.org/536353002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/texture_definition.cc (right):

https://codereview.chromium.org/536353002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/texture_definition.cc:88: NOTREACHED();
This one is called too since it's used for webgl buffers with android webview.

[no sievers, 2014-09-08 17:22:44]

On 2014/09/06 04:02:57, reveman wrote:
>
https://codereview.chromium.org/536353002/diff/40001/content/common/gpu/strea...
> File content/common/gpu/stream_texture_android.cc (right):
> 
>
https://codereview.chromium.org/536353002/diff/40001/content/common/gpu/strea...
> content/common/gpu/stream_texture_android.cc:206: NOTREACHED();
> On 2014/09/05 21:06:09, sievers wrote:
> > Don't we trigger this from
> > WebMediaPlayerAndroid::copyVideoTextureToPlatformTexture()?
> 
> Ah, maybe. I just assumed it would not based on BindTexImage never being
called.
> Maybe we should allow Bind/ReleaseTexImage to be called too for consistency?
And
> actually call them if these images are being bound to textures as other
images.

Yea that makes sense. They can just return true for now.

Btw this would have shown on the gpu bot at least for stream_texture_android.cc
if the only ones that exercise this path weren't currently disabled due to
crbug.com/334204. Test coverage for the other Webview paths is also in the
works.

[commit-bot: I haz the power, 2014-09-10 03:35:42]

Patchset 3 (id:??) landed as
https://crrev.com/ce8fbe800ffe1478c43459e2e9341ceb4b7a7001
Cr-Commit-Position: refs/heads/master@{#293404}