User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11 CoolNovo/2.0.2.26 Steps to reproduce: Visit the following WebGL page in Firefox 14.0a1 (2012-04-20): http://www.oak3d.com/demo/Core_Character.html Actual results: Panda model with skeletal animation was rendered uncorrectly. See attachment, the left part of shader_bug_in_webgl.jpg We have spoted the problem in the hardware-accelerated skeletal animation shader. The shader code is as following: attribute vec3 position; //vertex position attribute vec3 normal; //vertex normal attribute vec4 bone_idx; //the bone index list of the current vertex attribute vec4 bone_weight; //the bone weight list of the current vertex uniform vec4 bone_mat_list[32 * 3]; //the bone transform list uniform mat4 matWorld, matViewProj; //world transform matrix and projection transform matrix void main(void) { mat4 anim_mat = mat4(0.0); if(bone_idx[0] > -0.5) { //check if the first bone is valid for the current vertex int cur_bone_idx = int(bone_idx[0]); //get the 4x3 matrix from the three rows vec4 row0 = bone_mat_list[cur_bone_idx * 3]; vec4 row1 = bone_mat_list[cur_bone_idx * 3 + 1]; vec4 row2 = bone_mat_list[cur_bone_idx * 3 + 2]; //regenerate 4x4 matrix mat4 bone_mat = mat4(row0.x, row1.x, row2.x, 0.0, row0.y, row1.y, row2.y, 0.0, row0.z, row1.z, row2.z, 0.0, row0.w, row1.w, row2.w, 1.0); //blend the current bone transform into the final transform anim_mat += bone_weight[0] * bone_mat; } if(bone_idx[1] > -0.5) { //check if the second bone is valid for the current vertex int cur_bone_idx = int(bone_idx[1]); //get the 4x3 matrix from the three rows vec4 row0 = bone_mat_list[cur_bone_idx * 3]; vec4 row1 = bone_mat_list[cur_bone_idx * 3 + 1]; vec4 row2 = bone_mat_list[cur_bone_idx * 3 + 2]; //regenerate 4x4 matrix mat4 bone_mat = mat4(row0.x, row1.x, row2.x, 0.0, row0.y, row1.y, row2.y, 0.0, row0.z, row1.z, row2.z, 0.0, row0.w, row1.w, row2.w, 1.0); //blend the current bone transform into the final transform anim_mat += bone_weight[1] * bone_mat; } if(bone_idx[2] > -0.5) { //check if the third bone is valid for the current vertex int cur_bone_idx = int(bone_idx[2]); //get the 4x3 matrix from the three rows vec4 row0 = bone_mat_list[cur_bone_idx * 3]; vec4 row1 = bone_mat_list[cur_bone_idx * 3 + 1]; vec4 row2 = bone_mat_list[cur_bone_idx * 3 + 2]; //regenerate 4x4 matrix mat4 bone_mat = mat4(row0.x, row1.x, row2.x, 0.0, row0.y, row1.y, row2.y, 0.0, row0.z, row1.z, row2.z, 0.0, row0.w, row1.w, row2.w, 1.0); //blend the current bone transform into the final transform anim_mat += bone_weight[2] * bone_mat; } if(bone_idx[3] > -0.5) { //check if the fourth bone is valid for the current vertex int cur_bone_idx = int(bone_idx[3]); //get the 4x3 matrix from the three rows vec4 row0 = bone_mat_list[cur_bone_idx * 3]; vec4 row1 = bone_mat_list[cur_bone_idx * 3 + 1]; vec4 row2 = bone_mat_list[cur_bone_idx * 3 + 2]; //regenerate 4x4 matrix mat4 bone_mat = mat4(row0.x, row1.x, row2.x, 0.0, row0.y, row1.y, row2.y, 0.0, row0.z, row1.z, row2.z, 0.0, row0.w, row1.w, row2.w, 1.0); //blend the current bone transform into the final transform anim_mat += bone_weight[3] * bone_mat; } //transform the vertex position to the world space vec4 pos_world = matWorld * anim_mat * vec4(position, 1.0); //transform the vertex position to the post-projection space gl_Position = matViewProj * pos_world; } Expected results: Display a panda model, and use WASD to make it walk. See attachment, the right part of shader_bug_in_webgl.jpg

