Add flow control between renderer and GPU processes, and, on Mac OS X,...

Add flow control between renderer and GPU processes, and, on Mac OS X,
between GPU process and browser process. Thanks to Al Patrick for the
renderer<->GPU flow control mechanism.

This CL prevents the renderer from issuing more OpenGL work than the
GPU process can execute, and, on the Mac, prevents the combination of
the renderer and GPU processes from transmitting more frames via
IOSurfaces from the GPU to the browser process than can be handled by
the GPU.

This fix causes the renderer to block inside ggl::SwapBuffers() when
it gets too far ahead. Different strategies can be considered going
forward, including measuring frame rates in the GPU and renderer
processes and trying to match them via techniques such as delaying the
execution of some timers. However, despite the general undesirability
of blocking the render thread, this fix results in a significant
performance improvement.

With this fix integrated, a fill-limited test case from Chris Rogers
displays at 60 FPS instead of 15 FPS on a Mac Pro. Gregg Tavares'
aquarium from webglsamples.googlecode.com displays at 30 FPS instead
of 4 or 5 FPS on a MacBook Pro. The frame rates measured at the
JavaScript level now match the actual frame rate of the browser, where
previously they were much higher since they were issuing more work
than the browser could render.

A few other minor OpenGL bugs potentially impacting the correctness of
the Mac compositor are being fixed as well in this CL.

Verified that WebGL, CSS 3D and YouTube (Core Animation plugin)
content all work.

BUG=63539
TEST=none


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

[Ken Russell (switch to Gerrit), 2010-11-24 23:30:31]

Please review. Thanks.

[Nico, 2010-11-24 23:43:29]

Drive-by

Some amazing progress in this CL!

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/ac...
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc
(right):

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/ac...
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:103:
glClearColor(1.0f, 1.0f, 1.0f, 1.0f);
doh

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:394:
CGLSetCurrentContext(0);
Why did you add this line?

http://codereview.chromium.org/5317007/diff/1/chrome/renderer/ggl/ggl.cc
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/5317007/diff/1/chrome/renderer/ggl/ggl.cc#newc...
chrome/renderer/ggl/ggl.cc:40: // actually reach the screen.
True, but the browser process would show the latest of the N frames the renderer
produces, not the first. So it's not all wasted work, it reduces display
latency.

[apatrick_chromium, 2010-11-24 23:56:09]

LGTM for my part

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1067: private:
indentation

http://codereview.chromium.org/5317007/diff/1/gpu/command_buffer/service/gpu_...
File gpu/command_buffer/service/gpu_processor.cc (right):

