cc: Activate when going invisible.

cc: Activate when going invisible.

This patch ensures that we always activate when we become invisible.
The reason being is that when we come back to being visible again,
we don't need to rasterize both active and pending trees. We can
just rasterize what used to be the pending tree.

Since we set RequiresHighResToDraw on becoming visible, this also
ensures that we don't checkerboard on becoming visible.

BUG=410000, 417598
R=danakj, enne, brianderson

Committed: https://crrev.com/6b968c06b3641d5cb45917039de447f8337edd4c
Cr-Commit-Position: refs/heads/master@{#297303}

[vmpstr, 2014-09-25 18:35:44]

PTAL.

This ensures that we activate when becoming invisible and seems to fix the
checkerboard even without manage before draw thing.

[danakj, 2014-09-25 18:38:32]

https://codereview.chromium.org/605823002/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/605823002/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:5060: ++activation_count_;
can you test that RequiresHighRes was true when we became visible? Also, is it
racey or can you guarantee we'll activate before visible happens and we
SetRequiresHighRes? Or test that resource tiles are used here not checkerboards?

There should be a scheduler unit test of some sort too.

[danakj, 2014-09-25 18:40:27]

https://codereview.chromium.org/605823002/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/605823002/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:5060: ++activation_count_;
On 2014/09/25 18:38:32, danakj wrote:
> can you test that RequiresHighRes was true when we became visible? Also, is it
> racey or can you guarantee we'll activate before visible happens and we
> SetRequiresHighRes? Or test that resource tiles are used here not
checkerboards?
> 
> There should be a scheduler unit test of some sort too.

Maybe the scheduler test could demonstrate that it's not racey and it'll always
do the activate before making things visible again?

[brianderson, 2014-09-25 18:47:29]

Can you add a test to scheduler_unittests.cc that tests the visibility behavior.
You can probably copy one of the DidLoseOutputSurface tests as a starting point.

https://codereview.chromium.org/605823002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/605823002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:289: if (!visible_)
This can be deleted and the comment above should be updated since we don't
actually need to worry about checkerboard anymore with the high res flag and all
that.

https://codereview.chromium.org/605823002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:302: // If we're not visible, we should
force activation.
Can you explain the whole checkerboarding issue?

Also, the comments in PendingActivationsShouldBeForced and
PendingDrawsShouldBeAborted indicate that they are only used to avoid deadlock,
which will no longer be true.

[vmpstr, 2014-09-25 19:42:12]

PTAL

https://codereview.chromium.org/605823002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/605823002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:289: if (!visible_)
On 2014/09/25 18:47:29, brianderson wrote:
> This can be deleted and the comment above should be updated since we don't
> actually need to worry about checkerboard anymore with the high res flag and
all
> that.

Done.

https://codereview.chromium.org/605823002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:302: // If we're not visible, we should
force activation.
On 2014/09/25 18:47:29, brianderson wrote:
> Can you explain the whole checkerboarding issue?
> 
> Also, the comments in PendingActivationsShouldBeForced and
> PendingDrawsShouldBeAborted indicate that they are only used to avoid
deadlock,
> which will no longer be true.

Done.

https://codereview.chromium.org/605823002/diff/1/cc/trees/layer_tree_host_uni...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/605823002/diff/1/cc/trees/layer_tree_host_uni...
cc/trees/layer_tree_host_unittest.cc:5060: ++activation_count_;
On 2014/09/25 18:38:32, danakj wrote:
> can you test that RequiresHighRes was true when we became visible? Also, is it
> racey or can you guarantee we'll activate before visible happens and we
> SetRequiresHighRes? Or test that resource tiles are used here not
checkerboards?
> 
> There should be a scheduler unit test of some sort too.

The activation with this change happens as a part of becoming not visible, so
it's not racey. I will test for RequiresHighRes though.

(and I'll do a scheduler test)

[brianderson, 2014-09-25 20:07:42]

https://codereview.chromium.org/605823002/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/605823002/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:287: if (!can_draw_)
Do we also set the high res required to draw flag on the pos edge of can_draw_?

If so, should we force activation in this case too? It might make resize more
responsive if we have less tiles to manage and rasterize.

If not, should we?

Then PendingDrawsShouldBeAborted and PendingActivationsShouldBeForced would be
the same thing.

[danakj, 2014-09-25 20:16:07]

https://codereview.chromium.org/605823002/diff/20001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/605823002/diff/20001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:287: if (!can_draw_)
On 2014/09/25 20:07:42, brianderson wrote:
> Do we also set the high res required to draw flag on the pos edge of
can_draw_?
> 
> If so, should we force activation in this case too? It might make resize more
> responsive if we have less tiles to manage and rasterize.
> 
> If not, should we?
> 
> Then PendingDrawsShouldBeAborted and PendingActivationsShouldBeForced would be
> the same thing.

That's a good question. We recently had to add a SetRequiresHighRes when making
a output surface, which is another case of CanDraw.

Having no root layer -> checkerboards sounds wrong. I think we guard against
that by checking for a twin when marking things active right now.

The other one is the viewport size changing. Then we stop drawing until we
activate. In that case it's less clear to me that we want to force high res to
complete if you were looking at low res. But it's also a super corner case and I
don't feel strongly if we were to do that.

[vmpstr, 2014-09-25 21:59:36]

On 2014/09/25 20:16:07, danakj wrote:
>
https://codereview.chromium.org/605823002/diff/20001/cc/scheduler/scheduler_s...
> File cc/scheduler/scheduler_state_machine.cc (left):
> 
>
https://codereview.chromium.org/605823002/diff/20001/cc/scheduler/scheduler_s...
> cc/scheduler/scheduler_state_machine.cc:287: if (!can_draw_)
> On 2014/09/25 20:07:42, brianderson wrote:
> > Do we also set the high res required to draw flag on the pos edge of
> can_draw_?
> > 
> > If so, should we force activation in this case too? It might make resize
more
> > responsive if we have less tiles to manage and rasterize.
> > 
> > If not, should we?
> > 
> > Then PendingDrawsShouldBeAborted and PendingActivationsShouldBeForced would
be
> > the same thing.
> 
> That's a good question. We recently had to add a SetRequiresHighRes when
making
> a output surface, which is another case of CanDraw.
> 
> Having no root layer -> checkerboards sounds wrong. I think we guard against
> that by checking for a twin when marking things active right now.
> 
> The other one is the viewport size changing. Then we stop drawing until we
> activate. In that case it's less clear to me that we want to force high res to
> complete if you were looking at low res. But it's also a super corner case and
I
> don't feel strongly if we were to do that.

Would you be ok with me filing a bug to investigate this? I'd like to land this
patch separately as a fix for visible/invisible checkerboard thing.

[danakj, 2014-09-25 22:02:25]

On Thu, Sep 25, 2014 at 5:59 PM, <vmpstr@chromium.org> wrote:

> On 2014/09/25 20:16:07, danakj wrote:
>
> https://codereview.chromium.org/605823002/diff/20001/cc/
> scheduler/scheduler_state_machine.cc
>
>> File cc/scheduler/scheduler_state_machine.cc (left):
>>
>
>
> https://codereview.chromium.org/605823002/diff/20001/cc/
> scheduler/scheduler_state_machine.cc#oldcode287
>
>> cc/scheduler/scheduler_state_machine.cc:287: if (!can_draw_)
>> On 2014/09/25 20:07:42, brianderson wrote:
>> > Do we also set the high res required to draw flag on the pos edge of
>> can_draw_?
>> >
>> > If so, should we force activation in this case too? It might make resize
>>
> more
>
>> > responsive if we have less tiles to manage and rasterize.
>> >
>> > If not, should we?
>> >
>> > Then PendingDrawsShouldBeAborted and PendingActivationsShouldBeForced
>> would
>>
> be
>
>> > the same thing.
>>
>
>  That's a good question. We recently had to add a SetRequiresHighRes when
>>
> making
>
>> a output surface, which is another case of CanDraw.
>>
>
>  Having no root layer -> checkerboards sounds wrong. I think we guard
>> against
>> that by checking for a twin when marking things active right now.
>>
>
>  The other one is the viewport size changing. Then we stop drawing until we
>> activate. In that case it's less clear to me that we want to force high
>> res to
>> complete if you were looking at low res. But it's also a super corner
>> case and
>>
> I
>
>> don't feel strongly if we were to do that.
>>
>
> Would you be ok with me filing a bug to investigate this? I'd like to land
> this
> patch separately as a fix for visible/invisible checkerboard thing.
>

Please do :) And thanks!


> https://codereview.chromium.org/605823002/
>

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

[vmpstr, 2014-09-25 22:05:46]

Filed crbug.com/417878. (ptal this one though :P)

[brianderson, 2014-09-25 23:45:50]

lgtm

[danakj, 2014-09-29 16:54:08]

LGTM

[vmpstr, 2014-09-29 16:56:32]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-09-29 16:57:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/605823002/20001

[commit-bot: I haz the power, 2014-09-29 17:00:41]

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

[commit-bot: I haz the power, 2014-09-29 17:00:42]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/61917)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vmpstr, 2014-09-29 17:59:17]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-09-29 18:00:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/605823002/40001

[commit-bot: I haz the power, 2014-09-29 23:34:57]

Committed patchset #3 (id:40001) as ac0fceb1e4b1fcd74a69559de3083be4ef8fcfce

[commit-bot: I haz the power, 2014-09-29 23:35:54]

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