Mojo multiple command buffer support and sample

Mojo multiple command buffer support and sample

TBR=danakj@chromium.org for DEPS file shuffle that moves a "+ui/gfx"

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

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

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

[jamesr, 2014-08-12 21:49:36]

This depends on several patches that haven't landed yet.

This adds support for creating an offscreen command buffer to the NativeViewport
service and adds an app that uses offscreen command buffers to create OpenGL
resources that it passes to surfaces for delegation.  We don't have real sync
points between command buffers in mojo yet so this emulates them with a
glFinish(), which has the correct-but-way-too-slow semantics for this use case.

[darin (slow to review), 2014-08-13 04:44:25]

https://codereview.chromium.org/451753003/diff/20001/mojo/services/public/int...
File mojo/services/public/interfaces/native_viewport/native_viewport.mojom
(right):

https://codereview.chromium.org/451753003/diff/20001/mojo/services/public/int...
mojo/services/public/interfaces/native_viewport/native_viewport.mojom:19:
CreateOffscreenGLES2Context(CommandBuffer& gles2_client);
Is it necessary for this to be supplied by the NativeViewport? I could imagine a
use case for providing an app with access to GL but no access to the screen.
This might be used for compute tasks, etc. Access control is likely to be at the
per-interface level rather than at the per-method level.

Maybe this is something that could be changed later, but I'm curious if we
shouldn't separate these more from the start.

[jamesr, 2014-08-13 04:49:49]

On 2014/08/13 04:44:25, darin wrote:
>
https://codereview.chromium.org/451753003/diff/20001/mojo/services/public/int...
> File mojo/services/public/interfaces/native_viewport/native_viewport.mojom
> (right):
> 
>
https://codereview.chromium.org/451753003/diff/20001/mojo/services/public/int...
> mojo/services/public/interfaces/native_viewport/native_viewport.mojom:19:
> CreateOffscreenGLES2Context(CommandBuffer& gles2_client);
> Is it necessary for this to be supplied by the NativeViewport? I could imagine
a
> use case for providing an app with access to GL but no access to the screen.
> This might be used for compute tasks, etc. Access control is likely to be at
the
> per-interface level rather than at the per-method level.

It shouldn't be provided by NativeViewport.  The difficulty is that the service
side (in the GPU command buffer sense) needs to run on one thread and there's
not an easy way currently to mux multiple services onto one thread.  I'd like to
change it so that onscreen and offscreen contexts alike live on a separate
thread (and ideally in a separate process, a la the gpu process in content) but
that requires a lot of tricky per-platform work and might require running the
service in a different process on platforms where it's very difficult to talk to
the native windowing system from multiple threads.  For instance, on linux X can
be driven from different threads by making different X displays but glib is
single threaded and that's what our MessagePumpForUI does.  Solveable, but hard.
 Easier once we have the ability to run services in their own processes (which
is soon?)

> 
> Maybe this is something that could be changed later, but I'm curious if we
> shouldn't separate these more from the start.

IMO, the clients of this probably won't really care about where the offscreen
context comes from for most of their logic.  I think it's valuable to enable
folks who want to use this capability to forge ahead while we improve the gpu
stack in the shell.

[darin (slow to review), 2014-08-13 05:02:26]

On 2014/08/13 04:49:49, jamesr wrote:
> On 2014/08/13 04:44:25, darin wrote:
> >
>
https://codereview.chromium.org/451753003/diff/20001/mojo/services/public/int...
> > File mojo/services/public/interfaces/native_viewport/native_viewport.mojom
> > (right):
> > 
> >
>
https://codereview.chromium.org/451753003/diff/20001/mojo/services/public/int...
> > mojo/services/public/interfaces/native_viewport/native_viewport.mojom:19:
> > CreateOffscreenGLES2Context(CommandBuffer& gles2_client);
> > Is it necessary for this to be supplied by the NativeViewport? I could
imagine
> a
> > use case for providing an app with access to GL but no access to the screen.
> > This might be used for compute tasks, etc. Access control is likely to be at
> the
> > per-interface level rather than at the per-method level.
> 
> It shouldn't be provided by NativeViewport.  The difficulty is that the
service
> side (in the GPU command buffer sense) needs to run on one thread and there's
> not an easy way currently to mux multiple services onto one thread.  I'd like
to
> change it so that onscreen and offscreen contexts alike live on a separate
> thread (and ideally in a separate process, a la the gpu process in content)
but
> that requires a lot of tricky per-platform work and might require running the
> service in a different process on platforms where it's very difficult to talk
to
> the native windowing system from multiple threads.  For instance, on linux X
can
> be driven from different threads by making different X displays but glib is
> single threaded and that's what our MessagePumpForUI does.  Solveable, but
hard.
>  Easier once we have the ability to run services in their own processes (which
> is soon?)

It is totally possible for N services to live on the same thread. The
straightforward way is for all of them to be provided by the same app.
Another way is to play tricks with URL mapping so multiple app URLs
end up being serviced by the same app.


> > 
> > Maybe this is something that could be changed later, but I'm curious if we
> > shouldn't separate these more from the start.
> 
> IMO, the clients of this probably won't really care about where the offscreen
> context comes from for most of their logic.  I think it's valuable to enable
> folks who want to use this capability to forge ahead while we improve the gpu
> stack in the shell.

Yes, definitely.

[piman, 2014-08-13 05:35:22]

https://codereview.chromium.org/451753003/diff/20001/mojo/examples/surfaces_a...
File mojo/examples/surfaces_app/child_gl_impl.cc (right):

https://codereview.chromium.org/451753003/diff/20001/mojo/examples/surfaces_a...
mojo/examples/surfaces_app/child_gl_impl.cc:127: glFinish();  // Fake sync
point.
I think we should make the glInsertSyncPointCHROMIUM implementation do a
(shallow) finish instead, until we have proper support. I'd do that by making
CommandBufferClientImpl::InsertSyncPoint/InsertFutureSyncPoint directly do a
WaitForGetInRange.

