Add DriverBugWorkaroundsInGpuProcessPage to gpu_process_test.py

Add DriverBugWorkaroundsInGpuProcessPage to gpu_process_test.py

This test ensure that the fake "use_gpu_driver_workaround_for_testing" is present in both Browser process (GpuDataManager) and GPU process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909
R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org, ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org, palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds_in_gpu_process

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/ff4a814529b414738c2f10ed0e57f021bc3d78a6
Cr-Commit-Position: refs/heads/master@{#385407}

[zmo, 2015-11-20 18:19:09]

zmo@google.com changed reviewers:
+ zmo@google.com

[zmo, 2015-11-20 18:19:09]

I am not sure I agree with the spirit of this CL, i.e., passing back workarounds
through GPUInfo for validation purpose.

I like the python test though.

Others, what do you feel?

[Ken Russell (switch to Gerrit), 2015-11-21 01:06:30]

Thanks for this contribution. A couple of questions.

https://codereview.chromium.org/1463823002/diff/1/content/test/gpu/gpu_tests/...
File content/test/gpu/gpu_tests/driver_bug_workarounds.py (right):

https://codereview.chromium.org/1463823002/diff/1/content/test/gpu/gpu_tests/...
content/test/gpu/gpu_tests/driver_bug_workarounds.py:1: # Copyright 2015 The
Chromium Authors. All rights reserved.
Does this new test fail, as it should, when the fix in this CL is removed?

What about if the fix from http://crbug.com/553524 is removed (leaving the fix
in this CL present)?

We need to make sure this test actually has value before moving forward with it.

Also, have you checked to see whether this could be folded into either the
gpu_process.py or hardware_accelerated_feature.py tests? It's painful to add a
new top-level test right now, unfortunately. See context_lost.py or pixel.py for
examples of tests which do very different things in different tests (different
command lines, different validation, etc.).

[Ken Russell (switch to Gerrit), 2015-11-21 01:08:13]

On 2015/11/20 18:19:09, zmo wrote:
> I am not sure I agree with the spirit of this CL, i.e., passing back
workarounds
> through GPUInfo for validation purpose.
> 
> I like the python test though.
> 
> Others, what do you feel?

I think that if this test actually catches the regression that was present in
Issue 553524 that it should be added.

[zmo, 2015-11-21 01:15:28]

On 2015/11/21 01:08:13, Ken Russell wrote:
> On 2015/11/20 18:19:09, zmo wrote:
> > I am not sure I agree with the spirit of this CL, i.e., passing back
> workarounds
> > through GPUInfo for validation purpose.
> > 
> > I like the python test though.
> > 
> > Others, what do you feel?
> 
> I think that if this test actually catches the regression that was present in
> Issue 553524 that it should be added.

The python test works independent of the rest of this CL.

I feel we should just land the python test, but under content/test/gpu/.

[Ken Russell (switch to Gerrit), 2015-11-21 01:28:53]

On 2015/11/21 01:15:28, zmo wrote:
> On 2015/11/21 01:08:13, Ken Russell wrote:
> > On 2015/11/20 18:19:09, zmo wrote:
> > > I am not sure I agree with the spirit of this CL, i.e., passing back
> > workarounds
> > > through GPUInfo for validation purpose.
> > > 
> > > I like the python test though.
> > > 
> > > Others, what do you feel?
> > 
> > I think that if this test actually catches the regression that was present
in
> > Issue 553524 that it should be added.
> 
> The python test works independent of the rest of this CL.
> 
> I feel we should just land the python test, but under content/test/gpu/.

The python test in this CL is under that directory.

[Julien Isorce Samsung, 2015-11-21 14:27:16]

On 2015/11/21 01:06:30, Ken Russell wrote:
>
https://codereview.chromium.org/1463823002/diff/1/content/test/gpu/gpu_tests/...
> content/test/gpu/gpu_tests/driver_bug_workarounds.py:1: # Copyright 2015 The
> Chromium Authors. All rights reserved.
> Does this new test fail, as it should, when the fix in this CL is removed?

It does not fail when this CL is removed. In order to fail this test you need to
keep this CL and revert fix from http://crbug.com/553524

> 
> What about if the fix from http://crbug.com/553524 is removed (leaving the fix
> in this CL present)?

If you revert fix from http://crbug.com/553524 and just keep the unit test from
this current CL (1463823002), the test does not failed.

> 
> We need to make sure this test actually has value before moving forward with
it.
> 
> Also, have you checked to see whether this could be folded into either the
> gpu_process.py or hardware_accelerated_feature.py tests? It's painful to add a
> new top-level test right now, unfortunately. See context_lost.py or pixel.py
for
> examples of tests which do very different things in different tests (different
> command lines, different validation, etc.).

Ok I'll have a look, make sense.

On 2015/11/21 01:15:28, zmo wrote:
> The python test works independent of the rest of this CL.

This python test cannot catch bug http://crbug.com/553524 without the new CL.
That's why from the review it has been decided to not include the test and
investigate how to catch this bug.

zmo please try the following:
- run this test while keeping https://codereview.chromium.org/1463823002/ and
then reverting it. You will notice the test succeeds in both cases. Which is
bad.
- Then apply this CL and revert https://codereview.chromium.org/1463823002/, you
will notice that the test will be able to fail.

Ken, zmo, have a look at the change in gpu_data_manager_impl_private.cc (from
this CL), I think I should uncomment "//gpu_driver_bugs_ =
gpu_info_.driver_bugs_workarounds ". So that this CL actually would be not about
just passing back workarounds
through GPUInfo for validation purpose but also to avoid recomputation of
workaround list in GpuDataManagerImplPrivate::UpdateGpuInfoHelper.

I'll go for a consensus so if you have better solution for catching bug
http://crbug.com/553524 I'll be happy to try :)

[zmo, 2015-11-23 18:10:57]

Reading more on the test, it's testing the workarounds passed in from the
commandline switch. So here are my two points.

1) if on the browser side, we fail to include the workarounds passed in from the
commandline switch, but we only do that in the gpu process, then we should fix
this bug.

2) this test doesn't have much value, because passing in workarounds from the
commandline switch isn't the code path that users are facing.

[Ken Russell (switch to Gerrit), 2015-11-24 02:49:45]

