content: Allow software compositing on Chrome OS if requested

content: Allow software compositing on Chrome OS if requested

We don't want to fallback to software after GPU crash, but it's OK to
use software if we don't start a GPU process in the first place due
to the --disable-gpu command line flag.

This is used very rarely, but is a useful ability during hardware bringup
because it does not require a working GL driver. It used to work until
we got stricter about disabling software fallback on CrOS.

BUG=506034
TEST=Chrome OS link build with following flags in chrome_dev.conf:
  --disable-gpu
  --ozone-platform=drm

[spang, 2015-07-01 01:48:51]

spang@chromium.org changed reviewers:
+ jbauman@chromium.org

[danakj, 2015-07-01 18:34:01]

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

[danakj, 2015-07-01 18:34:02]

https://codereview.chromium.org/1216303004/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1216303004/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:182: // Software
fallback does not happen on Chrome OS.
I think you want to change here instead. There's no need to try GL a bunch of
times and fallback.

[spang, 2015-07-07 23:22:59]

On 2015/07/01 18:34:02, danakj wrote:
>
https://codereview.chromium.org/1216303004/diff/1/content/browser/compositor/...
> File content/browser/compositor/gpu_process_transport_factory.cc (right):
> 
>
https://codereview.chromium.org/1216303004/diff/1/content/browser/compositor/...
> content/browser/compositor/gpu_process_transport_factory.cc:182: // Software
> fallback does not happen on Chrome OS.
> I think you want to change here instead. There's no need to try GL a bunch of
> times and fallback.

Ah, I was wondering if there was a better place.

Does this version look right?

[danakj, 2015-07-07 23:42:45]

https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
content/browser/compositor/gpu_process_transport_factory.cc:181: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableGpu))
Hm, yes.. but I guess it means you have 2 places using this switch now:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...

and this makes things harder to reason about. What do you think John? Should the
#if CHROMEOS maybe live in the GpuDataManagerImplPrivate and this could always
respect the CanUseGpuBrowserCompositor?

Right now if you used --disable-gpu you'd end up disabling all GPU things except
the compositor, which is inconsistent and weird, because we make this #ifdef
decision over here instead of in the GpuDataManager.

I'm a little worried about making disable-gpu work on chromeos too. Suddenly
people may start using it in ways you didn't intend here. For example we had a
bunch of browser tests running chromeos with software compositing completely
accidentally before.

How hard is it to use a patched version of chrome in the cases where you'd want
to use this flag?

[spang, 2015-07-08 01:22:38]

On 2015/07/07 23:42:45, danakj wrote:
>
https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
> File content/browser/compositor/gpu_process_transport_factory.cc (right):
> 
>
https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
> content/browser/compositor/gpu_process_transport_factory.cc:181: if
> (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableGpu))
> Hm, yes.. but I guess it means you have 2 places using this switch now:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...
> 
> and this makes things harder to reason about. What do you think John? Should
the
> #if CHROMEOS maybe live in the GpuDataManagerImplPrivate and this could always
> respect the CanUseGpuBrowserCompositor?
> 
> Right now if you used --disable-gpu you'd end up disabling all GPU things
except
> the compositor, which is inconsistent and weird, because we make this #ifdef
> decision over here instead of in the GpuDataManager.
> 
> I'm a little worried about making disable-gpu work on chromeos too. Suddenly
> people may start using it in ways you didn't intend here. For example we had a
> bunch of browser tests running chromeos with software compositing completely
> accidentally before.
> 
> How hard is it to use a patched version of chrome in the cases where you'd
want
> to use this flag?

There's a (currently broken) chunk of code inside ui/ozone/platform/drm to
support software rendering. It doesn't really make sense to have that in the
repository if it can't be used without patching.

So I think we either have to remove that code, or make it work in unpatched
builds. If we remove the feature the patch to apply to restore this will be
large.

We've used this ability once so far, and when we did the CrOS builders were
actually producing images that used software rendering. This is enabled by
changing the arguments passed to chrome, we can't do that at all if we have to
apply patches.

[danakj, 2015-07-17 19:51:45]

