LayerTreeHostImpl knows if it has an output surface.

LayerTreeHostImpl knows if it has an output surface.
Surface loss is only detected via the callback.

We can tolerate multiple calls to InitializeRenderer without
needing the lost-surface callback in between.

Remove Renderer::IsContextLost, but keep GLRenderer::IsContextLost.

Remove ThreadProxy::CheckOutputSurfaceStatusOnImplThread

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

[danakj, 2014-08-06 18:56:44]

https://codereview.chromium.org/446973002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/446973002/diff/1/cc/output/gl_renderer.h#newc...
cc/output/gl_renderer.h:68: virtual bool IsContextLost();
can be made private?

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:260: have_output_surface_(false),
nit: have_valid_output_surface_ or something along those lines. we might have
one (not null) but it's lost, so let's try make that distinction clear in this
name

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1731:
client_->DidLoseOutputSurfaceOnImplThread();
This will call over to ThreadProxy::DidLoseOutputSurfaceOnImplThread which will
call CheckOutputSurfaceStatusOnImplThread which will early out because
have_output_surface_ is still true. I'm kinda surprised that isn't breaking a
test? Can you set have_output_surface_ false before calling anything else in
this method?

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_impl.h
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.h:292: bool IsContextLost();
do you think we can get rid of this method too? The only caller of this left
would be in SingleThreadProxy after drawing, and there's no point really to it
anymore, since if we see true, it means we've already told the proxy about the
loss via the callback.

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:6841: // We should be able to tolerate
a re-initialization of the
I don't think this is true. It should never happen because we never go about
requesting a new outputsurface/renderer until we see the callback now.

[danakj, 2014-08-06 18:57:10]

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1731:
client_->DidLoseOutputSurfaceOnImplThread();
On 2014/08/06 18:56:44, danakj wrote:
> This will call over to ThreadProxy::DidLoseOutputSurfaceOnImplThread which
will
> call CheckOutputSurfaceStatusOnImplThread which will early out because
> have_output_surface_ is still true. I'm kinda surprised that isn't breaking a
> test? Can you set have_output_surface_ false before calling anything else in
> this method?

Sorry ignore this I saw you removed that call.

[dneto, 2014-08-06 20:20:36]

I'll have a new patch to rename the state variable to
have_valid_output_surface_.

I wonder if I should put the DCHECK back into InitializeRenderer and remove the
test that calls it twice in a row?

https://codereview.chromium.org/446973002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/446973002/diff/1/cc/output/gl_renderer.h#newc...
cc/output/gl_renderer.h:68: virtual bool IsContextLost();
On 2014/08/06 18:56:43, danakj wrote:
> can be made private?

It's used in unit tests, class GLRendererShaderPixelTest.  Plumbing that check
via different means would be more disruptive.

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:260: have_output_surface_(false),
On 2014/08/06 18:56:43, danakj wrote:
> nit: have_valid_output_surface_ or something along those lines. we might have
> one (not null) but it's lost, so let's try make that distinction clear in this
> name

Done.  Yeah, I thought about using "valid" in the name too.  I should have.

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1731:
client_->DidLoseOutputSurfaceOnImplThread();
On 2014/08/06 18:56:44, danakj wrote:
> This will call over to ThreadProxy::DidLoseOutputSurfaceOnImplThread which
will
> call CheckOutputSurfaceStatusOnImplThread which will early out because
> have_output_surface_ is still true. I'm kinda surprised that isn't breaking a
> test? Can you set have_output_surface_ false before calling anything else in
> this method?

Acknowledged.

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:1731:
client_->DidLoseOutputSurfaceOnImplThread();
On 2014/08/06 18:57:10, danakj wrote:
> On 2014/08/06 18:56:44, danakj wrote:
> > This will call over to ThreadProxy::DidLoseOutputSurfaceOnImplThread which
> will
> > call CheckOutputSurfaceStatusOnImplThread which will early out because
> > have_output_surface_ is still true. I'm kinda surprised that isn't breaking
a
> > test? Can you set have_output_surface_ false before calling anything else in
> > this method?
> 
> Sorry ignore this I saw you removed that call.

Acknowledged.
Also, for clarity I've moved the setting of the state variable to before we
notify the resource provider and renderer.

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:6841: // We should be able to tolerate
a re-initialization of the
On 2014/08/06 18:56:44, danakj wrote:
> I don't think this is true. It should never happen because we never go about
> requesting a new outputsurface/renderer until we see the callback now.

So should I remove this test and add the
DCHECK(!renderer_||!have_valid_output_surface_)
back into InitializeRenderer?

[dneto, 2014-08-06 20:31:23]

I will investigate removing LayerTreeHostImpl::IsContextLost altogether.  Like
you said, the last call would be from within SingleThreadProxy::DoComposite,
which would not be called at all when the proxy has already been notified of a
lost context in the first place!

In that case, we would only need the state variable for a DCHECK and the extra
behaviour in the counting-lost-surface-calls unit test.

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_impl.h
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/446973002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.h:292: bool IsContextLost();
On 2014/08/06 18:56:44, danakj wrote:
> do you think we can get rid of this method too? The only caller of this left
> would be in SingleThreadProxy after drawing, and there's no point really to it
> anymore, since if we see true, it means we've already told the proxy about the
> loss via the callback.

Ah, and if we've told the proxy about the loss via the callback, then we'd never
get into SingleThreadProxy::DoComposite in the first place.  So we eliminate the
lost_output_surface handling altogether.  I'll give that a try too.

