Make it possible to use OSMesa on Android

Make it possible to use OSMesa on Android

This patch will start compiling OSMesa as a dependency of the
content_shell_apk target, and changes the GL implementation for Android
to recognize OSMesa as a valid implementation.

libosmesa.so will be separately included in ContentShell.apk, and
will be lazily loaded depending on whether OSMesa will be used or not.

When running a set of 54 layout tests on a Nexus 4, total time is
[69, 68, 68] ~68 seconds with this patch, [62, 60, 61] ~61 seconds
without this patch. Compensated for the setup time (13 seconds),
this means pixel tests are on average 14 percent slower. I believe
the win of (a) consistency with Chrome, (b) more reliable results and
(c) less cross-device differences warrants this, however.

Switching to OSMesa does also enable us to reliably enable impl-side
painting without getting garbage rendering on higher-end devices. At
time time it's not yet enabled by default however, as it makes
content_shell time out when ran on the Nexus 7.

BUG=232044, 248925, 250777
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252583

[Peter Beverloo, 2013-09-19 14:34:51]

+skyostil for GL and command buffer.

If you're happy I'll do the OWNERS dance.  This patch depends on
https://codereview.chromium.org/24253005/ for getting libosmesa.so in the APK.

[Sami, 2013-09-19 16:37:45]

lgtm modulo the favor below.

https://codereview.chromium.org/23868030/diff/2001/ui/gl/gl_context_android.cc
File ui/gl/gl_context_android.cc (right):

https://codereview.chromium.org/23868030/diff/2001/ui/gl/gl_context_android.c...
ui/gl/gl_context_android.cc:80: switch(GetGLImplementation()) {
Since you're here :) Could you change this to be a little more explicit about
what we support:

    switch(GetGLImplementation()) {
      case kGLImplementationMockGL:
        return scoped_refptr<GLContext>(new GLContextStub());
      case kGLImplementationOSMesaGL:
        context = new GLContextOSMesa(share_group);
        break;
      case kGLImplementationEGLGLES2:
        if (compatible_surface->GetHandle())
          context = new GLContextEGL(share_group);
        else
          context = new GLNonOwnedContext(share_group);
        break;
      default:
        NOTREACHED();
        return NULL;
    }

[Peter Beverloo, 2013-09-19 16:48:44]

+jochen for content/
+sievers for gpu/
+piman for ui/gl/

Once again, this commit is depending on another CL which allows us to pack
multiple shared libraries in an APK.
https://codereview.chromium.org/24253005/

https://codereview.chromium.org/23868030/diff/2001/ui/gl/gl_context_android.cc
File ui/gl/gl_context_android.cc (right):

https://codereview.chromium.org/23868030/diff/2001/ui/gl/gl_context_android.c...
ui/gl/gl_context_android.cc:80: switch(GetGLImplementation()) {
On 2013/09/19 16:37:46, Sami wrote:
> Since you're here :) Could you change this to be a little more explicit about
> what we support:
> 
>     switch(GetGLImplementation()) {
>       case kGLImplementationMockGL:
>         return scoped_refptr<GLContext>(new GLContextStub());
>       case kGLImplementationOSMesaGL:
>         context = new GLContextOSMesa(share_group);
>         break;
>       case kGLImplementationEGLGLES2:
>         if (compatible_surface->GetHandle())
>           context = new GLContextEGL(share_group);
>         else
>           context = new GLNonOwnedContext(share_group);
>         break;
>       default:
>         NOTREACHED();
>         return NULL;
>     }

Done.

[piman, 2013-09-19 19:48:37]

https://codereview.chromium.org/23868030/diff/7001/ui/gl/gl_implementation_an...
File ui/gl/gl_implementation_android.cc (right):

https://codereview.chromium.org/23868030/diff/7001/ui/gl/gl_implementation_an...
ui/gl/gl_implementation_android.cc:79: }
nit: since this looks like a copy&paste from gl_implementation_linux.cc, could
we have instead android also inherit gl_implementation_linux.cc, to avoid
duplication? It'll also give you the LoadLibrary for free.

[jochen (gone - plz use gerrit), 2013-09-20 13:10:19]

lgtm

[Peter Beverloo, 2013-10-03 19:46:41]

Updated, sorry for the delay.  I'm having some weird gyp issues in gl.gyp where
it doesn't want to build gl_implementation_linux.cc, if anyone has any idea
that'd be much appreciated.

https://codereview.chromium.org/23868030/diff/7001/ui/gl/gl_implementation_an...
File ui/gl/gl_implementation_android.cc (right):

https://codereview.chromium.org/23868030/diff/7001/ui/gl/gl_implementation_an...
ui/gl/gl_implementation_android.cc:79: }
On 2013/09/19 19:48:37, piman wrote:
> nit: since this looks like a copy&paste from gl_implementation_linux.cc, could
> we have instead android also inherit gl_implementation_linux.cc, to avoid
> duplication? It'll also give you the LoadLibrary for free.

