Perform glReadPixels with PBOs in the gpu, if PBOs are available.

Perform glReadPixels with PBOs in the gpu, if PBOs are available.
Make GL_ASYNC_PIXEL_TRANSFERS_COMPLETED_CHROMIUM wait for readpixel transfers.
Add signalQuery to get a callback when the transfer is done.
PLEASE NOTE: glMapBuffer does not wait for the readpixels transfer to complete anymore.
Nobody is currently relying on that behaviour.
Update gl_helper.cc and gl_renderer.cc to use queries.

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

[danakj, 2013-06-17 18:36:48]

https://codereview.chromium.org/16831004/diff/18001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/16831004/diff/18001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2254: 
nit: a single line of whitespace is sufficient.

https://codereview.chromium.org/16831004/diff/18001/cc/resources/sync_point_h...
File cc/resources/sync_point_helper.h (right):

https://codereview.chromium.org/16831004/diff/18001/cc/resources/sync_point_h...
cc/resources/sync_point_helper.h:27: static void SignalQuery(
nit: add a comment here to describe the function?

[hubbe, 2013-06-17 19:47:18]

https://codereview.chromium.org/16831004/diff/18001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/16831004/diff/18001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2254: 
On 2013/06/17 18:36:48, danakj wrote:
> nit: a single line of whitespace is sufficient.

Done.

https://codereview.chromium.org/16831004/diff/18001/cc/resources/sync_point_h...
File cc/resources/sync_point_helper.h (right):

https://codereview.chromium.org/16831004/diff/18001/cc/resources/sync_point_h...
cc/resources/sync_point_helper.h:27: static void SignalQuery(
On 2013/06/17 18:36:48, danakj wrote:
> nit: add a comment here to describe the function?

Done.

[piman, 2013-06-17 19:55:06]

This patch is too big, and does 2 separate things:
- add the signalQuery mechanism to get asynchronous notification for queries
- add the pbo-based asynchronous readpixels.

These 2 parts should be separable.

Also, I can haz tests?

https://codereview.chromium.org/16831004/diff/18001/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/16831004/diff/18001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:394:
base::Bind(&CopyTextureToImpl::ReadbackDone, AsWeakPtr(), request));
It is weird that you call signalQuery between begin and end. glGetQueryObject
would return GL_INVALID_OPERATION, and I think it would make sense that
SignalQuery behaves the same, as it's somewhat undefined what the behavior
should be.

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.cc:632:
!command_line.HasSwitch(switches::kForceSynchronousGLReadPixels)) {
nit: fold the command line option into the kGpuDriverBugWorkarounds logic

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:225: this, submit_count,
mem_params));
The problem here is that for async texture upload, you will delay completion
significantly. For correctness on those, we only need to guarantee that the
command has been submitted to GL, not that it has completed on the GPU. It'll
add possibly a frame of latency to uploads, which means we may have more
checkerboarding or more jank when we wait on the completion to produce a frame.

You may want to separate the completion query for readpixels, or keep track here
if you're waiting on any readpixels or not.

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:456: callbacks_.push_back(callback);
This should call the callback if the query is not pending any more.

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:460: // Seems like this channel is
shutting down with some callbacks
note: this could also happen if the query is destroyed explicitly.
I think it's still good to call the callback either way. It could be useful to
send a bool with the callback on whether the query has completed, or whether it
was destroyed - though arguably the client-side can know.

[commit-bot: I haz the power, 2013-06-25 20:01:13]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[hubbe, 2013-06-25 20:02:50]

Ok, I think it's ready for another look.
The signalQuery stuff was broken out and committed in a separate CL. (With
tests). content_gl_tests should test most of the code here, but perhaps I should
also add a readback test to gpu_ipc_browsertests.cc ?

https://codereview.chromium.org/16831004/diff/18001/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/16831004/diff/18001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:394:
base::Bind(&CopyTextureToImpl::ReadbackDone, AsWeakPtr(), request));
On 2013/06/17 19:55:06, piman wrote:
> It is weird that you call signalQuery between begin and end. glGetQueryObject
> would return GL_INVALID_OPERATION, and I think it would make sense that
> SignalQuery behaves the same, as it's somewhat undefined what the behavior
> should be.

