1136411 - WebGL 1.0.3 conformance error: conformance/attribs/gl-bindAttribLocation-matrix.html

Status: RESOLVED DUPLICATE of bug 1136410

(Core :: Graphics: CanvasWebGL, defect)

Description

[Luke]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150224030228

Steps to reproduce:

https://www.khronos.org/registry/webgl/sdk/tests/conformance/attribs/gl-bindAttribLocation-matrix.html?webglVersion=1


Actual results:

Only 41 of 156 passed


Expected results:

Chrome passes all the tests. Firefox fails the test with both the ANGLE and OpenGL backends.

Comment 1

[Jukka Jylänki]

Tested to fail on the following configurations:
 - GIADA, MACBOOK_AIR_WIN, MACBOOK_PRO_WIN, NEXUS-10, SURFACE, WINDBOX, HASWELL, HPOMEN

Did not fail on the following configurations:
 - SPARK, MACBOOK_AIR_OSX, MACBOOK_PRO_OSX, MACMINI, MACPRO, NEXUS-4, NEXUS-5

See https://bugzilla.mozilla.org/show_bug.cgi?id=1178601 for hardware configuration details of these systems.

Comment 2

[Kyle Fung]

Attachment: gl-bindAttribLocation-matrix.html

Reduced test case trying to bind a 2x2 matrix and a 2-vec.

Comment 3

[Kyle Fung]

Attachment: check-att-type.patch

This patch lets the WebGLProgram query the type of an attribute.

Comment 4

[Kyle Fung]

Attachment: pack-attachments.patch

This patch makes WebGLProgram check if its vertex attribute indices conflict with each other by querying each attribute's type and thus its size.

Then, it can ensure that there will be no index conflicts before it binds the attributes.

The code for this was kind of bulky so I moved it out into its own function (WebGLProgram::AllocateAttributeBindings()). I'm not entirely sure if my list of formats is exhaustive or completely correct though.

Comment 5

[Kyle Fung]

Attachment: pack-attachments-v2.patch

Fixed a warning message.

Comment 6

[Kyle Fung]

Attachment: pack-attachments-v3.patch

Drat. Made another mistake with the warning.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8647671 [details] [diff] [review]
check-att-type.patch

Review of attachment 8647671 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLShaderValidator.cpp
@@ +348,5 @@
> +ShaderValidator::FindAttribTypeByUserName(const std::string& userName,
> +                                          GLenum* const out_type) const
> +{
> +    const std::vector<sh::Attribute>& attribs = *ShGetAttributes(mHandle);
> +    for (auto itr = attribs.begin(); itr != attribs.end(); ++itr) {

You can actually do this now:
for (auto itr : attribs) {
}

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8647685 [details] [diff] [review]
pack-attachments-v3.patch

Review of attachment 8647685 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLProgram.cpp
@@ +992,5 @@
> +
> +        // Not enough room
> +        if (index + colCount > attribSlots.size()) {
> +            mContext->GenerateWarning("Not enough attribute indices to accomodate"
> +                                      " attribute %s.", name);

This new validation code *needs* comments about what it does and why it's doing it.

I'm not totally sure that the spec has this requirement, so you'll need some citations as well.

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to kfung from comment #4)
> Created attachment 8647673 [details] [diff] [review]
> pack-attachments.patch
> 
> This patch makes WebGLProgram check if its vertex attribute indices conflict
> with each other by querying each attribute's type and thus its size.
> 
> Then, it can ensure that there will be no index conflicts before it binds
> the attributes.
> 
> The code for this was kind of bulky so I moved it out into its own function
> (WebGLProgram::AllocateAttributeBindings()). I'm not entirely sure if my
> list of formats is exhaustive or completely correct though.

Index conflicts are not errors, IIRC.

Comment 10

[Kyle Fung]

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 8647685 [details] [diff] [review]
> pack-attachments-v3.patch
> 
> Review of attachment 8647685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLProgram.cpp
> @@ +992,5 @@
> > +
> > +        // Not enough room
> > +        if (index + colCount > attribSlots.size()) {
> > +            mContext->GenerateWarning("Not enough attribute indices to accomodate"
> > +                                      " attribute %s.", name);
> 
> This new validation code *needs* comments about what it does and why it's
> doing it.
> 
> I'm not totally sure that the spec has this requirement, so you'll need some
> citations as well.

I suppose you're right about it not being in the spec. The man for glBindAttribLocation states that

> GL_INVALID_VALUE is generated if index is greater than or equal to GL_MAX_VERTEX_ATTRIBS.

Although I think it's strange that it's allowed since it would seem that we can give matrix columns indices that go above the maximum index.

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to kfung from comment #10)
> (In reply to Jeff Gilbert [:jgilbert] from comment #8)
> > Comment on attachment 8647685 [details] [diff] [review]
> > pack-attachments-v3.patch
> > 
> > Review of attachment 8647685 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/canvas/WebGLProgram.cpp
> > @@ +992,5 @@
> > > +
> > > +        // Not enough room
> > > +        if (index + colCount > attribSlots.size()) {
> > > +            mContext->GenerateWarning("Not enough attribute indices to accomodate"
> > > +                                      " attribute %s.", name);
> > 
> > This new validation code *needs* comments about what it does and why it's
> > doing it.
> > 
> > I'm not totally sure that the spec has this requirement, so you'll need some
> > citations as well.
> 
> I suppose you're right about it not being in the spec. The man for
> glBindAttribLocation states that
> 
> > GL_INVALID_VALUE is generated if index is greater than or equal to GL_MAX_VERTEX_ATTRIBS.
> 
> Although I think it's strange that it's allowed since it would seem that we
> can give matrix columns indices that go above the maximum index.

ANGLE's translator does packing validation for us. Separately, binding more than one attrib to the same index is valid, though you'll get undefined results, I think?

Comment 12

[Kyle Fung]

Well according to the OpenGL spec:

> Applications are allowed to bind more than one user-defined attribute variable to the same generic vertex attribute index. This is called aliasing, and it is allowed only if just one of the aliased attributes is active in the executable program, or if no path through the shader consumes more than one attribute of a set of attributes aliased to the same location.

So I guess we will need a way to check for the case where aliasing is allowed?

I'm also currently rummaging through https://github.com/KhronosGroup/WebGL/issues/712 to make some sense out of this.

Comment 13

[Kyle Fung]

Since Chrome passes these tests, it's likely that upstream ANGLE does this validation correctly.
Should we be relying on the shader compiler to do this correctly? This test would still fail on some Linux platforms even after we start using upstream ANGLE.

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Kyle Fung from comment #13)
> Since Chrome passes these tests, it's likely that upstream ANGLE does this
> validation correctly.
> Should we be relying on the shader compiler to do this correctly? This test
> would still fail on some Linux platforms even after we start using upstream
> ANGLE.

We can wait on the ANGLE update and revisit this.

Comment 15

[Guilherme Lima]

Just tried this after the ANGLE update landed on Nightly, and on Windows 7 I get "PASS" in everything.

Comment 16

[Jukka Jylänki]

Tried this on the HASWELL computer from Comment 1, where it now passes in Nightly.

Comment 17

[Luke]

I can confirm that this is fixed under Windows. Since it's fixed by ANGLE, we need to have webgl.disable-angle=false. Also Linux still has the same results. 

I'm changing the platform from Windows -> Linux. I can file a new bug report if you'd prefer to close this one.

Comment 18

[Luke]

Still failing on Intel HD 4000 under Ubuntu 16.04
50.0a1 (2016-06-25)

Comment 19

[Kelsey Gilbert [:jgilbert]]

This is solved by bug 1136410.