Support line-continuation character in WebGL 2.0 (issue 2082483002 by qiankun.miao@intel.com)

[qianku...@intel.com]

Reviewers: Ken Russell, Zhenyao Mo, bajones, yunchao
CL: https://codereview.chromium.org/2082483002/

Message:
PTAL.


https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5473
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_translator.cc?l=112).
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.

Description:
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

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+11, -0 lines):
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp


Index: third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
index 1c7cf0f6b1bd990e571bc530520c1ecd66fd4eb4..9f8d01d1b3b05d186ef42053be7af4dafa0dd8b1 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
@@ -448,6 +448,14 @@ void StripComments::process(UChar c)
break;

case InSingleLineComment:
+ // Handle line-continuation character in single line comment.
+ // Advance string to next nonspace character and eat it.
+ if (c == '\\') {
+ while (peek(temp) && WTF::isASCIISpace(temp))
+ advance();
+ advance();
+ }
+
// The newline code at the top of this function takes care
// of resetting our state when we get out of the
// single-line comment. Swallow all other characters.
@@ -5461,6 +5469,9 @@ bool WebGLRenderingContextBase::validateSize(const char* functionName, GLint x,
bool WebGLRenderingContextBase::validateString(const char* functionName, const String& string)
{
for (size_t i = 0; i < string.length(); ++i) {
+ // line-continuation character \ is supported in WebGL 2.0.
+ if (isWebGL2OrHigher() && string[i] == '\\')
+ continue;
if (!validateCharacter(string[i])) {
synthesizeGLError(GL_INVALID_VALUE, functionName, "string not ASCII");
return false;

[z...@chromium.org]

https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode452
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/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5473
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_translator.cc?l=112).
> 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/

[k...@chromium.org]

https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode452
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/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5473
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5473:
if (isWebGL2OrHigher() && string[i] == '\\')

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.

https://codereview.chromium.org/2082483002/

[qianku...@intel.com]

Thanks your comments. I updated the CL. Please help to take another look.

https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode452
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/

[z...@chromium.org]

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode455
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/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5486
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/

[z...@chromium.org]

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode455
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.

https://codereview.chromium.org/2082483002/

[k...@chromium.org]

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/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5486
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/

[qianku...@intel.com]

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5486
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

https://codereview.chromium.org/2082483002/

[z...@chromium.org]

lgtm

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2082483002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5486
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5486:
synthesizeGLError(GL_INVALID_VALUE, "shaderSource", "line-continuation
character is not immediately preceding a new-line");

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.

https://codereview.chromium.org/2082483002/

[qianku...@intel.com]

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.

https://codereview.chromium.org/2082483002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2082483002/80001


https://codereview.chromium.org/2082483002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #5 (id:80001)

https://codereview.chromium.org/2082483002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ bit was unchecked.

https://codereview.chromium.org/2082483002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 5 (id:??) landed as
https://crrev.com/725597a30a83990474f491c3f8589750e63fc9f9
Cr-Commit-Position: refs/heads/master@{#403605}

https://codereview.chromium.org/2082483002/

[z...@chromium.org]

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?

https://codereview.chromium.org/2082483002/

[qianku...@intel.com]

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.

https://codereview.chromium.org/2082483002/