[TexMap] Remove GraphicsContext3D usage from TextureMapperShaderProgram

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

[TexMap] Remove GraphicsContext3D usage from TextureMapperShaderProgram

[Zan Dobersek]

Created attachment 317803 [details]
WIP

[Miguel Gomez]

(In reply to Zan Dobersek from comment #1)
> Created attachment 317803 [details]
> WIP

Zan, we can't just replace m_context->compileShader() with glCompileShader(), cause m_context->compileShader() uses ANGLE to translate the shader source to the appropriate GLSL version before actually calling glCompileShader().
Before doing this change we need to refactor TextureMapperShaderProgram to provide the appropriate shader code for each possible gl version: GLES2, OpenGL < 3.2 and OpenGL >= 3.2.

[Zan Dobersek]

Created attachment 317804 [details]
Patch

[Build Bot]

Attachment 317804 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:161:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

(In reply to Miguel Gomez from comment #2)
> (In reply to Zan Dobersek from comment #1)
> > Created attachment 317803 [details]
> > WIP
> 
> Zan, we can't just replace m_context->compileShader() with
> glCompileShader(), cause m_context->compileShader() uses ANGLE to translate
> the shader source to the appropriate GLSL version before actually calling
> glCompileShader().
> Before doing this change we need to refactor TextureMapperShaderProgram to
> provide the appropriate shader code for each possible gl version: GLES2,
> OpenGL < 3.2 and OpenGL >= 3.2.

Why can't we target specific GLSL and GLSL ES versions?

The shaders seem to compile fine with all the options enabled for both OpenGL and OpenGL ES.

The only problem is the Rect option, which depends on the GL_ARB_texture_rectangle extension, and that doesn't compile for GLSL ES 1.00 out-of-the-box. But on the other hand, this program option never gets set upstream or downstream, so program with that option never gets compiled.

[Miguel Gomez]

(In reply to Zan Dobersek from comment #5)
> (In reply to Miguel Gomez from comment #2)
> > (In reply to Zan Dobersek from comment #1)
> > > Created attachment 317803 [details]
> > > WIP
> > 
> > Zan, we can't just replace m_context->compileShader() with
> > glCompileShader(), cause m_context->compileShader() uses ANGLE to translate
> > the shader source to the appropriate GLSL version before actually calling
> > glCompileShader().
> > Before doing this change we need to refactor TextureMapperShaderProgram to
> > provide the appropriate shader code for each possible gl version: GLES2,
> > OpenGL < 3.2 and OpenGL >= 3.2.
> 
> Why can't we target specific GLSL and GLSL ES versions?
> 
> The shaders seem to compile fine with all the options enabled for both
> OpenGL and OpenGL ES.

Yes, we can target specific versions. We need GLSL 1.50 for OpenGL >= 3.2, GLSL ES for GLES and for OpenGL < 3.2 ANGLE uses the compatibility mode, which I guess is 1.10 (not sure).

For the case of OpenGL >= 3.2, we want to use a core profile, which I think also has implications in the shader code.

> The only problem is the Rect option, which depends on the
> GL_ARB_texture_rectangle extension, and that doesn't compile for GLSL ES
> 1.00 out-of-the-box. But on the other hand, this program option never gets
> set upstream or downstream, so program with that option never gets compiled.

I think there are more problems than just that. First, the shaders need to have a line with the GLSL version they are using (which is added by ANGLE). Then, in GLSL 1.50 the variables need to have the in/out qualifiers (added by ANGLE as well) while that's not needed in older versions. And I'm sure there are more differences. Bear in mind that the current code used for the shaders is like a pseudocode that gets properly translated by ANGLE.

What I was planning to do to find the differences we need to check between versions was to check the ouput of the ANGLE translation for the 3 targeted GLSL versions with all the options enabled, and then refactor TextureMapperShaderProgram somehow to generate that code depending on the OpenGL version available.

[Zan Dobersek]

Created attachment 317917 [details]
ANGLE translation of TexMap shaders on OpenGL ES 2.0

[Zan Dobersek]

Created attachment 317918 [details]
ANGLE translation of TexMap shaders on OpenGL 3.0

[Zan Dobersek]

Created attachment 317919 [details]
ANGLE translation of TexMap shaders on OpenGL 4.3

[Zan Dobersek]

(In reply to Zan Dobersek from comment #9)
> Created attachment 317919 [details]
> ANGLE translation of TexMap shaders on OpenGL 4.3

Forcing through GLSL 1.10 shaders on a core OpenGL profile works on Mesa drivers. But the spec doesn't guarantee that to be supported.

[Zan Dobersek]

Using the glslangValidator program[1], The vertex and fragment shader sources pass the validation when enforcing the 1.50 version through the '#version' directive. Only a few warnings are thrown about deprecated stuff used in the shaders:

> shaders/gl150.vert
> WARNING: 0:40: attribute deprecated in version 130; may be removed in future release
> WARNING: 0:40: varying deprecated in version 130; may be removed in future release
> WARNING: 0:40: varying deprecated in version 130; may be removed in future release
> WARNING: 0:40: varying deprecated in version 130; may be removed in future release

> shaders/gl150.frag
> WARNING: 0:54: varying deprecated in version 130; may be removed in future release
> WARNING: 0:54: varying deprecated in version 130; may be removed in future release
> WARNING: 0:54: varying deprecated in version 130; may be removed in future release

[1] https://www.khronos.org/opengles/sdk/tools/Reference-Compiler/

[Miguel Gomez]

Created attachment 324397 [details]
Patch

[Build Bot]

Attachment 324397 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:161:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Miguel Gomez]

(In reply to Miguel Gomez from comment #12)
> Created attachment 324397 [details]
> Patch

In the end the changes needed to the shaders code were:
- add the "version 150" directive when using OpenGL >= 3.2
- use in/out for input/output parameters definition with OpenGL >= 3.2
- use the default precision just on GLES2

I've tested the generated code with these changes using OpenGL versions higher and lower than 3.2 and everything seems to work. I also tested GLES2 with the rpi2 and a broadcom box. Seems to work fine as well.
But I was only able to test OpenGL with Mesa. I hope we don't have problems with other OpenGL implementations.

Also, when removing the GraphicsContext3D from VideoTextureCopierGStreamer, I had to add a VAO, as the one that was being used was provided by the GC3D. We will need to that as well when removing the GC3D from TextureMaperGL.

[Zan Dobersek]

Comment on attachment 324397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324397&action=review

> Source/WebCore/platform/graphics/gstreamer/VideoTextureCopierGStreamer.cpp:200
> +    if (GLContext::current()->version() >= 320)

This should check for m_vao being non-zero.

[Miguel Gomez]

Created attachment 324548 [details]
Patch

[Build Bot]

Attachment 324548 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:161:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Miguel Gomez]

Don, Fuji, I'm adding you to the CC as this could potentially trigger some rendering issues. Please feel free to blame me if you find any problem that could be related to this change :)

[WebKit Commit Bot]

Comment on attachment 324548 [details]
Patch

Clearing flags on attachment: 324548

Committed r223833: <https://trac.webkit.org/changeset/223833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.