Use MailboxManagerSync on android.

Use MailboxManagerSync on android.

Egl fences seem to be required for proper cross-context synchronization
on android/qualcomm.

Fixes flickering seen when using surfaces composition on various
android devices (domokit/sky_engine#36).

R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/63532259dbe96993cece84f7d86d98a313cccf71

[cdotstout, 2015-09-17 22:13:36]

cstout@chromium.org changed reviewers:
+ iansf@google.com, jamesr@chromium.org, tonyg@chromium.org

[cdotstout, 2015-09-17 22:13:36]

Inspired by the in_process_command_buffer implementation.

[tonyg, 2015-09-17 22:18:02]

Nice!!

I'm not a good reviewer for the patch, but:
- I think you can add "Fixes domokit/sky_engine#36" to the description to close
the bug once it is submitted.
- Can the flush added in https://github.com/domokit/sky_engine/pull/585 go away
now?

[jamesr, 2015-09-17 22:23:13]

The failures on linux seem real:

[0917/151152:FATAL:texture_definition.cc(258)] Check failed: fals

https://codereview.chromium.org/1348283004/diff/1/services/gles2/gpu_state.cc
File services/gles2/gpu_state.cc (right):

https://codereview.chromium.org/1348283004/diff/1/services/gles2/gpu_state.cc...
services/gles2/gpu_state.cc:18: // mailbox_manager_(new
gpu::gles2::MailboxManagerImpl) {
can you just leave a todo and not the commented-out code?  dead code is smelly

[cdotstout, 2015-09-17 22:31:09]

https://codereview.chromium.org/1348283004/diff/1/services/gles2/gpu_state.cc
File services/gles2/gpu_state.cc (right):

https://codereview.chromium.org/1348283004/diff/1/services/gles2/gpu_state.cc...
services/gles2/gpu_state.cc:18: // mailbox_manager_(new
gpu::gles2::MailboxManagerImpl) {
On 2015/09/17 22:23:13, jamesr wrote:
> can you just leave a todo and not the commented-out code?  dead code is smelly

Yes, Linux fails with MailboxManagerSync.  

If the overall approach in this CL seems correct (yes? let me know if not) then
I will add a factory to create the appropriate mailbox manager for linux vs
android.

[jamesr, 2015-09-17 22:34:12]

An alternative would be to force virtual contexts on for either this driver or
in general.  That would mux everything into a single EGL context and in theory
shouldn't require any fences.  However, I think we tested that and the flicker
still appeared.  So I'm not sure what's needed.  Maybe EGL fences enforce
additional synchronization that works around the flicker?

[jamesr, 2015-09-17 22:37:17]

Try forcing the test in gpu/command_buffer/service/gles2_cmd_decoder.cc for
workarounds().use_virtualized_gl_contexts to always evaluate true

[jamesr, 2015-09-17 22:41:15]

Wait, that's the wrong place. I don't remember where we decide to use virtual or
physically distinct contexts :/

[cdotstout, 2015-09-18 00:12:33]

On 2015/09/17 22:41:15, jamesr wrote:
> Wait, that's the wrong place. I don't remember where we decide to use virtual
or
> physically distinct contexts :/

Since this solution works, it's better than what we have today.  I'm happy to
look into context virtualization, but I don't think context virtualization is
currently enabled in chrome for all android devices.  The fences seems like a
safer solution so I propose we go with it for now.

Tony, yes the glFlush in sky_viewer should no longer be necessary (but I haven't
tested it).

[iansf, 2015-09-18 00:18:40]

I'm glad that you found a solution that doesn't require any glFlush or glFinish
calls!  Apart from the commented-out code and possibly removing the glFlush that
we put in in this commit so that if we have to revert for some reason we can
revert back to the partially-working state, this looks good to me.  I defer to
jamesr or tonyg for official approval, though.

[jamesr, 2015-09-18 02:28:56]

I'm 100% fine with having fences between context switches.  I'm just slightly
worried that it might not be the real bug (i.e. it's just papering over
something else by virtue of the synchronization that the fence implies) but we
can always deal with that later if there turn out to be more synchronization
bugs elsewhere.

[jamesr, 2015-09-18 02:31:35]

lgtm w/ nits

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/BUILD.gn
File services/gles2/BUILD.gn (right):

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/BUILD.gn...
services/gles2/BUILD.gn:25: if (is_android) {
this probably deserves a comment explaining why the choice of factory depends on
is_android

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/mailbox_...
File services/gles2/mailbox_manager_factory_sync.cc (right):

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/mailbox_...
services/gles2/mailbox_manager_factory_sync.cc:13: return new
gpu::gles2::MailboxManagerSync;
instead of having separate _sync and _nosync.cc we could just have one with an
#if defined(OS_ANDROID). i think policy decisions made in code tend to be easier
for developers to find than policy decisions made in the build system

[cdotstout, 2015-09-18 06:03:05]

Committed patchset #5 (id:80001) manually as
63532259dbe96993cece84f7d86d98a313cccf71 (presubmit successful).

[cdotstout, 2015-09-18 15:51:40]

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/BUILD.gn
File services/gles2/BUILD.gn (right):

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/BUILD.gn...
services/gles2/BUILD.gn:25: if (is_android) {
On 2015/09/18 02:31:35, jamesr wrote:
> this probably deserves a comment explaining why the choice of factory depends
on
> is_android

Done.

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/mailbox_...
File services/gles2/mailbox_manager_factory_sync.cc (right):

https://codereview.chromium.org/1348283004/diff/40001/services/gles2/mailbox_...
services/gles2/mailbox_manager_factory_sync.cc:13: return new
gpu::gles2::MailboxManagerSync;
On 2015/09/18 02:31:35, jamesr wrote:
> instead of having separate _sync and _nosync.cc we could just have one with an
> #if defined(OS_ANDROID). i think policy decisions made in code tend to be
easier
> for developers to find than policy decisions made in the build system

Done.