getUniform should support SAMPLER_2D or SAMPLER_CUBE

• Component: WebGL
• Status: RESOLVED FIXED

[Ben Vanik]

The getUniform call will return 'null' and set INVALID_VALUE when any uniforms of type SAMPLER_2D or SAMPLER_CUBE are queried. Looking at the implementation in WebGLRenderContext.cpp line 2122, there is a '// FIXME: what to do about samplers?' and both type values get ignored.

SAMPLER_2D and SAMPLER_CUBE are both integer values set with glUniformi[v] and as such can be queried from GL with glGetUniformiv. Firefox currently implements this and as such in that browser getUniform works for these types.

I've tested Chromium on both OSX and Windows (with ANGLE) with the following simple change and it works in my tests:
@@ -2117,9 +2117,13 @@
                     baseType = GraphicsContext3D::FLOAT;
                     length = 16;
                     break;
+                case GraphicsContext3D::SAMPLER_2D:
+                case GraphicsContext3D::SAMPLER_CUBE:
+                    baseType = GraphicsContext3D::INT;
+                    length = 1;
+                    break;
                 default:
                     // Can't handle this type
-                    // FIXME: what to do about samplers?
                     m_context->synthesizeGLError(GraphicsContext3D::INVALID_VALUE);
                     return WebGLGetInfo();
                 }


With these changes, you should actually be able to do things like this:
var samplerLoc = gl.getUniformLocation(program, "mySampler");
gl.uniform1i(samplerLoc, 1);
var value = gl.getUniform(program, samplerLoc);
assert(value == 1);

[Ben Vanik]

Here's the current workaround I'm using until this is fixed, in case anyone else needs it - just call installUniformHack after getting the gl context.

    function installUniformHack(gl) {
        var uniqueUniformId = 0;
        var original_getUniformLocation = gl.getUniformLocation;
        gl.getUniformLocation = function (program, name) {
            var result = original_getUniformLocation.apply(this, arguments);
            if (result) {
                if (!result.hasOwnProperty("__uniqueId")) {
                    result.__uniqueId = String(uniqueUniformId++);
                }
            }
            return result;
        };
        var original_uniform1i = gl.uniform1i;
        gl.uniform1i = function (loc, value) {
            var program = this.getParameter(this.CURRENT_PROGRAM);
            var lookaside = program.__uniformLookaside;
            if (!lookaside) {
                lookaside = program.__uniformLookaside = {};
            }
            lookaside[loc.__uniqueId] = value;
            original_uniform1i.apply(this, arguments);
        };
        var original_uniform1iv = gl.uniform1iv;
        gl.uniform1iv = function (loc, value) {
            var program = this.getParameter(this.CURRENT_PROGRAM);
            var lookaside = program.__uniformLookaside;
            if (!lookaside) {
                lookaside = program.__uniformLookaside = {};
            }
            lookaside[loc.__uniqueId] = value[0];
            original_uniform1iv.apply(this, arguments);
        };
        var original_getUniform = gl.getUniform;
        gl.getUniform = function (program, loc) {
            var lookaside = program.__uniformLookaside;
            if (lookaside) {
                if (lookaside.hasOwnProperty(loc.__uniqueId)) {
                    return lookaside[loc.__uniqueId];
                }
            }
            return original_getUniform.apply(this, arguments);
        };
    };

[Zhenyao Mo]

I'll incorporate Ben's patch and add some test cases.

[Zhenyao Mo]

Created attachment 80779 [details]
Patch

[Zhenyao Mo]

Will update the khronos side test once this is reviewed.

[Kenneth Russell]

Comment on attachment 80779 [details]
Patch

Looks good. Thanks for cleaning this up.

[Zhenyao Mo]

Committed r77311: <http://trac.webkit.org/changeset/77311>