[danakj, 2014-08-06 20:34:28]

On Wed, Aug 6, 2014 at 4:31 PM, <dneto@chromium.org> wrote:

> I will investigate removing LayerTreeHostImpl::IsContextLost altogether.
>  Like
> you said, the last call would be from within SingleThreadProxy::
> DoComposite,
> which would not be called at all when the proxy has already been notified
> of a
> lost context in the first place!
>
> In that case, we would only need the state variable for a DCHECK and the
> extra
> behaviour in the counting-lost-surface-calls unit test.


Cool, maybe we can just remove it then, and let unit tests do our
verification as needed.


>  https://codereview.chromium.org/446973002/diff/1/cc/trees/
> layer_tree_host_impl.h
> File cc/trees/layer_tree_host_impl.h (right):
>
> https://codereview.chromium.org/446973002/diff/1/cc/trees/
> layer_tree_host_impl.h#newcode292
>  cc/trees/layer_tree_host_impl.h:292: bool IsContextLost();
>
> On 2014/08/06 18:56:44, danakj wrote:
>
>> do you think we can get rid of this method too? The only caller of
>>
> this left
>
>> would be in SingleThreadProxy after drawing, and there's no point
>>
> really to it
>
>> anymore, since if we see true, it means we've already told the proxy
>>
> about the
>
>> loss via the callback.
>>
>
> Ah, and if we've told the proxy about the loss via the callback, then
> we'd never get into SingleThreadProxy::DoComposite in the first place.
> So we eliminate the lost_output_surface handling altogether.  I'll give
> that a try too.
>

Thanks! I'm not sure if you have dangling questions still at this point, so
please let me know if you do.

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

[dneto, 2014-08-07 17:05:58]

On 2014/08/06 20:34:28, danakj wrote:
> On Wed, Aug 6, 2014 at 4:31 PM, <mailto:dneto@chromium.org> wrote:
> 
> > I will investigate removing LayerTreeHostImpl::IsContextLost altogether.
> >  Like
> > you said, the last call would be from within SingleThreadProxy::
> > DoComposite,
> > which would not be called at all when the proxy has already been notified
> > of a
> > lost context in the first place!
> >
> > In that case, we would only need the state variable for a DCHECK and the
> > extra
> > behaviour in the counting-lost-surface-calls unit test.
> 
> 
> Cool, maybe we can just remove it then, and let unit tests do our
> verification as needed.
> 
> 
> >  https://codereview.chromium.org/446973002/diff/1/cc/trees/
> > layer_tree_host_impl.h
> > File cc/trees/layer_tree_host_impl.h (right):
> >
> > https://codereview.chromium.org/446973002/diff/1/cc/trees/
> > layer_tree_host_impl.h#newcode292
> >  cc/trees/layer_tree_host_impl.h:292: bool IsContextLost();
> >
> > On 2014/08/06 18:56:44, danakj wrote:
> >
> >> do you think we can get rid of this method too? The only caller of
> >>
> > this left
> >
> >> would be in SingleThreadProxy after drawing, and there's no point
> >>
> > really to it
> >
> >> anymore, since if we see true, it means we've already told the proxy
> >>
> > about the
> >
> >> loss via the callback.
> >>
> >
> > Ah, and if we've told the proxy about the loss via the callback, then
> > we'd never get into SingleThreadProxy::DoComposite in the first place.
> > So we eliminate the lost_output_surface handling altogether.  I'll give
> > that a try too.
> >
> 
> Thanks! I'm not sure if you have dangling questions still at this point, so
> please let me know if you do.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I had trouble removing LayerTreeHostImpl::IsContextLost because then
SingleThreadProxy can't see the context being lost and then 19 lost-context unit
tests fail based on the different behaviour, even though the new behaviour is
just fine.
I'll split that task into another patch.

In this third version of the patch, I've removed testing that we can call
LTHI::InitializeRenderer twice in a row.  The scheduler isn't supposed to do
that.
I've reviewed the scheduler unit tests, and the state machine has 8 tests for
losing the output surface, and the scheduler itself has another 11.  There are
many cases where it checks that it will schedule a
ScheduledActionBeginOutputSurfaceCreation once but not twice in a row.  I think
the test coverage is good.

[danakj, 2014-08-08 13:48:49]

LGTM since we're going to remove that method/variable anyway, but one thing:

https://codereview.chromium.org/446973002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/446973002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:6834: // twice in a row only causes
one subsequent
I think it'd be a bug if the OutputSurface called its lost callback more than
once ever, did you see this happening? I'm not sure we need to guard against
that in CC.

[dneto, 2014-08-08 14:09:12]

https://codereview.chromium.org/446973002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/446973002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:6834: // twice in a row only causes
one subsequent
On 2014/08/08 13:48:49, danakj wrote:
> I think it'd be a bug if the OutputSurface called its lost callback more than
> once ever, did you see this happening? I'm not sure we need to guard against
> that in CC.

No, I didn't see that occur, and I haven't read any code that would cause it.  
It felt strange to make a code change without a unit test, so I tested this.  I
added the new comment to say the unit test goes beyond the required behaviour.

[danakj, 2014-08-08 18:15:36]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-08-08 18:19:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dneto@chromium.org/446973002/40001

[commit-bot: I haz the power, 2014-08-08 22:53:15]

Change committed as 288457