Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2082483002:
Support line-continuation character in WebGL 2.0 (Closed)
DescriptionSupport line-continuation character in WebGL 2.0
BUG=295792
TEST=deqp/data/gles3/shaders/preprocessor.html
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/725597a30a83990474f491c3f8589750e63fc9f9
Cr-Commit-Position: refs/heads/master@{#403605}
Patch Set 1 #Patch Set 2 : Support line-continuation character in WebGL 2.0 #Patch Set 3 : Support line-continuation character in WebGL 2.0 #Patch Set 4 : rebase only #
Total comments: 6
Patch Set 5 : fix line-continuation semantic #
Total comments: 6
Messages
Total messages: 20 (4 generated)
Description was changed from ========== Support line-continuation character in WebGL 2.0 BUG=295792 TEST=deqp/data/gles3/shaders/preprocessor.html ========== to ========== Support line-continuation character in WebGL 2.0 BUG=295792 TEST=deqp/data/gles3/shaders/preprocessor.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
qiankun.miao@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org
PTAL. https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5473: if (isWebGL2OrHigher() && string[i] == '\\') This isn't very accurate to say WebGL 2.0 here. WebGL 2.0 + GLSL ES 1.00 doesn't support line-continuation, while WebGL 2.0 + GLSL ES 3.00 supports line-continuation. So, it should check shader version. But no shader version detecting in Blink side. Here we assume ANGLE or driver will do the final validation. Another problem is that ANGLE may change shader version according which GL context is used, see GetShaderOutputLanguageForContext (https://cs.chromium.org/chromium/src/gpu/command_buffer/service/shader_transl...). Different visions of GLSL may have different supports on line-continuation. For example, it's supported in GLSL ES 3.00.4, but it's not supported in GLSL 3.30.6.
https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:452: // Advance string to next nonspace character and eat it. Can you also add a comment, saying that line-continuation characters are processed before comment processing? https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5473: if (isWebGL2OrHigher() && string[i] == '\\') On 2016/06/29 05:31:21, qiankun wrote: > This isn't very accurate to say WebGL 2.0 here. WebGL 2.0 + GLSL ES 1.00 doesn't > support line-continuation, while WebGL 2.0 + GLSL ES 3.00 supports > line-continuation. So, it should check shader version. But no shader version > detecting in Blink side. Here we assume ANGLE or driver will do the final > validation. This is not true. See GLSL ES Spec 3.00.4 page 6, "Support of line continuation and support of UTF-8 characters within comments is optional in GLSL ES 1.00 when used with the OpenGL ES 2.0 API. However, support is mandated for both of these when a GLSL ES 1.00 shader is used with the OpenGL ES 3.0 API." Also, can you pass a WebGL version to validateCharacter() so we can handle the behavior inside that function? > > Another problem is that ANGLE may change shader version according which GL > context is used, see GetShaderOutputLanguageForContext > (https://cs.chromium.org/chromium/src/gpu/command_buffer/service/shader_transl...). > Different visions of GLSL may have different supports on line-continuation. For > example, it's supported in GLSL ES 3.00.4, but it's not supported in GLSL > 3.30.6. I am not sure we will support WebGL 2 on top of GLSL 3.30
https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:452: // Advance string to next nonspace character and eat it. On 2016/06/29 18:16:07, Zhenyao Mo wrote: > Can you also add a comment, saying that line-continuation characters are > processed before comment processing? This change doesn't seem to implement the behavior of the line continuation character in the GLSL ES 3.00.4 spec. No special handling should be needed inside a single-line comment. Per Section 3.1 "Character Set", the line continuation character only has that effect when immediately preceding a newline. https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5473: if (isWebGL2OrHigher() && string[i] == '\\') On 2016/06/29 18:16:07, Zhenyao Mo wrote: > On 2016/06/29 05:31:21, qiankun wrote: > > This isn't very accurate to say WebGL 2.0 here. WebGL 2.0 + GLSL ES 1.00 > doesn't > > support line-continuation, while WebGL 2.0 + GLSL ES 3.00 supports > > line-continuation. So, it should check shader version. But no shader version > > detecting in Blink side. Here we assume ANGLE or driver will do the final > > validation. > > This is not true. See GLSL ES Spec 3.00.4 page 6, "Support of line continuation > and support of UTF-8 characters within comments is optional in GLSL ES 1.00 when > used with the OpenGL ES 2.0 API. However, support is mandated for both of these > when a GLSL ES 1.00 shader is used with the OpenGL ES 3.0 API." > > Also, can you pass a WebGL version to validateCharacter() so we can handle the > behavior inside that function? > > > > > Another problem is that ANGLE may change shader version according which GL > > context is used, see GetShaderOutputLanguageForContext > > > (https://cs.chromium.org/chromium/src/gpu/command_buffer/service/shader_transl...). > > Different visions of GLSL may have different supports on line-continuation. > For > > example, it's supported in GLSL ES 3.00.4, but it's not supported in GLSL > > 3.30.6. > > I am not sure we will support WebGL 2 on top of GLSL 3.30 This particular change (to validateString) seems wrong. This method takes in the names of things like attribute and uniform locations. These should never contain the line continuation character.
Thanks your comments. I updated the CL. Please help to take another look. https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:452: // Advance string to next nonspace character and eat it. On 2016/06/29 18:16:07, Zhenyao Mo wrote: > Can you also add a comment, saying that line-continuation characters are > processed before comment processing? Done.
https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:455: if (peek(temp) && isNewline(temp)) What if it's "\r\n"? Two characters but still one line https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5486: synthesizeGLError(GL_INVALID_VALUE, "shaderSource", "line-continuation character is not immediately preceding a new-line"); From reading the spec, I am not sure if it's an error when not followed by a new-line immediately, or it's just retreated as a regular character? Ken?
https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:455: if (peek(temp) && isNewline(temp)) On 2016/06/30 16:10:39, Zhenyao Mo wrote: > What if it's "\r\n"? Two characters but still one line Never mind. It doesn't matter.
Sorry to keep going around on this. I'll LGTM it to unblock you and ask you to please add some more tests after you look again at the validateShaderSource method. https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5486: synthesizeGLError(GL_INVALID_VALUE, "shaderSource", "line-continuation character is not immediately preceding a new-line"); On 2016/06/30 16:10:39, Zhenyao Mo wrote: > From reading the spec, I am not sure if it's an error when not followed by a > new-line immediately, or it's just retreated as a regular character? Ken? From my reading of the spec, the continuation character should be allowed anywhere inside comments, but only have the behavior of concatenating the current and next line if it's at the very end of the current line, followed by the newline character. So I think this check is too strict. For the rest of the shader, it seems OK to reject the continuation character if it doesn't immediately precede a newline. At least a test of a continuation character in the middle of a single-line comment is needed.
https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5486: synthesizeGLError(GL_INVALID_VALUE, "shaderSource", "line-continuation character is not immediately preceding a new-line"); On 2016/07/01 00:42:30, Ken Russell wrote: > On 2016/06/30 16:10:39, Zhenyao Mo wrote: > > From reading the spec, I am not sure if it's an error when not followed by a > > new-line immediately, or it's just retreated as a regular character? Ken? > > From my reading of the spec, the continuation character should be allowed > anywhere inside comments, but only have the behavior of concatenating the > current and next line if it's at the very end of the current line, followed by > the newline character. So I think this check is too strict. > Comments have already been removed before doing validateShaderSource(). So no problem here. > For the rest of the shader, it seems OK to reject the continuation character if > it doesn't immediately precede a newline. > > At least a test of a continuation character in the middle of a single-line > comment is needed. Added in https://github.com/KhronosGroup/WebGL/pull/1827
lgtm https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5486: synthesizeGLError(GL_INVALID_VALUE, "shaderSource", "line-continuation character is not immediately preceding a new-line"); On 2016/07/01 10:32:01, qiankun wrote: > On 2016/07/01 00:42:30, Ken Russell wrote: > > On 2016/06/30 16:10:39, Zhenyao Mo wrote: > > > From reading the spec, I am not sure if it's an error when not followed by a > > > new-line immediately, or it's just retreated as a regular character? Ken? > > > > From my reading of the spec, the continuation character should be allowed > > anywhere inside comments, but only have the behavior of concatenating the > > current and next line if it's at the very end of the current line, followed by > > the newline character. So I think this check is too strict. > > > Comments have already been removed before doing validateShaderSource(). So no > problem here. > > > For the rest of the shader, it seems OK to reject the continuation character > if > > it doesn't immediately precede a newline. > > > > At least a test of a continuation character in the middle of a single-line > > comment is needed. > Added in https://github.com/KhronosGroup/WebGL/pull/1827 > Is this behavior (generating INVALID_VALUE if '\\' is not immediately followed by a new line) tested? If not, please also add a test case for that.
On 2016/07/01 16:53:50, Zhenyao Mo wrote: > > > > > For the rest of the shader, it seems OK to reject the continuation character > > if > > > it doesn't immediately precede a newline. > > > > > > At least a test of a continuation character in the middle of a single-line > > > comment is needed. > > Added in https://github.com/KhronosGroup/WebGL/pull/1827 > > > > Is this behavior (generating INVALID_VALUE if '\\' is not immediately followed > by a new line) tested? If not, please also add a test case for that. I will add a test to verify this. Thanks you zhenyao&ken for review. I am landing this now.
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Support line-continuation character in WebGL 2.0 BUG=295792 TEST=deqp/data/gles3/shaders/preprocessor.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Support line-continuation character in WebGL 2.0 BUG=295792 TEST=deqp/data/gles3/shaders/preprocessor.html CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel Committed: https://crrev.com/725597a30a83990474f491c3f8589750e63fc9f9 Cr-Commit-Position: refs/heads/master@{#403605} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/725597a30a83990474f491c3f8589750e63fc9f9 Cr-Commit-Position: refs/heads/master@{#403605}
Message was sent while issue was closed.
Qiankun, per discussion in https://github.com/KhronosGroup/WebGL/pull/1836 Our current blink side validation is too strict. We should only process '\\' if it's at the end of a commen line in blink because we strip away all comments in blink. Other than that, we should just pass it down to ANGLE translator to handle. Can you send a CL to change to this behavior?
Message was sent while issue was closed.
On 2016/07/06 17:46:29, Zhenyao Mo wrote: > Qiankun, per discussion in https://github.com/KhronosGroup/WebGL/pull/1836 > > Our current blink side validation is too strict. > > We should only process '\\' if it's at the end of a commen line in blink because > we strip away all comments in blink. > > Other than that, we should just pass it down to ANGLE translator to handle. > > Can you send a CL to change to this behavior? Done in https://codereview.chromium.org/2129883002/. PTAL. Also test updated: https://github.com/KhronosGroup/WebGL/pull/1849. Now related tests in preprocessor.html can pass now. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
