The default bug view has changed. See this FAQ.

WebGL skeletal animation rendered incorrectly

RESOLVED FIXED in Firefox 14

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Raul Hao, Assigned: bjacob)

Tracking

({regression})

14 Branch
mozilla15
x86
Windows 7
regression
Points:
---

Firefox Tracking Flags

(firefox12 unaffected, firefox13 unaffected, firefox14 fixed, firefox15 verified)

Details

(Whiteboard: regressed by Bug 732233 webgl-conformance)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

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
(Reporter)

Updated

5 years ago

Comment 1

5 years ago
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
Status: UNCONFIRMED → NEW
Component: Untriaged → Canvas: WebGL
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: untriaged → canvas.webgl

Updated

5 years ago
Whiteboard: regressed by Bug 732233
(Assignee)

Comment 2

5 years ago
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?
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
(Reporter)

Comment 4

5 years ago
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?
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 5

5 years ago
Will look at it tomorrow, sorry. I may have misinterpreted the spec.
(Assignee)

Comment 6

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

Comment 7

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

Updated

5 years ago
(Assignee)

Comment 8

5 years ago
I have identified the error, indeed partial uniform array updates are allowed and we were wrongly disallowing them. Patch coming shortly.
(Assignee)

Comment 9

5 years ago
Created attachment 617319 [details] [diff] [review]
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
Attachment #617319 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Blocks: 732233
Whiteboard: regressed by Bug 732233 → regressed by Bug 732233 webgl-test-needed webgl-conformance
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?
Attachment #617319 - Flags: review?(jgilbert) → review-
Created attachment 617888 [details] [diff] [review]
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.
Attachment #617319 - Attachment is obsolete: true
Attachment #617888 - Flags: review?(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.
Attachment #617888 - Flags: review?(jgilbert) → review-

Comment 13

5 years ago
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.
Created attachment 619916 [details] [diff] [review]
fix uniform setter validation: allow partial uniform array updates

Note that it's the other one that is the 'expected' value.
Attachment #617888 - Attachment is obsolete: true
Attachment #619916 - Flags: review?(jgilbert)
Summary: WebGL skeletal animation was rendered uncorrectly → WebGL skeletal animation rendered incorrectly
Created attachment 620003 [details] [diff] [review]
fix uniform setter validation: allow partial uniform array updates
Attachment #619916 - Attachment is obsolete: true
Attachment #619916 - Flags: review?(jgilbert)
Attachment #620003 - Flags: review?(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.
Attachment #620003 - Flags: review?(jgilbert) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/b13c1d8d2cda
Assignee: nobody → bjacob
Target Milestone: --- → mozilla14
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
Attachment #620003 - Flags: approval-mozilla-aurora?
Backed out:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7315f04e0303
(Assignee)

Updated

5 years ago
Attachment #620003 - Flags: approval-mozilla-aurora?
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/dc76558cbd41
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
Attachment #620003 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/dc76558cbd41
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla14 → mozilla15
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.
Attachment #620003 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/6490a4794f9e
status-firefox12: --- → unaffected
status-firefox13: --- → unaffected
status-firefox14: --- → fixed
status-firefox15: --- → fixed
(Assignee)

Updated

5 years ago
No longer blocks: 732233
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
(Assignee)

Updated

5 years ago
Whiteboard: regressed by Bug 732233 webgl-test-needed webgl-conformance → regressed by Bug 732233 webgl-conformance
(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.

Comment 32

5 years ago
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.
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.