Ensure GL initialization only happens once, and provide common init path

Ensure GL initialization only happens once, and provide common init path

Currently tests initialize GL by calling into methods that should be
internal to the gl bindings code. Instead, everyone should go through
GLSurface::InitializeOneOff.

Also GLSurface::InitializeOneOff early outs if it was already called,
leading to a pattern of initializing GL all over the place just in case
and not having a clear idea of where it should be set up. Instead, DCHECK
that it is not called more than once, and move calls to this method to
be during process startup for unit test suites instead of mid-test.

This adds two test variants of InitializeOneOff for tests to call, that
set up OSMesa or Mock GL bindings, via GLSurface::InitializeOneOff.

R=ben@chromium.org, piman@chromium.org, sievers@chromium.org, piman, sievers
BUG=270918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247793

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

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

[piman, 2014-01-24 19:22:20]

https://codereview.chromium.org/135213003/diff/370001/ui/compositor/test/defa...
File ui/compositor/test/default_context_factory.cc (left):

https://codereview.chromium.org/135213003/diff/370001/ui/compositor/test/defa...
ui/compositor/test/default_context_factory.cc:24: if
(!gfx::GLSurface::InitializeOneOff() ||
So, there are a few non-unit-test executables that use this code - e.g.
aura_bench, aura_demo, views_examples_exe (found by looking for
InitializeContextFactoryForTests). Do you want to add
GLSurface::InitializeOneOff in those, or somehow still have
InitializeContextFactoryForTests do something?

https://codereview.chromium.org/135213003/diff/370001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/135213003/diff/370001/ui/gl/gl_surface.cc#new...
ui/gl/gl_surface.cc:96: DCHECK(cmd->GetSwitchValueASCII(switches::kUseGL) ==
nit: maybe CHECK with an error message, so that naughty users that try to pass
--use-gl get a warning?

[danakj, 2014-01-24 20:17:23]

Done. PTAL

https://codereview.chromium.org/135213003/diff/370001/ui/compositor/test/defa...
File ui/compositor/test/default_context_factory.cc (left):

https://codereview.chromium.org/135213003/diff/370001/ui/compositor/test/defa...
ui/compositor/test/default_context_factory.cc:24: if
(!gfx::GLSurface::InitializeOneOff() ||
On 2014/01/24 19:22:23, piman wrote:
> So, there are a few non-unit-test executables that use this code - e.g.
> aura_bench, aura_demo, views_examples_exe (found by looking for
> InitializeContextFactoryForTests). Do you want to add
> GLSurface::InitializeOneOff in those, or somehow still have
> InitializeContextFactoryForTests do something?

I'll make them call GLSurface::InitializeOneOff() if they aren't already.

https://codereview.chromium.org/135213003/diff/370001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/135213003/diff/370001/ui/gl/gl_surface.cc#new...
ui/gl/gl_surface.cc:96: DCHECK(cmd->GetSwitchValueASCII(switches::kUseGL) ==
On 2014/01/24 19:22:23, piman wrote:
> nit: maybe CHECK with an error message, so that naughty users that try to pass
> --use-gl get a warning?

kk

[piman, 2014-01-24 20:24:35]

LGTM. +ben for ui/ stuff.

[danakj, 2014-01-25 01:37:21]

I updated the GLSurface code to not modify the command line anymore in tests.
This avoids problems where the test process spawns a child that runs a test, and
you have the parent's command line propagated to the child.

Also on mac, sandbox tests spawn a child process and then set up a sandbox. The
sandbox expects to be able to InitializeOneOff, so the test suite should not do
that ahead of time. So, I have the unit test suite not initializing GL in a
child process at all. I thought about doing this only on mac since that is where
it matters, but having identical behaviour between platforms seemed preferable.

[piman, 2014-01-25 01:48:13]

lgtm

[Ben Goodger (Google), 2014-01-25 02:45:25]

ui lgtm

[commit-bot: I haz the power, 2014-01-25 03:02:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/135213003/930001

[commit-bot: I haz the power, 2014-01-25 03:56:00]

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

[commit-bot: I haz the power, 2014-01-27 20:16:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/135213003/1290001

[danakj, 2014-01-29 01:06:11]

Daniel, can you please take a look, in particular at:

content/browser/browser_main_loop.cc
content/gpu/gpu_child_thread.cc
content/gpu/gpu_main.cc
gpu/config/gpu_info_collector_android.cc

These files have android-specific changes to them to init GL earlier than for
other platforms due to crbug.com/326295

[no sievers, 2014-01-29 01:54:22]

On 2014/01/29 01:06:11, danakj wrote:
> Daniel, can you please take a look, in particular at:
> 
> content/browser/browser_main_loop.cc
> content/gpu/gpu_child_thread.cc
> content/gpu/gpu_main.cc
> gpu/config/gpu_info_collector_android.cc
> 

Those files LGTM

[no sievers, 2014-01-29 02:00:21]

https://codereview.chromium.org/135213003/diff/1540001/gpu/config/gpu_info_co...
File gpu/config/gpu_info_collector_android.cc (right):

https://codereview.chromium.org/135213003/diff/1540001/gpu/config/gpu_info_co...
gpu/config/gpu_info_collector_android.cc:75: if (!eglMakeCurrent(display_,
draw_surface_, read_surface_, context_))
I think display_ at least might have to be valid. So maybe you want to keep the
early out from before.

[no sievers, 2014-01-29 02:14:57]

https://codereview.chromium.org/135213003/diff/1540001/gpu/config/gpu_info_co...
File gpu/config/gpu_info_collector_android.cc (right):

https://codereview.chromium.org/135213003/diff/1540001/gpu/config/gpu_info_co...
gpu/config/gpu_info_collector_android.cc:75: if (!eglMakeCurrent(display_,
draw_surface_, read_surface_, context_))
On 2014/01/29 02:00:22, sievers wrote:
> I think display_ at least might have to be valid. So maybe you want to keep
the
> early out from before.

Or you can do something like this:

if (display_ == EGL_NO_DISPLAY && gfx::GLContext::GetCurrent())
  display_ = gfx::GLContext::GetCurrent()->GetDisplay();

[danakj, 2014-01-29 19:05:58]

https://codereview.chromium.org/135213003/diff/1540001/gpu/config/gpu_info_co...
File gpu/config/gpu_info_collector_android.cc (right):

https://codereview.chromium.org/135213003/diff/1540001/gpu/config/gpu_info_co...
gpu/config/gpu_info_collector_android.cc:75: if (!eglMakeCurrent(display_,
draw_surface_, read_surface_, context_))
On 2014/01/29 02:14:58, sievers wrote:
> On 2014/01/29 02:00:22, sievers wrote:
> > I think display_ at least might have to be valid. So maybe you want to keep
> the
> > early out from before.
> 
> Or you can do something like this:
> 
> if (display_ == EGL_NO_DISPLAY && gfx::GLContext::GetCurrent())
>   display_ = gfx::GLContext::GetCurrent()->GetDisplay();

Ok I've put back the early out as we discussed, to handle the case where nothing
was current in the constructor.

[commit-bot: I haz the power, 2014-01-29 20:13:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/135213003/1560001

[commit-bot: I haz the power, 2014-01-30 01:04:57]

Change committed as 247793

[commit-bot: I haz the power, 2014-01-30 20:30:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/135213003/1580001

[commit-bot: I haz the power, 2014-01-30 22:05:45]

Change committed as 248049

[commit-bot: I haz the power, 2014-01-30 22:05:59]

CQ bit was unchecked on CL. Ignoring.

[danakj, 2014-01-31 16:08:01]

Mac GL unit tests expect to use real GL not osmesa. So I've updated the CL to
always use real GL on mac, which is similar to how we run browser tests.

[danakj, 2014-01-31 16:15:44]

On 2014/01/31 16:08:01, danakj wrote:
> Mac GL unit tests expect to use real GL not osmesa. So I've updated the CL to
> always use real GL on mac, which is similar to how we run browser tests.

Actually, this same problem is equal for all OS/bots on the gpu waterfall. They
just want to run tests with real GL. So I've moved the kUseGPUInTests flag to
gl_switches.h/cc and allow that to turn off osmesa for unit tests the same way
it does for browser tests.

[danakj, 2014-02-03 18:12:30]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-03 18:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/135213003/1640005

[danakj, 2014-02-03 19:53:24]

Committed patchset #36 manually as r248557 (presubmit successful).

[commit-bot: I haz the power, 2014-02-03 19:53:27]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 19:53:29]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 19:53:49]

CQ bit was unchecked on CL. Ignoring.