On 2015/11/21 14:27:16, j.isorce wrote:
> On 2015/11/21 01:06:30, Ken Russell wrote:
> >
>
https://codereview.chromium.org/1463823002/diff/1/content/test/gpu/gpu_tests/...
> > content/test/gpu/gpu_tests/driver_bug_workarounds.py:1: # Copyright 2015 The
> > Chromium Authors. All rights reserved.
> > Does this new test fail, as it should, when the fix in this CL is removed?
> 
> It does not fail when this CL is removed. In order to fail this test you need
to
> keep this CL and revert fix from http://crbug.com/553524
> 
> > 
> > What about if the fix from http://crbug.com/553524 is removed (leaving the
fix
> > in this CL present)?
> 
> If you revert fix from http://crbug.com/553524 and just keep the unit test
from
> this current CL (1463823002), the test does not failed.

OK, what I understand from these statements is that the code change in this CL
(ignoring the new test) plus the fix from http://crbug.com/553524 allow the test
added in this CL to act as a regression test.

> > 
> > We need to make sure this test actually has value before moving forward with
> it.
> > 
> > Also, have you checked to see whether this could be folded into either the
> > gpu_process.py or hardware_accelerated_feature.py tests? It's painful to add
a
> > new top-level test right now, unfortunately. See context_lost.py or pixel.py
> for
> > examples of tests which do very different things in different tests
(different
> > command lines, different validation, etc.).
> 
> Ok I'll have a look, make sense.

On second thought don't worry about doing this. Let's focus on getting this test
fully reviewed and landed.

> On 2015/11/21 01:15:28, zmo wrote:
> > The python test works independent of the rest of this CL.
> 
> This python test cannot catch bug http://crbug.com/553524 without the new CL.
> That's why from the review it has been decided to not include the test and
> investigate how to catch this bug.
> 
> zmo please try the following:
> - run this test while keeping https://codereview.chromium.org/1463823002/ and
> then reverting it. You will notice the test succeeds in both cases. Which is
> bad.
> - Then apply this CL and revert https://codereview.chromium.org/1463823002/,
you
> will notice that the test will be able to fail.
> 
> Ken, zmo, have a look at the change in gpu_data_manager_impl_private.cc (from
> this CL), I think I should uncomment "//gpu_driver_bugs_ =
> gpu_info_.driver_bugs_workarounds ". So that this CL actually would be not
about
> just passing back workarounds
> through GPUInfo for validation purpose but also to avoid recomputation of
> workaround list in GpuDataManagerImplPrivate::UpdateGpuInfoHelper.
> 
> I'll go for a consensus so if you have better solution for catching bug
> http://crbug.com/553524 I'll be happy to try :)

[Ken Russell (switch to Gerrit), 2015-11-24 03:00:04]

On 2015/11/23 18:10:57, zmo wrote:
> Reading more on the test, it's testing the workarounds passed in from the
> commandline switch. So here are my two points.
> 
> 1) if on the browser side, we fail to include the workarounds passed in from
the
> commandline switch, but we only do that in the gpu process, then we should fix
> this bug.

That was the previous bug (Issue 553524). To clarify (and to clarify my
understanding as well), the browser process was passing down the ID of one of
the driver bug workarounds to the GPU process, but because the GPU process was
recomputing the need for all of the workarounds itself, it was overwriting the
passed-in ones. about:gpu was reporting incorrect information since it was only
reporting the browser's view of the enabled driver bug workarounds, and the GPU
process's view of those workarounds was out of sync.

The bug's been fixed but there was no easy way to verify the fix.

The intent of the change here is to allow end-to-end testing of the enabled
workarounds, by passing back the GPU process's view of which workarounds are
enabled to the browser process.

> 2) this test doesn't have much value, because passing in workarounds from the
> commandline switch isn't the code path that users are facing.

The use of the fake workaround is only to verify the round-trip handling of the
driver bug workarounds.

j.isorce: please take a look at the fix for http://crbug.com/537776 and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
. If you could change the code so that it passes the fake driver bug workaround
from the browser to the GPU process only when the flag "--test-type=gpu" is
passed, maybe that would exercise the code paths better.

[Julien Isorce Samsung, 2015-11-24 17:16:27]

On 2015/11/24 02:49:45, Ken Russell wrote:
> OK, what I understand from these statements is that the code change in this CL
> (ignoring the new test) plus the fix from http://crbug.com/553524 allow the
test
> added in this CL to act as a regression test.

Exactly.

> The intent of the change here is to allow end-to-end testing of the enabled
> workarounds, by passing back the GPU process's view of which workarounds are
> enabled to the browser process.

Yes.

> j.isorce: please take a look at the fix for http://crbug.com/537776 and
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> . If you could change the code so that it passes the fake driver bug
workaround
> from the browser to the GPU process only when the flag "--test-type=gpu" is
> passed, maybe that would exercise the code paths better.

Ok I'll have a look. Note that "use_fake_gpu_driver_workaround" is disabled by
default so not passed to GPU process unless user explicitly provides
--use_fake_gpu_driver_workaround on command line.

But what I can do is in gpu_data_manager_impl_private.cc:

if (has switches::kTestType and value is "gpu" && not in sync) {
  LOG(ERROR) << "gpu driver bug workarounds not in sync, force it";
  gpu_driver_bugs_ = gpu_info_.driver_bugs_workarounds;
}

So that it forces sync and allows this CL to act as a regression test.

In this end the only downside of this CL for me is the new field in GPUInfo that
is only used for the test. (unless as I said we also use it to avoid the
re-computation in browser process)
Maybe I can check for "#if defined(OFFICIAL_BUILD)" ?

[zmo, 2015-11-24 17:34:53]

OK, my understanding is slightly different.

The original bug introduced by https://codereview.chromium.org/1406323005/ and
later fixed in https://codereview.chromium.org/1415603009/ doesn't affect
regular users.  It only affects testing, as the workarounds we manually pass in
to chrome through commandline switch get lost in GPU process.

Now when the full GPU info is sent back to browser process, we already have the
mechanism to recompute everything, including GPU driver bug workarounds.  After
this recomputation, browser process and GPU process are in sync.  If about:gpu
still displays the outdated information, then the fix should to notify the
about:gpu page to refresh.

My main point is, we don't need to send back computed workarounds from GPU
process to browser process.  This test only verifies a functionality purely for
testing, not for user facing code.

