cc: Make async readback path use async glRreadPixels.

cc: Make async readback path use async glReadPixels.

The current async readback path for the compositor did a synchronous
glReadPixels, but since the whole thing is asynchronous, there's no
need for the glReadPixels call to be synchronous.

This mimics the code from GLHelper to make the glReadPixels into an
async call.

Tested by all the compositor pixel tests.

Depends on: https://codereview.chromium.org/14126014/

R=piman
BUG=179896
NOTRY=true

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

[danakj, 2013-04-27 00:04:34]

Updated this on https://codereview.chromium.org/14126014/ now that all its
problems have been solved.

Real async gl readbacks working! Had to implement some mapBuffers() support in
TestWGC3D to keep unit tests working.

[piman, 2013-04-27 00:42:25]

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.cc#n...
cc/output/gl_renderer.cc:2115: if (is_async) {
I think you can make your life a bit easier if:
- you always use the buffer path
- you always make the finished_callback
- when sync, you finish, then call the finished_callback. when async you insert
the sync point and SignalSyncPoint on the finished_callback

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.cc#n...
cc/output/gl_renderer.cc:2162: sync_point);
nit: Given the above, and given that you only pass the sync_point for a DCHECK,
maybe you can save yourself some 'if' madness by instead storing the buffer in
pending_async_read_pixels_.back(), and DCHECK that instead, since you already
pass it to FinishedReadback. Saves a param too.

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.h#ne...
cc/output/gl_renderer.h:94: void GetFramebufferPixels(gfx::Rect rect,
CopyRenderPassCallback callback);
nit: can we avoid overloading? Especially if one is virtual it becomes super
confusing.

https://codereview.chromium.org/14273026/diff/3016/cc/resources/resource_prov...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/14273026/diff/3016/cc/resources/resource_prov...
cc/resources/resource_provider.h:130: // Finish all context operations, causing
any pendnig callbacks to be
typo: pendnig

https://codereview.chromium.org/14273026/diff/3016/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/test/pixel_test.cc#newc...
cc/test/pixel_test.cc:92: base::MessageLoop::current()->Run();
Any way we can use base::RunLoop instead (generally safer)?

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:548: } {
nit: we usually put those on separate lines.

Also, why do they need to be 2 separate tasks?

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:1185: TRACE_EVENT0("cc",
"ThreadProxy::LayerTreeHostClosedOnImplThread");
nit: copy & paste of trace, I saw that.

[danakj, 2013-04-29 14:59:05]

Thanks! PTAL

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.cc#n...
cc/output/gl_renderer.cc:2115: if (is_async) {
On 2013/04/27 00:42:25, piman wrote:
> I think you can make your life a bit easier if:
> - you always use the buffer path
> - you always make the finished_callback
> - when sync, you finish, then call the finished_callback. when async you
insert
> the sync point and SignalSyncPoint on the finished_callback

Thanks! This is what I was trying to do at first, then I thought "but I can't
make finish() guarantee the callback is run" but of course I don't need to
signal/wait for the callback at all if i finish() first..

So, will do!

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.cc#n...
cc/output/gl_renderer.cc:2162: sync_point);
On 2013/04/27 00:42:25, piman wrote:
> nit: Given the above, and given that you only pass the sync_point for a
DCHECK,
> maybe you can save yourself some 'if' madness by instead storing the buffer in
> pending_async_read_pixels_.back(), and DCHECK that instead, since you already
> pass it to FinishedReadback. Saves a param too.

Oh, good idea.

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/14273026/diff/3016/cc/output/gl_renderer.h#ne...
cc/output/gl_renderer.h:94: void GetFramebufferPixels(gfx::Rect rect,
CopyRenderPassCallback callback);
On 2013/04/27 00:42:25, piman wrote:
> nit: can we avoid overloading? Especially if one is virtual it becomes super
> confusing.

Oh gosh, yes you're right. Renamed back to GetFramebufferPixelsAsync() as I had
done at first..

https://codereview.chromium.org/14273026/diff/3016/cc/resources/resource_prov...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/14273026/diff/3016/cc/resources/resource_prov...
cc/resources/resource_provider.h:130: // Finish all context operations, causing
any pendnig callbacks to be
On 2013/04/27 00:42:25, piman wrote:
> typo: pendnig

Done.

https://codereview.chromium.org/14273026/diff/3016/cc/test/pixel_test.cc
File cc/test/pixel_test.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/test/pixel_test.cc#newc...
cc/test/pixel_test.cc:92: base::MessageLoop::current()->Run();
On 2013/04/27 00:42:25, piman wrote:
> Any way we can use base::RunLoop instead (generally safer)?

I will introduce myself to RunLoop. (Oh, it is neet.)

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:548: } {
On 2013/04/27 00:42:25, piman wrote:
> nit: we usually put those on separate lines.
> 
> Also, why do they need to be 2 separate tasks?

So, my thinking was that Finish will cause callbacks to be queued, but not run.
And we want them to run before we destroy the GLRenderer, or we'll lose those
readback requests.

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:1185: TRACE_EVENT0("cc",
"ThreadProxy::LayerTreeHostClosedOnImplThread");
On 2013/04/27 00:42:25, piman wrote:
> nit: copy & paste of trace, I saw that.

Done. *^_^*

[piman, 2013-04-29 17:29:13]

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:548: } {
On 2013/04/29 14:59:05, danakj wrote:
> On 2013/04/27 00:42:25, piman wrote:
> > nit: we usually put those on separate lines.
> > 
> > Also, why do they need to be 2 separate tasks?
> 
> So, my thinking was that Finish will cause callbacks to be queued, but not
run.
> And we want them to run before we destroy the GLRenderer, or we'll lose those
> readback requests.

Ok, I see. I didn't think it would generally work without race, but thinking
about it some more, it probably does:
- on gpu process, the sync point retire would be passed before the Finish is
done, so the signal IPC would be sent before the FlushSync reply
- on the IO thread, the signal task would be posted before the SyncMessageFilter
gets signalled to continue
- so by the time FinishGLOnImplThread signals the main thread, the task is
posted to finish the readback on the compositor thread, before the task is
posted for the LayerTreeHostClosedOnImplThread so we're good.

Definitely worth a comment though.

https://codereview.chromium.org/14273026/diff/18017/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14273026/diff/18017/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2196: if (src_pixels) {
Mmh, you unmapped the buffer here. src_pixel isn't guaranteed to point to valid
data anymore...

https://codereview.chromium.org/14273026/diff/18017/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2205: dest_pixels[dest_y + (x + 0)] = src_pixels[src_y
+ (x + 2)];
Should all this be replacing the memcpy?
Why do the memcpy at all?