Done.

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.cc:632:
!command_line.HasSwitch(switches::kForceSynchronousGLReadPixels)) {
On 2013/06/17 19:55:06, piman wrote:
> nit: fold the command line option into the kGpuDriverBugWorkarounds logic

Entry added, not sure exactly how it's supposed to work, so let me know if there
is something else I'm supposed to do.

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:225: this, submit_count,
mem_params));
On 2013/06/17 19:55:06, piman wrote:
> The problem here is that for async texture upload, you will delay completion
> significantly. For correctness on those, we only need to guarantee that the
> command has been submitted to GL, not that it has completed on the GPU. It'll
> add possibly a frame of latency to uploads, which means we may have more
> checkerboarding or more jank when we wait on the completion to produce a
frame.
> 
> You may want to separate the completion query for readpixels, or keep track
here
> if you're waiting on any readpixels or not.


I changed this so that WaitForReadPixels doesn't automatically create a new
fence and callbacks are called immediately if no readpixels is actually pending
right now.

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:456: callbacks_.push_back(callback);
On 2013/06/17 19:55:06, piman wrote:
> This should call the callback if the query is not pending any more.

Done. (In other CL)

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:460: // Seems like this channel is
shutting down with some callbacks
On 2013/06/17 19:55:06, piman wrote:
> note: this could also happen if the query is destroyed explicitly.
> I think it's still good to call the callback either way. It could be useful to
> send a bool with the callback on whether the query has completed, or whether
it
> was destroyed - though arguably the client-side can know.

Handled in other CL.

[piman, 2013-06-26 22:17:08]

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:225: this, submit_count,
mem_params));
On 2013/06/25 20:02:51, hubbe wrote:
> On 2013/06/17 19:55:06, piman wrote:
> > The problem here is that for async texture upload, you will delay completion
> > significantly. For correctness on those, we only need to guarantee that the
> > command has been submitted to GL, not that it has completed on the GPU.
It'll
> > add possibly a frame of latency to uploads, which means we may have more
> > checkerboarding or more jank when we wait on the completion to produce a
> frame.
> > 
> > You may want to separate the completion query for readpixels, or keep track
> here
> > if you're waiting on any readpixels or not.
> 
> 
> I changed this so that WaitForReadPixels doesn't automatically create a new
> fence and callbacks are called immediately if no readpixels is actually
pending
> right now.

So as I read it, and please explain to me if I missed something,
WaitForReadPixels will attach to the last fence inserted, whether it's within
the query or not. So in the case where you do an async readpixels followed by an
async upload, you'll still be waiting on the readpixels to be done and the fence
to be passed before returning the query for the async upload.

https://codereview.chromium.org/16831004/diff/40001/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/16831004/diff/40001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:389: request->query);
Don't you want to beginQuery before readPixels?

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

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3211: helper_->Finish();
Why do we need to do this?
DeleteQueries + WaitForCmd would ensure the query was destroyed & removed
server-side. After that point it won't write to the shared memory any more,
regardless of whether it completed or not at the GL level.

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.cc:633:
!command_line.HasSwitch(switches::kForceSynchronousGLReadPixels) &&
You don't need this any more. You have
--gpu-driver-bug-workarounds='disable_async_readpixels'

Also, looking at the origin of this flag (in
content/browser/renderer_host/compositing_iosurface_mac.mm), it sounds like you
need a blacklisting entry on intel drivers on older macs, where PBOs are exposed
but crash - I think you need to add it to
gpu/config/gpu_driver_bug_list_json.cc. zmo@ can probably help you.

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6774: }
It would be good to map the result before the pixels, so that if the pixels shm
was destroyed in the mean time, we can mark the result anyway.

