Implement asynchronous operation for RWHVP::CopyFromCompositingSurface on Mac

Implement asynchronous operation for RWHVP::CopyFromCompositingSurface on Mac

Current implementation blocks the UI thread for a long period of time. 
This change uses asynchronous operation to copy the frame buffer.

I have tested this manually with two cases:

1. Thumbnail generation

Tested thumbnails are generated successfully with --force-compositing-mode.
I have also verified that CopyTo() now completes almost instantaneously while
FinishCopy() completes in a relatively short time. Total time that UI thread
is blocked is about 1/4 of previous implementation.

2. Resource destruction

Manually tested that if CompositingIOSurface is destroyed before
asynchronous copy is finished then GL resources associated with the copy
is destroyed.

BUG=145587
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158395

[Alpha Left Google, 2012-09-18 00:08:44]

mazda: Please review the change because this affects the thumbnailer.

kbr: Please review the GL code and the general approach.

[Nico, 2012-09-18 02:42:19]

On Tue, Sep 18, 2012 at 9:08 AM,  <hclam@chromium.org> wrote:
> mazda: Please review the change because this affects the thumbnailer.
>
> kbr: Please review the GL code and the general approach.

(do you still want me to look at anything?)

[Alpha Left Google, 2012-09-18 02:44:58]

On 2012/09/18 02:42:19, Nico wrote:
> On Tue, Sep 18, 2012 at 9:08 AM,  <mailto:hclam@chromium.org> wrote:
> > mazda: Please review the change because this affects the thumbnailer.
> >
> > kbr: Please review the GL code and the general approach.
> 
> (do you still want me to look at anything?)

I think you're in OWNER. Please review if you feel comfortable. :)

[Ken Russell (switch to Gerrit), 2012-09-18 07:57:42]

The overall approach seems fine to me as long as enough tests have been run to
ensure it isn't leaking GPU resources or memory.

I'm traveling on business this week and don't have time to give it a really
thorough review. Perhaps jbauman or zmo can.

http://codereview.chromium.org/10917307/diff/11001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_mac.mm (right):

http://codereview.chromium.org/10917307/diff/11001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:498: const int
kDelayMilliseconds = 30;
Please figure out some way to avoid having a heuristic like this. It will only
introduce flakiness. Maybe you can poll every 30 ms for completion in a loop, up
to, for example, 1 second.

[Alpha Left Google, 2012-09-20 04:35:19]

jbauman: kbr@ suggested you for review.

reviewers:

I have updated the code to use GL_APPLE_FENCE and poll to see if the operation
is completed. This gets rid of the waiting heuristic if this extension is
available.

I have also tested the patch with OpenGL Profiler to make sure there's no leak.

[mazda, 2012-09-20 08:41:10]

