Bug 126718 – [WebGL] Have getProgramParameter return filtered results for ACTIVE_ATTRIBUTES and ACTIVE_UNIFORMS

• Component: WebGL
• Status: RESOLVED FIXED

[Brent Fulgham]

The Khronos WebGL test suite assumes that getProgramParameter only returns non-builtin attributes and uniforms.  For example, it assumes that getProgramParameter(ACTIVE_UNIFORMS) returns a count that omits built-in uniforms, such as gl_DepthRange.near, gl_DepthRange.far, etc.

OpenGL itself does return built-in values when you query "glGetActiveUniform" (see https://www.khronos.org/opengles/sdk/docs/man/xhtml/glGetActiveUniform.xml: The spec states that "The list of active uniform variables may include both built-in uniform variables (which begin with the prefix 'gl_') as well as user-defined uniform variable names."). Likewise, glGetActiveAttrib is specified to potentially return built-ins (see http://www.opengl.org/sdk/docs/man/xhtml/glGetActiveAttrib.xml).

OpenGL specifies that when glGetUniformLocation (or glGetAttribLocation) is called for a built-in name (such as gl_DepthRange.near) it should return -1.

The Khronos WebGL test suite treats its inability to retrieve location data for built-ins as an error, resulting in a number of failures.  These failures are consistent between WebKit and Firefox.

Chrome filters these values, and therefore gets a successful test run.

To provide comparable test compliance, we should filter our results the same way, so the web developers see a consistent use case on all platforms.

[Brent Fulgham]

Created attachment 220769 [details]
Patch

[Anders Carlsson]

Comment on attachment 220769 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1063
> +    OwnPtr<ActiveShaderSymbolCounts> m_shaderSymbolCount;

This should be an std::unique_ptr.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:327
> +    const ShaderSourceEntry& vertexEntry = m_shaderSourceMap.find(vertexShader)->value;
> +    const ShaderSourceEntry& fragmentEntry = m_shaderSourceMap.find(fragmentShader)->value;

You should assert that the map contains these entries.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:334
> +    for (const auto& it : fragmentEntry.uniformMap) {

This can probably just be

for (const auto& entry : fragmentEntry.uniformMap.values()) {

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:554
> +    // Everything went fine, reset our statistics

I don't think this comment is necessary.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:555
> +    m_shaderSymbolCount.release(); // Clear counts

Can just use = nullptr; here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1314
> +    ShaderSourceMap::const_iterator result = m_shaderSourceMap.find(shader);

auto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1349
> +    ShaderSourceMap::const_iterator result = m_shaderSourceMap.find(shader);

auto.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1375
> +    ShaderSourceMap::const_iterator result = m_shaderSourceMap.find(shader);

auto.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext3D.h', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:708:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:709:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:708:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:709:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:740:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:741:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:740:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:741:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1228:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1280:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1281:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 11 in 4 files


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

[Dean Jackson]

Comment on attachment 220769 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2801
> +        m_context->getNonBuiltinActiveSymbolCount(objectOrZero(program), pname, &value);

Nit: Should it be BuiltIn (capital I)? Not a big deal to me.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:2
> + * Copyright (C) 2009, 2013 Apple Inc. All rights reserved.

2014

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1021
> +        static bool isBuiltinName(const String& name)

Same here. Again, your call.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1025
> +            if (name.startsWith("gl_") || name.startsWith("webgl_") || name.startsWith("_webgl_"))
> +                return true;
> +            return false;

Just return (name.startsWith...)

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1056
> +            // Ignore the array components of these symbols for counting purposes

Nit: period.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:2
> + * Copyright (C) 2010, 2013 Apple Inc. All rights reserved.

2014

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:327
> -    ShaderSourceEntry vertexEntry = m_shaderSourceMap.find(vertexShader)->value;
> -    ShaderSourceEntry fragmentEntry = m_shaderSourceMap.find(fragmentShader)->value;
> +    const ShaderSourceEntry& vertexEntry = m_shaderSourceMap.find(vertexShader)->value;
> +    const ShaderSourceEntry& fragmentEntry = m_shaderSourceMap.find(fragmentShader)->value;

Cool!

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:555
> +    // Everything went fine, reset our statistics
> +    m_shaderSymbolCount.release(); // Clear counts

Nit: two missing periods.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1241
> +    // Need to calculate statistics

Nit: periods. There are a few more cases in this function too.

[Kenneth Russell]

It's possible there are bugs in the WebGL conformance suite in this area. https://github.com/KhronosGroup/WebGL/issues/236 describes a workaround.

Originally I thought there was a bug in the OpenGL ES conformance suite harness -- see https://github.com/KhronosGroup/WebGL/issues/390 -- but I've been told that this is probably not the case.

Any help confirming this one way or another would be helpful. The good news is that at least the WebGL implementations will all be consistent.

[Dean Jackson]

(In reply to comment #5)

> Any help confirming this one way or another would be helpful. The good news is that at least the WebGL implementations will all be consistent.

It seems that there is confusion in the area still. Well, we're confused! If the ES test suite says their test is correct, is it because they claim the built-ins are not "active" in this instance? If so, then we are probably correct to filter them out (even though it would be a bug in our OpenGL... or maybe non-ES OpenGL doesn't have the same behaviour as ES?). If not, we probably should back out the change here.

We haven't looked to see what Chromium is doing to filter out the names. Firefox is doing the same as WebKit, and failing the WebGL test.

I think for now we'll land the fix, become consistent with Chrome and pass the test in its current form.

[Brent Fulgham]

(In reply to comment #4)
> (From update of attachment 220769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220769&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2801
> > +        m_context->getNonBuiltinActiveSymbolCount(objectOrZero(program), pname, &value);
> 
> Nit: Should it be BuiltIn (capital I)? Not a big deal to me.

It looks like we typically use BuiltIn (capital I), so I will change!

[Brent Fulgham]

Created attachment 220783 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext3D.h', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:703:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:723:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:724:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:733:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:723:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:724:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:733:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:763:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:764:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:773:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:763:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:764:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:773:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1259:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1277:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1289:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 16 in 4 files


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

[Brent Fulgham]

Committed r161605: <http://trac.webkit.org/changeset/161605>

[Brent Fulgham]

<rdar://problem/15202048>