[zmo, 2015-11-24 17:38:35]

Also, gpu_driver_bug_list_->MakeDecision() doesn't just compute workarounds, but
also disabled_extensions (see the following line in
content/browser/gpu/gpu_data_manager_impl_private.cc).

If you want to pass back computed results, you also need to pass back
disabled_extensions.

I still think it's unnecessary, but if kbr feels it has value, I won't block it,
but at least the disabled_extension part needs to be addressed.

[Ken Russell (switch to Gerrit), 2015-11-24 18:33:27]

On 2015/11/24 17:34:53, zmo wrote:
> OK, my understanding is slightly different.
> 
> The original bug introduced by https://codereview.chromium.org/1406323005/ and
> later fixed in https://codereview.chromium.org/1415603009/ doesn't affect
> regular users.  It only affects testing, as the workarounds we manually pass
in
> to chrome through commandline switch get lost in GPU process.

That's not my understanding so let's work to understand the situation.
https://codereview.chromium.org/1406323005/ fixed a real bug where virtual GL
contexts weren't being enabled for the WebGL conformance tests as we thought
they were -- basically, for the browser's first run, any workarounds on Linux
that required full GPU info collection weren't being activated.

https://codereview.chromium.org/1415603009/ fixed a bug in that fix where any
passed-in workarounds on the command line were being blown away by the
recomputation. This could affect real users as well as testing.

> Now when the full GPU info is sent back to browser process, we already have
the
> mechanism to recompute everything, including GPU driver bug workarounds. 
After
> this recomputation, browser process and GPU process are in sync.  If about:gpu
> still displays the outdated information, then the fix should to notify the
> about:gpu page to refresh.
>
> My main point is, we don't need to send back computed workarounds from GPU
> process to browser process.  This test only verifies a functionality purely
for
> testing, not for user facing code.

The bug was that it was technically possible for the browser and GPU processes'
notions of the enabled workarounds to get out of sync. This was what happened
between those two patch sets. There needs to be one source of ground truth, and
I think it should be the GPU process, since that's the one that actually runs
the code.

> Also, gpu_driver_bug_list_->MakeDecision() doesn't just compute workarounds,
but
> also disabled_extensions (see the following line in
> content/browser/gpu/gpu_data_manager_impl_private.cc).
> 
> If you want to pass back computed results, you also need to pass back
> disabled_extensions.
> 
> I still think it's unnecessary, but if kbr feels it has value, I won't block
it,
> but at least the disabled_extension part needs to be addressed.

Thanks for pointing that out. It needs to be fixed in this CL. Is there anything
else you can think of that is missing?

[Ken Russell (switch to Gerrit), 2015-11-24 23:11:14]

j.isorce: zmo and I discussed this at length and the findings are summarized in
https://code.google.com/p/chromium/issues/detail?id=554909#c7 . Apologies for
sending you down an incorrect path, but we need to do some more work on the
underlying GPU info collection code before we can accept more patches. May I ask
you to work with Mo after he lands the next patch for this bug?

[Julien Isorce Samsung, 2015-11-24 23:58:01]

On 2015/11/24 23:11:14, Ken Russell wrote:
> j.isorce: zmo and I discussed this at length and the findings are summarized
in
> https://code.google.com/p/chromium/issues/detail?id=554909#c7 . Apologies for
> sending you down an incorrect path, but we need to do some more work on the
> underlying GPU info collection code before we can accept more patches. May I
ask
> you to work with Mo after he lands the next patch for this bug?

No problem, I think it is good decision. Let me know when it's ready.

[Julien Isorce Samsung, 2016-03-30 14:05:13]

Description was changed from

==========
Ensure exactitude for recomputation of browser process gpu driver bug
workarounds

This patch add new member "std::set<int> driver_bugs_workarounds"
to GPUInfo.

This allow the gpu process to send its running workarounds list
to IPC through existing GpuHostMsg_GraphicsInfoCollected message.

When receiveing this message, in GpuDataManager::UpdateGpuInfo the
browser process can parse this list and check that it matches with
its recomputed workarounds list.

This patch also add a unit test to catch any regression.
See crbug.com/553524.

XXX:
This patch could even always avoid to recompute the workarounds list
by enabling the commented part
"gpu_driver_bugs_=gpu_info_.driver_bugs_workarounds",
see comments in GpuDataManagerImplPrivate::UpdateGpuInfoHelper.

BUG=554909

R=dcheng@chromium.org, hendrikw@chromium.org, kbr@chromium.org, zmo@chromium.org

TEST=CHROME_DEVEL_SANDBOX=out/Release/chrome_sandbox
./content/test/gpu/run_gpu_test.py driver_bug_workarounds

Review URL:
==========

to

==========
Ensure exactitude for recomputation of browser process gpu driver bug
workarounds

This patch add new member "std::set<int> driver_bugs_workarounds"
to GPUInfo.

This allow the gpu process to send its running workarounds list
to IPC through existing GpuHostMsg_GraphicsInfoCollected message.

When receiveing this message, in GpuDataManager::UpdateGpuInfo the
browser process can parse this list and check that it matches with
its recomputed workarounds list.

This patch also add a unit test to catch any regression.
See crbug.com/553524.

XXX:
This patch could even always avoid to recompute the workarounds list
by enabling the commented part
"gpu_driver_bugs_=gpu_info_.driver_bugs_workarounds",
see comments in GpuDataManagerImplPrivate::UpdateGpuInfoHelper.

BUG=554909

R=dcheng@chromium.org, hendrikw@chromium.org, kbr@chromium.org, zmo@chromium.org

TEST=CHROME_DEVEL_SANDBOX=out/Release/chrome_sandbox
./content/test/gpu/run_gpu_test.py driver_bug_workarounds

Review URL:
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Julien Isorce Samsung, 2016-03-30 14:06:43]

Description was changed from

==========
Ensure exactitude for recomputation of browser process gpu driver bug
workarounds

This patch add new member "std::set<int> driver_bugs_workarounds"
to GPUInfo.

This allow the gpu process to send its running workarounds list
to IPC through existing GpuHostMsg_GraphicsInfoCollected message.

When receiveing this message, in GpuDataManager::UpdateGpuInfo the
browser process can parse this list and check that it matches with
its recomputed workarounds list.

This patch also add a unit test to catch any regression.
See crbug.com/553524.

