Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
826363002:
Webgl Info should have vendorid and deviceid of gpu. (Closed)
|
Created:
5 years, 11 months ago by sivag Modified:
5 years, 9 months ago CC:
blink-reviews, aandrey+blink_chromium.org, blink-reviews-html_chromium.org, dglazkov+blink, Rik Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionWebgl Info should have vendorid and deviceid of gpu.
Append vendorid and deviceid to the weblgcontentcreation
error. We do not collect GPU info that requires a context to
be created because of the cost at GPU process startup.
Mac fails to display the vendor, driver and renderer info.
R:kbr@chromium.org
BUG=412440
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191822
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : Add period at end of string. #
Total comments: 4
Patch Set 6 : Optimize test code using split. #Patch Set 7 : Correct indentation #Patch Set 8 : Rebasing TOT! #Patch Set 9 : Rebasing TOT! #Patch Set 10 : Code rework. #
Total comments: 4
Patch Set 11 : Addressed review comments. #Patch Set 12 : Fixed unittest issue. #
Messages
Total messages: 37 (13 generated)
siva.gunturi@samsung.com changed reviewers: + dglazkov@chromium.org, kbr@chromium.org, philipj@opera.com
ptal..
Sorry this has been sitting around. Does it depend on a Chromium-side CL? https://codereview.chromium.org/826363002/diff/30003/Source/core/html/canvas/... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/826363002/diff/30003/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContext.cpp:95: statusMessage.append("Could not create a WebGL context "); Maybe a period after context like in the above string.
This patch is dependent on below two cl's. https://codereview.chromium.org/813563004/ https://codereview.chromium.org/800483002/ https://codereview.chromium.org/826363002/diff/30003/Source/core/html/canvas/... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/826363002/diff/30003/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContext.cpp:95: statusMessage.append("Could not create a WebGL context "); On 2015/01/15 09:55:00, philipj wrote: > Maybe a period after context like in the above string. Done.
It looks like kbr@ should be back soon, he's probably more a better reviewer for WebGL stuff than me.
https://codereview.chromium.org/826363002/diff/70001/LayoutTests/fast/canvas/... File LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/826363002/diff/70001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/webgl/webgl-error-response.html:30: var vendoridInfo = e.statusMessage.substring(stringIndex, e.statusMessage.search(deviceidStr) - 2); All of these string searches are quite complex and fragile. With the comment below addressed, the fields in this exception message should be comma-separated, so you could use String.prototype.split(",") to split it into the various fields and then another split(" = ") to extract the values for the fields. Could you make that change? https://codereview.chromium.org/826363002/diff/70001/Source/core/html/canvas/... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/826363002/diff/70001/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContext.cpp:99: statusMessage.append(" VendorInfo = "); Please add a leading comma here to keep the formatting consistent.
@kbr, Ptal.. https://codereview.chromium.org/826363002/diff/70001/LayoutTests/fast/canvas/... File LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/826363002/diff/70001/LayoutTests/fast/canvas/... LayoutTests/fast/canvas/webgl/webgl-error-response.html:30: var vendoridInfo = e.statusMessage.substring(stringIndex, e.statusMessage.search(deviceidStr) - 2); On 2015/01/15 19:30:39, Ken Russell wrote: > All of these string searches are quite complex and fragile. With the comment > below addressed, the fields in this exception message should be comma-separated, > so you could use String.prototype.split(",") to split it into the various fields > and then another split(" = ") to extract the values for the fields. Could you > make that change? Done. https://codereview.chromium.org/826363002/diff/70001/Source/core/html/canvas/... File Source/core/html/canvas/WebGLRenderingContext.cpp (right): https://codereview.chromium.org/826363002/diff/70001/Source/core/html/canvas/... Source/core/html/canvas/WebGLRenderingContext.cpp:99: statusMessage.append(" VendorInfo = "); On 2015/01/15 19:30:39, Ken Russell wrote: > Please add a leading comma here to keep the formatting consistent. Done.
LGTM
The CQ bit was checked by siva.gunturi@samsung.com
The CQ bit was unchecked by siva.gunturi@samsung.com
On 2015/01/30 20:17:13, Ken Russell wrote: > LGTM @kbr, As per zmo's last comment https://codereview.chromium.org/800483002/#msg26 Do we want to display (vendorid, device id) only when vendor string or renderer string is not avaiable or we want to append the info always? Please suggest.
On 2015/02/02 13:49:59, sikugu wrote: > On 2015/01/30 20:17:13, Ken Russell wrote: > > LGTM > > @kbr, As per zmo's last comment > https://codereview.chromium.org/800483002/#msg26 > Do we want to display (vendorid, device id) only when vendor string or renderer > string > is not avaiable or we want to append the info always? > > Please suggest. Let's display both all the time. That way there's less confusion if something goes wrong.
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/826363002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/826363002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/42303)
siva.gunturi@samsung.com changed reviewers: - dglazkov@chromium.org
siva.gunturi@samsung.com changed reviewers: + dglazkov@chromium.org
There is a update in the code and this patch needs to be updated accordingly.
@kbr, ptal..
LGTM with nits https://codereview.chromium.org/826363002/diff/170001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/826363002/diff/170001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/webgl/webgl-error-response.html:29: if (gpuInfoStr[stringLocator].length > 0) Looking at this again I'm not sure it does what you want. It'll set "status" to true or false depending on whether the last "key = value" pair has a non-empty value. If you're looking for any non-empty value, then just do: if (gpuInfoStr[stringLocator].length == 0) status = false; https://codereview.chromium.org/826363002/diff/170001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/826363002/diff/170001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:608: statusMessage.append(", DriverId = " + String::number(glInfo.deviceId)); DeviceId, not DriverId
New patchsets have been uploaded after l-g-t-m from kbr@chromium.org
https://codereview.chromium.org/826363002/diff/170001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/webgl/webgl-error-response.html (right): https://codereview.chromium.org/826363002/diff/170001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/webgl/webgl-error-response.html:29: if (gpuInfoStr[stringLocator].length > 0) On 2015/02/13 17:32:58, Ken Russell wrote: > Looking at this again I'm not sure it does what you want. It'll set "status" to > true or false depending on whether the last "key = value" pair has a non-empty > value. If you're looking for any non-empty value, then just do: > > if (gpuInfoStr[stringLocator].length == 0) > status = false; Done. https://codereview.chromium.org/826363002/diff/170001/Source/core/html/canvas... File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/826363002/diff/170001/Source/core/html/canvas... Source/core/html/canvas/WebGLRenderingContextBase.cpp:608: statusMessage.append(", DriverId = " + String::number(glInfo.deviceId)); On 2015/02/13 17:32:58, Ken Russell wrote: > DeviceId, not DriverId Done.
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/826363002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)
On 2015/02/16 16:41:20, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...) This failure's legitimate -- it looks like when the test is run on a real GPU something is going wrong. My best guess is that there's a comma in the vendor or renderer string. You may need to use a different separator like "|". For better debuggability you should probably change the test so it does something like: var gpuInfoStr = splitStatus[i].split(" = "); if (gpuInfoStr.length < stringLocator || gpuInfoStr[stringLocator].length == 0) { testFailed("failed to find key/value pair for string \"" + gpuInfoStr + "\""); status = false; }
On 2015/02/16 17:13:49, Ken Russell wrote: > On 2015/02/16 16:41:20, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_blink_rel on tryserver.blink (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...) > > This failure's legitimate -- it looks like when the test is run on a real GPU > something is going wrong. My best guess is that there's a comma in the vendor or > renderer string. You may need to use a different separator like "|". For better > debuggability you should probably change the test so it does something like: > > var gpuInfoStr = splitStatus[i].split(" = "); > if (gpuInfoStr.length < stringLocator || gpuInfoStr[stringLocator].length == > 0) { > testFailed("failed to find key/value pair for string \"" + gpuInfoStr + > "\""); > status = false; > } @kbr, ptal..i fixed the unit test failure.
LGTM again.
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/826363002/210001
Message was sent while issue was closed.
Committed patchset #12 (id:210001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191822 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