[hubbe, 2013-06-28 22:17:49]

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/16831004/diff/18001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:225: this, submit_count,
mem_params));
On 2013/06/26 22:17:08, piman wrote:
> On 2013/06/25 20:02:51, hubbe wrote:
> > On 2013/06/17 19:55:06, piman wrote:
> > > The problem here is that for async texture upload, you will delay
completion
> > > significantly. For correctness on those, we only need to guarantee that
the
> > > command has been submitted to GL, not that it has completed on the GPU.
> It'll
> > > add possibly a frame of latency to uploads, which means we may have more
> > > checkerboarding or more jank when we wait on the completion to produce a
> > frame.
> > > 
> > > You may want to separate the completion query for readpixels, or keep
track
> > here
> > > if you're waiting on any readpixels or not.
> > 
> > 
> > I changed this so that WaitForReadPixels doesn't automatically create a new
> > fence and callbacks are called immediately if no readpixels is actually
> pending
> > right now.
> 
> So as I read it, and please explain to me if I missed something,
> WaitForReadPixels will attach to the last fence inserted, whether it's within
> the query or not. So in the case where you do an async readpixels followed by
an
> async upload, you'll still be waiting on the readpixels to be done and the
fence
> to be passed before returning the query for the async upload.


Fixed by implementing a new query type.

https://codereview.chromium.org/16831004/diff/40001/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/16831004/diff/40001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:389: request->query);
On 2013/06/26 22:17:08, piman wrote:
> Don't you want to beginQuery before readPixels?

Done.

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

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3211: helper_->Finish();
On 2013/06/26 22:17:08, piman wrote:
> Why do we need to do this?
> DeleteQueries + WaitForCmd would ensure the query was destroyed & removed
> server-side. After that point it won't write to the shared memory any more,
> regardless of whether it completed or not at the GL level.

That does sound perfectly reasonable, but then why doesn't the code work that
already?

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.cc:633:
!command_line.HasSwitch(switches::kForceSynchronousGLReadPixels) &&
On 2013/06/26 22:17:08, piman wrote:
> You don't need this any more. You have
> --gpu-driver-bug-workarounds='disable_async_readpixels'
> 
> Also, looking at the origin of this flag (in
> content/browser/renderer_host/compositing_iosurface_mac.mm), it sounds like
you
> need a blacklisting entry on intel drivers on older macs, where PBOs are
exposed
> but crash - I think you need to add it to
> gpu/config/gpu_driver_bug_list_json.cc. zmo@ can probably help you.

Removed the HasSwitch call.
I'm trying to find out if the workaround is strictly needed. We did some
investigating, and it would seem that the code that had a problem didn't
actually check if the driver claimed to support PBOs or now. I need to talk to
alpha (who wrote the workaround) and figure out if that particular mac/driver
combo actually lists PBOs as supported or not.

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6774: }
On 2013/06/26 22:17:08, piman wrote:
> It would be good to map the result before the pixels, so that if the pixels
shm
> was destroyed in the mean time, we can mark the result anyway.

I'm not sure I understand this, if the client destroys the memory buffer, what's
the point of letting them know if the readpixels worked or not?

[piman, 2013-06-28 23:35:46]

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

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3211: helper_->Finish();
On 2013/06/28 22:17:49, hubbe wrote:
> On 2013/06/26 22:17:08, piman wrote:
> > Why do we need to do this?
> > DeleteQueries + WaitForCmd would ensure the query was destroyed & removed
> > server-side. After that point it won't write to the shared memory any more,
> > regardless of whether it completed or not at the GL level.
> 
> That does sound perfectly reasonable, but then why doesn't the code work that
> already? 

I think you accidentally a word, and I don't understand what you're asking
exactly.

But I think you're right that the current code doesn't actually work correctly,
and that's probably what's causing crbug.com/253488

If you move the original WaitForCmd until after the DeleteQueriesEXTImmediate
(and remove the check below with the MustBeContextLost() ) it should be fine
though:
- if any query is pending, we delete it and finish. At this point the service
won't write to the shm any more.
- if no query is pending, we know the service won't write to the shm any more,
so no need to finish.

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6774: }
On 2013/06/28 22:17:49, hubbe wrote:
> On 2013/06/26 22:17:08, piman wrote:
> > It would be good to map the result before the pixels, so that if the pixels
> shm
> > was destroyed in the mean time, we can mark the result anyway.
> 
> I'm not sure I understand this, if the client destroys the memory buffer,
what's
> the point of letting them know if the readpixels worked or not?