XXX:
This patch could even always avoid to recompute the workarounds list
by enabling the commented part
"gpu_driver_bugs_=gpu_info_.driver_bugs_workarounds",
see comments in GpuDataManagerImplPrivate::UpdateGpuInfoHelper.

BUG=554909

R=dcheng@chromium.org, hendrikw@chromium.org, kbr@chromium.org, zmo@chromium.org

TEST=CHROME_DEVEL_SANDBOX=out/Release/chrome_sandbox
./content/test/gpu/run_gpu_test.py driver_bug_workarounds

Review URL:
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=dcheng@chromium.org, hendrikw@chromium.org, kbr@chromium.org, zmo@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

[Julien Isorce Samsung, 2016-03-30 14:09:37]

On 2015/11/24 23:58:01, j.isorce wrote:
> On 2015/11/24 23:11:14, Ken Russell wrote:
> > j.isorce: zmo and I discussed this at length and the findings are summarized
> in
> > https://code.google.com/p/chromium/issues/detail?id=554909#c7 . Apologies
for
> > sending you down an incorrect path, but we need to do some more work on the
> > underlying GPU info collection code before we can accept more patches. May I
> ask
> > you to work with Mo after he lands the next patch for this bug?
> 
> No problem, I think it is good decision. Let me know when it's ready.

Actually since in another CL I added a new testing IPC for
chrome.gpuBenchmarking.hasGpuProcess() I got the idea that I could also add a
new IPC to test gpu driver workarounds list.
What do you think of the "Patch Set 2" ? (I updated the CL description as well)

[Ken Russell (switch to Gerrit), 2016-03-30 22:06:42]

Description was changed from

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=dcheng@chromium.org, hendrikw@chromium.org, kbr@chromium.org, zmo@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

to

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=dcheng@chromium.org, kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

[Ken Russell (switch to Gerrit), 2016-03-30 22:07:07]

kbr@chromium.org changed reviewers:
+ ccameron@chromium.org, jbauman@chromium.org, piman@chromium.org
- hendrikw@chromium.org, zmo@google.com

[Ken Russell (switch to Gerrit), 2016-03-30 22:07:21]

Description was changed from

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=dcheng@chromium.org, kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

to

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

[Ken Russell (switch to Gerrit), 2016-03-30 22:07:21]

kbr@chromium.org changed reviewers:
- dcheng@chromium.org

[Ken Russell (switch to Gerrit), 2016-03-30 22:14:47]

Description was changed from

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

to

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

[Ken Russell (switch to Gerrit), 2016-03-30 22:14:47]

kbr@chromium.org changed reviewers:
+ dcheng@chromium.org, palmer@chromium.org

[Ken Russell (switch to Gerrit), 2016-03-30 22:17:42]

Thanks for pushing this forward. Several small naming convention comments, plus
a couple of larger questions.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_process.py (right):

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_process.py:20: function
VerifyDriverBugWorkarounds(workaround) {
nit: VerifyDriverBugWorkaroundIsPresent

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
File content/test/gpu/page_sets/gpu_process_tests.py (right):

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:178:
options.AppendExtraBrowserArgs('--use_testing_gpu_driver_workaround')
Update per naming nit below.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:181: class
DriverBugWorkaroundsGpuProcessPage(gpu_test_base.PageBase):
nit: DriverBugWorkaroundsInGpuProcessPage

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:185:
name='GpuProcess.driver_bug_workarounds_gpu_process',
nit: driver_bug_workarounds_in_gpu_process

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:192: %
'use_testing_gpu_driver_workaround'):
Update name.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:195: raise
page_test.Failure('browser and gpu process list of driver bug \
Grammar nit:
"Browser\'s and GPU process\'s lists of driver bug workarounds are not equal"

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:196: workarounds are not
equals')
Is this really what this test verifies -- that the browser process's and GPU
process's notions of the driver bug workarounds are equal? It seems to be
testing the GPU process's notions, via the new IPC, but it doesn't look to me
like it verifies that the browser's notion is the same.

https://codereview.chromium.org/1463823002/diff/20001/gpu/config/gpu_driver_b...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/1463823002/diff/20001/gpu/config/gpu_driver_b...
gpu/config/gpu_driver_bug_workaround_type.h:162:
use_testing_gpu_driver_workaround)                  \
nit: use_gpu_driver_workaround_for_testing

[dcheng, 2016-03-30 23:45:25]

https://codereview.chromium.org/1463823002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1463823002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:991: *gpu_driver_bug_workarounds =
workaround_names;
std::move() if you're feeling fancy. Alternatively, any reason to not just
clear() and push_back() directly into the output vector?

https://codereview.chromium.org/1463823002/diff/20001/content/renderer/gpu/gp...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/1463823002/diff/20001/content/renderer/gpu/gp...
content/renderer/gpu/gpu_benchmarking_extension.cc:936: v8::Isolate* isolate =
args->isolate();
Does Converter::ToV8() work?
https://code.google.com/p/chromium/codesearch#chromium/src/gin/converter_unit...

[piman, 2016-03-31 01:44:21]

https://codereview.chromium.org/1463823002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1463823002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:984: if (stub) {
This sounds potentially flaky - the IPC result depends on unrelated state (e.g.
whether or not the compositor has initialized).

If all you want is the workarounds, maybe you can refactor the code from
FeatureInfo that extracts them from the command-line, and move it to, say,
GpuChannelManager: the result is constant, and there's no point in doing it once
per context, actually.

[Julien Isorce Samsung, 2016-03-31 15:20:10]

Thx all of you for your great remarks, I'll upload a new Patch Set soon but in
the mean time you can see my replies.

https://codereview.chromium.org/1463823002/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1463823002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:984: if (stub) {
On 2016/03/31 01:44:21, piman wrote:
> This sounds potentially flaky - the IPC result depends on unrelated state
(e.g.
> whether or not the compositor has initialized).
> 
> If all you want is the workarounds, maybe you can refactor the code from
> FeatureInfo that extracts them from the command-line, and move it to, say,
> GpuChannelManager: the result is constant, and there's no point in doing it
once
> per context, actually.
I was not able to understand why it is done once per context so I just assumed
it was necessary somehow. Good idea about moving it to GpuChannelManager, I'll
try.

