cc: Add kDisableGpuRasterization to content_shell for layout tests

gpu: content: Explicitly disable gpu on desktop

Required for https://codereview.chromium.org/792233007/, we don't load
the blacklist for layout tests (because we use osmesa), but due to the
coming change to enable the gpu rasterization on desktop, and because
the blacklist is not loaded, the layout tests end up rasterizing with
via GPU rasterization.  Later when we try to capture the pixels, we end
up failing.

This change forces gpu rasterization off for desktop, and as a result,
allows the layout tests to correctly run on desktop.  It also removes
desktop gpu rasterization from the blacklist, if we ever enable it, we''ll
need better test coverage.

BUG=440518
Committed: https://crrev.com/fbc05418616608576acb69ae1f4fee12720bc0e5
Cr-Commit-Position: refs/heads/master@{#328015}

[hendrikw, 2015-02-28 00:10:55]

hendrikw@chromium.org changed reviewers:
+ piman@chromium.org

[hendrikw, 2015-02-28 00:10:55]

PTAL, thanks!!!

[hendrikw, 2015-04-10 19:43:19]

On 2015/02/28 00:10:55, hendrikw wrote:
> PTAL, thanks!!!

Hi!

* I've verified that --force-gpu-rasterization will turn on gpu even though
we're explicitly disabling it here
* this flag is only used for layout tests
* all layout tests have now been run with mesa & real hardware (windows, there
are plans to run android, and maybe mac)
* many bugs have been logged (several being mesa specific), and individual Skia
tests are being written to catch these going forward
* it looks like we probably won't ever run these daily with GPU, though this is
still up for discussion

I'd like to move forward with making gpu rasterization disabled by default for
layout tests so that I can enable gpu rasterization via about:flags on desktop.

PTAL, thanks!

[hendrikw, 2015-05-01 02:03:23]

I had a few cycles to look at this again.

To jog our memory, the last time we discussed this, you asked why
gpu_rasterization wasn't false when the blacklist isn't loaded?  The reason
being, it's a blacklist :)

I looked a bit, we don't have the concept of a whitelist, and I don't think it
makes sense to add one.  We could:

1. Add gpu_rasterization to the default blacklist in GpuDataManagerImplPrivate,
and reset it if we load a blacklist.  This seems questionable, inconsistent with
the other blacklisted features.
2. change it so that we blacklist software rasterization instead.  I don't think
it makes sense though, maybe because I naturally think of software as the
fallback when gpu isn't available. We might also have problems when we add new
devices (although you could argue that new devices probably should run with
gpu).
3. Load our own private blacklist in "content_shell --run-layout-test", where we
just add gpu rasterization to the blacklist.
4. Disable gpu rasterization in "content_shell --run-layout-test"
5. Load the blacklist correctly with content_shell.

3 and 4 are basically the same, but 4 is simpler (and happens to be this change)

I initially tried 5, but It met some resistance, specifically that loading the
blacklist caused the tests to slow down.  I think it tripped up the try bots
too.  Also, since we default to osmesa for these tests, the blacklist might not
actually be correct for all features.

WDYT?

[piman, 2015-05-01 18:34:54]

On Thu, Apr 30, 2015 at 7:03 PM, <hendrikw@chromium.org> wrote:

> I had a few cycles to look at this again.
>
> To jog our memory, the last time we discussed this, you asked why
> gpu_rasterization wasn't false when the blacklist isn't loaded?  The reason
> being, it's a blacklist :)
>
> I looked a bit, we don't have the concept of a whitelist, and I don't
> think it
> makes sense to add one.  We could:
>
> 1. Add gpu_rasterization to the default blacklist in
> GpuDataManagerImplPrivate,
> and reset it if we load a blacklist.  This seems questionable,
> inconsistent with
> the other blacklisted features.
> 2. change it so that we blacklist software rasterization instead.  I don't
> think
> it makes sense though, maybe because I naturally think of software as the
> fallback when gpu isn't available. We might also have problems when we add
> new
> devices (although you could argue that new devices probably should run with
> gpu).
> 3. Load our own private blacklist in "content_shell --run-layout-test",
> where we
> just add gpu rasterization to the blacklist.
> 4. Disable gpu rasterization in "content_shell --run-layout-test"
> 5. Load the blacklist correctly with content_shell.
>
> 3 and 4 are basically the same, but 4 is simpler (and happens to be this
> change)
>
> I initially tried 5, but It met some resistance, specifically that loading
> the
> blacklist caused the tests to slow down.  I think it tripped up the try
> bots
> too.  Also, since we default to osmesa for these tests, the blacklist
> might not
> actually be correct for all features.
>
> WDYT?
>

What I want is gpu rasterization to be off on all platforms except those
where we explicitly want to ship it (Android), and this should be
independent of the blacklist code.
I.e. to change IsGpuRasterizationEnabled in compositor_util.cc so that it
returns false on !Android unless flags are there to enable it.


>
> https://codereview.chromium.org/963963002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hendrikw, 2015-05-01 18:58:47]

On 2015/05/01 18:34:54, piman (Very slow to review) wrote:
> On Thu, Apr 30, 2015 at 7:03 PM, <mailto:hendrikw@chromium.org> wrote:
> 
> > I had a few cycles to look at this again.
> >
> > To jog our memory, the last time we discussed this, you asked why
> > gpu_rasterization wasn't false when the blacklist isn't loaded?  The reason
> > being, it's a blacklist :)
> >
> > I looked a bit, we don't have the concept of a whitelist, and I don't
> > think it
> > makes sense to add one.  We could:
> >
> > 1. Add gpu_rasterization to the default blacklist in
> > GpuDataManagerImplPrivate,
> > and reset it if we load a blacklist.  This seems questionable,
> > inconsistent with
> > the other blacklisted features.
> > 2. change it so that we blacklist software rasterization instead.  I don't
> > think
> > it makes sense though, maybe because I naturally think of software as the
> > fallback when gpu isn't available. We might also have problems when we add
> > new
> > devices (although you could argue that new devices probably should run with
> > gpu).
> > 3. Load our own private blacklist in "content_shell --run-layout-test",
> > where we
> > just add gpu rasterization to the blacklist.
> > 4. Disable gpu rasterization in "content_shell --run-layout-test"
> > 5. Load the blacklist correctly with content_shell.
> >
> > 3 and 4 are basically the same, but 4 is simpler (and happens to be this
> > change)
> >
> > I initially tried 5, but It met some resistance, specifically that loading
> > the
> > blacklist caused the tests to slow down.  I think it tripped up the try
> > bots
> > too.  Also, since we default to osmesa for these tests, the blacklist
> > might not
> > actually be correct for all features.
> >
> > WDYT?
> >
> 
> What I want is gpu rasterization to be off on all platforms except those
> where we explicitly want to ship it (Android), and this should be
> independent of the blacklist code.
> I.e. to change IsGpuRasterizationEnabled in compositor_util.cc so that it
> returns false on !Android unless flags are there to enable it.
> 
> 
> >
> > https://codereview.chromium.org/963963002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Right, I understood, you want to white-list it on Android (or certain Android
devices). Currently we blacklist all Android devices, except on certain models
where we mark it as an exception. See
gpu\config\software_rendering_list_json.cc.
(https://code.google.com/p/chromium/codesearch#chromium/src/gpu/config/softwar...)

This is the system we have to determine the state of a feature per device, I
don't think it's a good idea to introduce a different way to handle this.  

We could make software_rasterization a feature, and blacklist it on devices
where we want gpu rasterization, this will have the behaviour you're looking
for, but it doesn't seem right to me.

[piman, 2015-05-01 19:30:00]

On Fri, May 1, 2015 at 11:58 AM, <hendrikw@chromium.org> wrote:

> On 2015/05/01 18:34:54, piman (Very slow to review) wrote:
>
>  On Thu, Apr 30, 2015 at 7:03 PM, <mailto:hendrikw@chromium.org> wrote:
>>
>
>  > I had a few cycles to look at this again.
>> >
>> > To jog our memory, the last time we discussed this, you asked why
>> > gpu_rasterization wasn't false when the blacklist isn't loaded?  The
>> reason
>> > being, it's a blacklist :)
>> >
>> > I looked a bit, we don't have the concept of a whitelist, and I don't
>> > think it
>> > makes sense to add one.  We could:
>> >
>> > 1. Add gpu_rasterization to the default blacklist in
>> > GpuDataManagerImplPrivate,
>> > and reset it if we load a blacklist.  This seems questionable,
>> > inconsistent with
>> > the other blacklisted features.
>> > 2. change it so that we blacklist software rasterization instead.  I
>> don't
>> > think
>> > it makes sense though, maybe because I naturally think of software as
>> the
>> > fallback when gpu isn't available. We might also have problems when we
>> add
>> > new
>> > devices (although you could argue that new devices probably should run
>> with
>> > gpu).
>> > 3. Load our own private blacklist in "content_shell --run-layout-test",
>> > where we
>> > just add gpu rasterization to the blacklist.
>> > 4. Disable gpu rasterization in "content_shell --run-layout-test"
>> > 5. Load the blacklist correctly with content_shell.
>> >
>> > 3 and 4 are basically the same, but 4 is simpler (and happens to be this
>> > change)
>> >
>> > I initially tried 5, but It met some resistance, specifically that
>> loading
>> > the
>> > blacklist caused the tests to slow down.  I think it tripped up the try
>> > bots
>> > too.  Also, since we default to osmesa for these tests, the blacklist
>> > might not
>> > actually be correct for all features.
>> >
>> > WDYT?
>> >
>>
>
>  What I want is gpu rasterization to be off on all platforms except those
>> where we explicitly want to ship it (Android), and this should be
>> independent of the blacklist code.
>> I.e. to change IsGpuRasterizationEnabled in compositor_util.cc so that it
>> returns false on !Android unless flags are there to enable it.
>>
>
>
>  >
>> > https://codereview.chromium.org/963963002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Right, I understood, you want to white-list it on Android (or certain
> Android
> devices). Currently we blacklist all Android devices, except on certain
> models
> where we mark it as an exception. See
> gpu\config\software_rendering_list_json.cc.
> (
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/config/softwar...
> )
>
> This is the system we have to determine the state of a feature per device,
> I
> don't think it's a good idea to introduce a different way to handle this.
>

