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.

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.

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

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

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.

Fixed a warning message.

Drat. Made another mistake with the warning.

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 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.

(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.

(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.

(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?

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.

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.

(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.

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

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

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.

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

This is solved by bug 1136410.