https://codereview.chromium.org/1463823002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:991: *gpu_driver_bug_workarounds =
workaround_names;
On 2016/03/30 23:45:25, dcheng wrote:
> std::move() if you're feeling fancy. Alternatively, any reason to not just
> clear() and push_back() directly into the output vector?
No particular reason, I thought about you second suggestion but I was not
confident enough, no particular reason :) So I'll do what you suggested.

https://codereview.chromium.org/1463823002/diff/20001/content/renderer/gpu/gp...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/1463823002/diff/20001/content/renderer/gpu/gp...
content/renderer/gpu/gpu_benchmarking_extension.cc:936: v8::Isolate* isolate =
args->isolate();
On 2016/03/30 23:45:25, dcheng wrote:
> Does Converter::ToV8() work?
>
https://code.google.com/p/chromium/codesearch#chromium/src/gin/converter_unit...
gin::TryConvertToV8 works so I'll use it, thx.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/gpu_process.py (right):

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/gpu_process.py:20: function
VerifyDriverBugWorkarounds(workaround) {
On 2016/03/30 22:17:41, Ken Russell wrote:
> nit: VerifyDriverBugWorkaroundIsPresent

Done.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
File content/test/gpu/page_sets/gpu_process_tests.py (right):

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:178:
options.AppendExtraBrowserArgs('--use_testing_gpu_driver_workaround')
On 2016/03/30 22:17:42, Ken Russell wrote:
> Update per naming nit below.

Done.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:181: class
DriverBugWorkaroundsGpuProcessPage(gpu_test_base.PageBase):
On 2016/03/30 22:17:41, Ken Russell wrote:
> nit: DriverBugWorkaroundsInGpuProcessPage

Done.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:185:
name='GpuProcess.driver_bug_workarounds_gpu_process',
On 2016/03/30 22:17:41, Ken Russell wrote:
> nit: driver_bug_workarounds_in_gpu_process

Done.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:192: %
'use_testing_gpu_driver_workaround'):
On 2016/03/30 22:17:41, Ken Russell wrote:
> Update name.

Done.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:195: raise
page_test.Failure('browser and gpu process list of driver bug \
On 2016/03/30 22:17:42, Ken Russell wrote:
> Grammar nit:
> "Browser\'s and GPU process\'s lists of driver bug workarounds are not equal"

Done.

https://codereview.chromium.org/1463823002/diff/20001/content/test/gpu/page_s...
content/test/gpu/page_sets/gpu_process_tests.py:196: workarounds are not
equals')
On 2016/03/30 22:17:41, Ken Russell wrote:
> Is this really what this test verifies -- that the browser process's and GPU
> process's notions of the driver bug workarounds are equal? It seems to be
> testing the GPU process's notions, via the new IPC, but it doesn't look to me
> like it verifies that the browser's notion is the same.

Let me explain why I think it does, but maybe I am mistaken:
There is only one GpuDataManager and it leaves in the Browser Process. When
going to chrome://gpu page,
gpu_internal_ui.cc::GpuMessageHandler::OnGpuInfoUpdate() is triggered and calls
GpuDataManagerImpl::GetInstance()->GetDriverBugWorkarounds() and passes it to
web_ui()->CallJavascriptFunction(). So the list returned by above
GetDriverBugWorkarounds() is exactly what we see in the page chrome://gpu.
Then looking at GpuDataManagerImplPrivate::GetDriverBugWorkarounds() it actually
returns an equivalent to gpu_driver_bugs_ (which was created with
gpu_driver_bug_list_->MakeDecision(gpu_info_)).
So for me the comment really says what this test verifies. Ah also
VerifyDriverBugWorkaroundIsPresent does 2 things, browser/gpu list comparisons
and testing_workaround presence.

https://codereview.chromium.org/1463823002/diff/20001/gpu/config/gpu_driver_b...
File gpu/config/gpu_driver_bug_workaround_type.h (right):

https://codereview.chromium.org/1463823002/diff/20001/gpu/config/gpu_driver_b...
gpu/config/gpu_driver_bug_workaround_type.h:162:
use_testing_gpu_driver_workaround)                  \
On 2016/03/30 22:17:42, Ken Russell wrote:
> nit: use_gpu_driver_workaround_for_testing

Done.

[Julien Isorce Samsung, 2016-03-31 16:18:25]

Description was changed from

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
==========

to

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Julien Isorce Samsung, 2016-03-31 16:39:40]

I addressed all remarks in the "Patch Set 3" I just uploaded. See my replies in
previous comment.

> On 2016/03/31 01:44:21, piman wrote:
> > This sounds potentially flaky - the IPC result depends on unrelated state
> (e.g.
> > whether or not the compositor has initialized).
> > 
> > If all you want is the workarounds, maybe you can refactor the code from
> > FeatureInfo that extracts them from the command-line, and move it to, say,
> > GpuChannelManager: the result is constant, and there's no point in doing it
> once
> > per context, actually.

> I was not able to understand why it is done once per context so I just assumed
> it was necessary somehow. Good idea about moving it to GpuChannelManager, I'll
> try.
> 

So I had a look and I opted for another solution which is just to instantiate a
FeatureInfo locally (see Patch Set 3) as its constructor calls
FeatureInfo::InitializeBasicState which initializes the workarounds. I think it
is a less risky change. Maybe your solution can be done in another CL which
would consist of duplicating FeatureInfo::Workarounds to
GpuChannelManager::Workarounds, so from src/gpu to src/content, then
StringToWorkarounds helper to GpuChannelManager, and called from its
constructor. Finally GpuChannelManager will pass its workaround to FeatureInfo's
constructor to copy them. I think "Workaround" field should stay in FeatureInfo
as well since feature_info()->workarounds() is called in a lot of places for
convenience I guess and in some unit tests. Worth to mention that currently the
parsing is done only one time within a context group since a context group
shares the same instance of FeatureInfo. But a new parsing is done for each new
context that is not meant to be in a context group.

[piman, 2016-03-31 20:19:12]

