578843 - Fix uniform arrays

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix uniform arrays

This patch does two things. It replaces some INVALID_VALUE by INVALID_OPERATION and does as explained in this comment:


    // OpenGL ES 2.0 specifies that if foo is a uniform array, GetActiveUniform returns its name as "foo[0]".
    // See section 2.10 page 35 in the OpenGL ES 2.0.24 specification:
    //
    // > If the active uniform is an array, the uniform name returned in name will always
    // > be the name of the uniform array appended with "[0]".
    //
    // There is no such requirement in the OpenGL (non-ES) spec and indeed we have OpenGL implementations returning
    // "foo" instead of "foo[0]". So, when implementing WebGL on top of desktop OpenGL, we must check if the
    // returned name ends in [0], and if it doesn't, append that.


This allows us to get a bit farther in this test:

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/gl-uniform-arrays.html


The test still fails at line 263,

    var value = gl.getUniform(program, elemLoc);

as getUniform returns NS_ERROR_FAILURE. Need to investigate that separately.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 457435 [details] [diff] [review]
Fix uniform arrays

Won't this fail if, say, "foo[1]" is returned?  Though maybe that can't happen, I see "Only one active uniform variable will be reported for a uniform array." in the sdk man page at least.  Let's see what happens...

Also, any reason to not just unconditionally do this, instead of just on desktop GL?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

No very big reason not to do this on GLES, it's just that if the GLES implementation is correct, this is not needed. The cost of doing this is a little conditional branching, a little more executable code, 3 more bytes allocated per name. But, I can agree that's still worth it as this tricky difference between the GL and GLES spec could easily be overlooked by GLES implementers. So I'll do that.

As far as I understand, in case of an array, the only things that GL can return are foo or foo[0] so it should be enough to check for the foo case and append [0] in that case.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix uniform arrays, improved

This new patch
 - now does the [0] check unconditionally, which will help with buggy GLES implementatoins as discussed above
 - fixes the above-mentioned failure in GetUniform. It was quite tricky. Remember how we loop over active uniforms in GetUniform. The problem was that in the case of a uniform array, we needed to loop over the individual uniform components of that array.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

note: with this new patch, we now almost completely pass the above-mentioned test,

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/gl-uniform-arrays.html

The remaining failure is something I'll fix in a separate bug, basically we forget to validate the size of buffers passed to UniformXxx() functions.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/77a1e90f665d