Bug 87718 – Shader compiler unprepared to make ESSL output when GLES is used

• Component: WebGL
• Status: RESOLVED FIXED

[Roland Takacs]

Shader compiler generate only GLSL output.

[Roland Takacs]

Created attachment 144534 [details]
patch

[Noam Rosenthal]

Comment on attachment 144534 [details]
patch

I'm ok with this, but I'd rather let mrobinson/kbr take a look since I don't usually review common ANGLE code.

[Martin Robinson]

Comment on attachment 144534 [details]
patch

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

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:78
> +#if USE(OPENGL_ES_2)
> +        m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources);
> +        m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources);
> +#else
>          m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources);
>          m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources);
> +#endif

Perhaps this should be a runtime decision based on the result of isGLES2Compliant. Theoretically a compilation could support both OpenGL and OpenGL ES and I think this is the direction GTK+ is moving.

[Roland Takacs]

(In reply to comment #3)
> (From update of attachment 144534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144534&action=review
> 
> > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:78
> > +#if USE(OPENGL_ES_2)
> > +        m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources);
> > +        m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_ESSL_OUTPUT, &m_resources);
> > +#else
> >          m_fragmentCompiler = ShConstructCompiler(SH_FRAGMENT_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources);
> >          m_vertexCompiler = ShConstructCompiler(SH_VERTEX_SHADER, SH_WEBGL_SPEC, SH_GLSL_OUTPUT, &m_resources);
> > +#endif
> 
> Perhaps this should be a runtime decision based on the result of isGLES2Compliant. Theoretically a compilation could support both OpenGL and OpenGL ES and I think this is the direction GTK+ is moving.

You are right, the solution is better (and nicer) with isGLES2Compliant. I will do it!

[Kenneth Russell]

Agreed, isGLES2Compliant is the better way to make this decision.

To see how Chromium handles this, see GLES2DecoderImpl::InitializeShaderTranslator in src/gpu/command_buffer/service/gles2_cmd_decoder.cc and ShaderTranslator::Init and src/gpu/command_buffer/service/shader_translator.cc under http://src.chromium.org/viewvc/chrome/trunk/ .

[Roland Takacs]

Created attachment 144790 [details]
work in progress patch

Is it appropriate?

If yes I will add the initializeShaderCompilers method to the build files.
Otherwise please let me know how you imagine it.

[Noam Rosenthal]

Comment on attachment 144790 [details]
work in progress patch

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

I think this is the right direction, though has some style issues, as appropriate to a WIP patch :)

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:71
> +    ShShaderOutput shader_output = (isGLES2 ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT);

style.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:56
> +    bool initializeShaderCompilers(bool isGLES2);

This is kind of a boolean trap. I'd rather have an enum.

[Roland Takacs]

Created attachment 145581 [details]
WIP

should I to add my new function into some build file?

[Noam Rosenthal]

Comment on attachment 145581 [details]
WIP

Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp?

[Martin Robinson]

Comment on attachment 145581 [details]
WIP

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

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:470
> +    if (!(isGLES2Compliant() ? m_compiler.initializeShaderCompilers(SH_ESSL_OUTPUT) : m_compiler.initializeShaderCompilers(SH_GLSL_OUTPUT)))

This is a slightly odd choice... I probably would have written:

if (!m_compiler.initializeShaderCompilers(isGLES2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT))
    return;

[Kenneth Russell]

(In reply to comment #9)
> (From update of attachment 145581 [details])
> Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp?

It might break ports that support WebGL, use ANGLEWebKitBridge, but don't use GraphicsContext3DOpenGLCommon. I don't know which ports those might be (Qt?). Chromium doesn't use ANGLEWebKitBridge.

[Noam Rosenthal]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 145581 [details] [details])
> > Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp?
> 
I don't really know. Does Mac use any of this stuff?
Qt uses GC3DOpenGLCommon.

[Kenneth Russell]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (From update of attachment 145581 [details] [details] [details])
> > > Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp?
> > 
> I don't really know. Does Mac use any of this stuff?
> Qt uses GC3DOpenGLCommon.

The Mac port uses GC3DOpenGLCommon.

[Noam Rosenthal]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #9)
> > > > (From update of attachment 145581 [details] [details] [details] [details])
> > > > Wouldn't this break ports that don't use GraphicsContext3DOpenGLCommon.cpp?
> > > 
> > I don't really know. Does Mac use any of this stuff?
> > Qt uses GC3DOpenGLCommon.
> 
> The Mac port uses GC3DOpenGLCommon.

