Handle failures for GPU info collection

Handle failures for GPU info collection.

1.)Populate failure strings for GPU info collection when
context_info_state records fatal failure.

2.)The vendorid and deviceid of gpu should give
data on all platforms where context fails to
run.
ex:On mac devices gpu info strings are not
collected (vendor, renderer, driver version),
so populate vendorid and deviceid while collecting
webglinfo which should always be valid.

In case vendor, renderer, driver version are not
populated they will be indicated as "Not Available"
on those platforms.

BUG=412440
Committed: https://crrev.com/83b8f7a146d559484bd667badba90170b056f175
Cr-Commit-Position: refs/heads/master@{#315223}

[sivag, 2014-12-11 15:10:56]

siva.gunturi@samsung.com changed reviewers:
+ kbr@chromium.org, piman@chromium.org

[sivag, 2014-12-11 18:56:30]

On Some gpu's like mac we fail to get the
GPU information, in these cases , the failure of reading GPU info can be
understood by reading the  CollectInfoResult basic_info_state; in GPUInfo. 
When i tested this on Linux it always returns kCollectInfoNone and crashes in
this code. This state should be either success or failure, but returns None.

I think this is a bug and needs to be fixed?
CollectInfoResult is not getting updated properly? 
This can even give us information on fatal/nonfatal failures which will be
useful to fix crbug.com/277223 as well.

[sivag, 2014-12-12 11:10:15]

siva.gunturi@samsung.com changed reviewers:
+ sievers@chromium.org

[sivag, 2014-12-12 11:15:33]

siva.gunturi@samsung.com changed reviewers:
+ zmo@google.com

[sivag, 2014-12-12 11:15:33]

+Sievers as piman is slow to review,
This patch works well on aura desktop, the 
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/config/gpu_inf...
CollectInfoResult returns kCollectInfoSuccess.

But when running layout tests for webglcontentcreationerror the info collection
returns kCollectInfoNone and this causes the test to crash.

Any inputs on this?

[no sievers, 2014-12-12 18:41:27]

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:982: if
(gpu_info.basic_info_state != gpu::kCollectInfoSuccess) {
I think you want to check |context_info_state| instead since we are interested
in the GL strings here.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:985: // fail.Need to check the
root cause. Other references:crbug.com/222934
nit: space before "Need".. also just say TODO(sikugu): ... (crbug.com/222934)

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:987:
gl_info->basicInfoCollectionFailure.assign(blink::WebString::fromUTF8(
Looks like kCollectInfoNonFatalFailure gets set when an implementation failed to
parse GL_VERSION (i.e. it wasn't in the format that the code expected, and they
all pretty much seem to have specific expectations). GL_VENDOR and GL_RENDERER
might still be valid, because that happens first in the shared code in
CollectGraphicsInfoGL() which then calls the platform-specific
CollectDriverInfoGL().

To not regress anything here you might want to still assign the fields as before
in case of a non-fatal failure.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:995: "not "
nit: weird formatting, can fit in one line

[no sievers, 2014-12-12 18:46:26]

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:999: NOTREACHED();
So we will be hitting this? Because you commented that it ends up being
kCollectInfoNone somewhere.

[Ken Russell (switch to Gerrit), 2014-12-12 22:43:05]

zmo@ may be able to offer more guidance about the GPU info collection.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:982: if
(gpu_info.basic_info_state != gpu::kCollectInfoSuccess) {
On 2014/12/12 18:41:26, sievers wrote:
> I think you want to check |context_info_state| instead since we are interested
> in the GL strings here.

Agreed; |context_info_state| is what should be checked here.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:999: NOTREACHED();
On 2014/12/12 18:46:26, sievers wrote:
> So we will be hitting this? Because you commented that it ends up being
> kCollectInfoNone somewhere.

I had the same question.

[sivag, 2014-12-15 13:36:04]

ptal..

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:982: if
(gpu_info.basic_info_state != gpu::kCollectInfoSuccess) {
On 2014/12/12 18:41:26, sievers wrote:
> I think you want to check |context_info_state| instead since we are interested
> in the GL strings here.

My intention here is to check, whether the GPUInfo is populated or not.

context_info_state can give whether a context creation is success or failure.
The context validity is already checked @1024

But even in context_info_state success, there are cases where empty strings for
GPU infor driver, renderer, vendor are returned (in case of mac), this can be
determined only by basic_info_state fatal/non-fatal reasons which i wanted to
categorize and populate in webglcontextcreationerror status message.

Please correct me if i am wrong.

Though i can validate the info strings just by checking for empty strings, but
what i wanted here is the proper reason why glstrings are not populated.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:985: // fail.Need to check the
root cause. Other references:crbug.com/222934
On 2014/12/12 18:41:26, sievers wrote:
> nit: space before "Need".. also just say TODO(sikugu): ... (crbug.com/222934)

Done.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:987:
gl_info->basicInfoCollectionFailure.assign(blink::WebString::fromUTF8(
On 2014/12/12 18:41:26, sievers wrote:
> Looks like kCollectInfoNonFatalFailure gets set when an implementation failed
to
> parse GL_VERSION (i.e. it wasn't in the format that the code expected, and
they
> all pretty much seem to have specific expectations). GL_VENDOR and GL_RENDERER
> might still be valid, because that happens first in the shared code in
> CollectGraphicsInfoGL() which then calls the platform-specific
> CollectDriverInfoGL().
> 
> To not regress anything here you might want to still assign the fields as
before
> in case of a non-fatal failure.

GL_VENDOR, GL_RENDERER, GL_VERSION all these strings are empty in case of mac
gpu, which i need to investigate.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:995: "not "
On 2014/12/12 18:41:26, sievers wrote:
> nit: weird formatting, can fit in one line

Done.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:999: NOTREACHED();
On 2014/12/12 18:46:26, sievers wrote:
> So we will be hitting this? Because you commented that it ends up being
> kCollectInfoNone somewhere.

It works fine with browser build.
kCollectInfoNone is never hit. kCollectInfoSuccess is returned.

Tools/Scripts/run-webkit-tests --debug
LayoutTests/fast/canvas/webgl/webgl-error-response.html

While running webkit layout_tests with test_runner this is getting hit.
The gpu info collection flow is different here?

[Zhenyao Mo, 2014-12-15 18:57:12]

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

[Zhenyao Mo, 2014-12-15 18:57:13]

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:987:
gl_info->basicInfoCollectionFailure.assign(blink::WebString::fromUTF8(
I agree with sievers.  You should set the three string values if the state is
not kCollectInfoFatalFailure.  NonFatalFailure can be (now or in the future) the
case partial info collection fails, and you can still benefit from the ones that
are available.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:999: NOTREACHED();
On 2014/12/15 13:36:03, Sikugu wrote:
> On 2014/12/12 18:46:26, sievers wrote:
> > So we will be hitting this? Because you commented that it ends up being
> > kCollectInfoNone somewhere.
> 
> It works fine with browser build.
> kCollectInfoNone is never hit. kCollectInfoSuccess is returned.
> 
> Tools/Scripts/run-webkit-tests --debug
> LayoutTests/fast/canvas/webgl/webgl-error-response.html
> 
> While running webkit layout_tests with test_runner this is getting hit.
> The gpu info collection flow is different here?
> 
> 
> 

layout tests run with content_shell, where this GpuDataManagerImpl::Initialize()
may not be in the start up path.  You need to verify, because that changed a few
times, so I am no longer 100% sure.

[no sievers, 2014-12-15 21:18:21]

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:982: if
(gpu_info.basic_info_state != gpu::kCollectInfoSuccess) {
On 2014/12/15 13:36:03, Sikugu wrote:
> On 2014/12/12 18:41:26, sievers wrote:
> > I think you want to check |context_info_state| instead since we are
interested
> > in the GL strings here.
> 
> My intention here is to check, whether the GPUInfo is populated or not.
> 
> context_info_state can give whether a context creation is success or failure.
> The context validity is already checked @1024
> 
> But even in context_info_state success, there are cases where empty strings
for
> GPU infor driver, renderer, vendor are returned (in case of mac), this can be
> determined only by basic_info_state fatal/non-fatal reasons which i wanted to
> categorize and populate in webglcontextcreationerror status message.
> 
> Please correct me if i am wrong.
> 
> Though i can validate the info strings just by checking for empty strings, but
> what i wanted here is the proper reason why glstrings are not populated.
> 

|basic_info_state| is for CollectBasicGraphicsInfo(), which generally does not
include GL strings, which need a context to be queried. Therefore
|context_info_state| is set in CollectGraphicsInfoGL() which is the info you are
using here.

|context_info_state| has values to tell you exactly what happened, i.e. was not
initialized (kCollectInfoNone), was initialized (kCollectInfoSucces), could not
be initialized (kCollectInfoFatalFailure), could be partially initialized
(kCollectInfoNonFatalFailure).

If the value of the status field does not match what happened in the
initialization, then that needs to be fixed somewhere in that code. But what you
are adding here in the renderer should be added under the assumption that the
status field does make sense.

[sivag, 2014-12-16 12:51:28]

K i am checking now context_info_state
and this works fine. ptal..

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:987:
gl_info->basicInfoCollectionFailure.assign(blink::WebString::fromUTF8(
On 2014/12/15 18:57:12, Zhenyao Mo wrote:
> I agree with sievers.  You should set the three string values if the state is
> not kCollectInfoFatalFailure.  NonFatalFailure can be (now or in the future)
the
> case partial info collection fails, and you can still benefit from the ones
that
> are available.

Done.

https://codereview.chromium.org/800483002/diff/1/content/renderer/renderer_bl...
content/renderer/renderer_blink_platform_impl.cc:999: NOTREACHED();
On 2014/12/15 18:57:12, Zhenyao Mo wrote:
> On 2014/12/15 13:36:03, Sikugu wrote:
> > On 2014/12/12 18:46:26, sievers wrote:
> > > So we will be hitting this? Because you commented that it ends up being
> > > kCollectInfoNone somewhere.
> > 
> > It works fine with browser build.
> > kCollectInfoNone is never hit. kCollectInfoSuccess is returned.
> > 
> > Tools/Scripts/run-webkit-tests --debug
> > LayoutTests/fast/canvas/webgl/webgl-error-response.html
> > 
> > While running webkit layout_tests with test_runner this is getting hit.
> > The gpu info collection flow is different here?
> > 
> > 
> > 
> 
> layout tests run with content_shell, where this
GpuDataManagerImpl::Initialize()
> may not be in the start up path.  You need to verify, because that changed a
few
> times, so I am no longer 100% sure.

Done.

[no sievers, 2014-12-16 19:18:48]

lgtm

[Ken Russell (switch to Gerrit), 2014-12-16 23:55:47]

LGTM too - one minor comment.

https://codereview.chromium.org/800483002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/40001/content/renderer/rendere...
content/renderer/renderer_blink_platform_impl.cc:994: "GPUInfoCollectionFailure:
GPU initialization Failed. GPU"
Needs space at the end of this string for when it's concatenated.

[sivag, 2014-12-17 10:40:44]

https://codereview.chromium.org/800483002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/40001/content/renderer/rendere...
content/renderer/renderer_blink_platform_impl.cc:994: "GPUInfoCollectionFailure:
GPU initialization Failed. GPU"
On 2014/12/16 23:55:47, Ken Russell wrote:
> Needs space at the end of this string for when it's concatenated.

Done.

[sivag, 2014-12-17 10:40:51]

The CQ bit was checked by siva.gunturi@samsung.com

[commit-bot: I haz the power, 2014-12-17 10:41:10]

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

[commit-bot: I haz the power, 2014-12-17 10:49:42]

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

[commit-bot: I haz the power, 2014-12-17 10:49:43]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sivag, 2014-12-30 14:32:58]

ptal.. added vendorid and deviceid

[Zhenyao Mo, 2014-12-30 18:39:45]

https://codereview.chromium.org/800483002/diff/140001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/140001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:991:
blink::WebString::fromUTF8(gpu_info.gl_version));
Note that gpu_info.gl_version and gpu_info.driver_version are two different
things.

It's unclear what you truly want here.

[sivag, 2014-12-31 08:06:17]

siva.gunturi@samsung.com changed reviewers:
- zmo@google.com

[sivag, 2014-12-31 10:08:44]

Hi zmo,
Here is a description of what i am trying to 
add here.

The main aim of this patch is to populate
vendor, renderer, driver version when a webgl
context creation fails. But this GPUInfo is not
collected on some gpus ex: mac platform.

To give developer some info about the gpu in these
cases i am adding the vendorid and deviceid as suggested
in the bug https://code.google.com/p/chromium/issues/detail?id=445360.

And to detect the fatal failures while collecting info
i am using context_info_state and populating the error
strings in case any failure is detected.

https://codereview.chromium.org/800483002/diff/140001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/140001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:991:
blink::WebString::fromUTF8(gpu_info.gl_version));
On 2014/12/30 18:39:45, Zhenyao Mo wrote:
> Note that gpu_info.gl_version and gpu_info.driver_version are two different
> things.
> 
> It's unclear what you truly want here.

Done.

[Zhenyao Mo, 2015-01-02 18:22:45]

https://codereview.chromium.org/800483002/diff/160001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/160001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:992: gl_info->vendorId =
gpu_info.gpu.vendor_id;
I suggest instead of having separate fields of vendorId/deviceId, you can fold
them into vendorInfo/rendererInfo, for example, vendorInfo can be NVIDIA
(0x10de), rendererInfo can be Quadro 600 (vendor_id: 0x10de, device_id: 0x0df8)

[Zhenyao Mo, 2015-01-02 18:23:52]

https://codereview.chromium.org/800483002/diff/160001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/160001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:992: gl_info->vendorId =
gpu_info.gpu.vendor_id;
On 2015/01/02 18:22:45, Zhenyao Mo wrote:
> I suggest instead of having separate fields of vendorId/deviceId, you can fold
> them into vendorInfo/rendererInfo, for example, vendorInfo can be NVIDIA
> (0x10de), rendererInfo can be Quadro 600 (vendor_id: 0x10de, device_id:
0x0df8)

Probably you only need to use the IDs if vendor string or renderer string are
empty.

[sivag, 2015-02-03 18:46:51]

@zmo, kbr ptal once.
Before landing the connecting blink patch @
https://codereview.chromium.org/826363002/ ,here we want to display all the 5
parameters added above.

[Ken Russell (switch to Gerrit), 2015-02-03 18:55:50]

This still LGTM.

https://codereview.chromium.org/800483002/diff/160001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/800483002/diff/160001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:992: gl_info->vendorId =
gpu_info.gpu.vendor_id;
On 2015/01/02 18:23:52, Zhenyao Mo wrote:
> On 2015/01/02 18:22:45, Zhenyao Mo wrote:
> > I suggest instead of having separate fields of vendorId/deviceId, you can
fold
> > them into vendorInfo/rendererInfo, for example, vendorInfo can be NVIDIA
> > (0x10de), rendererInfo can be Quadro 600 (vendor_id: 0x10de, device_id:
> 0x0df8)
> 
> Probably you only need to use the IDs if vendor string or renderer string are
> empty.

I suggest we leave all of the fields here in order to know exactly what's going
on. Android devices will never have vendor_id or device_id since they're non-PCI
devices; some platforms might not have gl_vendor or gl_renderer strings.

[sivag, 2015-02-04 05:19:35]

The CQ bit was checked by siva.gunturi@samsung.com

[commit-bot: I haz the power, 2015-02-04 05:20:19]

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

[commit-bot: I haz the power, 2015-02-04 07:07:18]

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

[commit-bot: I haz the power, 2015-02-04 07:07:19]

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

[Ken Russell (switch to Gerrit), 2015-02-04 23:28:00]

On 2015/02/04 07:07:19, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Looks like there's a bug in this patch.

[sivag, 2015-02-06 05:31:40]

ptal..

[Ken Russell (switch to Gerrit), 2015-02-06 21:46:25]

lgtm

[sivag, 2015-02-08 01:56:59]

The CQ bit was checked by siva.gunturi@samsung.com

[commit-bot: I haz the power, 2015-02-08 01:57:55]

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

[commit-bot: I haz the power, 2015-02-08 02:58:44]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-08 02:59:24]

Patchset 10 (id:??) landed as
https://crrev.com/83b8f7a146d559484bd667badba90170b056f175
Cr-Commit-Position: refs/heads/master@{#315223}