Add a Display and ContextProvider concept to mojom, use to recreate

Add a Display and ContextProvider concept to mojom, use to recreate

This introduces two new concepts to make initializing and reinitializing
the objects needed to draw onscreen easier. This makes it easier to draw
things and will make recovering from lost contexts simpler.

The first is ContextProvider which is an interface for creating contexts
(in the GPU command buffer sense) associated with a piece of state (such
as drawing to a particular native viewport). This allows code to request
command buffer contexts associated with a particular native viewport
multiple times without requiring passing identifiers around since the
identity of the native viewport in question is contained in the identity
of the message pipe that the interface is bound to. This also makes it
possible for the gpu mojoms to produce viewport-bound contexts without
needing to be aware of the native viewport interfaces explicitly or
through opaque ID side channels.

The second is Display which is an interface used to submit frames that
should be drawn onscreen. A Display is somewhat like a Surface with a
few differences that make interacting with one simpler than interacting
with a Surface in general. Display is only drawn to and never embedded
in another frame so it does not need to have an explicit size or global
ID as those are only used when embedding one Surface's frame into
another. The only operations a Display has to support is receiving
frames, ACKing them and returning resources when unused.

A Display needs to know how to draw to a particular context so it can be
constructed from a ContextProvider if that ContextProvider is associated
with the desired native viewport. This means that in turn the native
viewport service doesn't have to know anything about surfaces, it only
needs to provide appropriate ContextProvider implementations to whomever
asks.

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/187e1faf437be935fe55ae769f0ecc19ce4c3db4

[jamesr, 2015-02-20 02:20:20]

qsr - this needs more work but I should be able to finish it up tomorrow so
posting as an FYI.  PS0 works for sample_app and surfaces_app but nothing view
manager based works yet and the viewport parameter listener isn't wired up (and
I don't think I have it right in the mojom).  There's no attempt here to
actually handle lost contexts in the general case but it should be a lot easier
to do so.  The surfaces implementation can also be simplified a lot more since
there's now a distinction between general surfaces and displays.

[qsr, 2015-02-20 11:25:46]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-02-20 11:25:47]

https://codereview.chromium.org/940293003/diff/1/examples/sample_app/gles2_cl...
File examples/sample_app/gles2_client_impl.cc (right):

https://codereview.chromium.org/940293003/diff/1/examples/sample_app/gles2_cl...
examples/sample_app/gles2_client_impl.cc:131: MojoTimeTicks(16667));
You can lose the context between this post task and the call to Draw, no? Should
you check for the context in Draw too?

Also, (but not sure if it is worth it for this app), maybe we still want to
update the cube data while we do not have a context, but we just do not want to
try to draw...

https://codereview.chromium.org/940293003/diff/1/mojo/services/gpu/public/int...
File mojo/services/gpu/public/interfaces/gpu.mojom (right):

https://codereview.chromium.org/940293003/diff/1/mojo/services/gpu/public/int...
mojo/services/gpu/public/interfaces/gpu.mojom:26: // If a
viewport_parameter_listener is provided it will be when vsync
It seems you are missing some part of this sentence.

https://codereview.chromium.org/940293003/diff/1/mojo/services/surfaces/publi...
File mojo/services/surfaces/public/interfaces/display.mojom (right):

https://codereview.chromium.org/940293003/diff/1/mojo/services/surfaces/publi...
mojo/services/surfaces/public/interfaces/display.mojom:17: // The Display will
be resized to the size of |frame|'s root pass.
Does pass has a specific meaning in this context? I'm feeling that it might not
be the right word here.

[jamesr, 2015-02-20 22:01:04]

jamesr@chromium.org changed reviewers:
+ abarth@chromium.org
- qsr@chromium.org

[jamesr, 2015-02-20 22:01:05]

Thanks for taking a look, Ben.  I believe I've fixed everything in the latest
patch.

This patch does not handle lost context events in the view manager, but
everything is set up in a way where doing so should be pretty straightforward.

Adam - could you take a look?