OK then. This looks an ok direction to me, though Martin's comments are valid.

[Roland Takacs]

Created attachment 145764 [details]
patch

[Roland Takacs]

Created attachment 145957 [details]
patch

updated the changelog because since yesterday the changelog is changed.

[WebKit Review Bot]

Comment on attachment 145957 [details]
patch

Attachment 145957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12908084

New failing tests:
css1/font_properties/font.html

[WebKit Review Bot]

Created attachment 146128 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Noam Rosenthal]

(In reply to comment #18)
> Created an attachment (id=146128) [details]
> Archive of layout-test-results from ec2-cr-linux-03
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

This looks flaky.

[Noam Rosenthal]

*** Bug 60617 has been marked as a duplicate of this bug. ***

[Martin Robinson]

Comment on attachment 145957 [details]
patch

This patch introduces a way to misuse the ANGLEWebKitBridge, by running validateShaderSource without calling initializeShaderCompilers first. I think it would be better for ANGLEWebKitBridge to take a ShShaderOutput argument in the constructor and then to call initializeShaderCompilers during the first call to validateShaderSource.

At the very least validateShaderSource should ASSERT builtCompilers.

[Martin Robinson]

(In reply to comment #21)

> At the very least validateShaderSource should ASSERT builtCompilers.

By the way, apologies for not noticing this in an earlier review!

[Roland Takacs]

Created attachment 149280 [details]
patch

If I add a new argument to the ANGLEWebKitBridge constructor, the m_compiler (member of GraphicsContext3D class) must be initialized with the parameter at the GraphicsContext3D constructor ( on qt, this located at the GraphicsContext3DQt.cpp ).

If I would like to initialize the m_compiler (ANGLEWebKitBridge) at the GC3D constructor, it looks like:

GraphicsContext3D::GraphicsContext3D(....)
   : .....
   , .....
#if (opengles)
   , m_compiler(SH_ESSL_OUTPUT)
#else
   , m_compiler(SH_GLSL_OUTPUT)
#endif
   , ...
   , ...

So I could not initialize the m_compiler runtime (with isGles2Compliant()) only compiling time. [AFAIK]

Because of this, I would rather add a new member ( m_shaderOutput) to ANGLEWebKitBridge, and it is initialized at its constructor by macro checking.

Let me know how do you think if it not the best solution. :)

[Martin Robinson]

(In reply to comment #23)
> Created an attachment (id=149280) [details]
> patch
> 
> If I add a new argument to the ANGLEWebKitBridge constructor, the m_compiler (member of GraphicsContext3D class) must be initialized with the parameter at the GraphicsContext3D constructor ( on qt, this located at the GraphicsContext3DQt.cpp ).
> 
> If I would like to initialize the m_compiler (ANGLEWebKitBridge) at the GC3D constructor, it looks like:
> 
> GraphicsContext3D::GraphicsContext3D(....)
>    : .....
>    , .....
> #if (opengles)
>    , m_compiler(SH_ESSL_OUTPUT)
> #else
>    , m_compiler(SH_GLSL_OUTPUT)
> #endif
>    , ...
>    , ...
> 

isGles2Compliant is not virtual, so it should be safe to call it during construction. Does:

m_compile(isGles2Compliant() ? SH_ESSL_OUTPUT : SH_GLSL_OUTPUT)

work?

[Roland Takacs]

Created attachment 149322 [details]
patch

[Noam Rosenthal]

Comment on attachment 149322 [details]
patch

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

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:35
> +ANGLEWebKitBridge::ANGLEWebKitBridge(ShShaderOutput shaderOutput = SH_GLSL_OUTPUT) :

Shouldn't the default argument be in the header?

[Build Bot]

Comment on attachment 149322 [details]
patch

Attachment 149322 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13090218

[Noam Rosenthal]

Comment on attachment 149322 [details]
patch

Patch doesn't compile, default argument is at the wrong place.

[Roland Takacs]

(In reply to comment #26)
> (From update of attachment 149322 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149322&action=review
> 
> > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:35
> > +ANGLEWebKitBridge::ANGLEWebKitBridge(ShShaderOutput shaderOutput = SH_GLSL_OUTPUT) :
> 
> Shouldn't the default argument be in the header?

Yes, you are right. Fixed.

[Roland Takacs]

Created attachment 149474 [details]
patch

[WebKit Review Bot]

Comment on attachment 149474 [details]
patch

Clearing flags on attachment: 149474

Committed r121259: <http://trac.webkit.org/changeset/121259>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.