On 2016/03/31 16:39:40, j.isorce wrote:
> I addressed all remarks in the "Patch Set 3" I just uploaded. See my replies
in
> previous comment.
> 
> > On 2016/03/31 01:44:21, piman wrote:
> > > This sounds potentially flaky - the IPC result depends on unrelated state
> > (e.g.
> > > whether or not the compositor has initialized).
> > > 
> > > If all you want is the workarounds, maybe you can refactor the code from
> > > FeatureInfo that extracts them from the command-line, and move it to, say,
> > > GpuChannelManager: the result is constant, and there's no point in doing
it
> > once
> > > per context, actually.
> 
> > I was not able to understand why it is done once per context so I just
assumed
> > it was necessary somehow. Good idea about moving it to GpuChannelManager,
I'll
> > try.
> > 
> 
> So I had a look and I opted for another solution which is just to instantiate
a
> FeatureInfo locally (see Patch Set 3) as its constructor calls
> FeatureInfo::InitializeBasicState which initializes the workarounds. I think
it
> is a less risky change. Maybe your solution can be done in another CL which
> would consist of duplicating FeatureInfo::Workarounds to
> GpuChannelManager::Workarounds, so from src/gpu to src/content, then
> StringToWorkarounds helper to GpuChannelManager, and called from its
> constructor. Finally GpuChannelManager will pass its workaround to
FeatureInfo's
> constructor to copy them. I think "Workaround" field should stay in
FeatureInfo
> as well since feature_info()->workarounds() is called in a lot of places for
> convenience I guess and in some unit tests. Worth to mention that currently
the
> parsing is done only one time within a context group since a context group
> shares the same instance of FeatureInfo. But a new parsing is done for each
new
> context that is not meant to be in a context group.

Can you add a TODO to follow up with that change?
FYI, https://codereview.chromium.org/1845563005/ is about to land which moves
content/common/gpu to gpu/ipc, you may want to rebase on top of that.

I guess other than that this approach is ok.

[Julien Isorce Samsung, 2016-04-01 17:13:19]

On 2016/03/31 20:19:12, piman wrote:
> Can you add a TODO to follow up with that change?
> FYI, https://codereview.chromium.org/1845563005/ is about to land which moves
> content/common/gpu to gpu/ipc, you may want to rebase on top of that.
> 
> I guess other than that this approach is ok.

Done I added the TODO in "Patch Set 4".

Could someone hit the "CQ dry run" button ? Thx

[Julien Isorce Samsung, 2016-04-01 17:21:43]

> On 2016/03/31 20:19:12, piman wrote:
> > FYI, https://codereview.chromium.org/1845563005/ is about to land which
moves
> > content/common/gpu to gpu/ipc, you may want to rebase on top of that.

Also note that locally I did: "git cl patch 1845563005" then I did cherry-pick
of my patch and git managed to do it automatically. I re-compiled and test still
passes :)

[piman, 2016-04-01 19:13:10]

lgtm

[piman, 2016-04-01 19:13:16]

The CQ bit was checked by piman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-01 19:13:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/60001

[palmer, 2016-04-01 19:19:54]

