Closed Bug 892169 Opened 7 years ago Closed 7 years ago

WebGL2 Bypass ANGLE shader compilation

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: guillaume.abadie, Assigned: guillaume.abadie)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Bypass ANGLE shader compilation

See https://wiki.mozilla.org/Platform/GFX/WebGL2
Assignee: nobody → gabadie
Blocks: webgl2
Depends on: 890311
Blocks: 892546
Attached patch patch revision 1 (obsolete) — Splinter Review
Attachment #774408 - Flags: review?(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.
Attachment #774408 - Flags: review?(jgilbert) → review-
(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.
Attached patch patch revision 2 (obsolete) — Splinter Review
Attachment #774408 - Attachment is obsolete: true
Attachment #775054 - Flags: review?(jgilbert)
Attached patch patch revision 3 (obsolete) — Splinter Review
Collisions fixes
Attachment #775054 - Attachment is obsolete: true
Attachment #775054 - Flags: review?(jgilbert)
Attachment #776734 - Flags: review?(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 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.
Attachment #776734 - Flags: review?(jgilbert) → review-
(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
Attached patch patch revision 4Splinter Review
Attachment #776734 - Attachment is obsolete: true
Attachment #776820 - Flags: review?(jgilbert)
Attachment #776820 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f57adec8f404
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.