Make Android CompositorImpl use scheduled SingleThreadProxy

Make Android CompositorImpl use scheduled SingleThreadProxy

This is a follow-up to https://codereview.chromium.org/134623005/ to
remove the manual scheduling from CompositorImpl and instead use the
scheduler.

BUG=437603

[enne (OOO), 2014-06-19 01:09:27]

This is a rough draft followup to the SingleThreadProxy/SchedulerClient change,
to handle the CompositorImpl issues that were punted on there.

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

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:429:
root_window_->RequestVSyncUpdate();
This maybe could be in a better place.

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:455: // TODO(enne):
Does this need to get piped to output surface somehow?
brianderson: I'm a little unclear about vsync parameters.  Is this something
that this compositor should be responsible for somehow routing to
Scheduler::CommitVSyncParameters, or does that happen elsewhere that I'm not
seeing?

[no sievers, 2014-06-19 17:13:26]

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

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:313:
weak_factory_.GetWeakPtr()));
We then need to defer the host_->SetLayerTreeHostClientReady() from line 299 to
OnGpuChannelEstablished() also or factory->GetGpuChannel() in
CreateOutputSurface() races with this and might return NULL.

Also it wouldn't work when we lost the channel.
Unfortunately on Android we also cannot use EstablishGpuChannelSync when you run
the GPU thread in its own process, since it would deadlock (opening the channel
needs to synchronously go back from the gpu thread to the main UI thread here).

That's why I had the bail-out hack in Composite(), but now it seems we need to
propagate this to cc to not try to create the OutputSurface after we lost the
context and until we are ready again.

[no sievers, 2014-06-19 17:49:22]

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

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:286:
settings.highp_threshold_min = 2048;
If we set...
settings.throttle_frame_production = true;
settings.begin_frame_scheduling_enabled = true;

...and then above make DirectOutputSurface above implement SetNeedsBeginFrame()
which requests the vsync update...

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:429:
root_window_->RequestVSyncUpdate();
On 2014/06/19 01:09:27, enne wrote:
> This maybe could be in a better place.

...then line 429 can go away here...

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:455: // TODO(enne):
Does this need to get piped to output surface somehow?
On 2014/06/19 01:09:27, enne wrote:
> brianderson: I'm a little unclear about vsync parameters.  Is this something
> that this compositor should be responsible for somehow routing to
> Scheduler::CommitVSyncParameters, or does that happen elsewhere that I'm not
> seeing?

...and it would take care of the vsync scheduling. When OnVSync happens and
|needs_begin_frame| is true, then we should call
OutputSurfaceClient(LTHImpl)::BeginFrame().

Brian, does that make sense at all?

[brianderson, 2014-06-24 23:50:13]

https://codereview.chromium.org/344743002/diff/1/cc/trees/single_thread_proxy.cc
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/344743002/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:146: SetNeedsCommit();
Until we have commit-less updates, we'll should introduce the concept of
ScheduleAnimation to cc's scheduler. Maybe expose it as SetWantsCommit. We can
save that for a follow up to this patch though.

https://codereview.chromium.org/344743002/diff/1/cc/trees/single_thread_proxy...
cc/trees/single_thread_proxy.cc:259: DCHECK(scheduler_on_impl_thread_);
Why is this DCHECK no longer valid?

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

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:214:
host_->SetNeedsCommit();
Should this be SetNeedsRedraw? If this is meant to trigger the first commit, the
scheduler will do that automatically when there is a new OutputSurface.

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:286:
settings.highp_threshold_min = 2048;
On 2014/06/19 17:49:21, sievers wrote:
> If we set...
> settings.throttle_frame_production = true;
> settings.begin_frame_scheduling_enabled = true;
> 
> ...and then above make DirectOutputSurface above implement
SetNeedsBeginFrame()
> which requests the vsync update...

+1. I think DirectOutputSurface is now known as OutputSurfaceWithoutParent?

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:426:
client_->OnSwapBuffersCompleted(pending_swapbuffers_);
Can we get rid of pending_swapbuffers_? Implementations of
OnSwapBuffersCompleted all seem to ignore the argument anyway.

Then we can get rid of CompositorImpl::DidPostSwapBuffers().

Also, do we need to call client_->OnSwapBuffersCompleted() here? It looks like
Android only uses it to call mSurfaceView.SetBackgroundResource(0):
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...

@sievers: What is that for? Can it be triggered any other way?

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:455: // TODO(enne):
Does this need to get piped to output surface somehow?
On 2014/06/19 17:49:21, sievers wrote:
> On 2014/06/19 01:09:27, enne wrote:
> > brianderson: I'm a little unclear about vsync parameters.  Is this something
> > that this compositor should be responsible for somehow routing to
> > Scheduler::CommitVSyncParameters, or does that happen elsewhere that I'm not
> > seeing?
> 
> ...and it would take care of the vsync scheduling. When OnVSync happens and
> |needs_begin_frame| is true, then we should call
> OutputSurfaceClient(LTHImpl)::BeginFrame().
> 
> Brian, does that make sense at all?

Yes, that makes sense and is how the scheduler expects the OutputSurface to
behave.

root_window_->RequestVSyncUpdate() should be called in two cases:
SetNeedsBeginFrame goes from false to true.
Immediately from the vsync callback when if SetNeedsBeginFrame is still true.
The scheduler will automatically handle BeginFrames that pile up.

[no sievers, 2014-06-25 00:03:26]

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

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:426:
client_->OnSwapBuffersCompleted(pending_swapbuffers_);
On 2014/06/24 23:50:12, brianderson wrote:
> Can we get rid of pending_swapbuffers_? Implementations of
> OnSwapBuffersCompleted all seem to ignore the argument anyway.
> 
> Then we can get rid of CompositorImpl::DidPostSwapBuffers().
> 
> Also, do we need to call client_->OnSwapBuffersCompleted() here? It looks like
> Android only uses it to call mSurfaceView.SetBackgroundResource(0):
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...
> 
> @sievers: What is that for? Can it be triggered any other way?

There is one call site in Chrome for Android, where it checks
pending_swapbuffers_ to be 0 to defer and throttle native Android view
invalidations. However, I think we can do that better now without having to look
at swap completion.

[danakj, 2014-06-25 17:28:54]

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

https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:214:
host_->SetNeedsCommit();
On 2014/06/24 23:50:12, brianderson wrote:
> Should this be SetNeedsRedraw? If this is meant to trigger the first commit,
the
> scheduler will do that automatically when there is a new OutputSurface.

This should probably just go away?

[mithro-old, 2014-07-02 06:09:54]

On 2014/06/25 17:28:54, danakj wrote:
>
https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/compositor_impl_android.cc (right):
> 
>
https://codereview.chromium.org/344743002/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/compositor_impl_android.cc:214:
> host_->SetNeedsCommit();
> On 2014/06/24 23:50:12, brianderson wrote:
> > Should this be SetNeedsRedraw? If this is meant to trigger the first commit,
> the
> > scheduler will do that automatically when there is a new OutputSurface.
> 
> This should probably just go away?

This looks like a great patch! Any chance it'll be able to land soon?

Tim

[enne (OOO), 2014-07-02 17:31:01]

On 2014/07/02 06:09:54, mithro wrote:

> This looks like a great patch! Any chance it'll be able to land soon?

Not too soon.  It depends on https://codereview.chromium.org/134623005/ and
https://codereview.chromium.org/348093004/.