https://codereview.chromium.org/1463823002/diff/60001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1463823002/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:763:
IPC_MESSAGE_HANDLER(GpuChannelMsg_GetDriverBugWorkArounds,
Do we need this in all builds, including production? If not, maybe we can guard
it with an #ifdef or otherwise keep it out of production builds. But if you do
need it, OK. Just checking.

https://codereview.chromium.org/1463823002/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:985: // TODO(j.isorce): Do the extraction of
workarounds in GpuChannelManager's
Please attach a crbug.com link to all TODOs.

[Julien Isorce Samsung, 2016-04-01 20:20:57]

On 2016/04/01 19:19:54, palmer wrote:
>
https://codereview.chromium.org/1463823002/diff/60001/content/common/gpu/gpu_...
> content/common/gpu/gpu_channel.cc:763:
> IPC_MESSAGE_HANDLER(GpuChannelMsg_GetDriverBugWorkArounds,
> Do we need this in all builds, including production? If not, maybe we can
guard
> it with an #ifdef or otherwise keep it out of production builds. But if you do
> need it, OK. Just checking.

Good question, I'll let other to answer but worth to mention that currently it
is only needed for the new browser test that this CL adds.
Is gpu_tests/gpu_process.py run by production builds ?
In the case I have to guard it, should I add !defined(GOOGLE_CHROME_BUILD) ?
!defined(OFFICIAL_BUILD) ? Also should I just guard the handlee or should I
guard the IPC definition as well + the send ? Well almost all the c++ code
actually.

> 
>
https://codereview.chromium.org/1463823002/diff/60001/content/common/gpu/gpu_...
> content/common/gpu/gpu_channel.cc:985: // TODO(j.isorce): Do the extraction of
> workarounds in GpuChannelManager's
> Please attach a http://crbug.com link to all TODOs.

Done: I open http://crbug.com/599964, I'll add it in the next Patch Set.

[piman, 2016-04-01 20:26:23]

https://codereview.chromium.org/1463823002/diff/60001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/1463823002/diff/60001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel.cc:763:
IPC_MESSAGE_HANDLER(GpuChannelMsg_GetDriverBugWorkArounds,
On 2016/04/01 19:19:54, palmer wrote:
> Do we need this in all builds, including production? If not, maybe we can
guard
> it with an #ifdef or otherwise keep it out of production builds. But if you do
> need it, OK. Just checking.

It needs to be in the binary for any configuration we test, but it could be
behind a command-line flag. Are you specifically concerned about security, or
binary size?

[commit-bot: I haz the power, 2016-04-01 21:17:48]

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

[commit-bot: I haz the power, 2016-04-01 21:17:49]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[palmer, 2016-04-01 22:17:32]

> It needs to be in the binary for any configuration we test, but it could be
> behind a command-line flag. Are you specifically concerned about security, or
> binary size?

Makes sense.

Not security per se, just general tightness. Putting it behind a command-line
flag would be very cool, but to be clear I don't see an immediate risk, so LGTM.

[Julien Isorce Samsung, 2016-04-02 23:17:03]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-02 23:17:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/100001

[commit-bot: I haz the power, 2016-04-02 23:19:05]

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

[commit-bot: I haz the power, 2016-04-02 23:19:06]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Julien Isorce Samsung, 2016-04-02 23:22:31]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-02 23:22:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/120001

[commit-bot: I haz the power, 2016-04-03 00:25:02]

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

[commit-bot: I haz the power, 2016-04-03 00:25:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Julien Isorce Samsung, 2016-04-03 09:39:04]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-03 09:39:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/140001

[commit-bot: I haz the power, 2016-04-03 10:46:22]

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

[commit-bot: I haz the power, 2016-04-03 10:46:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Julien Isorce Samsung, 2016-04-03 13:39:25]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-03 13:39:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/160001

[commit-bot: I haz the power, 2016-04-03 14:47:21]

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

[commit-bot: I haz the power, 2016-04-03 14:47:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Julien Isorce Samsung, 2016-04-03 18:31:55]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-03 18:32:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/180001

[commit-bot: I haz the power, 2016-04-03 19:44:48]

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

[commit-bot: I haz the power, 2016-04-03 19:44:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Julien Isorce Samsung, 2016-04-04 01:22:01]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 01:22:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/200001

[commit-bot: I haz the power, 2016-04-04 04:12:57]

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

[commit-bot: I haz the power, 2016-04-04 04:12:59]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Julien Isorce Samsung, 2016-04-04 08:22:04]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 08:22:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/220001

[commit-bot: I haz the power, 2016-04-04 09:54:40]

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

[commit-bot: I haz the power, 2016-04-04 09:54:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Julien Isorce Samsung, 2016-04-04 11:21:23]

On 2016/04/04 09:54:45, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

So time to give some updates: this CL has no intention to contain any fix, just
a add a new browser test. See "Patch Set 5" for latest state. The browser test
was supposed to just catch the regression that the CL
https://codereview.chromium.org/1415603009 fixed. i.e. adding a gpu driver
workaround manually from the command line.

But this test has reveal other bugs since this test also check exactitude
between the workaround list from GpuDataManager (Browser process) and
FeatureInfo (GPU process).
The bugs are the following: in chrome://gpu it does not show *ACTIVE* after
vendor_id/device_id section. Also it does not show additional GPU, only GPU0 is
shown.
So I plan to create a new bug and a specific CL for this that will parse these
fields from chrome://gpu. But for now I am trying to fix these bugs in this CL.
The fixes for this first other bug is:
https://codereview.chromium.org/1463823002/patch/220001/230001 and
https://codereview.chromium.org/1463823002/patch/220001/230010

Second bug is on Windows: all workarounds that depends on gl_renderer won't be
applied (ex: "id": 129, os: win, "gl_renderer": "ANGLE.*", ->
force_cube_complete). The fix is the add of "|| defined(OS_WIN)" in gpu_main.cc
so that gpu::ApplyGpuDriverBugWorkarounds is called so that FeatureInfo can
parse it.
Not sure if I will include this fix in the CL for now.

Also third bug is that secondary_gpu are never passed to the gpu process and it
can be problematic if the active gpu is no the primary gpu. Indeed it will setup
workarounds using the inactive gpu information. For this one I have no fix
currently since it requires bigger change and also it does not seem there is a
bot that would match these conditions. So I'll keep this TODO for later.



Last problem is that my fix for *ACTIVE* (mentioned above in first bug) seems to
not work on the win bot:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
:

GPU device 0: VENDOR = 0x1002 (ATI), DEVICE = 0x6779
GPU device 1: VENDOR = 0x102b, DEVICE = 0x534

At least it shows 2 gpu, was not the case before. Also the fix has resolved the
test on macosx bot which has 2 gpu as well (nvidia, amd).

So I'll keep debugging within this CL for now.

[Julien Isorce Samsung, 2016-04-04 14:09:04]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 14:09:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/260001

[commit-bot: I haz the power, 2016-04-04 15:02:44]

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

[commit-bot: I haz the power, 2016-04-04 15:02:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Julien Isorce Samsung, 2016-04-04 15:29:21]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 15:29:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/280001

[commit-bot: I haz the power, 2016-04-04 17:04:20]

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

[commit-bot: I haz the power, 2016-04-04 17:04:22]

Dry run: This issue passed the CQ dry run.

[Julien Isorce Samsung, 2016-04-04 19:57:09]

On 2016/04/04 17:04:22, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

It passed :)

Initially this test was only adding a browser test. But now this CL contains 2
fixes since the test actually reveals them, one on a osx bot and the other on a
win bot. Instead of opening a new bug ticket I would like to keep them in this
CL since only this test can catch these 2 problems. If ok then I will update the
CL description.

So please review again this CL, the IPC remains the same but the test has
changed a bit. Now it does the comparison in python to help debugging. But the
actual newest things are the 2 fixes.
1) One fix is to pass driver_date through the command line, + the
gpu::IdentifyActiveGPU call when receiving GpuInfo updates from the GPU process
(so that it can uses gl_renderer). It fixes a bug revealed by the win bot where
a workaround relies on the driver date.
2) Second fix is https://codereview.chromium.org/1463823002/patch/280001/290012,
which avoid to lose secondary_gpu. Indeed when receiving updates from the gpu
process, the secondary gpus vector was always overwritten by empty vector since
the gpu process is currently never aware of any secondary gpu. It fixes a bug
revealed by a osx bot where its secondary gpu is the active gpu.

In addition I found that there is a possible improvement which is to make the
gpu process aware of the secondary gpus. All TODO I added are related to this
possible improvement. I'll make these TODO to point to the same bug id, if you
think this enhancement can happen.

So please re-review. Thx.

[Ken Russell (switch to Gerrit), 2016-04-05 03:31:24]

The most recent changes to this CL are concerning, and I don't think they're
going in the right direction.

I haven't studied the test failures in detail, but the high-level intent of this
test was to ensure that the browser process's and GPU process's notions of the
driver bug workarounds were identical, by testing for the presence of a fake
workaround. Mock data was to be passed in on the command line in order to
exercise the blacklist decision code paths, etc.

The current version of this CL seems to indicate that a full picture of the GPU
state -- i.e., presence of multiple ones, which one is active, driver date, etc.
-- needs to be sent back and forth, where it doesn't seem to be necessary now.
This seems wrong, at least for testing purposes.

If the logic that computes these workarounds is wrong, then please make that
clear, and point out how it is wrong. If the changes in this CL highlighted
below are being made only for testing purposes, that's not a good direction, and
exactly how the mocking is done needs to be rethought.

https://codereview.chromium.org/1463823002/diff/280001/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1463823002/diff/280001/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl_private.cc:649:
gpu::IdentifyActiveGPU(&gpu_info_);
This change looks wrong. It's not guaranteed at this point in the code that the
GPUInfo is complete, and IdentifyActiveGPU essentially requires it to be
complete (i.e., that it contains GL_RENDERER and GL_VENDOR strings). This is why
IdenifyActiveGPU is currently only called once the GL strings are available.

