gpu: Add async upload functions.

gpu: Add async upload functions.

Just adding the API first to unblock other CLs and so
we can start to use the API. Async tasks are just
done synchronously, and the completion query is set
immediately when it executes in the GPU process.

BUG=161337
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170516

[reveman, 2012-11-29 00:25:23]

https://codereview.chromium.org/11412232/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3690: if (!pixels) {
this won't work. |pixels| is an offset when a transfer buffer is bound and that
offset is always 0 with our current usage in the compositor.

if you want to allow async texture allocation using this extension you can do
that when no transfer buffer is bound and pixels is NULL. However, I think I'd
prefer a separate API entry point for this as having it be valid to call this
function without a transfer buffer bound and pixels=NULL but not valid with
pixels!=NULL seems awkward.

[epennerAtGoogle, 2012-11-29 00:38:11]

https://codereview.chromium.org/11412232/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3690: if (!pixels) {
On 2012/11/29 00:25:23, David Reveman wrote:
> this won't work. |pixels| is an offset when a transfer buffer is bound and
that
> offset is always 0 with our current usage in the compositor.
> 
> if you want to allow async texture allocation using this extension you can do
> that when no transfer buffer is bound and pixels is NULL. However, I think I'd
> prefer a separate API entry point for this as having it be valid to call this
> function without a transfer buffer bound and pixels=NULL but not valid with
> pixels!=NULL seems awkward.

Good point. It does seem a tad awkward, but I think it still makes sense in this
context (to disable the transfer buffer and pass NULL). If we add another
allocation function we'll still need this function too, for when
allocation+upload occurs together.

[greggman, 2012-11-29 02:18:17]

It seems like this can be a client side only function similar to
glMapTexSubImage2DCHROMIUM.

glAsyncTexImage2DCHROMIUM would validate the memory then issue the texImage2D
command

build_gles2_cmd_buffer.py has to be run manually
$ cd src/gpu/command_buffer
$ python build_gles2_cmd_buffer.py

This is so we can easily review the changes that get generated.

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/client/...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3691:
helper_->AsyncTexImage2DCHROMIUM(
You can just issue texImage2DCHROMIUM here no?

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3707:
helper_->AsyncTexImage2DCHROMIUM(
You can just issue texImage2DCHROMIUM here no?

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3755:
helper_->AsyncTexSubImage2DCHROMIUM(
You can just issue texSubImage2DCHROMIUM here no?

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
gpu/command_buffer/service/feature_info.cc:238:
AddExtensionString("GL_CHROMIUM_async_tasks");
Seems like this should maybe be more specific. Like
GL_CHROMIUM_asycn_texture_upload?

What do you think?

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9368: error::Error
GLES2DecoderImpl::HandleAsyncTexImage2DCHROMIUM(
You don't need this do you? AsyncTexImage2D can just call the texImage2D command
on the client side.

See MapTexSubImage2DCHROMIUM for an example of a function that has no
implementation on the service side

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
gpu/command_buffer/service/query_manager.cc:165: class AsyncTasksCompletedQuery
: public QueryManager::Query {
CommandsIssuedQuery already handles this case doesn't it?

[epennerAtGoogle, 2012-11-29 03:47:22]

At the moment yes this would as purely client side changes, but this CL is just
stubbing out the API. The plan is to have another thread in the GPU process
which uploads to the textures outside of GL.

Unless.. do you think this could still use the same functions
(glTexImage2D/glTexSubImage2D), but use an enable flag to enable an "async mode"
on the service side? It could even share the same client side functions that
way.

So I can see three options (open to any of them really):
1.) Share same client functions with enable flag which could be checked on the
service side.
2.) Have different client functions with hidden enable flag (not sure how best
to do this)
3.) Keep different client/service side functions.

It would certainly share a lot of code with 1/2. The only issue would be if we
need to change the function parameters in some way. I don't anticipate that
though...

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
gpu/command_buffer/service/feature_info.cc:238:
AddExtensionString("GL_CHROMIUM_async_tasks");
On 2012/11/29 02:18:18, greggman wrote:
> Seems like this should maybe be more specific. Like
> GL_CHROMIUM_asycn_texture_upload?
> 
> What do you think?

It was like that before, but I plan to add Async readbacks as well. Although I
could make them separate:
- GL_CHROMIUM_async_texture_upload
- GL_CHROMIUM_async_texture_readbacks.
I have a very weak preference to keep it as just one. Whichever you think is
best.

[reveman, 2012-11-29 04:13:31]

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
gpu/command_buffer/service/feature_info.cc:238:
AddExtensionString("GL_CHROMIUM_async_tasks");
On 2012/11/29 03:47:23, epennerAtGoogle wrote:
> On 2012/11/29 02:18:18, greggman wrote:
> > Seems like this should maybe be more specific. Like
> > GL_CHROMIUM_asycn_texture_upload?
> > 
> > What do you think?
> 
> It was like that before, but I plan to add Async readbacks as well. Although I
> could make them separate:
> - GL_CHROMIUM_async_texture_upload
> - GL_CHROMIUM_async_texture_readbacks.
> I have a very weak preference to keep it as just one. Whichever you think is
> best.

"task" is a bit ambiguous. if use one extension maybe
CHROMIUM_async_pixel_transfer?

also not sure async is a good description. makes it seem like standard texture
uploads are synchronous. maybe CHROMIUM_idle_pixel_transfer or 
CHROMIUM_unsequenced_pixel_transfer is better?

[greggman, 2012-11-29 04:38:58]

On 2012/11/29 03:47:22, epennerAtGoogle wrote:
> At the moment yes this would as purely client side changes, but this CL is
just
> stubbing out the API. The plan is to have another thread in the GPU process
> which uploads to the textures outside of GL.
> 
> Unless.. do you think this could still use the same functions
> (glTexImage2D/glTexSubImage2D), but use an enable flag to enable an "async
mode"
> on the service side? It could even share the same client side functions that
> way.
> 
> So I can see three options (open to any of them really):
> 1.) Share same client functions with enable flag which could be checked on the
> service side.
> 2.) Have different client functions with hidden enable flag (not sure how best
> to do this)
> 3.) Keep different client/service side functions.
> 
> It would certainly share a lot of code with 1/2. The only issue would be if we
> need to change the function parameters in some way. I don't anticipate that
> though...

