Bug 126775 – [WebGL] Correct uniform input validation for texture sampler uniforms

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Brent Fulgham]

I noticed that https://sketchfab.com/show/d7d5a375d4bf4f91a522a1148dbd3f6a does not render properly in the nightly from 1-9-2013.

JavaScript console indicates:

WebGL: INVALID_VALUE: uniform1: invalid texture unit.

[Brent Fulgham]

(In reply to comment #0)
> WebGL: INVALID_VALUE: uniform1: invalid texture unit.

Typo: I meant to say:

WebGL: INVALID_VALUE: uniform1i: invalid texture unit.

[Radar WebKit Bug Importer]

<rdar://problem/15795320>

[Brent Fulgham]

This stopped working in http://trac.webkit.org/changeset/157271.

However, it looks like the new code is doing the right thing, and preventing us from walking past the end of the available texture units.

[Brent Fulgham]

Created attachment 220893 [details]
Patch

[Brent Fulgham]

This was a bug in the way the Int32Array type was being accessed. The code was attempting to cast the Int32Array* to a GC3DInt*, which resulted in garbage.

Also added a test case to help avoid this in the future.

[Dean Jackson]

Comment on attachment 220893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220893&action=review

r+, but please copy into a new test.

> Source/WebCore/ChangeLog:8
> +        Revised webgl/1.0.2/resources/webgl_test_files/conformance/uniforms/uniform-samplers-test.html

We shouldn't do that. We should try to keep the webgl/1.0.2 files in sync with Khronos. Instead, copy the test into fast/canvas/webgl and make the changes there.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4205
> +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (Int32Array* Variation)");

Should we keep this in here? It might be really noisy.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4225
> +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (GC3Dint* Variation)");

Ditto.

[Brent Fulgham]

(In reply to comment #6)
> (From update of attachment 220893 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220893&action=review
> 
> r+, but please copy into a new test.

OK!

> > Source/WebCore/ChangeLog:8
> > +        Revised webgl/1.0.2/resources/webgl_test_files/conformance/uniforms/uniform-samplers-test.html
> 
> We shouldn't do that. We should try to keep the webgl/1.0.2 files in sync with Khronos. Instead, copy the test into fast/canvas/webgl and make the changes there.

Done.
 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4205
> > +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (Int32Array* Variation)");
> 
> Should we keep this in here? It might be really noisy.

Removed.
 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4225
> > +    LOG(WebGL, "Calling WebGLRenderingContext::uniform1iv (GC3Dint* Variation)");
> 
> Ditto.
Removed.

[Brent Fulgham]

Committed r161684: <http://trac.webkit.org/changeset/161684>

[Darin Adler]

This patch seems to have added a test with no test results: fast/canvas/webgl/uniform-samplers-test.html

Also, the test seems to be failing on my computer.

[Brent Fulgham]

(In reply to comment #9)
> This patch seems to have added a test with no test results: fast/canvas/webgl/uniform-samplers-test.html
> 
> Also, the test seems to be failing on my computer.

Fixing now.

[Brent Fulgham]

Reopened to add missing test.

[Brent Fulgham]

Created attachment 220951 [details]
Patch

[Brent Fulgham]

Committed r161794: <http://trac.webkit.org/changeset/161794>