Regression window Good: http://hg.mozilla.org/mozilla-central/rev/686e5bcf747b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120416 Firefox/14.0a1 ID:20120416122413 Bad: http://hg.mozilla.org/mozilla-central/rev/e74b044c18b8 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120416 Firefox/14.0a1 ID:20120416125713 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=686e5bcf747b&tochange=e74b044c18b8 Suspected: b56db6eab47c Benoit Jacob — Bug 732233 - Explicitly enforce spec in uniform setters - r=jgilbert Graphics Adapter Description: ATI Radeon HD 4300/4500 Series Vendor ID: 0x1002 Device ID: 0x954f Adapter RAM: 512 Adapter Drivers: aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64 Driver Version: 8.951.0.0 Driver Date: 3-8-2012 Direct2D Enabled: true DirectWrite Enabled: true (6.1.7601.17776) ClearType Parameters: Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 200 WebGL Renderer: Google Inc. -- ANGLE (ATI Radeon HD 4300/4500 Series) -- OpenGL ES 2.0 (ANGLE 1.0.0.963) GPU Accelerated Windows: 1/1 Direct3D 10 AzureBackend: direct2d

Please follow these steps: 1. go to about:config, set webgl.verbose=true 2. open the Web Console, make sure that JS warnings are enabled 3. reload this WebGL page do you see any WebGL warning?

In fact, following these steps myself, I got a large number of such WebGL errors: [19:11:44.167] Error: WebGL: Uniform4fv: expected an array of length a multiple of 4 and at least 384, got an array of length 300 @ http://www.oak3d.com/demo/Oak3D.js:1 A priori, this is a bug in this Web page, and exactly the kind of error that bug 732233 intends to catch. This Web page is uploading too much data to too small GPU-side arrays, which could give quite bad results. So I'm going to close this bug as invalid. Please reopen if you think that this page is not making this error and that Firefox is wrongly thinking that it is.

We declared a vec4 array with length 32 x 3 and the uploading data will never meet this limit, but usually less than this length. Do you mean this behavior become invalid in the latest firefox?

Will look at it tomorrow, sorry. I may have misinterpreted the spec.

oh ok, so indeed you're passing a smaller array, not a larger one. i could well have gotten that case wrong.

We have modified and updated the demo to make it run normally on Firefox Nightly, now the online test case is not available.

I have identified the error, indeed partial uniform array updates are allowed and we were wrongly disallowing them. Patch coming shortly.

Indeed we were being too string with uniform array setters. This passes conformance tests and allows this demo to run. That demo also runs in Chrome. All it does is partial uniform array updates and that should be allowed. Try: https://tbpl.mozilla.org/?tree=Try&rev=024a7df5acb7

Comment on attachment 617319 [details] [diff] [review] fix uniform setter validation: allow partial uniform array updates Review of attachment 617319 [details] [diff] [review]: ----------------------------------------------------------------- Can we switch out 'cnt' and 'dim*dim' for 'elemSize'? ::: content/canvas/src/WebGLContextGL.cpp @@ +4165,5 @@ > } \ > PRUint32 arrayLength = JS_GetTypedArrayLength(wa); \ > const WebGLUniformInfo& info = location_object->Info(); \ > + if (arrayLength == 0 || \ > + arrayLength % cnt) \ It looks like this is missing a check that assures arrayLength is not > expectedArrayLength. @@ +4184,5 @@ > arrayLength); \ > } \ > \ > MakeContextCurrent(); \ > + PRUint32 numElementsToUpload = NS_MIN(info.arraySize, arrayLength/cnt); \ Is it really required that we silently allow too-large uploads by truncating them, but still require 'arrayLength % cnt == 0'? @@ +4220,5 @@ > } \ > PRUint32 arrayLength = JS_GetTypedArrayLength(wa); \ > const WebGLUniformInfo& info = location_object->Info(); \ > + if (arrayLength == 0 || \ > + arrayLength % (dim*dim)) \ Can we do arrayLength % expectedArrayLength?

Applied your elemSize naming idea. Yes, the spec requires that we silently truncate too large arrays but require array length to be a multiple of elemSize.