That way:
1- we don't need to finish gl
2- the implementation is "conformant", so that we don't need to hack client
code.

https://codereview.chromium.org/451753003/diff/20001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/451753003/diff/20001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:160: DLOG(WARNING) << "Context lost";
client()->LostContext(reason) here?

[jamesr, 2014-08-13 23:37:50]

This makes a gpu service.  The interface is provided by
mojo:mojo_native_viewport_service since I haven't figured out how to fake out
the url loader (yet).  Dave is trying to move native viewport out of the shell
which will change how this is loaded so I don't want to muck with that *too*
much.  For now, you just have to do:

mojo::GpuPtr gpu_service;
app->ConnectToService("mojo:mojo_native_viewport_service",
                      &gpu_service);

In order to create onscreen contexts, we have to get the accelerated widget's
identity from the native viewport to the gpu service.  For this patch, I've
added a uint64 pararameter to the NativeViewport::OnCreated callback that the
caller can pass through to the Gpu service.  It's actually the
gfx::AcceleratedWidget under the hood, but we can add an indirection here if we
want through a map or whatever.  Long term we aren't going to expose the native
viewport service or the ability to create onscreen contexts to anybody except
for the view manager service, so it probably doesn't matter all too much how
pretty this is.

The extra interface adds a bit more asynchronousity to the creation flow for
apps that want to do onscreen rendering.  This is a bit annoying.

https://codereview.chromium.org/451753003/diff/20001/mojo/examples/surfaces_a...
File mojo/examples/surfaces_app/child_gl_impl.cc (right):

https://codereview.chromium.org/451753003/diff/20001/mojo/examples/surfaces_a...
mojo/examples/surfaces_app/child_gl_impl.cc:127: glFinish();  // Fake sync
point.
On 2014/08/13 05:35:22, piman (OOO) wrote:
> I think we should make the glInsertSyncPointCHROMIUM implementation do a
> (shallow) finish instead, until we have proper support. I'd do that by making
> CommandBufferClientImpl::InsertSyncPoint/InsertFutureSyncPoint directly do a
> WaitForGetInRange.
> 
> That way:
> 1- we don't need to finish gl
> 2- the implementation is "conformant", so that we don't need to hack client
> code.

Aha, that's better.  Couldn't figure out how to do a command buffer finish.

https://codereview.chromium.org/451753003/diff/20001/mojo/services/gles2/comm...
File mojo/services/gles2/command_buffer_impl.cc (right):

https://codereview.chromium.org/451753003/diff/20001/mojo/services/gles2/comm...
mojo/services/gles2/command_buffer_impl.cc:160: DLOG(WARNING) << "Context lost";
On 2014/08/13 05:35:22, piman (OOO) wrote:
> client()->LostContext(reason) here?

done (went with kUnknown for now)

[piman, 2014-08-15 03:10:23]

lgtm

[jamesr, 2014-08-15 23:41:13]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-08-15 23:43:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/451753003/100001

[commit-bot: I haz the power, 2014-08-16 02:52:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-16 02:57:18]

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

[commit-bot: I haz the power, 2014-08-16 02:57:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jamesr, 2014-08-16 18:37:00]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-08-16 18:37:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/451753003/100001

[commit-bot: I haz the power, 2014-08-16 20:45:50]

Committed patchset #6 (100001) as 290157

[tdanderson, 2014-08-16 22:24:52]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/480723002/ by tdanderson@chromium.org.

The reason for reverting is: Reverting because this closed the tree due to a
failed compile step on Linux Builder (dbg)(32). See
http://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg%....

[tdanderson, 2014-08-16 22:25:02]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/478103002/ by tdanderson@chromium.org.

The reason for reverting is: Reverting because this closed the tree due to a
failed compile step on Linux Builder (dbg)(32). See
http://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg%....

[jamesr, 2014-08-18 21:38:09]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-08-18 21:38:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/451753003/120001

[commit-bot: I haz the power, 2014-08-18 23:17:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[commit-bot: I haz the power, 2014-08-19 00:45:36]

Committed patchset #7 (120001) as 290431

[ojan, 2014-08-19 21:08:10]

On 2014/08/19 at 00:45:36, commit-bot wrote:
> Committed patchset #7 (120001) as 290431

Was this second commit done through the CQ? The commit description doesn't say
it was done manually, but the last patch uploaded here doesn't have trybot runs.
I'm trying to figure out whether there's an infra bug to fix here since this
patch ended up breaking the compile.

[jamesr, 2014-08-19 21:10:22]

On 2014/08/19 21:08:10, ojan-only-code-yellow-reviews wrote:
> On 2014/08/19 at 00:45:36, commit-bot wrote:
> > Committed patchset #7 (120001) as 290431
> 
> Was this second commit done through the CQ? The commit description doesn't say
> it was done manually, but the last patch uploaded here doesn't have trybot
runs.
> I'm trying to figure out whether there's an infra bug to fix here since this
> patch ended up breaking the compile.

Both lands were via the CQ.  There is an infra bug here:

https://code.google.com/p/chromium/issues/detail?id=404923

that led to both tree closures.  I'm trying to manually run against
linux_rel_precise32 to avoid breaking things a 3rd time.

[jamesr, 2014-08-20 16:28:43]

patch set 10 passes all the default trybots and the linux_rel_precise32 bot, so
trying again.

[jamesr, 2014-08-20 16:28:50]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-08-20 16:30:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamesr@chromium.org/451753003/180001

[commit-bot: I haz the power, 2014-08-20 17:42:12]

Committed patchset #10 (180001) as 290859