cc: Don't recreate lost OutputSurface when hidden

cc: Don't recreate lost OutputSurface when hidden

On Android when Chrome is backgrounded, the OOM killer might
kill the GPU process. In that case let's avoid respinning it
until we become visible again.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/837db3422b25064ee2278ef822c95a83877db3d2
Cr-Commit-Position: refs/heads/master@{#349014}

[no sievers, 2015-09-11 00:48:52]

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

[no sievers, 2015-09-11 00:51:04]

Brian, does this look ok to you?

I'm just worried that this might cause some weird corner-case interactions with
the OOM killer if we immediately try to respin the process when the system is
under memory pressure and we're not even foregrounded.

It maybe makes https://codereview.chromium.org/1287043002/ a bit nicer too if we
defer RequestNewOutputSurface() until we become visible again.

[brianderson, 2015-09-12 00:34:22]

Yep - lgtm!

[danakj, 2015-09-14 18:21:15]

On Fri, Sep 11, 2015 at 5:34 PM, <brianderson@chromium.org> wrote:

> Yep - lgtm!
>

Some test tho please? :)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2015-09-14 22:45:37]

On 2015/09/14 18:21:15, danakj wrote:
> On Fri, Sep 11, 2015 at 5:34 PM, <mailto:brianderson@chromium.org> wrote:
> 
> > Yep - lgtm!
> >
> 
> Some test tho please? :)

I've added TestNoRequestOutputSurfaceWhenInvisible. But I also had to update a
bunch of tests, sorry for missing that earlier.

Brian, can you take another look at the test changes too?

[brianderson, 2015-09-15 18:37:46]

lgtm with one nit.

https://codereview.chromium.org/1336813002/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/1336813002/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1195:
TEST(SchedulerStateMachineTest, TestRequestCommitInvisible) {
While you are at it, can you rename this test to
TestNoRequestCommitWhenInvisible?

https://codereview.chromium.org/1336813002/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1213: state.SetVisible(true);
Above this line, can you also check for ACTION_NONE with a comment indicating
that we also shouldn't request an output surface during initialization until we
are visible?

Interestingly, I was just looking at
https://code.google.com/p/chromium/issues/detail?id=163696 doing GPU bug triage.

During tab restore, every new tab creates its contexts simultaneously. This
patch by itself will not change that behavior since, strangely, every Renderer
pulses with SetVisible(true) for a short period during initialization.

If we remove the SetVisible(true) pulse though we could likely reduce the CPU
fighting slightly for the initial tab restore at the cost of first tab switch
responsiveness. I'd be happy with that tradeoff since recent work to improve
session restore has shown that most restored tabs are never looked at.

Should we open up a followup bug?

[no sievers, 2015-09-15 20:10:20]

https://codereview.chromium.org/1336813002/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/1336813002/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1195:
TEST(SchedulerStateMachineTest, TestRequestCommitInvisible) {
On 2015/09/15 18:37:46, brianderson wrote:
> While you are at it, can you rename this test to
> TestNoRequestCommitWhenInvisible?

Done.

https://codereview.chromium.org/1336813002/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:1213: state.SetVisible(true);
On 2015/09/15 18:37:46, brianderson wrote:
> Above this line, can you also check for ACTION_NONE with a comment indicating
> that we also shouldn't request an output surface during initialization until
we
> are visible?
> 

Done. Note that the default for LayerTreeHost is visible though (unlike the
scheduler) and we will eventually set the scheduler's visibility to match LTH's.
I can make an attempt to change LTH to default to invisible also, which I think
would allow us to remove SetLTHReady/SetCanStart() (since instead you can just
not make it visible then).



> Interestingly, I was just looking at
> https://code.google.com/p/chromium/issues/detail?id=163696 doing GPU bug
triage.
> 
> During tab restore, every new tab creates its contexts simultaneously. This
> patch by itself will not change that behavior since, strangely, every Renderer
> pulses with SetVisible(true) for a short period during initialization.
> 
> If we remove the SetVisible(true) pulse though we could likely reduce the CPU
> fighting slightly for the initial tab restore at the cost of first tab switch
> responsiveness. I'd be happy with that tradeoff since recent work to improve
> session restore has shown that most restored tabs are never looked at.
> 
> Should we open up a followup bug?

Sounds good. I assume LTH defaulting to visible could then actually be part of
the problem here.

[no sievers, 2015-09-15 20:10:52]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-09-15 20:10:52]

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/1336813002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2015-09-15 20:11:06]

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

[no sievers, 2015-09-15 20:14:31]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2015-09-15 20:15:56]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-09-15 20:15:56]

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/1336813002/#ps70003
(title: " ")

[commit-bot: I haz the power, 2015-09-15 20:16:51]

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

[commit-bot: I haz the power, 2015-09-15 22:06:40]

Committed patchset #5 (id:70003)

[commit-bot: I haz the power, 2015-09-15 22:07:14]

Patchset 5 (id:??) landed as
https://crrev.com/837db3422b25064ee2278ef822c95a83877db3d2
Cr-Commit-Position: refs/heads/master@{#349014}

[commit-bot: I haz the power, 2015-09-23 12:50:31]

Patchset 5 (id:??) landed as
https://crrev.com/837db3422b25064ee2278ef822c95a83877db3d2
Cr-Commit-Position: refs/heads/master@{#349014}