747619 - WebGL skeletal animation rendered incorrectly

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Raul Hao]

Attachment: 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

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

Comment 1

[Alice0775 White]

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

Comment 2

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

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?

Comment 3

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

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.

Comment 4

[Raul Hao]

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?

Comment 5

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

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

Comment 6

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

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

Comment 7

[Raul Hao]

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

Comment 8

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

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

Comment 9

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

Attachment: fix uniform setter validation: allow partial uniform array updates

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 10

[Kelsey Gilbert [:jgilbert]]

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?

Comment 11

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

Attachment: fix uniform setter validation: allow partial uniform array updates

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 12

[Kelsey Gilbert [:jgilbert]]

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.

Comment 13

[Katelyn Gadd (:kael)]

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.

Comment 14

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

Attachment: fix uniform setter validation: allow partial uniform array updates

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

Comment 15

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

Attachment: fix uniform setter validation: allow partial uniform array updates

Comment 16

[Kelsey Gilbert [:jgilbert]]

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 17

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/b13c1d8d2cda

Comment 18

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

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

Comment 19

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

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

Comment 20

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

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

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hey, my aligned backslashes! ;)

Comment 22

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

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

Comment 23

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/dc76558cbd41

Comment 24

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

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 25

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/dc76558cbd41

Comment 26

[Alex Keybl [:akeybl]]

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.

Comment 27

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

http://hg.mozilla.org/releases/mozilla-aurora/rev/6490a4794f9e

Comment 28

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

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

Comment 29

[Paul Silaghi, QA [:pauly]]

(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?

Comment 30

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

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

Comment 31

[Paul Silaghi, QA [:pauly]]

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

Comment 32

[Ioana (away)]

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.