Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1495113004:
(Reland)WebGL 2: validate uniform block index (Closed)
Descriptionvalidate uniform block index.
should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK.
BUG=565347
TEST=deqp/functional/gles3/negativestateapi.html
Committed: https://crrev.com/68aa7b244494083167fbd3b62ab7315f1c1c242d
Cr-Commit-Position: refs/heads/master@{#363471}
Patch Set 1 #
Total comments: 7
Patch Set 2 : addressed feedback from kbr@ and zmo@ #
Messages
Total messages: 42 (19 generated)
Description was changed from ========== validate uniform block index BUG=565347 ========== to ========== validate uniform block index to fix negativestateapi.html crash. should generate INVALID_ENUM when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 ==========
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
Description was changed from ========== validate uniform block index to fix negativestateapi.html crash. should generate INVALID_ENUM when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 ========== to ========== validate uniform block index to fix negativestateapi.html crash. should generate INVALID_ENUM when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ==========
A small patch, PTAL. Thanks a lot!
Description was changed from ========== validate uniform block index to fix negativestateapi.html crash. should generate INVALID_ENUM when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ========== to ========== validate uniform block index. should generate INVALID_ENUM when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ==========
https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2324: synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid uniform block index"); Shouldn't this be an INVALID_VALUE instead? It's not really an ENUM type
https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2324: synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid uniform block index"); On 2015/12/04 20:17:10, Zhenyao Mo wrote: > Shouldn't this be an INVALID_VALUE instead? It's not really an ENUM type Yes, it should be INVALID_VALUE: "If uniformBlockIndex is greater then or equal to the value of ACTIVE_UNIFORM_BLOCKS, the error INVALID_VALUE is generated." ES 3.0.4, p. 60 https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2395: if (!validateUniformBlockIndex("getActiveUniformBlockParameter", program, uniformBlockIndex)) getActiveUniformBlockParameter -> uniformBlockBinding
Thanks for your review, Zhenyao and Ken. The typos are fixed, could you have a look? https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2324: synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid uniform block index"); On 2015/12/04 22:49:25, Ken Russell wrote: > On 2015/12/04 20:17:10, Zhenyao Mo wrote: > > Shouldn't this be an INVALID_VALUE instead? It's not really an ENUM type > > Yes, it should be INVALID_VALUE: > > "If uniformBlockIndex is greater then or equal to the value of > ACTIVE_UNIFORM_BLOCKS, the error INVALID_VALUE is generated." > > ES 3.0.4, p. 60 Yeah. This is a little wired. But I would like to follow the spec. https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2395: if (!validateUniformBlockIndex("getActiveUniformBlockParameter", program, uniformBlockIndex)) On 2015/12/04 22:49:25, Ken Russell wrote: > getActiveUniformBlockParameter -> uniformBlockBinding Done.
https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2324: synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid uniform block index"); On 2015/12/04 23:39:43, yunchao wrote: > On 2015/12/04 22:49:25, Ken Russell wrote: > > On 2015/12/04 20:17:10, Zhenyao Mo wrote: > > > Shouldn't this be an INVALID_VALUE instead? It's not really an ENUM type > > > > Yes, it should be INVALID_VALUE: > > > > "If uniformBlockIndex is greater then or equal to the value of > > ACTIVE_UNIFORM_BLOCKS, the error INVALID_VALUE is generated." > > > > ES 3.0.4, p. 60 > > Yeah. This is a little wired. But I would like to follow the spec. The spec says to generate an INVALID_VALUE error, not INVALID_ENUM. Please follow the spec.
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Sorry, stay to late to submit this CL yesterday. And misunderstand you this morning... :( have been updated now. https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2324: synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid uniform block index"); On 2015/12/04 23:46:03, Ken Russell wrote: > On 2015/12/04 23:39:43, yunchao wrote: > > On 2015/12/04 22:49:25, Ken Russell wrote: > > > On 2015/12/04 20:17:10, Zhenyao Mo wrote: > > > > Shouldn't this be an INVALID_VALUE instead? It's not really an ENUM type > > > > > > Yes, it should be INVALID_VALUE: > > > > > > "If uniformBlockIndex is greater then or equal to the value of > > > ACTIVE_UNIFORM_BLOCKS, the error INVALID_VALUE is generated." > > > > > > ES 3.0.4, p. 60 > > > > Yeah. This is a little wired. But I would like to follow the spec. > > The spec says to generate an INVALID_VALUE error, not INVALID_ENUM. Please > follow the spec. Done.
Description was changed from ========== validate uniform block index. should generate INVALID_ENUM when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ========== to ========== validate uniform block index. should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ==========
lgtm
On 2015/12/04 23:56:54, yunchao wrote: > Sorry, stay to late to submit this CL yesterday. And misunderstand you this > morning... :( > > have been updated now. > > https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp > (right): > > https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2324: > synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid uniform block index"); > On 2015/12/04 23:46:03, Ken Russell wrote: > > On 2015/12/04 23:39:43, yunchao wrote: > > > On 2015/12/04 22:49:25, Ken Russell wrote: > > > > On 2015/12/04 20:17:10, Zhenyao Mo wrote: > > > > > Shouldn't this be an INVALID_VALUE instead? It's not really an ENUM type > > > > > > > > Yes, it should be INVALID_VALUE: > > > > > > > > "If uniformBlockIndex is greater then or equal to the value of > > > > ACTIVE_UNIFORM_BLOCKS, the error INVALID_VALUE is generated." > > > > > > > > ES 3.0.4, p. 60 > > > > > > Yeah. This is a little wired. But I would like to follow the spec. > > > > The spec says to generate an INVALID_VALUE error, not INVALID_ENUM. Please > > follow the spec. > > Done. Thanks. I updated the CL description too. LGTM
On 2015/12/05 00:00:16, Ken Russell wrote: > On 2015/12/04 23:56:54, yunchao wrote: > > Sorry, stay to late to submit this CL yesterday. And misunderstand you this > > morning... :( > > > > have been updated now. > > > > > https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp > > (right): > > > > > https://codereview.chromium.org/1495113004/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2324: > > synthesizeGLError(GL_INVALID_ENUM, functionName, "invalid uniform block > index"); > > On 2015/12/04 23:46:03, Ken Russell wrote: > > > On 2015/12/04 23:39:43, yunchao wrote: > > > > On 2015/12/04 22:49:25, Ken Russell wrote: > > > > > On 2015/12/04 20:17:10, Zhenyao Mo wrote: > > > > > > Shouldn't this be an INVALID_VALUE instead? It's not really an ENUM > type > > > > > > > > > > Yes, it should be INVALID_VALUE: > > > > > > > > > > "If uniformBlockIndex is greater then or equal to the value of > > > > > ACTIVE_UNIFORM_BLOCKS, the error INVALID_VALUE is generated." > > > > > > > > > > ES 3.0.4, p. 60 > > > > > > > > Yeah. This is a little wired. But I would like to follow the spec. > > > > > > The spec says to generate an INVALID_VALUE error, not INVALID_ENUM. Please > > > follow the spec. > > > > Done. > > Thanks. I updated the CL description too. LGTM Thanks a lot.
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495113004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495113004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495113004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495113004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by yunchao.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495113004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495113004/40001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495113004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495113004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yunchao.he@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1495113004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1495113004/40001
Message was sent while issue was closed.
Description was changed from ========== validate uniform block index. should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ========== to ========== validate uniform block index. should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== validate uniform block index. should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html ========== to ========== validate uniform block index. should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html Committed: https://crrev.com/68aa7b244494083167fbd3b62ab7315f1c1c242d Cr-Commit-Position: refs/heads/master@{#363471} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/68aa7b244494083167fbd3b62ab7315f1c1c242d Cr-Commit-Position: refs/heads/master@{#363471}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/1502143004/ by jmadill@chromium.org. The reason for reverting is: Fails the WebGL 2 CTS: http://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.9%20Release%20... http://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Debug%20%28NVID... http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28Int... WebglConformance.conformance2_state_gl_enum_tests: Failure: getError expected: INVALID_ENUM. Was INVALID_VALUE : gl.getActiveUniformBlockParameter(program, 0, desktopGL['UNIFORM_BLOCK_NAME_LENGTH']) should return INVALID_ENUM. .
Message was sent while issue was closed.
On 2015/12/07 16:24:35, Jamie Madill wrote: > A revert of this CL (patchset #2 id:40001) has been created in > https://codereview.chromium.org/1502143004/ by mailto:jmadill@chromium.org. > > The reason for reverting is: Fails the WebGL 2 CTS: > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.9%20Release%20... > http://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Debug%20%28NVID... > http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28Int... > > WebglConformance.conformance2_state_gl_enum_tests: > Failure: getError expected: INVALID_ENUM. Was INVALID_VALUE : > gl.getActiveUniformBlockParameter(program, 0, > desktopGL['UNIFORM_BLOCK_NAME_LENGTH']) should return INVALID_ENUM. > . The failure above has been fixed by this pr: https://github.com/KhronosGroup/WebGL/pull/1358. Zhenyao and/or Brandon, could you help to roll WebGL conformance test suite? Then I can re-land this patch. Thanks a lot!
Message was sent while issue was closed.
Description was changed from ========== validate uniform block index. should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html Committed: https://crrev.com/68aa7b244494083167fbd3b62ab7315f1c1c242d Cr-Commit-Position: refs/heads/master@{#363471} ========== to ========== validate uniform block index. should generate INVALID_VALUE when index >= the value of ACTIVE_UNIFORM_BLOCK. BUG=565347 TEST=deqp/functional/gles3/negativestateapi.html Committed: https://crrev.com/68aa7b244494083167fbd3b62ab7315f1c1c242d Cr-Commit-Position: refs/heads/master@{#363471} ==========
On 2015/12/21 02:31:17, yunchao wrote: > On 2015/12/07 16:24:35, Jamie Madill wrote: > > A revert of this CL (patchset #2 id:40001) has been created in > > https://codereview.chromium.org/1502143004/ by mailto:jmadill@chromium.org. > > > > The reason for reverting is: Fails the WebGL 2 CTS: > > > > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%2010.9%20Release%20... > > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Linux%20Debug%20%28NVID... > > > http://build.chromium.org/p/chromium.gpu.fyi/builders/Win7%20Release%20%28Int... > > > > WebglConformance.conformance2_state_gl_enum_tests: > > Failure: getError expected: INVALID_ENUM. Was INVALID_VALUE : > > gl.getActiveUniformBlockParameter(program, 0, > > desktopGL['UNIFORM_BLOCK_NAME_LENGTH']) should return INVALID_ENUM. > > . > > The failure above has been fixed by this pr: > https://github.com/KhronosGroup/WebGL/pull/1358. > > Zhenyao and/or Brandon, could you help to roll WebGL conformance test suite? > Then I can re-land this patch. Thanks a lot! The roll is here: https://codereview.chromium.org/1544673002/ When it lands, I can reland this for you. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