https://codereview.chromium.org/940293003/diff/1/examples/sample_app/gles2_cl...
File examples/sample_app/gles2_client_impl.cc (right):

https://codereview.chromium.org/940293003/diff/1/examples/sample_app/gles2_cl...
examples/sample_app/gles2_client_impl.cc:131: MojoTimeTicks(16667));
On 2015/02/20 11:25:47, qsr wrote:
> You can lose the context between this post task and the call to Draw, no?
Should
> you check for the context in Draw too?

fixed

> 
> Also, (but not sure if it is worth it for this app), maybe we still want to
> update the cube data while we do not have a context, but we just do not want
to
> try to draw...

no, it doesn't make sense to spend time updating the math on the cube when we
can't draw it.

https://codereview.chromium.org/940293003/diff/1/mojo/services/gpu/public/int...
File mojo/services/gpu/public/interfaces/gpu.mojom (right):

https://codereview.chromium.org/940293003/diff/1/mojo/services/gpu/public/int...
mojo/services/gpu/public/interfaces/gpu.mojom:26: // If a
viewport_parameter_listener is provided it will be when vsync
On 2015/02/20 11:25:47, qsr wrote:
> It seems you are missing some part of this sentence.

Done.

https://codereview.chromium.org/940293003/diff/1/mojo/services/surfaces/publi...
File mojo/services/surfaces/public/interfaces/display.mojom (right):

https://codereview.chromium.org/940293003/diff/1/mojo/services/surfaces/publi...
mojo/services/surfaces/public/interfaces/display.mojom:17: // The Display will
be resized to the size of |frame|'s root pass.
On 2015/02/20 11:25:47, qsr wrote:
> Does pass has a specific meaning in this context? I'm feeling that it might
not
> be the right word here.

mojo.Pass is a mojom type and |frame| has a field called |passes|. Expanded on
this comment a bit

[jamesr, 2015-02-20 22:01:55]

https://codereview.chromium.org/940293003/diff/40001/services/surfaces/displa...
File services/surfaces/display_impl.cc (right):

https://codereview.chromium.org/940293003/diff/40001/services/surfaces/displa...
services/surfaces/display_impl.cc:51: new
mojo::ContextProviderMojo(gles2_client.PassMessagePipe()))));
in particular, here we should listen to the context going away and issue another
context_provider_->Create(...) call to get a new context.