On 2015/07/08 01:22:38, spang wrote:
> On 2015/07/07 23:42:45, danakj wrote:
> >
>
https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
> > File content/browser/compositor/gpu_process_transport_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
> > content/browser/compositor/gpu_process_transport_factory.cc:181: if
> > (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableGpu))
> > Hm, yes.. but I guess it means you have 2 places using this switch now:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...
> > 
> > and this makes things harder to reason about. What do you think John? Should
> the
> > #if CHROMEOS maybe live in the GpuDataManagerImplPrivate and this could
always
> > respect the CanUseGpuBrowserCompositor?
> > 
> > Right now if you used --disable-gpu you'd end up disabling all GPU things
> except
> > the compositor, which is inconsistent and weird, because we make this #ifdef
> > decision over here instead of in the GpuDataManager.
> > 
> > I'm a little worried about making disable-gpu work on chromeos too. Suddenly
> > people may start using it in ways you didn't intend here. For example we had
a
> > bunch of browser tests running chromeos with software compositing completely
> > accidentally before.
> > 
> > How hard is it to use a patched version of chrome in the cases where you'd
> want
> > to use this flag?
> 
> There's a (currently broken) chunk of code inside ui/ozone/platform/drm to
> support software rendering. It doesn't really make sense to have that in the
> repository if it can't be used without patching.
> 
> So I think we either have to remove that code, or make it work in unpatched
> builds. If we remove the feature the patch to apply to restore this will be
> large.
> 
> We've used this ability once so far, and when we did the CrOS builders were
> actually producing images that used software rendering. This is enabled by
> changing the arguments passed to chrome, we can't do that at all if we have to
> apply patches.

I dunno. I want to say we should not support software compositing, but I don't
want to be a pain either. But keeping around modes that people will start to use
that we don't intend is unfortunate.

[spang, 2015-07-17 20:14:59]

On 2015/07/17 19:51:45, danakj wrote:
> On 2015/07/08 01:22:38, spang wrote:
> > On 2015/07/07 23:42:45, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
> > > File content/browser/compositor/gpu_process_transport_factory.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1216303004/diff/20001/content/browser/composi...
> > > content/browser/compositor/gpu_process_transport_factory.cc:181: if
> > > (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableGpu))
> > > Hm, yes.. but I guess it means you have 2 places using this switch now:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...
> > > 
> > > and this makes things harder to reason about. What do you think John?
Should
> > the
> > > #if CHROMEOS maybe live in the GpuDataManagerImplPrivate and this could
> always
> > > respect the CanUseGpuBrowserCompositor?
> > > 
> > > Right now if you used --disable-gpu you'd end up disabling all GPU things
> > except
> > > the compositor, which is inconsistent and weird, because we make this
#ifdef
> > > decision over here instead of in the GpuDataManager.
> > > 
> > > I'm a little worried about making disable-gpu work on chromeos too.
Suddenly
> > > people may start using it in ways you didn't intend here. For example we
had
> a
> > > bunch of browser tests running chromeos with software compositing
completely
> > > accidentally before.
> > > 
> > > How hard is it to use a patched version of chrome in the cases where you'd
> > want
> > > to use this flag?
> > 
> > There's a (currently broken) chunk of code inside ui/ozone/platform/drm to
> > support software rendering. It doesn't really make sense to have that in the
> > repository if it can't be used without patching.
> > 
> > So I think we either have to remove that code, or make it work in unpatched
> > builds. If we remove the feature the patch to apply to restore this will be
> > large.
> > 
> > We've used this ability once so far, and when we did the CrOS builders were
> > actually producing images that used software rendering. This is enabled by
> > changing the arguments passed to chrome, we can't do that at all if we have
to
> > apply patches.
> 
> I dunno. I want to say we should not support software compositing, but I don't
> want to be a pain either. But keeping around modes that people will start to
use
> that we don't intend is unfortunate.

Do you think we will be able to deprecate SW in any meaningful way if we don't
allow it on CrOS? Right now it's working, except for the fact that we prevent it
in an #ifdef. So I guess we haven't had any maintenance benefits so far.

The problem with GL is that vendors are very protective of their driver source,
so it ends up being one of the hairiest drivers to integrate. Allowing software
mode if you opt-in via flags let's everyone else make progress while that work
happens.