The path you're taking looks okay then.

Although just for fun I googled async texture uploads and these interesting
topics came up. Maybe you already saw them

http://developer.download.nvidia.com/GTC/PDF/GTC2012/PresentationPDF/S0356-GT...

http://www.seas.upenn.edu/~pcozzi/OpenGLInsights/OpenGLInsights-AsynchronousB...

https://developer.apple.com/library/mac/#documentation/graphicsimaging/Concep...

I don't know if any of those would influence the design.

> 
>
https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
> File gpu/command_buffer/service/feature_info.cc (right):
> 
>
https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
> gpu/command_buffer/service/feature_info.cc:238:
> AddExtensionString("GL_CHROMIUM_async_tasks");
> On 2012/11/29 02:18:18, greggman wrote:
> > Seems like this should maybe be more specific. Like
> > GL_CHROMIUM_asycn_texture_upload?
> > 
> > What do you think?
> 
> It was like that before, but I plan to add Async readbacks as well. Although I
> could make them separate:
> - GL_CHROMIUM_async_texture_upload
> - GL_CHROMIUM_async_texture_readbacks.
> I have a very weak preference to keep it as just one. Whichever you think is
> best.

[greggman, 2012-11-29 04:45:18]

please run build_gles2_cmd_decoder.py and upload the files it changes to this CL

Otherwise the one thing I noticed in those docs that might influence the design
one of the articles mentioned the overhead of shared contexts. So if you're
going to make use another context on another thread to upload the textures that
might not be as fast? I think you said you already tested this though. I'm not
sure we have the option of only 1 context on most platforms though even with the
virtual context stuff so it's probably a moot point.

[epennerAtGoogle, 2012-11-29 19:28:42]

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/11412232/diff/1013/gpu/command_buffer/service...
gpu/command_buffer/service/feature_info.cc:238:
AddExtensionString("GL_CHROMIUM_async_tasks");
On 2012/11/29 04:13:32, David Reveman wrote:
> On 2012/11/29 03:47:23, epennerAtGoogle wrote:
> > On 2012/11/29 02:18:18, greggman wrote:
> > > Seems like this should maybe be more specific. Like
> > > GL_CHROMIUM_asycn_texture_upload?
> > > 
> > > What do you think?
> > 
> > It was like that before, but I plan to add Async readbacks as well. Although
I
> > could make them separate:
> > - GL_CHROMIUM_async_texture_upload
> > - GL_CHROMIUM_async_texture_readbacks.
> > I have a very weak preference to keep it as just one. Whichever you think is
> > best.
> 
> "task" is a bit ambiguous. if use one extension maybe
> CHROMIUM_async_pixel_transfer?
> 
> also not sure async is a good description. makes it seem like standard texture
> uploads are synchronous. maybe CHROMIUM_idle_pixel_transfer or 
> CHROMIUM_unsequenced_pixel_transfer is better?

