Make --disable-gpu-vsync work with CoreAnimation

Make --disable-gpu-vsync work with CoreAnimation

CoreAnimation provides GPU backpressure from the browser process to the
renderer process by only acknowledging frames when they are displayed.
This mechanism has the side-effect of throttling the renderer to about
vsync, because the layer will not request to be displayed much more
than once per vsync.

Relieve this pressure by not going into isAsynchronous mode when
receiving new frames when vsync is disabled.

This exposed an issue where unluckily-timed setNeedsDisplay calls could
be dropped on the floor, hanging the renderer because it would never
get its ack. Work around this by calling setNeedsDisplay and
displayIfNeeded explicitly, and, if the draw is not triggered by that,
sending the ack explicitly. This bug has never triggered without vsync
disabled, but apply the workaround in both situations, to be sure.

Use a ScopedClosureRunner to ensure that the swap ack be issued in
CompositorSwapBuffers, rather than sprinkling ack calls all over the
function. Remove the TODOs because the scoped runner should solve
the problem they were working around.

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

[ccameron, 2014-02-20 10:42:58]

Ptal. Thanks!

https://codereview.chromium.org/171513012/diff/60001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_layer_mac.mm (right):

https://codereview.chromium.org/171513012/diff/60001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:197: 
This is to match the behavior of WebKit. The claim is that the super method will
trigger a required flush (I have not observed a change in behavior).

[Ken Russell (switch to Gerrit), 2014-02-20 23:01:52]

https://codereview.chromium.org/171513012/diff/100001/content/browser/rendere...
File content/browser/renderer_host/compositing_iosurface_layer_mac.mm (right):

https://codereview.chromium.org/171513012/diff/100001/content/browser/rendere...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:71:
renderWidgetHostView_->SendPendingSwapAck();
If we're mostly sending swap acks upon receiving new frames (gotNewFrame,
timerSinceGotNewFrameFired), is it still necessary to do so inside
CompositorSwapBuffers, which is after the frame is processed (if I remember
correctly)?

Should the code be consistent about where the swap acks are sent? If the system
is fragile to even one lost swap ack, then it seems to me that we should ack
upon receipt of each frame, and never when it's actually processed, if frames
can be dropped by Core Animation for throttling purposes.

[ccameron, 2014-02-21 00:48:23]

https://codereview.chromium.org/171513012/diff/100001/content/browser/rendere...
File content/browser/renderer_host/compositing_iosurface_layer_mac.mm (right):

https://codereview.chromium.org/171513012/diff/100001/content/browser/rendere...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:71:
renderWidgetHostView_->SendPendingSwapAck();
On 2014/02/20 23:01:53, Ken Russell wrote:
> If we're mostly sending swap acks upon receiving new frames (gotNewFrame,
> timerSinceGotNewFrameFired), is it still necessary to do so inside
> CompositorSwapBuffers, which is after the frame is processed (if I remember
> correctly)?

CompositorSwapBuffers only does the ack if the frame will not be drawn (it's
hidden, the window is 0x0 in size, etc). CompositorSwapBuffers will not do the
ack if the frame is expected to be drawn -- it will wait for the actual draw
function to complete before doing the ack.

> Should the code be consistent about where the swap acks are sent? If the
system
> is fragile to even one lost swap ack, then it seems to me that we should ack
> upon receipt of each frame, and never when it's actually processed, if frames
> can be dropped by Core Animation for throttling purposes.

The acks are the mechanism through which the browser can signal back-pressure to
the renderer. I asked brianderson and nduca about this, because it would require
less thought to just ack immediately and communicate backpressure in some way
that would consistently drop the renderer, back to 30fps (for example).

I trust isAsynchronous:YES to always get called in short order (if it doesn't,
we have all sorts of serious issues).

I don't trust setNeedsDisplay, and so I always do the
setNeedsDisplay+displayIfNeeded+ackIfNotDisplayed triplet. This happens in two
cases -- (1) vsync-disabled, which is not-optimal-user-experience territory and
(2) the edge case where you happen to drop out of animating mode while a frame
is pending, a case that will only happen if you somehow suspend the run loop for
a second at the exact wrong moment (highly unlikely, but could happen in
theory).

[Ken Russell (switch to Gerrit), 2014-02-21 22:21:50]

OK. Thanks for the careful thought about this. I trust your judgment. LGTM

[ccameron, 2014-02-21 22:47:02]

Thanks. Hopefully reality will corroborate my model of this.

[ccameron, 2014-02-21 22:47:14]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-02-21 22:48:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/171513012/100001

[commit-bot: I haz the power, 2014-02-22 00:44:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-22 00:44:45]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[ccameron, 2014-02-22 00:48:26]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-02-22 00:50:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/171513012/100001

[commit-bot: I haz the power, 2014-02-22 01:35:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/171513012/100001

[commit-bot: I haz the power, 2014-02-22 08:33:48]

Change committed as 252784