Bug 38709 - getActiveUniform must ensure names of arrays end in "[0]"
Summary: getActiveUniform must ensure names of arrays end in "[0]"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-06 17:48 PDT by Kenneth Russell
Modified: 2010-06-11 08:17 PDT (History)
6 users (show)

See Also:


Attachments
patch (23.37 KB, patch)
2010-06-10 14:52 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff
revised patch: responding to kbr's review (23.21 KB, patch)
2010-06-10 18:04 PDT, Zhenyao Mo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-05-06 17:48:44 PDT
In order to match OpenGL ES 2.0 semantics, getActiveUniform must ensure that if the name of an array uniform is fetched, the suffix "[0]" is present. Desktop GL does not enforce this rule. See section 2.10.4 "Shader Variables" in the ES spec version 2.0.24 (p. 34, GetActiveUniform), and compare to section 2.15.3 "Shader Variables" in the OpenGL 2.1 spec version dated 2006-12-01 (p. 80).
Comment 1 Zhenyao Mo 2010-06-10 14:52:20 PDT
Created attachment 58414 [details]
patch
Comment 2 Kenneth Russell 2010-06-10 17:47:04 PDT
Comment on attachment 58414 [details]
patch

The changes generally look good, in particular the strengthening of the test case. We should consider using bindAttribLocation to avoid some of the swapping logic, but that isn't necessary to do now. A couple of comments:

WebCore/html/canvas/WebGLRenderingContext.cpp:966
 +          }
I know this is how Chromium's command buffer version is written, but here it would be much more concisely expressed as:

if (info.size > 1 && !info.name.endsWith("[0]"))
    info.name.append("[0]");


LayoutTests/fast/canvas/webgl/resources/intArrayUniformShader.vert:7
 +                  + ival2[0] + ival2[1];
The formatting is unnecessarily weird here.
Comment 3 Zhenyao Mo 2010-06-10 18:04:48 PDT
Created attachment 58429 [details]
revised patch: responding to kbr's review
Comment 4 Kenneth Russell 2010-06-10 18:09:13 PDT
Comment on attachment 58429 [details]
revised patch: responding to kbr's review

Looks good. Assuming you re-ran the test after the change to the string manipulation code.
Comment 5 Zhenyao Mo 2010-06-10 18:49:37 PDT
(In reply to comment #4)
> (From update of attachment 58429 [details])
> Looks good. Assuming you re-ran the test after the change to the string manipulation code.

Yes I did.  However, my Mac GL driver returns with "[0]", so the code path is not covered by the test on my Mac.
Comment 6 Dimitri Glazkov (Google) 2010-06-10 21:35:19 PDT
Comment on attachment 58429 [details]
revised patch: responding to kbr's review

Weeeeird. But ok.
Comment 7 WebKit Commit Bot 2010-06-11 08:17:08 PDT
Comment on attachment 58429 [details]
revised patch: responding to kbr's review

Clearing flags on attachment: 58429

Committed r61020: <http://trac.webkit.org/changeset/61020>
Comment 8 WebKit Commit Bot 2010-06-11 08:17:14 PDT
All reviewed patches have been landed.  Closing bug.