http://codereview.chromium.org/5317007/diff/1/gpu/command_buffer/service/gpu_...
gpu/command_buffer/service/gpu_processor.cc:127:
MessageLoop::current()->PostDelayedTask(
Rather than repeatedly poll for progress every 1ms, you could post the task to
continue processing this command buffer from the IPC message handler for the
SwapBuffers ack.

[Ken Russell (switch to Gerrit), 2010-11-25 01:02:08]

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:394:
CGLSetCurrentContext(0);
On 2010/11/24 23:43:29, Nico wrote:
> Why did you add this line?

OpenGL's rules are that a context can only be current on one thread at a time.
Generally, if a context is current on one thread, then attempts to make it
current on another thread result in an error. Because the context was being left
current on the display link thread, it seemed possible that the main thread
would not be able to correctly make it current for some of its operations. At
one point during the development of this CL I thought that that might be the
reason for the drastic slowdowns on Mac OS X. It turned out not to be, but I
think this is still needed for correctness.

http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1067: private:
On 2010/11/24 23:56:09, apatrick_chromium wrote:
> indentation

Done.

http://codereview.chromium.org/5317007/diff/1/chrome/renderer/ggl/ggl.cc
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/5317007/diff/1/chrome/renderer/ggl/ggl.cc#newc...
chrome/renderer/ggl/ggl.cc:40: // actually reach the screen.
On 2010/11/24 23:43:29, Nico wrote:
> True, but the browser process would show the latest of the N frames the
renderer
> produces, not the first.

Correct.

> So it's not all wasted work, it reduces display
> latency.

That's possible. More investigation is needed. In Safari, where Core Animation's
built-in rate limiting mechanisms are in use, Chris Rogers' fill-limited test
case reaches a steady state of 60 FPS. If we increase this number here to 2 then
Chromium claims 120 FPS, but the browser is only displaying 60 FPS. In that
situation we are definitely doing more rendering work than desired. We may need
to do some sort of adaptive throttling to get the best behavior.

http://codereview.chromium.org/5317007/diff/1/gpu/command_buffer/service/gpu_...
File gpu/command_buffer/service/gpu_processor.cc (right):

http://codereview.chromium.org/5317007/diff/1/gpu/command_buffer/service/gpu_...
gpu/command_buffer/service/gpu_processor.cc:127:
MessageLoop::current()->PostDelayedTask(
On 2010/11/24 23:56:09, apatrick_chromium wrote:
> Rather than repeatedly poll for progress every 1ms, you could post the task to
> continue processing this command buffer from the IPC message handler for the
> SwapBuffers ack.

Thanks, this is an excellent suggestion and cleans up an area I was not
comfortable with. I've retested with this change.

[Nico, 2010-11-25 01:08:32]

LG for what I looked at, thanks for bearing with my questions.

On Wed, Nov 24, 2010 at 5:02 PM,  <kbr@chromium.org> wrote:
>
>
http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> (right):
>
>
http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:394:
> CGLSetCurrentContext(0);
> On 2010/11/24 23:43:29, Nico wrote:
>>
>> Why did you add this line?
>
> OpenGL's rules are that a context can only be current on one thread at a
> time. Generally, if a context is current on one thread, then attempts to
> make it current on another thread result in an error. Because the
> context was being left current on the display link thread, it seemed
> possible that the main thread would not be able to correctly make it
> current for some of its operations. At one point during the development
> of this CL I thought that that might be the reason for the drastic
> slowdowns on Mac OS X. It turned out not to be, but I think this is
> still needed for correctness.
>
>
http://codereview.chromium.org/5317007/diff/1/chrome/browser/renderer_host/re...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1067:
> private:
> On 2010/11/24 23:56:09, apatrick_chromium wrote:
>>
>> indentation
>
> Done.
>
> http://codereview.chromium.org/5317007/diff/1/chrome/renderer/ggl/ggl.cc
> File chrome/renderer/ggl/ggl.cc (right):
>
>
http://codereview.chromium.org/5317007/diff/1/chrome/renderer/ggl/ggl.cc#newc...
> chrome/renderer/ggl/ggl.cc:40: // actually reach the screen.
> On 2010/11/24 23:43:29, Nico wrote:
>>
>> True, but the browser process would show the latest of the N frames
>
> the renderer
>>
>> produces, not the first.
>
> Correct.
>
>> So it's not all wasted work, it reduces display
>> latency.
>
> That's possible. More investigation is needed. In Safari, where Core
> Animation's built-in rate limiting mechanisms are in use, Chris Rogers'
> fill-limited test case reaches a steady state of 60 FPS. If we increase
> this number here to 2 then Chromium claims 120 FPS, but the browser is
> only displaying 60 FPS. In that situation we are definitely doing more
> rendering work than desired. We may need to do some sort of adaptive
> throttling to get the best behavior.
>
>
http://codereview.chromium.org/5317007/diff/1/gpu/command_buffer/service/gpu_...
> File gpu/command_buffer/service/gpu_processor.cc (right):
>
>
http://codereview.chromium.org/5317007/diff/1/gpu/command_buffer/service/gpu_...
> gpu/command_buffer/service/gpu_processor.cc:127:
> MessageLoop::current()->PostDelayedTask(
> On 2010/11/24 23:56:09, apatrick_chromium wrote:
>>
>> Rather than repeatedly poll for progress every 1ms, you could post the
>
> task to
>>
>> continue processing this command buffer from the IPC message handler
>
> for the
>>
>> SwapBuffers ack.
>
> Thanks, this is an excellent suggestion and cleans up an area I was not
> comfortable with. I've retested with this change.
>
> http://codereview.chromium.org/5317007/
>

[nduca, 2010-11-26 19:07:36]

LGTM.

Dying to get some actual timing diagrams made for this to see how this looks in
practice.

http://codereview.chromium.org/5317007/diff/34003/chrome/renderer/ggl/ggl.cc
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/5317007/diff/34003/chrome/renderer/ggl/ggl.cc#...
chrome/renderer/ggl/ggl.cc:38: // in the general case. On Mac OS X it seems we
really want this to be 1,
Just checking my understanding, the 1 is because the IOSurface isnt' double
buffered so >1 would cause us to render into the IOSurface before it is
presented?

[Ken Russell (switch to Gerrit), 2010-11-26 20:23:09]

http://codereview.chromium.org/5317007/diff/34003/chrome/renderer/ggl/ggl.cc
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/5317007/diff/34003/chrome/renderer/ggl/ggl.cc#...
chrome/renderer/ggl/ggl.cc:38: // in the general case. On Mac OS X it seems we
really want this to be 1,
On 2010/11/26 19:07:37, nduca wrote:
> Just checking my understanding, the 1 is because the IOSurface isnt' double
> buffered so >1 would cause us to render into the IOSurface before it is
> presented?

No. It's because if we increase it to 2 then the renderer process races ahead
and produces 2 frames for each 1 that is actually displayed to the screen. For
at least one of our test cases this makes JavaScript think that it's rendering
at 120 FPS when in fact it's only rendering at 60 FPS. I think we need to get to
a baseline where JavaScript has a correct notion of the frame rate, which is
achieved by setting this to 1 as well as the number of outstanding swap buffers
calls in gpu_processor.cc to 1.