Do not send a frame swap ack from the browser until the frame is drawn

Do not send a frame swap ack from the browser until the frame is drawn

Code to do this was recently removed because it was no longer being
used in r241225 (https://codereview.chromium.org/116103002). This is
now necessary again to throttle renderers when using CoreAnimation.

Frames are acked only when they are drawn. Both the CoreAnimation and
the non-CoreAnimation paths are changed to behave this way. By virtue
of the fact that the non-CoreAnimation path draws immediately, this
should have no functional effect on that path.

This differs from the mechanism deleted in the aforementioned patch in
two ways.

First, it uses a scoped_ptr of a struct instead of a vector of pairs to
store the information about the swap being returned. This should improve
readability in that the ack has struct names instead of just first and
second in the pair, and in that the scoped_ptr does not suggest support
for multiple pending swaps (which does not exist).

Second, it does makes RWHVMac ack frames more aggressively when
inside a draw call, rather than adding a call from RWHImpl to the view,
to effect the ack (which is what the old way does).

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

[ccameron, 2014-02-14 01:31:41]

I'm considering a "I delete the code and re-commit it" meme...

[piman, 2014-02-14 02:15:56]

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:251: const
base::Closure& about_to_wait_for_backing_store_callback);
Do you really need this? There's only a handful of callers of
GetBackingStore(true), you could SendPendingAcks before calling GetBackingStore
at these locations, no?

[ccameron, 2014-02-14 02:19:06]

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:868:
about_to_wait_for_backing_store_callback_.Run();
The call that we need it before is this guy. It's a loop that pumps the renderer
for frames until it gets one of the right size.

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.h:251: const
base::Closure& about_to_wait_for_backing_store_callback);
On 2014/02/14 02:15:57, piman wrote:
> Do you really need this? There's only a handful of callers of
> GetBackingStore(true), you could SendPendingAcks before calling
GetBackingStore
> at these locations, no?

See the remark in the .cc -- the caller we need is inside a loop inside RWHI

[piman, 2014-02-14 02:59:01]

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/165703002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:868:
about_to_wait_for_backing_store_callback_.Run();
On 2014/02/14 02:19:06, ccameron1 wrote:
> The call that we need it before is this guy. It's a loop that pumps the
renderer
> for frames until it gets one of the right size.

RWHVMac could send the ack right away when receiving the
AcceleratedBuffersSwapped if it's the wrong size?

You'd wrap the calls to GetBackingStore(true) by something like

BackingStoreMac* RWHVMac::GetBackingStoreSync() {
  waiting_for_backing_store_ = true;
  SendPendingAcks();
  BackingStoreMac* bs = rwh->GetBackingStore(true);
  waiting_for_backing_store_ = false;
  return bs;
}

Then in RWHVMac::AcceleratedSurfaceBuffersSwapped() {
  // [...]
  AddPendingSwapAck();     
  if (size != expected_size && waiting_for_backing_store_) {
    SendPendingAcks();
    return;
  }
}

That even lets you skip a bunch of the logic that you don't care about since
it's not a frame of the right size.

We do something essentially similar in RWHVAura, minus the "block the main
thread" part (instead we prevent the compositor from drawing). See the
RenderWidgetHostViewAura::ShouldSkipFrame and callers.

[ccameron, 2014-02-14 07:53:16]

Yes, you're right, that's the easy way to do it.

I didn't go so far as to try to elide work when we get a frame of the wrong size
-- there's not much work to elide -- we don't draw anything, and I'd prefer not
to assume the code works when I skip a few steps here and there.

https://codereview.chromium.org/165703002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/165703002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:442:
CHECK(!about_to_validate_and_paint_);
Last time I tried to enable CA there were lots of crashes. I can't 100% believe
that this can't happen (in either CA or not-CA), so this will help me at least
say "hey, it wasn't this".

[ccameron, 2014-02-14 19:30:44]

(oh, and ptal)

[ccameron, 2014-02-18 21:30:40]

Pinging again.

[piman, 2014-02-18 21:47:03]

lgtm

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

https://codereview.chromium.org/165703002/diff/340001/content/browser/rendere...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:22: // resize.
This may potentially pump the run loop waiting for a frame.
nit: I don't think we actually pump the loop - we just wait synchronously on the
renderer message (dispatching just a select few incoming ones).

https://codereview.chromium.org/165703002/diff/340001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/165703002/diff/340001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:442:
CHECK(!about_to_validate_and_paint_);
nit: DCHECK.

[ccameron, 2014-02-18 22:33:10]

Thanks!!

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

https://codereview.chromium.org/165703002/diff/340001/content/browser/rendere...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:22: // resize.
This may potentially pump the run loop waiting for a frame.
On 2014/02/18 21:47:04, piman wrote:
> nit: I don't think we actually pump the loop - we just wait synchronously on
the
> renderer message (dispatching just a select few incoming ones).

Changed to waitForResizedFrameInContext.

https://codereview.chromium.org/165703002/diff/340001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/165703002/diff/340001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:442:
CHECK(!about_to_validate_and_paint_);
On 2014/02/18 21:47:04, piman wrote:
> nit: DCHECK.

Done.

[ccameron, 2014-02-18 22:33:14]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-02-18 22:34:05]

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

[ccameron, 2014-02-18 22:56:57]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2014-02-18 23:10:31]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-02-18 23:13:58]

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

[commit-bot: I haz the power, 2014-02-19 01:57:27]

Change committed as 251918