I like async-pixel-transfer the most, so I'll use that unless there is another
preference. Similarly I'll use async-pixel-transfers-completed for the query.

[epennerAtGoogle, 2012-11-29 19:54:31]

On 2012/11/29 04:45:18, greggman wrote:
> please run build_gles2_cmd_decoder.py and upload the files it changes to this
CL
> 
> Otherwise the one thing I noticed in those docs that might influence the
design
> one of the articles mentioned the overhead of shared contexts. So if you're
> going to make use another context on another thread to upload the textures
that
> might not be as fast? I think you said you already tested this though. I'm not
> sure we have the option of only 1 context on most platforms though even with
the
> virtual context stuff so it's probably a moot point.

Those docs are interesting thanks! I think the functionality is intended to be
like 'multi threaded PBOs' in the GTC presentation (some synchronization is
needed, unlike single-thread PBOs that will block on use of the texture). The
main difference is that the second thread is still controlled entirely through
GL commands on the main GL thread. There is a lot of subtle issues that arise
with this, so I think the initial strategy is to have a lot of restrictions on
use.

Android will avoid using full context sharing by using EGLImages to write to
textures on another thread. From tests this was working really well. Although
the API is such that any form of 'delayed execution' could be used to implement
it.

Okay, I'm going to do the name change for the string to 'async pixel transfers'
and fix the bug in asyncTexImage2D. I kind of like maybe merging the API into
the standard GL commands, but maybe that can be later, once we confirm the API
won't change any further.

[epenner, 2012-11-30 01:21:11]

Addressed feedback and added autogenerated code. PTAL.

