892169 - WebGL2 Bypass ANGLE shader compilation

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guillaume Abadie]

Bypass ANGLE shader compilation

See https://wiki.mozilla.org/Platform/GFX/WebGL2

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 774408 [details] [diff] [review]
patch revision 1

Review of attachment 774408 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextGL.cpp
@@ +4256,5 @@
>          }
>  
>          const char *s = sourceCString.get();
>  
> +#define WEBGL2_BYPASS_ANGLE

We can't use a define here because we need to keep WebGL1 going through ANGLE.

@@ +4274,5 @@
> +         *    - one for the driver, that enable GL_EXT_gpu_shader4
> +         *    - one for the angle compilor, to get informations about vertex attributes
> +         *      and uniforms
> +         */
> +        static const char *byPassPrefixSearch = "#version experimental-webgl2";

'bypass' is one word, so don't camelCase the middle of it.
Also, I would copy GLSL ES in this regard and use `#version 200`.

@@ +4277,5 @@
> +         */
> +        static const char *byPassPrefixSearch = "#version experimental-webgl2";
> +        static const char *byPassANGLEPrefix[] = {"precision mediump float;\n"
> +                                                  "#define gl_VertexID 0\n"
> +                                                  "#define gl_InstanceID 0\n",

Add a newline after the comma, since it's hard to see that each of these are just two part arrays. Also, just specify the size of these arrays so that we don't mess up.

@@ +4283,5 @@
> +                                                  "#extension GL_EXT_draw_buffers : enable\n"
> +                                                  "#define gl_PrimitiveID 0\n"};
> +        static const char *byPassDriverPrefix[] = {"#extension GL_EXT_gpu_shader4 : enable\n",
> +                                                   "#extension GL_EXT_gpu_shader4 : enable\n"};
> +        static const char *byPassKeywordsPrefix[] = {"#define in attribute\n"

This won't work since function params can have in/out qualifiers. We really just need to specify the version of GL(ES)SL we need.

@@ +4299,5 @@
> +            const char *originalShader = strstr(s, byPassPrefixSearch) + strlen(byPassPrefixSearch);
> +            int originalShaderSize = strlen(s) - (originalShader - s);
> +            int byPassShaderCodeSize = originalShaderSize + 4096 + 1;
> +
> +            byPassANGLEShaderCode = new char [byPassShaderCodeSize];

If we're dynamically allocing, we should be using nsAutoArrayPtr. For this sort of string manip, though, we should be using one of our nsCString(iirc) types.

@@ +4466,5 @@
>          shader->SetCompileStatus(ok);
> +
> +#ifdef WEBGL2_BYPASS_ANGLE
> +        if (byPassANGLE) {
> +            delete [] byPassANGLEShaderCode;

We should be using some form of RAII to have this cleanup done automagically.

Comment 3

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 774408 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 774408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +4256,5 @@
> >          }
> >  
> >          const char *s = sourceCString.get();
> >  
> > +#define WEBGL2_BYPASS_ANGLE
> 
> We can't use a define here because we need to keep WebGL1 going through
> ANGLE.
> 
> @@ +4274,5 @@
> > +         *    - one for the driver, that enable GL_EXT_gpu_shader4
> > +         *    - one for the angle compilor, to get informations about vertex attributes
> > +         *      and uniforms
> > +         */
> > +        static const char *byPassPrefixSearch = "#version experimental-webgl2";
> 
> 'bypass' is one word, so don't camelCase the middle of it.
> Also, I would copy GLSL ES in this regard and use `#version 200`.
> 
> @@ +4277,5 @@
> > +         */
> > +        static const char *byPassPrefixSearch = "#version experimental-webgl2";
> > +        static const char *byPassANGLEPrefix[] = {"precision mediump float;\n"
> > +                                                  "#define gl_VertexID 0\n"
> > +                                                  "#define gl_InstanceID 0\n",
> 
> Add a newline after the comma, since it's hard to see that each of these are
> just two part arrays. Also, just specify the size of these arrays so that we
> don't mess up.
> 
> @@ +4283,5 @@
> > +                                                  "#extension GL_EXT_draw_buffers : enable\n"
> > +                                                  "#define gl_PrimitiveID 0\n"};
> > +        static const char *byPassDriverPrefix[] = {"#extension GL_EXT_gpu_shader4 : enable\n",
> > +                                                   "#extension GL_EXT_gpu_shader4 : enable\n"};
> > +        static const char *byPassKeywordsPrefix[] = {"#define in attribute\n"
> 
> This won't work since function params can have in/out qualifiers. We really
> just need to specify the version of GL(ES)SL we need.
> 
> @@ +4299,5 @@
> > +            const char *originalShader = strstr(s, byPassPrefixSearch) + strlen(byPassPrefixSearch);
> > +            int originalShaderSize = strlen(s) - (originalShader - s);
> > +            int byPassShaderCodeSize = originalShaderSize + 4096 + 1;
> > +
> > +            byPassANGLEShaderCode = new char [byPassShaderCodeSize];
> 
> If we're dynamically allocing, we should be using nsAutoArrayPtr. For this
> sort of string manip, though, we should be using one of our nsCString(iirc)
> types.
> 
> @@ +4466,5 @@
> >          shader->SetCompileStatus(ok);
> > +
> > +#ifdef WEBGL2_BYPASS_ANGLE
> > +        if (byPassANGLE) {
> > +            delete [] byPassANGLEShaderCode;
> 
> We should be using some form of RAII to have this cleanup done automagically.

"We can't use a define here because we need to keep WebGL1 going through ANGLE."
Don't worry about it. I introduced this macro for one purpose : To use #ifdef to clearly show the difference after patch application. But the bypass will be enable only if it's a webgl2 context and if it has "#version experimental-webgl2" Therefore when Angle will be able to manage WebGL 2's GLSL, that gonna be very easy to remove this code thanks by #ifdefs.

"If we're dynamically allocing, we should be using nsAutoArrayPtr. For this sort of string manip, though, we should be using one of our nsCString(iirc) types."
"We should be using some form of RAII to have this cleanup done automatically."
You right, but it has been much faster for me to do this way. And this patch is a short time code, waiting for Angle can manage webgl 2's shader, and it have no risks to fail like that. My mind is we can tolerate that.

"Also, I would copy GLSL ES in this regard and use `#version 200`."
Well, the thing is this is not exactly a webgl GLSL 2 yet. That is why I did like that. There are somethings we can't do with this cooked language, like the out qualifier in functions parameters because of the #define out varying in the vertex shader. I decided to do so because we use much more out for varying qualifiers rather than function parameters. And that way, the shader looks more like a GLSL ES 3.

Comment 4

[Guillaume Abadie]

Attachment: patch revision 2

Comment 5

[Guillaume Abadie]

Attachment: patch revision 3

Collisions fixes

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #3)
> (In reply to Jeff Gilbert [:jgilbert] from comment #2)
> > Comment on attachment 774408 [details] [diff] [review]
> > patch revision 1
> > 
> > Review of attachment 774408 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/canvas/src/WebGLContextGL.cpp
> > @@ +4256,5 @@
> > >          }
> > >  
> > >          const char *s = sourceCString.get();
> > >  
> > > +#define WEBGL2_BYPASS_ANGLE
> > 
> > We can't use a define here because we need to keep WebGL1 going through
> > ANGLE.
> > 
> > @@ +4274,5 @@
> > > +         *    - one for the driver, that enable GL_EXT_gpu_shader4
> > > +         *    - one for the angle compilor, to get informations about vertex attributes
> > > +         *      and uniforms
> > > +         */
> > > +        static const char *byPassPrefixSearch = "#version experimental-webgl2";
> > 
> > 'bypass' is one word, so don't camelCase the middle of it.
> > Also, I would copy GLSL ES in this regard and use `#version 200`.
> > 
> > @@ +4277,5 @@
> > > +         */
> > > +        static const char *byPassPrefixSearch = "#version experimental-webgl2";
> > > +        static const char *byPassANGLEPrefix[] = {"precision mediump float;\n"
> > > +                                                  "#define gl_VertexID 0\n"
> > > +                                                  "#define gl_InstanceID 0\n",
> > 
> > Add a newline after the comma, since it's hard to see that each of these are
> > just two part arrays. Also, just specify the size of these arrays so that we
> > don't mess up.
> > 
> > @@ +4283,5 @@
> > > +                                                  "#extension GL_EXT_draw_buffers : enable\n"
> > > +                                                  "#define gl_PrimitiveID 0\n"};
> > > +        static const char *byPassDriverPrefix[] = {"#extension GL_EXT_gpu_shader4 : enable\n",
> > > +                                                   "#extension GL_EXT_gpu_shader4 : enable\n"};
> > > +        static const char *byPassKeywordsPrefix[] = {"#define in attribute\n"
> > 
> > This won't work since function params can have in/out qualifiers. We really
> > just need to specify the version of GL(ES)SL we need.
> > 
> > @@ +4299,5 @@
> > > +            const char *originalShader = strstr(s, byPassPrefixSearch) + strlen(byPassPrefixSearch);
> > > +            int originalShaderSize = strlen(s) - (originalShader - s);
> > > +            int byPassShaderCodeSize = originalShaderSize + 4096 + 1;
> > > +
> > > +            byPassANGLEShaderCode = new char [byPassShaderCodeSize];
> > 
> > If we're dynamically allocing, we should be using nsAutoArrayPtr. For this
> > sort of string manip, though, we should be using one of our nsCString(iirc)
> > types.
> > 
> > @@ +4466,5 @@
> > >          shader->SetCompileStatus(ok);
> > > +
> > > +#ifdef WEBGL2_BYPASS_ANGLE
> > > +        if (byPassANGLE) {
> > > +            delete [] byPassANGLEShaderCode;
> > 
> > We should be using some form of RAII to have this cleanup done automagically.
> 
> "We can't use a define here because we need to keep WebGL1 going through
> ANGLE."
> Don't worry about it. I introduced this macro for one purpose : To use
> #ifdef to clearly show the difference after patch application. But the
> bypass will be enable only if it's a webgl2 context and if it has "#version
> experimental-webgl2" Therefore when Angle will be able to manage WebGL 2's
> GLSL, that gonna be very easy to remove this code thanks by #ifdefs.

> 
> "If we're dynamically allocing, we should be using nsAutoArrayPtr. For this
> sort of string manip, though, we should be using one of our nsCString(iirc)
> types."
> "We should be using some form of RAII to have this cleanup done
> automatically."
> You right, but it has been much faster for me to do this way. And this patch
> is a short time code, waiting for Angle can manage webgl 2's shader, and it
> have no risks to fail like that. My mind is we can tolerate that.
This is going in Trunk, so no, we can't play it significantly less safe than normal. Historically, sometimes these things stick around in some form longer than we intend, and can be a source of bugs.
If this whole patch is going to be fixed by the ANGLE update I'm working on, we shouldn't bother fixing this patch up for landing.
> 
> "Also, I would copy GLSL ES in this regard and use `#version 200`."
> Well, the thing is this is not exactly a webgl GLSL 2 yet. That is why I did
> like that. There are somethings we can't do with this cooked language, like
> the out qualifier in functions parameters because of the #define out varying
> in the vertex shader. I decided to do so because we use much more out for
> varying qualifiers rather than function parameters. And that way, the shader
> looks more like a GLSL ES 3.
I suppose, but we're trying to prototype here. We already know that we want to get to something like `#version 200` at some point, so why not now? Would `proto-200` be better?

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776734 [details] [diff] [review]
patch revision 3

Review of attachment 776734 [details] [diff] [review]:
-----------------------------------------------------------------

RAII for being safe with memory is non-optional.

::: content/canvas/src/WebGLContextGL.cpp
@@ +4277,5 @@
> +         */
> +        static const char *bypassPrefixSearch = "#version experimental-webgl2";
> +        static const char *bypassANGLEPrefix[2] = {"precision mediump float;\n"
> +                                                   "#define gl_VertexID 0\n"
> +                                                   "#define gl_InstanceID 0\n",

Please use newlines to put some visual whitespace between this line and the one below it. It's hard to see the comma at a glance.

@@ +4299,5 @@
> +            const char *originalShader = strstr(s, bypassPrefixSearch) + strlen(bypassPrefixSearch);
> +            int originalShaderSize = strlen(s) - (originalShader - s);
> +            int bypassShaderCodeSize = originalShaderSize + 4096 + 1;
> +
> +            bypassANGLEShaderCode = new char [bypassShaderCodeSize];

Use RAII for these allocations.

@@ +4306,5 @@
> +            strcat(bypassANGLEShaderCode, originalShader);
> +
> +            bypassDriverShaderCode = new char [bypassShaderCodeSize];
> +            strcpy(bypassDriverShaderCode, bypassDriverPrefix[bypassStage]);
> +            strcat(bypassDriverShaderCode, bypassKeywordsPrefix[bypassStage]);

This code should not be cat'd into the version going to the driver, unless we have very good reasons to use the now-deprecated 'attribute' and 'varying' keywords.

Comment 8

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Comment on attachment 776734 [details] [diff] [review]
> patch revision 3
> 
> Review of attachment 776734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> RAII for being safe with memory is non-optional.
> 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +4277,5 @@
> > +         */
> > +        static const char *bypassPrefixSearch = "#version experimental-webgl2";
> > +        static const char *bypassANGLEPrefix[2] = {"precision mediump float;\n"
> > +                                                   "#define gl_VertexID 0\n"
> > +                                                   "#define gl_InstanceID 0\n",
> 
> Please use newlines to put some visual whitespace between this line and the
> one below it. It's hard to see the comma at a glance.
Added in patch revision 4
> 
> @@ +4299,5 @@
> > +            const char *originalShader = strstr(s, bypassPrefixSearch) + strlen(bypassPrefixSearch);
> > +            int originalShaderSize = strlen(s) - (originalShader - s);
> > +            int bypassShaderCodeSize = originalShaderSize + 4096 + 1;
> > +
> > +            bypassANGLEShaderCode = new char [bypassShaderCodeSize];
> 
> Use RAII for these allocations.
Used nsTArray<char> in patch revision 4
> 
> @@ +4306,5 @@
> > +            strcat(bypassANGLEShaderCode, originalShader);
> > +
> > +            bypassDriverShaderCode = new char [bypassShaderCodeSize];
> > +            strcpy(bypassDriverShaderCode, bypassDriverPrefix[bypassStage]);
> > +            strcat(bypassDriverShaderCode, bypassKeywordsPrefix[bypassStage]);
> 
> This code should not be cat'd into the version going to the driver, unless
> we have very good reasons to use the now-deprecated 'attribute' and
> 'varying' keywords.
Removed bypassKeywordsPrefix in patch revision 4

Comment 9

[Guillaume Abadie]

Attachment: patch revision 4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f57adec8f404

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f57adec8f404