Bug 126411 – [WebGL] Correct symbol lookup logic to handle 1-element arrays

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

[Brent Fulgham]

The optimization made in Bug 122607 introduced a bug when dealing with array types.  As documented in the ANGLEWebKitBridge.cpp file, ANGLE identifies arrays by affixing "[0]" to the name.

The new logic incorrectly assumed arrays would always have sizes > 1, and deleted the "[0]" characters at the end of the name. However, there are a number of cases where single-element arrays are used, and the new logic prevented WebKit from finding them in its symbol store.

The attached patch corrects the problem so that the following three.js examples work properly:

http://threejs.org/examples/#webgl_lights_hemisphere
http://threejs.org/examples/#webgl_materials2

[Brent Fulgham]

<rdar://problem/15394564>

[Brent Fulgham]

Created attachment 220266 [details]
Patch

[WebKit Commit Bot]

Attachment 220266 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 220266 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3179
> +                String arrayBrackets = "[" + String::number(index) + "]";
> +                String uniformName = info.name + arrayBrackets;

I guess we could do this in one line, although I always forget the optimisation rules for string building.

[Roger Fong]

I think this'll fail if we have a size 1 array, say soo int foo[1] or something, but then try to getUniformLocation of foo[0].

[Brent Fulgham]

Committed r161242: <http://trac.webkit.org/changeset/161242>

[Brent Fulgham]

Roger pointed out that we might do the wrong thing now if we try to access the zeroeth element of an array, because we always strip off the "[0]" bracket and only test for "[x]" when we have an array size greater than 1.

[Build Bot]

Comment on attachment 220266 [details]
Patch

Attachment 220266 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4513015000989696

New failing tests:
webgl/1.0.2/conformance/glsl/misc/shader-with-array-of-structs-containing-arrays.html
webgl/1.0.2/conformance/glsl/misc/shader-with-similar-uniform-array-names.html

[Build Bot]

Created attachment 220270 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Brent Fulgham]

Created attachment 220271 [details]
Patch

[Brent Fulgham]

(In reply to comment #8)
> (From update of attachment 220266 [details])
> Attachment 220266 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/4513015000989696
> 
> New failing tests:
> webgl/1.0.2/conformance/glsl/misc/shader-with-array-of-structs-containing-arrays.html
> webgl/1.0.2/conformance/glsl/misc/shader-with-similar-uniform-array-names.html

This may have been caused by the 0-th element case Roger pointed out.

[Brent Fulgham]

Created attachment 220274 [details]
Patch (with Test Case)

[Brent Fulgham]

Committed r161247: <http://trac.webkit.org/changeset/161247>

[Darin Adler]

Comment on attachment 220266 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3179
>> +                String arrayBrackets = "[" + String::number(index) + "]";
>> +                String uniformName = info.name + arrayBrackets;
> 
> I guess we could do this in one line, although I always forget the optimisation rules for string building.

This needs to be done in one line. It will do less memory allocation.

    String uniformName = info.name + "[" + String::number(index) + "]";

[Brent Fulgham]

Reopen to incorporate Darin's suggestion.

[Brent Fulgham]

Created attachment 220460 [details]
Patch

[Brent Fulgham]

Committed r161378: <http://trac.webkit.org/changeset/161378>