Remove cc::LayerTreeHostImpl::IsContextLost

Remove cc::LayerTreeHostImpl::IsContextLost

LTHI::PrepareToDraw returns DRAW_ABORTED_CONTEXT_LOST when the
layer tree host knows that the context has been lost.

In the single threaded case, the layer tree host is notified
of surface loss after DoCommit completes, not in the middle.

BUG=402511

[dneto, 2014-08-15 17:44:32]

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:3296: }
This fixes a readability issue reported when I first tried to upload the patch.

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:68: if (!context3d_)
Case #4 in LayerTreeHostContextTestLostContextSucceeds required this guard.  It
says "Losing the context and recreating it any number of time should succeed".  
It triggers a Commit 10 times in a row while calling this LoseContext() call in
the middle of the commit.  With the changes to STP::DoCommit, this pointer is
already NULL and without the guard we will crash on a null pointer exception.
I think this guard keeps the spirit of the test because the context will still
be gone after the call to LoseContext terminates.

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:663: case 3:
The remaining changes to this file are a result of different scheduling of the
DoCommit.

[danakj, 2014-08-15 17:45:16]

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1105: if (!have_valid_output_surface_)
For this to happen, we'd have to get the callback of a lost context in between
the Scheduler deciding to draw a frame and here. Is that possible? If it is, is
it worth having state/branches for when we could just draw to the lost context?

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:665: // http://crbug.com/287250
FYI enne this will go away with your Scheduler CL again.

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1110: case 7:
where's 6? should it be the same as case 5?

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:317: SetNeedsCommitOnImplThread();
Should this move to STP::DidLoseOutputSurface (ie happen at the same time we
tell the LTH)?

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:322:
base::Bind(&LayerTreeHost::DidLoseOutputSurface,
Can you add a SingleThreadProxy::DidLoseOutputSurface() and post that instead,
then have that call LTH?

You'll also want to add a "base::WeakPtrFactory<SingleThreadProxy>
weak_factory_;" to this class and post the task with that weak factory. You can
steal that from
https://codereview.chromium.org/134623005/diff/2230001/cc/trees/single_thread...

[dneto, 2014-08-15 17:58:20]

I will try your suggestions

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1105: if (!have_valid_output_surface_)
On 2014/08/15 17:45:15, danakj wrote:
> For this to happen, we'd have to get the callback of a lost context in between
> the Scheduler deciding to draw a frame and here. Is that possible? If it is,
is
> it worth having state/branches for when we could just draw to the lost
context?

In a first attempt, I made LTHI::CanDraw return false when LTHI did not have a
valid output surface.  That caused mayhem in the layer-tree-host context-loss
unit tests, as it causes context loss in many of the points between starting
PrepareToDraw and DidDrawAllLayers, since many steps do DCHECK( CanDraw() ).

Is your suggestion similar?  I still am puzzled by when things can get
scheduled.

Or maybe I wasn't radical enough.  Could we always return true from
STP::DoComposite? And in that case we could also simplify
STP::CompositeImmediately a little.

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:322:
base::Bind(&LayerTreeHost::DidLoseOutputSurface,
On 2014/08/15 17:45:15, danakj wrote:
> Can you add a SingleThreadProxy::DidLoseOutputSurface() and post that instead,
> then have that call LTH?
> 
> You'll also want to add a "base::WeakPtrFactory<SingleThreadProxy>
> weak_factory_;" to this class and post the task with that weak factory. You
can
> steal that from
>
https://codereview.chromium.org/134623005/diff/2230001/cc/trees/single_thread...

Good point.  Yes, I can do this.

[danakj, 2014-08-15 18:00:12]

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1105: if (!have_valid_output_surface_)
On 2014/08/15 17:58:20, dneto wrote:
> On 2014/08/15 17:45:15, danakj wrote:
> > For this to happen, we'd have to get the callback of a lost context in
between
> > the Scheduler deciding to draw a frame and here. Is that possible? If it is,
> is
> > it worth having state/branches for when we could just draw to the lost
> context?
> 
> In a first attempt, I made LTHI::CanDraw return false when LTHI did not have a
> valid output surface.  That caused mayhem in the layer-tree-host context-loss
> unit tests, as it causes context loss in many of the points between starting
> PrepareToDraw and DidDrawAllLayers, since many steps do DCHECK( CanDraw() ).
> 
> Is your suggestion similar?  I still am puzzled by when things can get
> scheduled.
> 
> Or maybe I wasn't radical enough.  Could we always return true from
> STP::DoComposite? And in that case we could also simplify
> STP::CompositeImmediately a little.

Ya, make DoComposite return void. I'm suggesting that if the context is lost and
we're about to draw.. oh well draw anyways. The scheduler/embedder will hear
about it later and do something with it. Less branches keeps things simpler.

[dneto, 2014-08-15 18:17:17]

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:317: SetNeedsCommitOnImplThread();
On 2014/08/15 17:45:15, danakj wrote:
> Should this move to STP::DidLoseOutputSurface (ie happen at the same time we
> tell the LTH)?

Actually we might just remove this, since LTH::DidLoseOutputSurface calls
LTH::SetNeedsCommit which then comes back into proxy_->SetNeedsCommit which then
calls client_->ScheduleComposite

That's the same effect as SetNeedsCommitOnImplThread.
I'll see if that works without too much mayhem.

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

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:317: SetNeedsCommitOnImplThread();
On 2014/08/15 18:17:17, dneto wrote:
> On 2014/08/15 17:45:15, danakj wrote:
> > Should this move to STP::DidLoseOutputSurface (ie happen at the same time we
> > tell the LTH)?
> 
> Actually we might just remove this, since LTH::DidLoseOutputSurface calls
> LTH::SetNeedsCommit which then comes back into proxy_->SetNeedsCommit which
then
> calls client_->ScheduleComposite
> 
> That's the same effect as SetNeedsCommitOnImplThread.
> I'll see if that works without too much mayhem.

Oh good SGTM

[dneto, 2014-08-15 18:54:41]

On 2014/08/15 18:18:10, danakj wrote:
>
https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
> File cc/trees/single_thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/478703002/diff/20001/cc/trees/single_thread_p...
> cc/trees/single_thread_proxy.cc:317: SetNeedsCommitOnImplThread();
> On 2014/08/15 18:17:17, dneto wrote:
> > On 2014/08/15 17:45:15, danakj wrote:
> > > Should this move to STP::DidLoseOutputSurface (ie happen at the same time
we
> > > tell the LTH)?
> > 
> > Actually we might just remove this, since LTH::DidLoseOutputSurface calls
> > LTH::SetNeedsCommit which then comes back into proxy_->SetNeedsCommit which
> then
> > calls client_->ScheduleComposite
> > 
> > That's the same effect as SetNeedsCommitOnImplThread.
> > I'll see if that works without too much mayhem.
> 
> Oh good SGTM

Yeah, that works.  It also eliminates differences between single and multi
threaded cases in the unit tests. Yay.  It will be in my next patch.  Still
working on it.

[danakj, 2014-08-15 18:55:28]

Awesome I was hoping for that :)


On Fri, Aug 15, 2014 at 2:54 PM, <dneto@chromium.org> wrote:

> On 2014/08/15 18:18:10, danakj wrote:
>
> https://codereview.chromium.org/478703002/diff/20001/cc/
> trees/single_thread_proxy.cc
>
>> File cc/trees/single_thread_proxy.cc (right):
>>
>
>
> https://codereview.chromium.org/478703002/diff/20001/cc/
> trees/single_thread_proxy.cc#newcode317
>
>> cc/trees/single_thread_proxy.cc:317: SetNeedsCommitOnImplThread();
>> On 2014/08/15 18:17:17, dneto wrote:
>> > On 2014/08/15 17:45:15, danakj wrote:
>> > > Should this move to STP::DidLoseOutputSurface (ie happen at the same
>> time
>>
> we
>
>> > > tell the LTH)?
>> >
>> > Actually we might just remove this, since LTH::DidLoseOutputSurface
>> calls
>> > LTH::SetNeedsCommit which then comes back into proxy_->SetNeedsCommit
>> which
>> then
>> > calls client_->ScheduleComposite
>> >
>> > That's the same effect as SetNeedsCommitOnImplThread.
>> > I'll see if that works without too much mayhem.
>>
>
>  Oh good SGTM
>>
>
> Yeah, that works.  It also eliminates differences between single and multi
> threaded cases in the unit tests. Yay.  It will be in my next patch.  Still
> working on it.
>
> https://codereview.chromium.org/478703002/
>

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

[dneto, 2014-08-20 18:01:24]

Now STP::DoComposite will draw even if the context is lost.

https://codereview.chromium.org/478703002/diff/60001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:426: bool
SingleThreadProxy::DoComposite(base::TimeTicks frame_begin_time,
DoComposite still needs to be able to return a false status.  Otherwise we get a
few dozen failures in LTHI::SwapBuffers since the FakeOutputSurface assumes a GL
context even in the Delegated rendering scenario.
Fixing that would cause still more churn.

[danakj, 2014-08-20 18:22:10]

Awesome!

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:68: if (!context3d_)
Is this still needed?

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:669: case 3:
did you verify that you still need this extra case?

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1002: case 4:
did you verify that you still need this extra case?

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1116: case 7:
case 6? or should case 6 be up with case 5?

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:383: if (HasImplThread())
{} because the else needs them

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:385: else
{} because the body is more than one line

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:463: if (HasImplThread())
{}

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:465: else
{}

https://codereview.chromium.org/478703002/diff/60001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:426: bool
SingleThreadProxy::DoComposite(base::TimeTicks frame_begin_time,
On 2014/08/20 18:01:23, dneto wrote:
> DoComposite still needs to be able to return a false status.  Otherwise we get
a
> few dozen failures in LTHI::SwapBuffers since the FakeOutputSurface assumes a
GL
> context even in the Delegated rendering scenario.
> Fixing that would cause still more churn.

Ah, yes, we return false if not visible etc.

https://codereview.chromium.org/478703002/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:448:
(void)layer_tree_host_impl_->PrepareToDraw(frame);
don't need (void) here

[danakj, 2014-08-20 18:23:04]

+enne FYI probably a bunch of these test changes will revert with the scheduler
client patch. Did you wanna try land that without turning it on in the
ui::Compositor before this lands since that's been in the works for a few months
now, or you ok with this going in first?

[enne (OOO), 2014-08-20 18:27:13]

It looks like this is nearly ready to land.  I can just rebase.  I am planning
on landing that patch first without turning it on in ui::Compositor, but I need
to go track down a bad bug first.

[dneto, 2014-08-20 19:15:21]

I'll have a new patch shortly.

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:68: if (!context3d_)
On 2014/08/20 18:22:10, danakj wrote:
> Is this still needed?

Yes.  Otherwise 6 tests crash with null ptr exception.  The STP::DoCommit calls
CommitComplete hook which in some tests calls LoseContext a second time when we
haven't yet recovered from context loss.

These tests are:
   
LayerTreeHostContextTestLostContextAndEvictTextures.LoseBeforeEvict_SingleThread_DelegatingRenderer
   
LayerTreeHostContextTestLostContextAndEvictTextures.LoseBeforeEvict_SingleThread_DirectRenderer
   
LayerTreeHostContextTestLostContextSucceeds.RunSingleThread_DelegatingRenderer
    LayerTreeHostContextTestLostContextSucceeds.RunSingleThread_DirectRenderer
   
LayerTreeHostContextTestLostContextSucceedsWithContent.RunSingleThread_DelegatingRenderer
   
LayerTreeHostContextTestLostContextSucceedsWithContent.RunSingleThread_DirectRenderer

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:669: case 3:
On 2014/08/20 18:22:10, danakj wrote:
> did you verify that you still need this extra case?

Yes, it's still needed.

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1116: case 7:
On 2014/08/20 18:22:10, danakj wrote:
> case 6? or should case 6 be up with case 5?

Single thread case does reach case 6, so changing 7 --> 6 causes failure.

The multi-thread case never goes past 5, only single-thread does.  I don't think
it adds anything to make 5 and 6 the same.

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:383: if (HasImplThread())
On 2014/08/20 18:22:10, danakj wrote:
> {} because the else needs them

Excellent. Control structures without braces freak me out.  Perl requires them,
which I like very much.

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:385: else
On 2014/08/20 18:22:10, danakj wrote:
> {} because the body is more than one line

Acknowledged.

https://codereview.chromium.org/478703002/diff/60001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:448:
(void)layer_tree_host_impl_->PrepareToDraw(frame);
On 2014/08/20 18:22:10, danakj wrote:
> don't need (void) here

Acknowledged.

[danakj, 2014-08-20 19:19:13]

LGTM

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/478703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1116: case 7:
On 2014/08/20 19:15:20, dneto wrote:
> On 2014/08/20 18:22:10, danakj wrote:
> > case 6? or should case 6 be up with case 5?
> 
> Single thread case does reach case 6, so changing 7 --> 6 causes failure.
> 
> The multi-thread case never goes past 5, only single-thread does.  I don't
think
> it adds anything to make 5 and 6 the same.

Ah ok, since we don't reach 5 we won't reach 6, sure.

[dneto, 2014-08-21 21:59:20]

On 2014/08/20 19:19:13, danakj wrote:
> LGTM


I'm investigating the failure on the Linux GPU tests.  

Test GpuCrash.GPUProcessCrashesExactlyOnce fails by timing out on the second
iteration of trying to crash the page.  The python driver says it's waiting for
the test to "fully reset" before crashing the GPU process.  I don't know exactly
what's going wrong, but I can't see why even the first iteration would go
through.

[enne (OOO), 2014-08-22 17:02:36]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2014-08-22 17:03:17]

Oops.  *blush*

[dneto, 2014-08-25 15:40:34]

On 2014/08/22 17:03:17, enne wrote:
> Oops.  *blush*

After picking up enne's https://codereview.chromium.org/134623005, I started
fresh and now
https://codereview.chromium.org/505713004/ replaces this review.

[danakj, 2014-08-25 15:42:14]

Ok I've marked this one closd