Done, except for the problem I pointed out in gl.gyp..

https://codereview.chromium.org/23868030/diff/13001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/23868030/diff/13001/ui/gl/gl.gyp#newcode268
ui/gl/gl.gyp:268: 'gl_implementation_linux.h',
I've tried a ton of things (regular inclusions like this, sources/ include
expressions) to get gl_implementation_linux.cc to be included in Android's
target, but it doesn't want to work.

Does anyone know the gyp structure in ui/gl/ who can understand why a normal
override of the OS-specific regexp (filename_rules.gypi:32) doesn't work here? 
I should be able to use:

'sources/': [
  ['include', 'gl_implementation_linux\\.cc$'],
],

but that doesn't work (tried with a full path, no escaping, regexp for (cc|h),
and so on)...

[piman, 2013-10-03 19:54:17]

https://codereview.chromium.org/23868030/diff/13001/ui/gl/gl.gyp
File ui/gl/gl.gyp (right):

https://codereview.chromium.org/23868030/diff/13001/ui/gl/gl.gyp#newcode268
ui/gl/gl.gyp:268: 'gl_implementation_linux.h',
On 2013/10/03 19:46:42, Peter Beverloo wrote:
> I've tried a ton of things (regular inclusions like this, sources/ include
> expressions) to get gl_implementation_linux.cc to be included in Android's
> target, but it doesn't want to work.
> 
> Does anyone know the gyp structure in ui/gl/ who can understand why a normal
> override of the OS-specific regexp (filename_rules.gypi:32) doesn't work here?

> I should be able to use:
> 
> 'sources/': [
>   ['include', 'gl_implementation_linux\\.cc$'],
> ],
> 
> but that doesn't work (tried with a full path, no escaping, regexp for (cc|h),
> and so on)...

Not sure... Feel free to rename this (though I can't come up with a good name),
and add it explicitly on linux/android.

[no sievers, 2013-10-03 20:06:00]

On 2013/10/03 19:54:17, piman wrote:
> https://codereview.chromium.org/23868030/diff/13001/ui/gl/gl.gyp
> File ui/gl/gl.gyp (right):
> 
> https://codereview.chromium.org/23868030/diff/13001/ui/gl/gl.gyp#newcode268
> ui/gl/gl.gyp:268: 'gl_implementation_linux.h',
> On 2013/10/03 19:46:42, Peter Beverloo wrote:
> > I've tried a ton of things (regular inclusions like this, sources/ include
> > expressions) to get gl_implementation_linux.cc to be included in Android's
> > target, but it doesn't want to work.
> > 
> > Does anyone know the gyp structure in ui/gl/ who can understand why a normal
> > override of the OS-specific regexp (filename_rules.gypi:32) doesn't work
here?
> 
> > I should be able to use:
> > 
> > 'sources/': [
> >   ['include', 'gl_implementation_linux\\.cc$'],
> > ],
> > 
> > but that doesn't work (tried with a full path, no escaping, regexp for
(cc|h),
> > and so on)...
> 
> Not sure... Feel free to rename this (though I can't come up with a good
name),
> and add it explicitly on linux/android.

You could split out the part you are interested into gl_implementation_osmesa.cc

I bet gyp just won't let you include *_linux.cc if you are not building for
Linux.

[Peter Beverloo, 2013-10-04 14:24:53]