Is it possible to make FinishCopy work even after RWHVM is deleted?
I'd like to enable thumbnailer to take snapshot just before closing a tab
(http://crbug.com/125400 ).

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_mac.mm (right):

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:297: if
(!has_io_surface)
The original code allows the case |has_io_surface| is false. Is it OK to change
the behavior?

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:405: if
(copy_context_.started) {
This will make successive calls of CompositingIOSurfaceMac::CopyTo in a short
period of time likely to fail. If this function is used from more than one
clients frequently, the probability of success is affected by the clients.
(e.g. While tab is captured using this function, taking thumbnails is likely to
fail). I think it's better to avoid it.
(queueing |copy_context_| or using PostDelayedTask?)

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:410:
TRACE_EVENT0("browser", "CompositingIOSurfaceMac::CopyTo()");
Is it better to put this below the check of MapIOSurfaceToTexture so that
TRACE_EVENT0 does not include the case of returning immediately?

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:413: if
(!MapIOSurfaceToTexture(io_surface_handle_)) {
Is there any reason why this is moved after CGLSetCurrentContext(cglContext_)?

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:423: use_fence =
Would it be better to extract this into a function?

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:444: const int
kDestWidth = copy_context_.dest_size.width();
s/kDestWidth/dest_width/

Because this is not a compile-time constant.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:445: const int
kDestHeight = copy_context_.dest_size.height();
Ditto.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:713:
memcpy(copy_context_.out_buf, buf, kDestWidth * kDestHeight * 4);
s/kDestWidth * kDestHeight/copy_context_.dest_size.GetArea()/

Then kDestWidth and kDestHeight can be removed.

[Alpha Left Google, 2012-09-21 01:08:31]

Thanks for review.

I can make FinishCopy to be called when CompositingIOSurface is deleted. I'm
just not sure whether this is a good approach, the reason is this adds extra
time for the tab and stall in the UI thread. I suggest we discuss this on the
bug to explore other options, if we decided to do a synchronous FinishCopy call
during destruction I'm happy to change that after this lands.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_mac.mm (right):

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:297: if
(!has_io_surface)
On 2012/09/20 08:41:11, mazda wrote:
> The original code allows the case |has_io_surface| is false. Is it OK to
change
> the behavior?

Good catch I forgot to remove this bit.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:405: if
(copy_context_.started) {
On 2012/09/20 08:41:11, mazda wrote:
> This will make successive calls of CompositingIOSurfaceMac::CopyTo in a short
> period of time likely to fail. If this function is used from more than one
> clients frequently, the probability of success is affected by the clients.
> (e.g. While tab is captured using this function, taking thumbnails is likely
to
> fail). I think it's better to avoid it.
> (queueing |copy_context_| or using PostDelayedTask?)

I'm not sure we should allow multiple copies at the same time. If we queue more
CopyContext it is easy to leak resources I think we should make sure the user is
throttling the call to CopyFromCompositingSurface correctly, meaning no more
than one outstanding call.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:410:
TRACE_EVENT0("browser", "CompositingIOSurfaceMac::CopyTo()");
On 2012/09/20 08:41:11, mazda wrote:
> Is it better to put this below the check of MapIOSurfaceToTexture so that
> TRACE_EVENT0 does not include the case of returning immediately?

Sure will do.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:413: if
(!MapIOSurfaceToTexture(io_surface_handle_)) {
On 2012/09/20 08:41:11, mazda wrote:
> Is there any reason why this is moved after CGLSetCurrentContext(cglContext_)?

I'm following the strcuture in DrawIOSurface, I'm surprised that it maps
correctly before setting the context.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:423: use_fence =
On 2012/09/20 08:41:11, mazda wrote:
> Would it be better to extract this into a function?

Could do.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:444: const int
kDestWidth = copy_context_.dest_size.width();
On 2012/09/20 08:41:11, mazda wrote:
> s/kDestWidth/dest_width/
> 
> Because this is not a compile-time constant.

Done.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:445: const int
kDestHeight = copy_context_.dest_size.height();
On 2012/09/20 08:41:11, mazda wrote:
> Ditto.

Done.

http://codereview.chromium.org/10917307/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:713:
memcpy(copy_context_.out_buf, buf, kDestWidth * kDestHeight * 4);
On 2012/09/20 08:41:11, mazda wrote:
> s/kDestWidth * kDestHeight/copy_context_.dest_size.GetArea()/
> 
> Then kDestWidth and kDestHeight can be removed.

Done.

[Alpha Left Google, 2012-09-21 17:44:34]

Ping.

[Ken Russell (switch to Gerrit), 2012-09-21 21:47:38]

FWIW the current version of this patch looks good to me.

[Nico, 2012-09-22 02:07:26]

mazda: happy too?

[mazda, 2012-09-22 04:42:13]

> I can make FinishCopy to be called when CompositingIOSurface is deleted. I'm
> just not sure whether this is a good approach, the reason is this adds extra
> time for the tab and stall in the UI thread. I suggest we discuss this on the
> bug to explore other options, if we decided to do a synchronous FinishCopy
call
> during destruction I'm happy to change that after this lands.
Sure. Let's discuss other options later.
This CL LGTM.

[Nico, 2012-09-22 11:12:21]

lgtm stamp since it looks good to kbr and mazda.

(I'm not convinced that GL errors need to be checked after every single opengl
call fwiw.)

http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_mac.h (right):

http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.h:53: // |4 *
dst_pixel_size.width() * dst_pixel_size.height()| bytes.
Describe |callback|, mention that only one callback can be in flight at once.

http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.h:161: fence = 0;
you're not setting src_rect and dest_size to their empty values. I suppose
that's intentional?

http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_mac.mm (right):

http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_mac.mm:116:
"GL_APPLE_fence") != NULL;
do we not have a real "HasExtension()" function anywhere? this is pretty icky.

[Alpha Left Google, 2012-09-24 19:04:25]

On 2012/09/22 11:12:21, Nico wrote:
> lgtm stamp since it looks good to kbr and mazda.
> 
> (I'm not convinced that GL errors need to be checked after every single opengl
> call fwiw.)

They are not compiled in release build and sometimes they do catch interesting
things. I actually hit an error with this code thanks for CHECK_GL_ERROR.

> 
>
http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
> File content/browser/renderer_host/compositing_iosurface_mac.h (right):
> 
>
http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
> content/browser/renderer_host/compositing_iosurface_mac.h:53: // |4 *
> dst_pixel_size.width() * dst_pixel_size.height()| bytes.
> Describe |callback|, mention that only one callback can be in flight at once.

Done.

> 
>
http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
> content/browser/renderer_host/compositing_iosurface_mac.h:161: fence = 0;
> you're not setting src_rect and dest_size to their empty values. I suppose
> that's intentional?

Yes I try to keep the reset method simple, and those fields are not accessed
after reset.

> 
>
http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
> File content/browser/renderer_host/compositing_iosurface_mac.mm (right):
> 
>
http://codereview.chromium.org/10917307/diff/21001/content/browser/renderer_h...
> content/browser/renderer_host/compositing_iosurface_mac.mm:116:
> "GL_APPLE_fence") != NULL;
> do we not have a real "HasExtension()" function anywhere? this is pretty icky.

There is one in the GPU code ui/gl/gl_context.h but we don't use that in the
browser process.

[commit-bot: I haz the power, 2012-09-24 19:06:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/10917307/17003

[commit-bot: I haz the power, 2012-09-24 21:32:44]

Change committed as 158395