Closed Bug 1136411 Opened 7 years ago Closed 5 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

39 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1136410

People

(Reporter: lukebenes, Assigned: kyle_fung)

References

(Blocks 1 open bug)

Details

(Whiteboard: webgl-conformance gfx-noted)

Attachments

(3 files, 2 obsolete files)

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.
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Whiteboard: webgl-conformance → webgl-conformance gfx-noted
Summary: WebGL conformance error: conformance/attribs/gl-bindAttribLocation-matrix.html → WebGL 1.0.3 conformance error: conformance/attribs/gl-bindAttribLocation-matrix.html
No longer blocks: webgl-1.0.2
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.
Assignee: nobody → kfung
Blocks: 1136410
This patch lets the WebGLProgram query the type of an attribute.
Attachment #8647671 - Flags: review?(jgilbert)
Attached patch pack-attachments.patch (obsolete) — Splinter Review
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.
Attachment #8647673 - Flags: review?(jgilbert)
Attached patch pack-attachments-v2.patch (obsolete) — Splinter Review
Fixed a warning message.
Attachment #8647673 - Attachment is obsolete: true
Attachment #8647673 - Flags: review?(jgilbert)
Attachment #8647683 - Flags: review?(jgilbert)
Drat. Made another mistake with the warning.
Attachment #8647683 - Attachment is obsolete: true
Attachment #8647683 - Flags: review?(jgilbert)
Attachment #8647685 - Flags: review?(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) {
}
Attachment #8647671 - Flags: review?(jgilbert) → review+
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.
Attachment #8647685 - Flags: review?(jgilbert) → review-
(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.
Depends on: 1179280
Flags: needinfo?(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.
Flags: needinfo?(jgilbert)
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.
OS: Windows 7 → Linux
Blocks: 1193526
No longer blocks: 1193526
Still failing on Intel HD 4000 under Ubuntu 16.04
50.0a1 (2016-06-25)
This is solved by bug 1136410.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1136410
Blocks: 1286768
You need to log in before you can comment on or make changes to this bug.