compositor_util.cc is the system we use for every single compositor
feature. I don't think we should do it differently for GPU rasterization.

>
> We could make software_rasterization a feature, and blacklist it on devices
> where we want gpu rasterization, this will have the behaviour you're
> looking
> for, but it doesn't seem right to me.
>
>
> https://codereview.chromium.org/963963002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hendrikw, 2015-05-01 19:53:14]

On 2015/05/01 19:30:00, piman (Very slow to review) wrote:
> On Fri, May 1, 2015 at 11:58 AM, <mailto:hendrikw@chromium.org> wrote:
> 
> > On 2015/05/01 18:34:54, piman (Very slow to review) wrote:
> >
> >  On Thu, Apr 30, 2015 at 7:03 PM, <mailto:hendrikw@chromium.org> wrote:
> >>
> >
> >  > I had a few cycles to look at this again.
> >> >
> >> > To jog our memory, the last time we discussed this, you asked why
> >> > gpu_rasterization wasn't false when the blacklist isn't loaded?  The
> >> reason
> >> > being, it's a blacklist :)
> >> >
> >> > I looked a bit, we don't have the concept of a whitelist, and I don't
> >> > think it
> >> > makes sense to add one.  We could:
> >> >
> >> > 1. Add gpu_rasterization to the default blacklist in
> >> > GpuDataManagerImplPrivate,
> >> > and reset it if we load a blacklist.  This seems questionable,
> >> > inconsistent with
> >> > the other blacklisted features.
> >> > 2. change it so that we blacklist software rasterization instead.  I
> >> don't
> >> > think
> >> > it makes sense though, maybe because I naturally think of software as
> >> the
> >> > fallback when gpu isn't available. We might also have problems when we
> >> add
> >> > new
> >> > devices (although you could argue that new devices probably should run
> >> with
> >> > gpu).
> >> > 3. Load our own private blacklist in "content_shell --run-layout-test",
> >> > where we
> >> > just add gpu rasterization to the blacklist.
> >> > 4. Disable gpu rasterization in "content_shell --run-layout-test"
> >> > 5. Load the blacklist correctly with content_shell.
> >> >
> >> > 3 and 4 are basically the same, but 4 is simpler (and happens to be this
> >> > change)
> >> >
> >> > I initially tried 5, but It met some resistance, specifically that
> >> loading
> >> > the
> >> > blacklist caused the tests to slow down.  I think it tripped up the try
> >> > bots
> >> > too.  Also, since we default to osmesa for these tests, the blacklist
> >> > might not
> >> > actually be correct for all features.
> >> >
> >> > WDYT?
> >> >
> >>
> >
> >  What I want is gpu rasterization to be off on all platforms except those
> >> where we explicitly want to ship it (Android), and this should be
> >> independent of the blacklist code.
> >> I.e. to change IsGpuRasterizationEnabled in compositor_util.cc so that it
> >> returns false on !Android unless flags are there to enable it.
> >>
> >
> >
> >  >
> >> > https://codereview.chromium.org/963963002/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > Right, I understood, you want to white-list it on Android (or certain
> > Android
> > devices). Currently we blacklist all Android devices, except on certain
> > models
> > where we mark it as an exception. See
> > gpu\config\software_rendering_list_json.cc.
> > (
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/config/softwar...
> > )
> >
> > This is the system we have to determine the state of a feature per device,
> > I
> > don't think it's a good idea to introduce a different way to handle this.
> >
> 
> compositor_util.cc is the system we use for every single compositor
> feature. I don't think we should do it differently for GPU rasterization.
> 
> >
> > We could make software_rasterization a feature, and blacklist it on devices
> > where we want gpu rasterization, this will have the behaviour you're
> > looking
> > for, but it doesn't seem right to me.
> >
> >
> > https://codereview.chromium.org/963963002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