[danakj, 2013-04-29 17:30:54]

https://codereview.chromium.org/14273026/diff/18017/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14273026/diff/18017/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2196: if (src_pixels) {
On 2013/04/29 17:29:13, piman wrote:
> Mmh, you unmapped the buffer here. src_pixel isn't guaranteed to point to
valid
> data anymore...

Oh.. right. This is silly. I will do this instead of the memcpy.

[danakj, 2013-04-30 01:30:51]

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/14273026/diff/3016/cc/trees/thread_proxy.cc#n...
cc/trees/thread_proxy.cc:548: } {
On 2013/04/29 17:29:13, piman wrote:
> On 2013/04/29 14:59:05, danakj wrote:
> > On 2013/04/27 00:42:25, piman wrote:
> > > nit: we usually put those on separate lines.
> > > 
> > > Also, why do they need to be 2 separate tasks?
> > 
> > So, my thinking was that Finish will cause callbacks to be queued, but not
> run.
> > And we want them to run before we destroy the GLRenderer, or we'll lose
those
> > readback requests.
> 
> Ok, I see. I didn't think it would generally work without race, but thinking
> about it some more, it probably does:
> - on gpu process, the sync point retire would be passed before the Finish is
> done, so the signal IPC would be sent before the FlushSync reply
> - on the IO thread, the signal task would be posted before the
SyncMessageFilter
> gets signalled to continue
> - so by the time FinishGLOnImplThread signals the main thread, the task is
> posted to finish the readback on the compositor thread, before the task is
> posted for the LayerTreeHostClosedOnImplThread so we're good.
> 
> Definitely worth a comment though.

Done.

https://codereview.chromium.org/14273026/diff/18017/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/14273026/diff/18017/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:2205: dest_pixels[dest_y + (x + 0)] = src_pixels[src_y
+ (x + 2)];
On 2013/04/29 17:29:13, piman wrote:
> Should all this be replacing the memcpy?
> Why do the memcpy at all?

Done.

[piman, 2013-04-30 01:43:42]

LGTM, thanks!

[danakj, 2013-04-30 01:44:33]

Thank you :)

[commit-bot: I haz the power, 2013-04-30 01:44:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14273026/33001

[commit-bot: I haz the power, 2013-04-30 18:58:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14273026/47002

[commit-bot: I haz the power, 2013-04-30 20:28:41]

Failed to trigger a try job on win_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-04-30 20:29:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14273026/48018

[aelias_OOO_until_Jul13, 2013-04-30 23:50:45]

Oops, I accidentally uploaded to the wrong issue and didn't ctrl-C it in time. 
Please reupload, sorry about that.

[commit-bot: I haz the power, 2013-04-30 23:51:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14273026/48018

[danakj, 2013-04-30 23:56:53]

I didnt know you could do that, hah

[commit-bot: I haz the power, 2013-05-01 00:01:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14273026/48018

[commit-bot: I haz the power, 2013-05-01 00:04:03]

Change committed as 197519