Cache OpenGL textures and other objects in CompositingIOSurfaceMac copy/transform code.

Cache OpenGL textures and other objects in CompositingIOSurfaceMac copy/transform code.

On Macs using AMD Radeon and/or Intel HD Graphics GPUs, the deletion of textures, FBOs, and other OpenGL objects for each frame capture was causing GPU pipeline stalls.  This was causing average capture time to be too high; in many cases, not allowing even a 30 FPS rate of capture.  With this change, the pipeline stalls have been removed.

Confirmed:

1. Visually, using our tab capture/playback tools (several hours of video watching).
2. Via Chrome browser tracing tool.
3. Via Apple's OpenGL Profiler tool (no operations other than glReadPixels and glFlush take more than 1 ms to complete).

BUG=223326
TEST=See above; and ran content_unittests.

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

[miu, 2013-04-06 03:12:25]

To split the workload (although this isn't a huge CL):

Alpha: Please review changes in compositing_iosurface_mac.*
Nick: Please review changes in compositing_iosurface_transformer*

NOTE 1: Refactoring is badly needed in compositing_iosurface_mac.* for the copy
impl.
NOTE 2: I tested both synch and asynch paths, although I left asynch disabled,
per http://crbug.com/225877.

Thanks,
Yuri

[ncarter (slow), 2013-04-15 23:38:19]

lgtm for transformer_*, though I am not an owner here.

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_transformer_mac.cc
(right):

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.cc:144:
glDrawBuffers(1, kColorAttachments);
This seems unnecessary.

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.cc:283: if
(texture_sizes_[which] != size) {
Might be worth TRACE_EVENTING this since it shouldn't happen commonly and would
be a perf warning if it does.

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.cc:287:
DCHECK(glGetError() == GL_NO_ERROR);
Why DCHECK glGetError() here specifically (is this debugging cruft?)

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_transformer_mac.h
(right):

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.h:97: void
PrepareTexture(CachedTexture which, const gfx::Size& size);
Should this return the texture name?

[Alpha Left Google, 2013-04-16 01:49:07]

Thanks for fixing this! The logic around readback badly needs a refactoring,
LGTM to unblock you from doing the right stuff.

[miu, 2013-04-16 03:51:22]

Thanks all!  :)

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_transformer_mac.cc
(right):

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.cc:144:
glDrawBuffers(1, kColorAttachments);
On 2013/04/15 23:38:19, ncarter wrote:
> This seems unnecessary.

It probably is.  I was just playing it safe.  Done.

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.cc:283: if
(texture_sizes_[which] != size) {
On 2013/04/15 23:38:19, ncarter wrote:
> Might be worth TRACE_EVENTING this since it shouldn't happen commonly and
would
> be a perf warning if it does.

Done.  Good call.

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.cc:287:
DCHECK(glGetError() == GL_NO_ERROR);
On 2013/04/15 23:38:19, ncarter wrote:
> Why DCHECK glGetError() here specifically (is this debugging cruft?)

Done.  Yep.  Leftover debugging cruft.  I had a problem and put these everywhere
to narrow down where the error occurred.

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/compositing_iosurface_transformer_mac.h
(right):

https://codereview.chromium.org/13749002/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/compositing_iosurface_transformer_mac.h:97: void
PrepareTexture(CachedTexture which, const gfx::Size& size);
On 2013/04/15 23:38:19, ncarter wrote:
> Should this return the texture name?

If the method was public, I'd probably do that and rename it to something like
GetCachedTexture().  As an internal helper, it doesn't really matter.

[miu, 2013-04-16 18:46:12]

Committed patchset #2 manually as r194404 (presubmit successful).