right, and compositor_util uses the blacklist.

[hendrikw, 2015-05-01 21:31:37]

To capture our offline conversation:

Gpu rasterization is an untested feature (android already breaks the rule), the
original change would allow someone to turn on gpu rasterization on desktop, and
no test would catch it.

Instead, we remove desktop from the blacklist and explicitly disable gpu
rasterization on any non-android device (can still be turned on via experimental
flags).

PTAL

[piman, 2015-05-01 22:07:31]

lgtm

[piman, 2015-05-01 22:07:54]

https://codereview.chromium.org/963963002/diff/40001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/963963002/diff/40001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:21: "version": "10.4",
Oh, sorry, you need to bump this.

[hendrikw, 2015-05-01 22:12:26]

On 2015/05/01 22:07:54, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/963963002/diff/40001/gpu/config/software_rend...
> File gpu/config/software_rendering_list_json.cc (right):
> 
>
https://codereview.chromium.org/963963002/diff/40001/gpu/config/software_rend...
> gpu/config/software_rendering_list_json.cc:21: "version": "10.4",
> Oh, sorry, you need to bump this.

good catch!

[hendrikw, 2015-05-01 22:36:07]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-05-01 22:36:08]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/963963002/#ps80001 (title:
"merge - update version # again")

[commit-bot: I haz the power, 2015-05-01 22:37:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/963963002/80001

[commit-bot: I haz the power, 2015-05-01 23:46:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-01 23:47:02]

Patchset 5 (id:??) landed as
https://crrev.com/fbc05418616608576acb69ae1f4fee12720bc0e5
Cr-Commit-Position: refs/heads/master@{#328015}