Done.  Including _linux.cc files in other directories works fine, I don't know
why it doesn't work here.

[piman, 2013-10-04 17:58:18]

On 2013/10/04 14:24:53, Peter Beverloo wrote:
> Done.  Including _linux.cc files in other directories works fine, I don't know
> why it doesn't work here.

LGTM, thanks.

[no sievers, 2013-10-04 21:48:54]

lgtm with one nit.

Cool, now we can run cc pixel tests.

https://codereview.chromium.org/23868030/diff/29001/ui/gl/gl_implementation_o...
File ui/gl/gl_implementation_osmesa.h (right):

https://codereview.chromium.org/23868030/diff/29001/ui/gl/gl_implementation_o...
ui/gl/gl_implementation_osmesa.h:15: base::NativeLibrary LoadLibrary(const
base::FilePath& filename);
Do the LoadLibrary function maybe fit better into gl_implementation.h/.cc?
They are not really specific to Mesa and also used to load other (e.g. EGL)
libraries.

[Peter Beverloo, 2013-10-25 15:40:55]

To give a status-update about this patch: using OSMesa works fine on most
devices, but content_shell just times out when using this on a Nexus 7.  I have
not yet been able to track that down, but it's important enough to wait for.

I've removed the bits that enable this by default for layout tests until that
can be resolved.  Is this patch ok to land in its current form?

https://codereview.chromium.org/23868030/diff/29001/ui/gl/gl_implementation_o...
File ui/gl/gl_implementation_osmesa.h (right):

https://codereview.chromium.org/23868030/diff/29001/ui/gl/gl_implementation_o...
ui/gl/gl_implementation_osmesa.h:15: base::NativeLibrary LoadLibrary(const
base::FilePath& filename);
On 2013/10/04 21:48:54, sievers wrote:
> Do the LoadLibrary function maybe fit better into gl_implementation.h/.cc?
> They are not really specific to Mesa and also used to load other (e.g. EGL)
> libraries.

They feel like base/-level methods to me.  I'd prefer to limit exposure as far
as we can, or formally move them to ui/base/.

[no sievers, 2013-10-25 19:08:02]

On 2013/10/25 15:40:55, Peter Beverloo wrote:
> To give a status-update about this patch: using OSMesa works fine on most
> devices, but content_shell just times out when using this on a Nexus 7.  I
have
> not yet been able to track that down, but it's important enough to wait for.
> 
> I've removed the bits that enable this by default for layout tests until that
> can be resolved.  Is this patch ok to land in its current form?
> 

Sure, you should definitely land this. It also enables us to try and run cc
pixel tests.

[no sievers, 2013-10-25 19:08:13]

https://codereview.chromium.org/23868030/diff/29001/ui/gl/gl_implementation_o...
File ui/gl/gl_implementation_osmesa.h (right):

https://codereview.chromium.org/23868030/diff/29001/ui/gl/gl_implementation_o...
ui/gl/gl_implementation_osmesa.h:15: base::NativeLibrary LoadLibrary(const
base::FilePath& filename);
On 2013/10/25 15:40:55, Peter Beverloo wrote:
> On 2013/10/04 21:48:54, sievers wrote:
> > Do the LoadLibrary function maybe fit better into gl_implementation.h/.cc?
> > They are not really specific to Mesa and also used to load other (e.g. EGL)
> > libraries.
> 
> They feel like base/-level methods to me.  I'd prefer to limit exposure as far
> as we can, or formally move them to ui/base/.

It just felt a bit weird that gl_implementation_android.cc needs to include
gl_implementation_osmesa.h regardless of osmesa-specific functionality. Now that
I look at them, the functions don't look very useful anyways (one is a
one-liner, the other one adds a VLOG on failure), so you could probably also
just remove/inline them.

[no sievers, 2013-11-11 20:20:05]