[greggman, 2012-11-30 03:01:51]

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3231: BufferTracker::Buffer*
buffer = GetBoundPixelUnpackTransferBufferIfValid(
GetBoundPixelUnpackTransferBufferIfValid generates GL errors so if it returns
NULL you can't continue.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3292: if
(!bound_pixel_unpack_transfer_buffer_id_ || !buffer) {
same as above. If buffer is NULL errors have already been generated.

[reveman, 2012-11-30 03:10:20]

https://codereview.chromium.org/11412232/diff/5039/content/common/gpu/client/...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/11412232/diff/5039/content/common/gpu/client/...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:609: //
Async task functions.
Async pixel transfer functions?

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3239: 0, 0);
GetBoundPixelUnpackTransferBufferIfValid above will generate an error if no
valid buffer is bound. You need to avoid that if calling this function without a
buffer bound and pixels=NULL is valid. Looks like you just need to move the
GetBoundPixelUnpa... call below.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3245: SetGLError(
GetBoundPixelUnpackTransferBufferIfValid should already have called SetGLError.
No need to call it again here.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3293: SetGLError(
Same here. GetBoundPixelUnpackTransferBufferIfValid should already have called
SetGLError.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
gpu/command_buffer/service/query_manager.cc:165: class AsyncTasksCompletedQuery
: public QueryManager::Query {
AsyncPixelTransfersCompletedQuery?

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
gpu/command_buffer/service/query_manager.cc:192: // to return a count of tasks
completed instead.
The actual completion of the query will be the indication that async pixel
transfers are done, right? The query result will be used for something else.
Like number of successful transfers if we allow async operations to fail or be
cancelled.

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/GLES2/...
File third_party/khronos/GLES2/gl2ext.h (right):

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/GLES2/...
third_party/khronos/GLES2/gl2ext.h:2157: /* GL_CHROMIUM_async_tasks */
GL_CHROMIUM_async_pixel_transfers

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/README...
File third_party/khronos/README.chromium (right):

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/README...
third_party/khronos/README.chromium:36: - Added
GL_CHROMIUM_async_texture_uploads
GL_CHROMIUM_async_pixel_transfers

[epennerAtGoogle, 2012-11-30 04:14:22]

Sorry about that. I think I erased some of the name changes while messing with
git. Fixed those and the other comments. Ptal.

https://codereview.chromium.org/11412232/diff/5039/content/common/gpu/client/...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/11412232/diff/5039/content/common/gpu/client/...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:609: //
Async task functions.
On 2012/11/30 03:10:20, David Reveman wrote:
> Async pixel transfer functions?

Done.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3231: BufferTracker::Buffer*
buffer = GetBoundPixelUnpackTransferBufferIfValid(
On 2012/11/30 03:01:51, greggman wrote:
> GetBoundPixelUnpackTransferBufferIfValid generates GL errors so if it returns
> NULL you can't continue.

Done.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3239: 0, 0);
On 2012/11/30 03:10:20, David Reveman wrote:
> GetBoundPixelUnpackTransferBufferIfValid above will generate an error if no
> valid buffer is bound. You need to avoid that if calling this function without
a
> buffer bound and pixels=NULL is valid. Looks like you just need to move the
> GetBoundPixelUnpa... call below.

Done.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3245: SetGLError(
On 2012/11/30 03:10:20, David Reveman wrote:
> GetBoundPixelUnpackTransferBufferIfValid should already have called
SetGLError.
> No need to call it again here.

Done.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3292: if
(!bound_pixel_unpack_transfer_buffer_id_ || !buffer) {
On 2012/11/30 03:01:51, greggman wrote:
> same as above. If buffer is NULL errors have already been generated.

Done.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/client/...
gpu/command_buffer/client/gles2_implementation.cc:3293: SetGLError(
On 2012/11/30 03:10:20, David Reveman wrote:
> Same here. GetBoundPixelUnpackTransferBufferIfValid should already have called
> SetGLError.

Done.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
gpu/command_buffer/service/query_manager.cc:165: class AsyncTasksCompletedQuery
: public QueryManager::Query {
On 2012/11/30 03:10:20, David Reveman wrote:
> AsyncPixelTransfersCompletedQuery?

Done.

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
gpu/command_buffer/service/query_manager.cc:192: // to return a count of tasks
completed instead.
On 2012/11/30 03:10:20, David Reveman wrote:
> The actual completion of the query will be the indication that async pixel
> transfers are done, right? The query result will be used for something else.
> Like number of successful transfers if we allow async operations to fail or be
> cancelled.

I was thinking it could return the number of transfers completed "so far" but
that was looking difficult to do, so I'm planning to just mark it as complete
when all the transfers are complete with a simple "1" as the result.

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/GLES2/...
File third_party/khronos/GLES2/gl2ext.h (right):

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/GLES2/...
third_party/khronos/GLES2/gl2ext.h:2157: /* GL_CHROMIUM_async_tasks */
On 2012/11/30 03:10:20, David Reveman wrote:
> GL_CHROMIUM_async_pixel_transfers

Done.

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/README...
File third_party/khronos/README.chromium (right):

https://codereview.chromium.org/11412232/diff/5039/third_party/khronos/README...
third_party/khronos/README.chromium:36: - Added
GL_CHROMIUM_async_texture_uploads
On 2012/11/30 03:10:20, David Reveman wrote:
> GL_CHROMIUM_async_pixel_transfers

Done.

[greggman, 2012-11-30 04:27:27]

One more thing.

The functions return true or false. Why? That seems very un GL like. Looking at
them they should never fail if called correctly so it seems like they should be
void like all other GL functions. No?

[greggman, 2012-11-30 04:32:12]

https://codereview.chromium.org/11412232/diff/11014/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/11014/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3268: if (height == 0 || width
== 0) {
It seems like it would be best not to exit early here and let the call go
through to the service with height/width = 0. That way queries can still just
work. No need for the user to special case width or height = 0.

[epennerAtGoogle, 2012-11-30 06:16:26]

I removed the boolean. I agree I wasn't seeing the need for it in the end.

https://codereview.chromium.org/11412232/diff/11014/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/11412232/diff/11014/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3268: if (height == 0 || width
== 0) {
On 2012/11/30 04:32:12, greggman wrote:
> It seems like it would be best not to exit early here and let the call go
> through to the service with height/width = 0. That way queries can still just
> work. No need for the user to special case width or height = 0.

Done.

[greggman, 2012-11-30 07:05:41]

lgtm

[reveman, 2012-11-30 10:02:59]

lgtm

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/11412232/diff/5039/gpu/command_buffer/service...
gpu/command_buffer/service/query_manager.cc:192: // to return a count of tasks
completed instead.
On 2012/11/30 04:14:22, epennerAtGoogle wrote:
> On 2012/11/30 03:10:20, David Reveman wrote:
> > The actual completion of the query will be the indication that async pixel
> > transfers are done, right? The query result will be used for something else.
> > Like number of successful transfers if we allow async operations to fail or
be
> > cancelled.
> 
> I was thinking it could return the number of transfers completed "so far" but
> that was looking difficult to do, so I'm planning to just mark it as complete
> when all the transfers are complete with a simple "1" as the result.

I think that's good for now.

[commit-bot: I haz the power, 2012-11-30 15:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11412232/14026

[commit-bot: I haz the power, 2012-11-30 16:41:28]

Retried try job too often on win_rel for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-30 17:13:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/11412232/14026

[commit-bot: I haz the power, 2012-11-30 19:57:19]

Change committed as 170516