So, for the synchronous ReadPixels, if the memory buffer (for the pixels) is
destroyed, we lose the context, which will result into an error on the client
side. It'd be nice to have the same behavior for the async one.
In particular, the buffer for the pixels is entirely in control of the user on
the client side, and so it's easy for the client to mess up and release the
buffer before the ReadPixels completed. OTOH the buffer for the result is
managed by the command buffer infrastructure, so it's a lot harder to mess up.

So the idea is, if we map the buffer for result first, we still early exit if it
fails ('cause there's nothing we can do anyway, but it shouldn't happen in
practice), and then if the buffer for pixels fails to map, we can send back an
error.

https://codereview.chromium.org/16831004/diff/59001/gpu/command_buffer/client...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/16831004/diff/59001/gpu/command_buffer/client...
gpu/command_buffer/client/gl_in_process_context.cc:557: // of them should be
finished too. We want to make sure that
That's not true though.
An async texture upload doesn't complete in order wrt other GL commands (or each
other for that matter).
Besides, the dequeue is filled in order of SignalQuery, not EngQuery, and so
even if the queries did complete in order (of EndQuery!), you couldn't have this
guarantee.

[hubbe, 2013-06-29 00:13:56]

PTAL

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

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3211: helper_->Finish();
On 2013/06/28 23:35:46, piman wrote:
> On 2013/06/28 22:17:49, hubbe wrote:
> > On 2013/06/26 22:17:08, piman wrote:
> > > Why do we need to do this?
> > > DeleteQueries + WaitForCmd would ensure the query was destroyed & removed
> > > server-side. After that point it won't write to the shared memory any
more,
> > > regardless of whether it completed or not at the GL level.
> > 
> > That does sound perfectly reasonable, but then why doesn't the code work
that
> > already? 
> 
> I think you accidentally a word, and I don't understand what you're asking
> exactly.
> 
> But I think you're right that the current code doesn't actually work
correctly,
> and that's probably what's causing crbug.com/253488
> 
> If you move the original WaitForCmd until after the DeleteQueriesEXTImmediate
> (and remove the check below with the MustBeContextLost() ) it should be fine
> though:
> - if any query is pending, we delete it and finish. At this point the service
> won't write to the shm any more.
> - if no query is pending, we know the service won't write to the shm any more,
> so no need to finish.

Ok, I'm changing it. Seems to work in my tests at least. :)

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/feature_info.cc:633:
!command_line.HasSwitch(switches::kForceSynchronousGLReadPixels) &&
On 2013/06/28 22:17:49, hubbe wrote:
> On 2013/06/26 22:17:08, piman wrote:
> > You don't need this any more. You have
> > --gpu-driver-bug-workarounds='disable_async_readpixels'
> > 
> > Also, looking at the origin of this flag (in
> > content/browser/renderer_host/compositing_iosurface_mac.mm), it sounds like
> you
> > need a blacklisting entry on intel drivers on older macs, where PBOs are
> exposed
> > but crash - I think you need to add it to
> > gpu/config/gpu_driver_bug_list_json.cc. zmo@ can probably help you.
> 
> Removed the HasSwitch call.
> I'm trying to find out if the workaround is strictly needed. We did some
> investigating, and it would seem that the code that had a problem didn't
> actually check if the driver claimed to support PBOs or now. I need to talk to
> alpha (who wrote the workaround) and figure out if that particular mac/driver
> combo actually lists PBOs as supported or not.

Talked to alpha, and it seems that the workaround is indeed needed, so I went
ahead and added it. Not sure if anybody around here has a mac like that to test
it on though...

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16831004/diff/40001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6774: }
On 2013/06/28 23:35:46, piman wrote:
> On 2013/06/28 22:17:49, hubbe wrote:
> > On 2013/06/26 22:17:08, piman wrote:
> > > It would be good to map the result before the pixels, so that if the
pixels
> > shm
> > > was destroyed in the mean time, we can mark the result anyway.
> > 
> > I'm not sure I understand this, if the client destroys the memory buffer,
> what's
> > the point of letting them know if the readpixels worked or not?
> 
> So, for the synchronous ReadPixels, if the memory buffer (for the pixels) is
> destroyed, we lose the context, which will result into an error on the client
> side. It'd be nice to have the same behavior for the async one.
> In particular, the buffer for the pixels is entirely in control of the user on
> the client side, and so it's easy for the client to mess up and release the
> buffer before the ReadPixels completed. OTOH the buffer for the result is
> managed by the command buffer infrastructure, so it's a lot harder to mess up.
> 
> So the idea is, if we map the buffer for result first, we still early exit if
it
> fails ('cause there's nothing we can do anyway, but it shouldn't happen in
> practice), and then if the buffer for pixels fails to map, we can send back an
> error.