Comment on attachment 617888 [details] [diff] [review] fix uniform setter validation: allow partial uniform array updates Review of attachment 617888 [details] [diff] [review]: ----------------------------------------------------------------- I would ask to have us add in all the protection parens for the macro args, but we should be de-macroing this ASAP. ::: content/canvas/src/WebGLContextGL.cpp @@ +4180,5 @@ > \ > nsIWebGLUniformLocation* ploc = aLocation; \ > OBTAIN_UNIFORM_LOCATION(#name ": location") \ > int elementSize = location_object->ElementSize(); \ > + if (elemSize != elementSize) { \ Hah, oops. Probably switch out 'elementSize' here with 'expectedElementSize'. @@ +4220,5 @@ > NS_IMETHODIMP \ > WebGLContext::name(nsIWebGLUniformLocation* aLocation, bool aTranspose, \ > const JS::Value& aValue, JSContext* aCx) \ > { \ > + int elemSize = dim*dim; \ Please make this safe for dim = 'a + b': int elemSize = (dim)*(dim); @@ +4236,5 @@ > if (!wa || !JS_IsFloat32Array(wa, aCx)) { \ > return ErrorInvalidValue(#name ": array must be of Float32 type"); \ > } \ > int elementSize = location_object->ElementSize(); \ > + if (elemSize != elementSize) { \ 'elemSize != expectedElem(ent)Size' is probably better. @@ +4246,5 @@ > } \ > PRUint32 arrayLength = JS_GetTypedArrayLength(wa, aCx); \ > const WebGLUniformInfo& info = location_object->Info(); \ > + if (arrayLength == 0 || \ > + arrayLength % (elemSize)) \ This doesn't need parens around elemSize, anymore.

This appears to affect webgl-2d's use of Uniform3fv as well, but I am not entirely certain - in webgl2d's case as far as I can tell it is uploading 9 floats to a mat3x3 uniform, which should be the correct amount, but Firefox is demanding 18 or 27 floats instead. Maybe I am just terrible at WebGL.

Note that it's the other one that is the 'expected' value.

Comment on attachment 620003 [details] [diff] [review] fix uniform setter validation: allow partial uniform array updates Review of attachment 620003 [details] [diff] [review]: ----------------------------------------------------------------- As soon as this hits central, I'd like to rip out all the macros.

Comment on attachment 620003 [details] [diff] [review] fix uniform setter validation: allow partial uniform array updates [Approval Request Comment] Regression caused by (bug #): bug 732233 User impact if declined: real-world webgl apps fail to work properly in firefox 14 Testing completed (on m-c, etc.): inbound Risk to taking this patch (and alternatives if risky): very low risk String changes made by this patch: none

Backed out: http://hg.mozilla.org/integration/mozilla-inbound/rev/7315f04e0303

Try: https://tbpl.mozilla.org/?tree=Try&rev=3f42a4f743b4

Hey, my aligned backslashes! ;)

We're removing these ugly macros very soon anyways, so that doesn't matter.

Comment on attachment 620003 [details] [diff] [review] fix uniform setter validation: allow partial uniform array updates [Approval Request Comment] Regression caused by (bug #): 732233 User impact if declined: certain WebGL applications are broken, due to a clear bug in our code. Testing completed (on m-c, etc.): m-i Risk to taking this patch (and alternatives if risky): seems low risk to me. Can't say no risk as this code, first introduced in bug 732233, is somewhat security sensitive in that it guards against exploiting certain driver bugs, so a bug there could re-allow that. String changes made by this patch: none

Comment on attachment 620003 [details] [diff] [review] fix uniform setter validation: allow partial uniform array updates [Triage Comment] Broken WebGL apps, and we're very early in the cycle. Approving for Aurora 14.

Conformance test added, covering valid uniform array setter calls with smaller and with larger arrays: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/uniforms/gl-uniform-arrays.html

(In reply to Raul Hao from comment #0) > Created attachment 617194 [details] > shader_bug_in_webgl.jpg > > User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/535.11 (KHTML, like > Gecko) Chrome/17.0.963.56 Safari/535.11 CoolNovo/2.0.2.26 > > Steps to reproduce: > > Visit the following WebGL page in Firefox 14.0a1 (2012-04-20): > > http://www.oak3d.com/demo/Core_Character.html > > > Actual results: > > Panda model with skeletal animation was rendered uncorrectly. See > attachment, the left part of shader_bug_in_webgl.jpg Cannot reproduce the issue on a Nvidia Geforce 210 card. Is this something specific to ATI cards?

No, it should have been device-independent. Also, I did reproduce on NVIDIA.

The walking panda looks fine to me on Nightly 2012-04-20 contrary to actual results from comment 0.

Verified as fixed with: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0 (20120731150526) The panda animation looks fine now and it doesn't generate any errors or warnings in the console.