Automatically redraw the display on resume.

Automatically redraw the display on resume.

When resuming, automatically redraw the display with the latest known
frame available. This fixes #99 and obsoletes
https://codereview.chromium.org/1110423002/. The app no longer crashes
on rotation after resuming.

I tested with both mojo:ganesh_app and sky_viewer (home.sky and
spinning_cube.sky). I didn't encounter any issue/crash with reusing
frames across lost contexts with them.

[etiennej, 2015-04-30 10:41:21]

etiennej@chromium.org changed reviewers:
+ jamesr@chromium.org

[etiennej, 2015-04-30 10:41:21]

Note: I'll be OOO for the next 3 weeks; Colin will be covering me on this issue
during this time.

[jamesr, 2015-05-01 00:15:40]

The mojom ContextProvider interface is not specific to viewports, it can be used
for any situation where the contexts need to be associated with some state. 
This sort of callback should be on NativeViewport

https://codereview.chromium.org/1116883002/diff/20001/mojo/services/gpu/publi...
File mojo/services/gpu/public/interfaces/context_provider.mojom (right):

https://codereview.chromium.org/1116883002/diff/20001/mojo/services/gpu/publi...
mojo/services/gpu/public/interfaces/context_provider.mojom:29: // The callback
for this method will be called when a new viewport is
this shouldn't be needed - the callback to Create() will be called when a new
buffer is created, won't it?

[blundell, 2015-05-04 14:14:06]

On 2015/05/01 00:15:40, jamesr wrote:
> The mojom ContextProvider interface is not specific to viewports, it can be
used
> for any situation where the contexts need to be associated with some state. 
> This sort of callback should be on NativeViewport
> 
>
https://codereview.chromium.org/1116883002/diff/20001/mojo/services/gpu/publi...
> File mojo/services/gpu/public/interfaces/context_provider.mojom (right):
> 
>
https://codereview.chromium.org/1116883002/diff/20001/mojo/services/gpu/publi...
> mojo/services/gpu/public/interfaces/context_provider.mojom:29: // The callback
> for this method will be called when a new viewport is
> this shouldn't be needed - the callback to Create() will be called when a new
> buffer is created, won't it?

Hi James,

I started looking into this issue today. The answer to your second question is
no: the callback to Create() is not called is because that callback was reset in
the previous call to CreateAndReturnCommandBuffer(). For that reason,
OnscreenContextProvider does not even create a new command buffer when it gets
the call to |SetAcceleratedWidget()| with a non-null widget on resumption from
backgrounding.

Here is what the flow is with this CL patched in: OnscreenContextProvider calls
DisplayImpl::OnViewportCreated. The latter calls Draw(), which fails with error
messages about the context being lost and results in a callback to
DisplayImpl::OutputSurfaceLost(). *That* call results in
OnscreenContextProvider::Create() being called, and the actual successful draw
then happens in the callback from that method to
DisplayImpl::OnContextCreated().

It seems to me that the root problem is that OnscreenContextProvider knows when
the context has gone away from its end (i.e., when
OnscreenContextProvider::SetAcceleratedWidget(nullptr) is called), but it
doesn't inform the client that had asked it to create that context. I'm thinking
that a solution is to create a ContextLossObserver interface in
context_provider.mojom and pass a |ContextLossObserver?| to
ContextProvider::Create(). OnscreenContextProvider will call this observer when
the context has gone away and then reset it. DisplayImpl will set up a
ContextLossObserver that calls back to DisplayImpl::OutputSurfaceLost, which
will then result in OnscreenContextProvider creating and returning a new command
buffer when it can do so (i.e., when |SetAcceleratedWidget() is called again
with a non-null value).

Does this seem like a reasonable approach to you, or are there other directions
that you think we should be looking in?

[blundell, 2015-05-04 15:11:14]

On 2015/05/04 14:14:06, blundell wrote:
> On 2015/05/01 00:15:40, jamesr wrote:
> > The mojom ContextProvider interface is not specific to viewports, it can be
> used
> > for any situation where the contexts need to be associated with some state. 
> > This sort of callback should be on NativeViewport
> > 
> >
>
https://codereview.chromium.org/1116883002/diff/20001/mojo/services/gpu/publi...
> > File mojo/services/gpu/public/interfaces/context_provider.mojom (right):
> > 
> >
>
https://codereview.chromium.org/1116883002/diff/20001/mojo/services/gpu/publi...
> > mojo/services/gpu/public/interfaces/context_provider.mojom:29: // The
callback
> > for this method will be called when a new viewport is
> > this shouldn't be needed - the callback to Create() will be called when a
new
> > buffer is created, won't it?
> 
> Hi James,
> 
> I started looking into this issue today. The answer to your second question is
> no: the callback to Create() is not called is because that callback was reset
in
> the previous call to CreateAndReturnCommandBuffer(). For that reason,
> OnscreenContextProvider does not even create a new command buffer when it gets
> the call to |SetAcceleratedWidget()| with a non-null widget on resumption from
> backgrounding.
> 
> Here is what the flow is with this CL patched in: OnscreenContextProvider
calls
> DisplayImpl::OnViewportCreated. The latter calls Draw(), which fails with
error
> messages about the context being lost and results in a callback to
> DisplayImpl::OutputSurfaceLost(). *That* call results in
> OnscreenContextProvider::Create() being called, and the actual successful draw
> then happens in the callback from that method to
> DisplayImpl::OnContextCreated().
> 
> It seems to me that the root problem is that OnscreenContextProvider knows
when
> the context has gone away from its end (i.e., when
> OnscreenContextProvider::SetAcceleratedWidget(nullptr) is called), but it
> doesn't inform the client that had asked it to create that context. I'm
thinking
> that a solution is to create a ContextLossObserver interface in
> context_provider.mojom and pass a |ContextLossObserver?| to
> ContextProvider::Create(). OnscreenContextProvider will call this observer
when
> the context has gone away and then reset it. DisplayImpl will set up a
> ContextLossObserver that calls back to DisplayImpl::OutputSurfaceLost, which
> will then result in OnscreenContextProvider creating and returning a new
command
> buffer when it can do so (i.e., when |SetAcceleratedWidget() is called again
> with a non-null value).
> 
> Does this seem like a reasonable approach to you, or are there other
directions
> that you think we should be looking in?

qsr@ helped me realize that most of the flow I'm talking about already exists;
it just needed to be kicked off.

Let's continue discussion on https://codereview.chromium.org/1123623003/.