This seems a little silly as async readpixels doesn't actually check the result
(result_shm_id == 0), but I changed it anyways.

https://codereview.chromium.org/16831004/diff/59001/gpu/command_buffer/client...
File gpu/command_buffer/client/gl_in_process_context.cc (right):

https://codereview.chromium.org/16831004/diff/59001/gpu/command_buffer/client...
gpu/command_buffer/client/gl_in_process_context.cc:557: // of them should be
finished too. We want to make sure that
On 2013/06/28 23:35:46, piman wrote:
> That's not true though.
> An async texture upload doesn't complete in order wrt other GL commands (or
each
> other for that matter).
> Besides, the dequeue is filled in order of SignalQuery, not EngQuery, and so
> even if the queries did complete in order (of EndQuery!), you couldn't have
this
> guarantee.

Async readpixels (and maybe other queries) *do* finish in the order they were
scheduled, and it's not unreasonable to assume that if you call signalQuery
right after endQuery, the callbacks will come back in a reasonable order.
(gl_helper.cc makes this assumption right now, so it's either change this code,
gl_helper.cc or both...)

Also, on a grander scale, the tab capture code prefers to have the frames
returned in order, which becomes much easier if you can rely on the callbacks to
maintain their order.

I added a comment clarifying that this doesn't apply to all query types though.

[hubbe, 2013-07-01 21:17:58]

Sorry, forgot to upload after merge.
Only gl_helper.cc and gl_in_process_context.cc should have changed since friday
though.

[piman, 2013-07-01 22:24:08]

LGTM + a few nits.

https://codereview.chromium.org/16831004/diff/93001/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/16831004/diff/93001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:482: request = request_queue_.front();
nit: can we avoid hiding the method parameter? It makes the code harder to read.

https://codereview.chromium.org/16831004/diff/93001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:483: if (!request->done) break;
nit: break on next line

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2523: 
nit: extra blank line unnecessary.

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:197: virtual
~AsyncPixelTransfersCompletedQuery();
nit: it looks like you didn't end up needing changes to this class. Revert?

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:260: , public
base::SupportsWeakPtr<AsyncReadPixelsCompletedQuery> {
nit: , on previous line.

[hubbe, 2013-07-01 22:34:40]

https://codereview.chromium.org/16831004/diff/93001/content/common/gpu/client...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/16831004/diff/93001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:482: request = request_queue_.front();
On 2013/07/01 22:24:08, piman wrote:
> nit: can we avoid hiding the method parameter? It makes the code harder to
read.

Done.

https://codereview.chromium.org/16831004/diff/93001/content/common/gpu/client...
content/common/gpu/client/gl_helper.cc:483: if (!request->done) break;
On 2013/07/01 22:24:08, piman wrote:
> nit: break on next line

Done.

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2523: 
On 2013/07/01 22:24:08, piman wrote:
> nit: extra blank line unnecessary.

Done.

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:197: virtual
~AsyncPixelTransfersCompletedQuery();
On 2013/07/01 22:24:08, piman wrote:
> nit: it looks like you didn't end up needing changes to this class. Revert?

Done.

https://codereview.chromium.org/16831004/diff/93001/gpu/command_buffer/servic...
gpu/command_buffer/service/query_manager.cc:260: , public
base::SupportsWeakPtr<AsyncReadPixelsCompletedQuery> {
On 2013/07/01 22:24:08, piman wrote:
> nit: , on previous line.

Done.

[commit-bot: I haz the power, 2013-07-01 23:05:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/16831004/103001

[commit-bot: I haz the power, 2013-07-01 23:14:19]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-02 03:54:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/16831004/121001

[commit-bot: I haz the power, 2013-07-02 06:49:30]

Change committed as 209625