https://codereview.chromium.org/1463823002/diff/280001/gpu/config/gpu_info_co...
File gpu/config/gpu_info_collector.cc (right):

https://codereview.chromium.org/1463823002/diff/280001/gpu/config/gpu_info_co...
gpu/config/gpu_info_collector.cc:183: if (context_gpu_info.secondary_gpus.size()
> 0)
This change is concerning. If it's only needed for testing purposes then the
caller should pass down a flag indicating that, and the behavior should only
change in that situation.

[Julien Isorce Samsung, 2016-04-05 05:02:06]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-05 05:02:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/300001

[Julien Isorce Samsung, 2016-04-05 05:07:08]

Description was changed from

==========
Add DriverBugWorkaroundsGpuProcessPage to gpu_process_test.py

There is no driver_bugs_workarounds field in GpuInfo so this new test
ensure that the list of workarounds are the same between browser process
(GpuDataManager) and gpu process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add DriverBugWorkaroundsInGpuProcessPage to gpu_process_test.py

This test ensure that the fake "use_gpu_driver_workaround_for_testing" is
present in both Browser process (GpuDataManager) and GPU process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds_in_gpu_process

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Julien Isorce Samsung, 2016-04-05 05:12:57]

On 2016/04/05 03:31:24, Ken Russell wrote:
> The most recent changes to this CL are concerning, and I don't think they're
> going in the right direction.
> 
> I haven't studied the test failures in detail, but the high-level intent of
this
> test was to ensure that the browser process's and GPU process's notions of the
> driver bug workarounds were identical, by testing for the presence of a fake
> workaround. Mock data was to be passed in on the command line in order to
> exercise the blacklist decision code paths, etc.
> 
> The current version of this CL seems to indicate that a full picture of the
GPU
> state -- i.e., presence of multiple ones, which one is active, driver date,
etc.
> -- needs to be sent back and forth, where it doesn't seem to be necessary now.
> This seems wrong, at least for testing purposes.
> 
> If the logic that computes these workarounds is wrong, then please make that
> clear, and point out how it is wrong. If the changes in this CL highlighted
> below are being made only for testing purposes, that's not a good direction,
and
> exactly how the mocking is done needs to be rethought.
> 
>
https://codereview.chromium.org/1463823002/diff/280001/content/browser/gpu/gp...
> File content/browser/gpu/gpu_data_manager_impl_private.cc (right):
> 
>
https://codereview.chromium.org/1463823002/diff/280001/content/browser/gpu/gp...
> content/browser/gpu/gpu_data_manager_impl_private.cc:649:
> gpu::IdentifyActiveGPU(&gpu_info_);
> This change looks wrong. It's not guaranteed at this point in the code that
the
> GPUInfo is complete, and IdentifyActiveGPU essentially requires it to be
> complete (i.e., that it contains GL_RENDERER and GL_VENDOR strings). This is
why
> IdenifyActiveGPU is currently only called once the GL strings are available.
> 
>
https://codereview.chromium.org/1463823002/diff/280001/gpu/config/gpu_info_co...
> File gpu/config/gpu_info_collector.cc (right):
> 
>
https://codereview.chromium.org/1463823002/diff/280001/gpu/config/gpu_info_co...
> gpu/config/gpu_info_collector.cc:183: if
(context_gpu_info.secondary_gpus.size()
> > 0)
> This change is concerning. If it's only needed for testing purposes then the
> caller should pass down a flag indicating that, and the behavior should only
> change in that situation.

No problem I understand your concerns. I just uploaded "Patch Set 16" to remove
the latest changes and to reduce the test to its initial intention: catch the
regression fixed by CL https://codereview.chromium.org/1415603009. I updated the
CL description as well since the test now only check presence of the fake
workaround. I removed the compute the symmetric difference of the the 2 lists
since it has revealed bugs. I'll open separate bugs for them.

[commit-bot: I haz the power, 2016-04-05 06:17:21]

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

[commit-bot: I haz the power, 2016-04-05 06:17:23]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-04-06 00:09:48]

Thanks, the latest patch set LGTM.

[Julien Isorce Samsung, 2016-04-06 07:37:21]

The CQ bit was checked by j.isorce@samsung.com

[Julien Isorce Samsung, 2016-04-06 07:37:22]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1463823002/#ps300001
(title: "Remove the fixes and only keep the test that check presence of the fake
workaround")

[commit-bot: I haz the power, 2016-04-06 07:37:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463823002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463823002/300001

[commit-bot: I haz the power, 2016-04-06 08:56:52]

Description was changed from

==========
Add DriverBugWorkaroundsInGpuProcessPage to gpu_process_test.py

This test ensure that the fake "use_gpu_driver_workaround_for_testing" is
present in both Browser process (GpuDataManager) and GPU process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds_in_gpu_process

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add DriverBugWorkaroundsInGpuProcessPage to gpu_process_test.py

This test ensure that the fake "use_gpu_driver_workaround_for_testing" is
present in both Browser process (GpuDataManager) and GPU process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds_in_gpu_process

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-06 08:56:54]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-04-06 08:58:51]

Description was changed from

==========
Add DriverBugWorkaroundsInGpuProcessPage to gpu_process_test.py

This test ensure that the fake "use_gpu_driver_workaround_for_testing" is
present in both Browser process (GpuDataManager) and GPU process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds_in_gpu_process

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add DriverBugWorkaroundsInGpuProcessPage to gpu_process_test.py

This test ensure that the fake "use_gpu_driver_workaround_for_testing" is
present in both Browser process (GpuDataManager) and GPU process (FeatureInfo).

This new test would have catch the regression fixed by CL
https://codereview.chromium.org/1415603009.

BUG=554909

R=kbr@chromium.org, zmo@chromium.org, jbauman@chromium.org,
ccameron@chromium.org, piman@chromium.org, dcheng@chromium.org,
palmer@chromium.org

TEST=./content/test/gpu/run_gpu_test.py driver_bug_workarounds_in_gpu_process

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/ff4a814529b414738c2f10ed0e57f021bc3d78a6
Cr-Commit-Position: refs/heads/master@{#385407}
==========

[commit-bot: I haz the power, 2016-04-06 08:58:53]

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