https://codereview.chromium.org/23868030/diff/40001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/23868030/diff/40001/content/content_shell.gyp...
content/content_shell.gypi:859: {
Can this somehow move to the libosmesa target itself or to its own target?

Other targets might need this too, and it creates a gyp warning if I simply
duplicate it for other targets.

[Peter Beverloo, 2013-11-12 20:22:09]

On 2013/11/11 20:20:05, sievers wrote:
> https://codereview.chromium.org/23868030/diff/40001/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
https://codereview.chromium.org/23868030/diff/40001/content/content_shell.gyp...
> content/content_shell.gypi:859: {
> Can this somehow move to the libosmesa target itself or to its own target?
> 
> Other targets might need this too, and it creates a gyp warning if I simply
> duplicate it for other targets.

I had to create an empty target in order for GYP's rule processing order to work
nicely with it.  The action can't just move to the osmesa target because then we
create a circular dependency, and we can't use postbuild rules because a script
depend on it to be an actual target.

What do you think about this solution?

[no sievers, 2013-11-12 22:31:46]

On 2013/11/12 20:22:09, Peter Beverloo wrote:
> On 2013/11/11 20:20:05, sievers wrote:
> >
https://codereview.chromium.org/23868030/diff/40001/content/content_shell.gypi
> > File content/content_shell.gypi (right):
> > 
> >
>
https://codereview.chromium.org/23868030/diff/40001/content/content_shell.gyp...
> > content/content_shell.gypi:859: {
> > Can this somehow move to the libosmesa target itself or to its own target?
> > 
> > Other targets might need this too, and it creates a gyp warning if I simply
> > duplicate it for other targets.
> 
> I had to create an empty target in order for GYP's rule processing order to
work
> nicely with it.  The action can't just move to the osmesa target because then
we
> create a circular dependency, and we can't use postbuild rules because a
script
> depend on it to be an actual target.
> 
> What do you think about this solution?

Maybe it's actually a problem of the 'extra_native_lib' handling that this
doesn't work?
Also looks like extra_native_libs is not used otherwise.

But feel free to land whatever works for content shell. I'll try to get it to
work with cc unittests afterwards.

[jochen (gone - plz use gerrit), 2014-01-03 10:43:42]

any updates on this?

[Peter Beverloo, 2014-02-19 15:43:26]

Large rebase, apologies for the long delay.  Notable changes for the reviewers:

  * jochen: Enabling OSMesa by default in shell_main_delegate.cc. Verified it
works on Nexus 4, 5 and 7.
  * /ui/gl/: Added OSMesa to InitializeDebugGLBindings and ClearGLBindings,
which matches what other platforms do.

I also verified the results are consistent between the devices, so that's a very
big win :-).

[jochen (gone - plz use gerrit), 2014-02-19 17:23:16]

lgtm

[Sami, 2014-02-19 18:42:43]

lgtm, thanks Peter!

https://codereview.chromium.org/23868030/diff/460001/ui/gl/gl_implementation_...
File ui/gl/gl_implementation_android.cc (right):

https://codereview.chromium.org/23868030/diff/460001/ui/gl/gl_implementation_...
ui/gl/gl_implementation_android.cc:85: return
InitializeStaticGLBindingsOSMesaGL();
nit: To make this a tiny bit more consistent, instead of returning here just
fall through to the "return true" at the bottom of this function.

https://codereview.chromium.org/23868030/diff/460001/ui/gl/gl_implementation_...
File ui/gl/gl_implementation_osmesa.h (right):

https://codereview.chromium.org/23868030/diff/460001/ui/gl/gl_implementation_...
ui/gl/gl_implementation_osmesa.h:5: #ifndef UI_GL_GL_IMPLEMENTATION_OSMESA_
Heh, oops :)

[Peter Beverloo, 2014-02-20 16:57:52]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-02-20 16:58:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/23868030/460001

[commit-bot: I haz the power, 2014-02-20 17:07:53]

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

[commit-bot: I haz the power, 2014-02-20 17:07:54]

Try jobs failed on following builders: linux_chromium_rel

[Peter Beverloo, 2014-02-21 12:15:20]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-02-21 12:15:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/23868030/590001

[commit-bot: I haz the power, 2014-02-21 14:10:23]

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

[commit-bot: I haz the power, 2014-02-21 14:10:24]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Peter Beverloo, 2014-02-21 14:13:37]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-02-21 14:13:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peter@chromium.org/23868030/590001

[commit-bot: I haz the power, 2014-02-21 17:08:35]

Change committed as 252583