8 years ago
7 years ago


(Reporter: bjacob, Assigned: bjacob)



Firefox Tracking Flags

(Not tracked)



(2 attachments)

Created attachment 457435 [details] [diff] [review]
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:

The test still fails at line 263,

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

as getUniform returns NS_ERROR_FAILURE. Need to investigate that separately.


8 years ago
Attachment #457435 - Attachment is patch: true
Attachment #457435 - Attachment mime type: application/octet-stream → text/plain
Attachment #457435 - Flags: review?(vladimir)
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?
Attachment #457435 - Flags: review?(vladimir) → review+

Comment 2

8 years ago
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

8 years ago
Created attachment 457645 [details] [diff] [review]
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.
Attachment #457645 - Flags: review?(vladimir)

Comment 4

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

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

8 years ago
Last Resolved: 8 years ago
Resolution: --- → FIXED


7 years ago
Assignee: nobody → bjacob
You need to log in before you can comment on or make changes to this bug.