[qsr, 2015-02-23 12:32:52]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-02-23 12:32:53]

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/samp...
File examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/samp...
examples/sample_app/sample_app.cc:51: gles2_client_->SetSize(*size);
I don't know if it matters that much, but this seems weird. We do not have
guarantee that we get a context with the requested size (and in fact, we mostly
don't on Android where the context size is always the screen size). Should we
wait for the OnMetriscChanged call before setting the size?

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/spin...
File examples/sample_app/spinning_cube.cc (right):

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/spin...
examples/sample_app/spinning_cube.cc:484: void SpinningCube::OnGLContextLost() {
If I recall correctly, we never sent this. If you need this, it should be called
from: GLES2ClientImpl::ContextLost(), otherwise, we can remove both this, and
the state OnGLContextLost()

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
services/native_viewport/native_viewport_impl.cc:115:
create_callback_.Run(metrics_.Clone());
You might not want to fix this now, but this is incorrect.

On android, you only know the right metrics after the first call to
NativeViewportImpl::OnMetricsChanged (on all other platform, you are fine, and
you could even have called this on the Create method).

 What I did to fix this is have platform_viewport_->Init returns a
mojo::ViewportMetricsPtr, and allow to return a null one. If you get one, you
can run your callback immediately. Otherwise, you run it on the first time you
receive OnMetricsChanged

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
File services/native_viewport/native_viewport_impl.h (right):

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
services/native_viewport/native_viewport_impl.h:18: class ApplicationImpl;
Isn't the new style that we do not forward declare anymore, but do the real
include instead?

https://codereview.chromium.org/940293003/diff/60001/services/surfaces/displa...
File services/surfaces/display_impl.cc (right):

https://codereview.chromium.org/940293003/diff/60001/services/surfaces/displa...
services/surfaces/display_impl.cc:44: if (!display_) {
When do we expect display to not be null? If I understand correctly, we get
there only from the constructor of this object, so this should never happen.
Should this be a DCHECK?

https://codereview.chromium.org/940293003/diff/60001/services/surfaces/displa...
services/surfaces/display_impl.cc:53: factory_.Create(cc_id_);
I'm probably missing something, but where is this surface destroyed? We are not
doing it here, and I do not see any destory call in display either.

[qsr, 2015-02-26 12:41:03]

James, any idea when you will go back to this?

[jamesr, 2015-03-04 00:19:51]

PTAL

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/samp...
File examples/sample_app/sample_app.cc (right):

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/samp...
examples/sample_app/sample_app.cc:51: gles2_client_->SetSize(*size);
On 2015/02/23 12:32:52, qsr wrote:
> I don't know if it matters that much, but this seems weird. We do not have
> guarantee that we get a context with the requested size (and in fact, we
mostly
> don't on Android where the context size is always the screen size). Should we
> wait for the OnMetriscChanged call before setting the size?

The size isn't needed until it's time to draw, really, all gles2_client does it
store it so it can perform math at draw time. It won't actually draw until it
gets a context from the ContextProvider which is after the actual native widget
is created.  Deferring setting the size until OnMetricsChanged adds another
state that gles2_client has to worry about where it has a context but no size.

If we end up drawing a frame or few with the wrong size nothing really bad
happens.  Maybe it looks weird but this is just a pretty dumb demo.  Whenever
the real size comes up we'll start drawing cubes that look better.

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/spin...
File examples/sample_app/spinning_cube.cc (right):

https://codereview.chromium.org/940293003/diff/60001/examples/sample_app/spin...
examples/sample_app/spinning_cube.cc:484: void SpinningCube::OnGLContextLost() {
On 2015/02/23 12:32:52, qsr wrote:
> If I recall correctly, we never sent this. If you need this, it should be
called
> from: GLES2ClientImpl::ContextLost(), otherwise, we can remove both this, and
> the state OnGLContextLost()

Oh, good catch.  Added a call in GLES2ClientImpl::ContextLost().  We want to
call this so that we will recreate programs/buffers/etc when getting a new
context.

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
services/native_viewport/native_viewport_impl.cc:115:
create_callback_.Run(metrics_.Clone());
On 2015/02/23 12:32:52, qsr wrote:
> You might not want to fix this now, but this is incorrect.
> 
> On android, you only know the right metrics after the first call to
> NativeViewportImpl::OnMetricsChanged (on all other platform, you are fine, and
> you could even have called this on the Create method).
> 
>  What I did to fix this is have platform_viewport_->Init returns a
> mojo::ViewportMetricsPtr, and allow to return a null one. If you get one, you
> can run your callback immediately. Otherwise, you run it on the first time you
> receive OnMetricsChanged

Hmm, interesting.  I'll leave a TODO here that the metrics at this point may be
incorrect.  Of course, on any platform the widget could get resized at any point
after creation so calling code should be robust to this but it'd still be nice
to improve.

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
File services/native_viewport/native_viewport_impl.h (right):

https://codereview.chromium.org/940293003/diff/60001/services/native_viewport...
services/native_viewport/native_viewport_impl.h:18: class ApplicationImpl;
On 2015/02/23 12:32:53, qsr wrote:
> Isn't the new style that we do not forward declare anymore, but do the real
> include instead?

Actually this can go away completely.  I just re-sorted the forward decls but
this one is not used any more.

The Google C++ style guide says this:

"You may forward declare ordinary classes in order to avoid unnecessary
#includes."

which I believe is a weakening of earlier language that more strongly encouraged
forward decls, but it's not prescriptive one way or the other.

https://codereview.chromium.org/940293003/diff/60001/services/surfaces/displa...
File services/surfaces/display_impl.cc (right):

https://codereview.chromium.org/940293003/diff/60001/services/surfaces/displa...
services/surfaces/display_impl.cc:44: if (!display_) {
On 2015/02/23 12:32:53, qsr wrote:
> When do we expect display to not be null? If I understand correctly, we get
> there only from the constructor of this object, so this should never happen.
> Should this be a DCHECK?

You're right, this could be a DCHECK now.  When the TODO on line 48 is resolved
then we'll get to this path after a lost context and will need to recreate a
display but that's for a later time.

Changed to a DCHECK

https://codereview.chromium.org/940293003/diff/60001/services/surfaces/displa...
services/surfaces/display_impl.cc:53: factory_.Create(cc_id_);
On 2015/02/23 12:32:53, qsr wrote:
> I'm probably missing something, but where is this surface destroyed? We are
not
> doing it here, and I do not see any destory call in display either.

It's destroyed implicitly by destroying the factory, which destroys all surfaces
constructed via the factory, but having it be more explicit is probably better. 
I'll add a Destroy call in ~DisplayImpl

[qsr, 2015-03-04 10:33:18]

https://codereview.chromium.org/940293003/diff/80001/examples/surfaces_app/em...
File examples/surfaces_app/embedder.cc (right):

https://codereview.chromium.org/940293003/diff/80001/examples/surfaces_app/em...
examples/surfaces_app/embedder.cc:7: #include "base/bind.h"
Doesn't seem useful.

https://codereview.chromium.org/940293003/diff/80001/mojo/services/gpu/public...
File mojo/services/gpu/public/interfaces/context_provider.mojom (right):

https://codereview.chromium.org/940293003/diff/80001/mojo/services/gpu/public...
mojo/services/gpu/public/interfaces/context_provider.mojom:21: // vsync
parameters change for this context.
Do you want to comment about when the returned buffer can be null? Given that we
wait for a context to be available, returning an null pointer will only happen
if the consumer is calling this while another request is still in progress.

https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
services/native_viewport/native_viewport_impl.cc:112: void
NativeViewportImpl::OnAcceleratedWidgetAvailable(
You need to add another method notifying you of the lost of the widget, and you
need to call this, on android, from PlatformViewportAndroid::SurfaceDestroyed

https://codereview.chromium.org/940293003/diff/80001/services/surfaces/surfac...
File services/surfaces/surfaces_service_application.h (right):

https://codereview.chromium.org/940293003/diff/80001/services/surfaces/surfac...
services/surfaces/surfaces_service_application.h:21: class SurfacesScheduler;
I guess I won't convince you to do a proper include instead?

https://codereview.chromium.org/940293003/diff/80001/shell/android/native_vie...
File shell/android/native_viewport_application_loader.cc (right):

https://codereview.chromium.org/940293003/diff/80001/shell/android/native_vie...
shell/android/native_viewport_application_loader.cc:41: new
native_viewport::NativeViewportImpl(app_.get(), gpu_state_,
This cuts android head.

Or to be less cryptic, the new first parameter of the constructor is the
headless parameter.

[jamesr, 2015-03-04 23:00:27]

PTAL

https://codereview.chromium.org/940293003/diff/80001/examples/surfaces_app/em...
File examples/surfaces_app/embedder.cc (right):

https://codereview.chromium.org/940293003/diff/80001/examples/surfaces_app/em...
examples/surfaces_app/embedder.cc:7: #include "base/bind.h"
On 2015/03/04 10:33:18, qsr wrote:
> Doesn't seem useful.

Ah right, this isn't used

https://codereview.chromium.org/940293003/diff/80001/mojo/services/gpu/public...
File mojo/services/gpu/public/interfaces/context_provider.mojom (right):

https://codereview.chromium.org/940293003/diff/80001/mojo/services/gpu/public...
mojo/services/gpu/public/interfaces/context_provider.mojom:21: // vsync
parameters change for this context.
On 2015/03/04 10:33:18, qsr wrote:
> Do you want to comment about when the returned buffer can be null? Given that
we
> wait for a context to be available, returning an null pointer will only happen
> if the consumer is calling this while another request is still in progress.

Done.

https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
services/native_viewport/native_viewport_impl.cc:112: void
NativeViewportImpl::OnAcceleratedWidgetAvailable(
On 2015/03/04 10:33:18, qsr wrote:
> You need to add another method notifying you of the lost of the widget, and
you
> need to call this, on android, from PlatformViewportAndroid::SurfaceDestroyed

What do mean by "need to"?  The previous code didn't have any mechanism for this
so I don't see why adding something new is necessary for this patch.  Adding
additional hooks for new functionality should be in a separate patch.

https://codereview.chromium.org/940293003/diff/80001/services/surfaces/surfac...
File services/surfaces/surfaces_service_application.h (right):

https://codereview.chromium.org/940293003/diff/80001/services/surfaces/surfac...
services/surfaces/surfaces_service_application.h:21: class SurfacesScheduler;
On 2015/03/04 10:33:18, qsr wrote:
> I guess I won't convince you to do a proper include instead?

What exactly is "proper" about having an include?  The only use of this type in
this header is as a template parameter to scoped_ptr<> which works just fine
with a forward declaration.

https://codereview.chromium.org/940293003/diff/80001/shell/android/native_vie...
File shell/android/native_viewport_application_loader.cc (right):

https://codereview.chromium.org/940293003/diff/80001/shell/android/native_vie...
shell/android/native_viewport_application_loader.cc:41: new
native_viewport::NativeViewportImpl(app_.get(), gpu_state_,
On 2015/03/04 10:33:18, qsr wrote:
> This cuts android head.
> 
> Or to be less cryptic, the new first parameter of the constructor is the
> headless parameter.

Ouch!  Fixed.

[qsr, 2015-03-04 23:22:29]

LGTM

https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
File services/native_viewport/native_viewport_impl.cc (right):

https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
services/native_viewport/native_viewport_impl.cc:112: void
NativeViewportImpl::OnAcceleratedWidgetAvailable(

> What do mean by "need to"?  The previous code didn't have any mechanism for
this
> so I don't see why adding something new is necessary for this patch.  Adding
> additional hooks for new functionality should be in a separate patch.

 I mean that until now, returning from the background on android for the
spinning cube showed a black screen. After this change, it will crash. This is
because you changed the app to request a new context  when it losts its original
one, but you didn't made this class aware that the widget is not valid anymore,
so it will gladly create a context that reference a dangling pointer. Given that
a black screen is not much better than a crash, we can wait for a follow-up CL
to solve this, but this is still a regression in behavior.

[jamesr, 2015-03-04 23:23:43]

On 2015/03/04 23:22:29, qsr wrote:
> LGTM
> 
>
https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
> File services/native_viewport/native_viewport_impl.cc (right):
> 
>
https://codereview.chromium.org/940293003/diff/80001/services/native_viewport...
> services/native_viewport/native_viewport_impl.cc:112: void
> NativeViewportImpl::OnAcceleratedWidgetAvailable(
> 
> > What do mean by "need to"?  The previous code didn't have any mechanism for
> this
> > so I don't see why adding something new is necessary for this patch.  Adding
> > additional hooks for new functionality should be in a separate patch.
> 
>  I mean that until now, returning from the background on android for the
> spinning cube showed a black screen. After this change, it will crash. This is
> because you changed the app to request a new context  when it losts its
original
> one, but you didn't made this class aware that the widget is not valid
anymore,
> so it will gladly create a context that reference a dangling pointer. Given
that
> a black screen is not much better than a crash, we can wait for a follow-up CL
> to solve this, but this is still a regression in behavior.

Gotcha.  This is already a big patch, so I'd really rather keep things separate.
 I think it'll be a lot easier to get to a non-crashy and non-black-screen state
after this patch and it'll be easier to review and understand history doing
things one step at a time.

[jamesr, 2015-03-04 23:49:58]

Committed patchset #6 (id:100001) manually as
187e1faf437be935fe55ae769f0ecc19ce4c3db4 (presubmit successful).