android: Remove custom browser compositor scheduling logic

android: Remove custom browser compositor scheduling logic

Use cc scheduler instead without a deadline.
This is mostly obsolete now that Surfaces allow independent updates
between renderer and browser.

BUG=455894
Committed: https://crrev.com/7f045e4f2e9f7142c79e2eeb30b70716a1ea8f0b
Cr-Commit-Position: refs/heads/master@{#365861}

[no sievers, 2015-12-12 00:44:47]

Description was changed from

==========
android: Remove custom browser compositor scheduling logic

Use cc scheduler instead without a deadline.
This is mostly obsolete now that Surfaces allow independent updates
between renderer and browser.

BUG=455894
==========

to

==========
android: Remove custom browser compositor scheduling logic

Use cc scheduler instead without a deadline.
This is mostly obsolete now that Surfaces allow independent updates
between renderer and browser.

BUG=455894
==========

[no sievers, 2015-12-12 00:44:47]

sievers@chromium.org changed reviewers:
+ brianderson@chromium.org

[no sievers, 2015-12-12 00:45:02]

Hey Brian, can you take a look?

[brianderson, 2015-12-12 01:41:46]

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (left):

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:484: if
(readback_pending) {
Does this break readbacks at all?

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:178: if
(NeedsBeginFrames()) {
This check shouldn't be needed. The scheduler should be immune to spurious
BeginFrames, and actually wants them in case it suddenly decides it wants
BeginFrames.

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:401:
host_->SetNeedsCommit();
Is this needed? I think the impl side requests it if resources have been
discarded or if the output surface is lost.

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:432:
host_->SetNeedsCommit();
Was this why the SetNeedsCommit was needed above? Maybe remove the visibility
check?

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:609:
client_->OnSwapBuffersCompleted(pending_swapbuffers_);
It doesn't look like any implementers look at pending_swapbuffers_. Can you
delete the pending_swapbuffers_ member variable?

Also, can this be removed all together? I don't understand what needs to be tied
to when a swap occurs. What happens if there's no damage and we don't actually
swap?

[brianderson, 2015-12-14 20:14:26]

cc+=mithro, since this is related to how the Browser content will interact with
the DisplayScheduler on Android.

[no sievers, 2015-12-17 00:52:32]

Brian, can you take another look?

This needs https://codereview.chromium.org/1531153002/ to fix the flickering I
mentioned.

Otherwise regarding the commit and draw happen in separate tasks: that's just
how scheduler does it (it always posts a task in ScheduleBeginImplFrameDeadline
even if there is no deadline). I cannot really claim that that is a major
problem though. There'd obviously be other cases where we might delay draw (like
being swap limited).

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (left):

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:484: if
(readback_pending) {
On 2015/12/12 01:41:46, brianderson wrote:
> Does this break readbacks at all?

The pending requests will still signal completion (but not success) when layers
are torn down. I'm not so worried about this now, since I only added it
speculatively a while back when there was talk about a use case for doing
readbacks while hiding the activity (which made pending readbacks here likely in
theory), but we actually didn't end up making that change.

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:178: if
(NeedsBeginFrames()) {
On 2015/12/12 01:41:46, brianderson wrote:
> This check shouldn't be needed. The scheduler should be immune to spurious
> BeginFrames, and actually wants them in case it suddenly decides it wants
> BeginFrames.

Done.

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:401:
host_->SetNeedsCommit();
On 2015/12/12 01:41:46, brianderson wrote:
> Is this needed? I think the impl side requests it if resources have been
> discarded or if the output surface is lost.

You're right, not needed. Removed.

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:432:
host_->SetNeedsCommit();
On 2015/12/12 01:41:46, brianderson wrote:
> Was this why the SetNeedsCommit was needed above? Maybe remove the visibility
> check?

This is actually what the browser will call if it wants to draw (let's say for a
tab stack animation or what not). I had to change it to SetNeedsAnimate(), which
is actually the right thing, since SetNeedsCommit() is ignored during
UpdateLTH().
What the caller is trying to say here is 'call me again next frame when I update
my animation (i.e. I want to update layer properties in the next frame instead
of doing any invalidation now)'.

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:609:
client_->OnSwapBuffersCompleted(pending_swapbuffers_);
On 2015/12/12 01:41:46, brianderson wrote:
> It doesn't look like any implementers look at pending_swapbuffers_. Can you
> delete the pending_swapbuffers_ member variable?
> 
> Also, can this be removed all together? I don't understand what needs to be
tied
> to when a swap occurs. What happens if there's no damage and we don't actually
> swap?

Unfortunately this is used in CompositorViewHolder.java, see
onSwapBuffersCompleted() where |pendingSwapBuffersCount == 0| is used to detect
when we go idle here in order to flush pending invalidations for Java Views
(IIRC).

[no sievers, 2015-12-17 01:10:34]

The CQ bit was checked by sievers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 02:35:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513933003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513933003/60001

[commit-bot: I haz the power, 2015-12-17 03:40:20]

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

[commit-bot: I haz the power, 2015-12-17 03:40:20]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[brianderson, 2015-12-17 06:14:53]

lgtm

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/1513933003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:609:
client_->OnSwapBuffersCompleted(pending_swapbuffers_);
On 2015/12/17 00:52:32, sievers wrote:
> On 2015/12/12 01:41:46, brianderson wrote:
> > It doesn't look like any implementers look at pending_swapbuffers_. Can you
> > delete the pending_swapbuffers_ member variable?
> > 
> > Also, can this be removed all together? I don't understand what needs to be
> tied
> > to when a swap occurs. What happens if there's no damage and we don't
actually
> > swap?
> 
> Unfortunately this is used in CompositorViewHolder.java, see
> onSwapBuffersCompleted() where |pendingSwapBuffersCount == 0| is used to
detect
> when we go idle here in order to flush pending invalidations for Java Views
> (IIRC).

Ok. I see where it's used now and am okay keeping the code as-is for this patch.
It seems like an odd signal to rely on though. I wonder if it makes more sense
to pipe the BeginImplFrameNotExpectedSoon event up (as a content-is-idle signal)
to flush invalidations.

[no sievers, 2015-12-17 17:15:23]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-12-17 17:15:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1513933003/#ps80001
(title: "clang")

[commit-bot: I haz the power, 2015-12-17 17:15:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513933003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513933003/80001

[commit-bot: I haz the power, 2015-12-17 18:03:38]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[no sievers, 2015-12-17 18:03:49]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-12-17 18:03:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1513933003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-12-17 18:05:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513933003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513933003/100001

[commit-bot: I haz the power, 2015-12-17 19:17:12]

Description was changed from

==========
android: Remove custom browser compositor scheduling logic

Use cc scheduler instead without a deadline.
This is mostly obsolete now that Surfaces allow independent updates
between renderer and browser.

BUG=455894
==========

to

==========
android: Remove custom browser compositor scheduling logic

Use cc scheduler instead without a deadline.
This is mostly obsolete now that Surfaces allow independent updates
between renderer and browser.

BUG=455894
==========

[commit-bot: I haz the power, 2015-12-17 19:17:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-17 19:18:02]

Description was changed from

==========
android: Remove custom browser compositor scheduling logic

Use cc scheduler instead without a deadline.
This is mostly obsolete now that Surfaces allow independent updates
between renderer and browser.

BUG=455894
==========

to

==========
android: Remove custom browser compositor scheduling logic

Use cc scheduler instead without a deadline.
This is mostly obsolete now that Surfaces allow independent updates
between renderer and browser.

BUG=455894

Committed: https://crrev.com/7f045e4f2e9f7142c79e2eeb30b70716a1ea8f0b
Cr-Commit-Position: refs/heads/master@{#365861}
==========

[commit-bot: I haz the power, 2015-12-17 19:18:03]

Patchset 6 (id:??) landed as
https://crrev.com/7f045e4f2e9f7142c79e2eeb30b70716a1ea8f0b
Cr-Commit-Position: refs/heads/master@{#365861}

[gone, 2015-12-21 19:15:28]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1536323003/ by dfalcantara@chromium.org.

The reason for reverting is: Speculatively reverting CL to try and fix observer
flakiness.
https://crbug.com/571030

Apologies if I'm off base!.