1128044 - WebGL Conformance Test: conformance/glsl/misc/shader-varying-packing-restrictions.html fails with ANGLE D3D11

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Luke]

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

Steps to reproduce:

1. Go to: https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/glsl/misc/shader-varying-packing-restrictions.html


Actual results:

test: shaders with 15 varyings of mat2 (one past maximum) should fail
FAIL [unexpected link status] shaders with 15 varyings of mat2 (one past maximum) should fail


Expected results:

test: shaders with 15 varyings of mat2 (one past maximum) should fail
PASS shaders with 15 varyings of mat2 (one past maximum) should fail

Only Firefox Nightly38.0a1 (2015-01-30) with the D3D11 backend fails the test. The following browser all pass:
Chrome 42.0.2291.1 canary
IE 11.
Firefox Nightly38.0a1 (2015-01-30) - webgl.disable-angle = true
Firefox Nightly38.0a1 (2015-01-30) - webgl.angle.try-d3d11 = false

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Enforce-packing-restrictions-for-varyings.patch

Comment 2

[u480271]

Comment on attachment 8576271 [details] [diff] [review]
0001-Enforce-packing-restrictions-for-varyings.patch

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

::: dom/canvas/WebGLShaderValidator.cpp
@@ +224,4 @@
>          for (auto itrFrag = fragList.begin(); itrFrag != fragList.end(); ++itrFrag) {
>              static const char prefix[] = "gl_";
> +            if (StartsWith(itrFrag->name, prefix)) {
> +                if (itrFrag->staticUse) {

Should this explicitly check for gl_FragCoord, gl_FrontFacing, and  gl_PointCoord, or is that handled by itrFrag->staticUse?

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #2)
> Comment on attachment 8576271 [details] [diff] [review]
> 0001-Enforce-packing-restrictions-for-varyings.patch
> 
> Review of attachment 8576271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLShaderValidator.cpp
> @@ +224,4 @@
> >          for (auto itrFrag = fragList.begin(); itrFrag != fragList.end(); ++itrFrag) {
> >              static const char prefix[] = "gl_";
> > +            if (StartsWith(itrFrag->name, prefix)) {
> > +                if (itrFrag->staticUse) {
> 
> Should this explicitly check for gl_FragCoord, gl_FrontFacing, and 
> gl_PointCoord, or is that handled by itrFrag->staticUse?

There's no need to check explicitly, since the compile would fail if it's a variable which shouldn't be there. We can just assume it's a valid var. I believe any gl_ vars that come back as a varying should count towards packing, not just those three per se.

Comment 4

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/509dbdfe3323

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: hotfix-packing-restrictions.diff

https://hg.mozilla.org/integration/mozilla-inbound/rev/d768961b5362

Actually caught by warnings-as-errors!

Retroactive review from Dan.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: hotfix-no-std-vector-data.diff

https://hg.mozilla.org/integration/mozilla-inbound/rev/acf73f2ea7d9

We can't use std::vector::data() yet on android buildbots, so we need to switch to nsTArray here.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: hotfix-no-nstarray-init-list-append.diff

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cfb9e9de85

Another one. I checked that this compiled on my linux box.

Comment 8

[u480271]

Comment on attachment 8576412 [details] [diff] [review]
hotfix-packing-restrictions.diff

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

::: dom/canvas/WebGLShaderValidator.cpp
@@ +260,5 @@
>                  *out_log = error;
>                  return false;
>              }
>  
> +            if (staticVertUse && itrFrag->staticUse) {

if above also has itrFlag->staticUse, so fold into:

if (itrFlag->staticUse) {
  if (!definedInVertShader) {
    nsPrintfCString error("Varying `%s` has static-use in the frag"	
                          " shader, but is undeclared in the vert"	
                          " shader.", itrFrag->name.c_str());
    *out_log = error;	
    return false;	
  }

  if (staticVertUse) {
    staticUseVaryingList.push_back({itrFrag->type,
                                    (int)itrFrag->elementCount()});  }	
  }
}

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #8)
> Comment on attachment 8576412 [details] [diff] [review]
> hotfix-packing-restrictions.diff
> 
> Review of attachment 8576412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLShaderValidator.cpp
> @@ +260,5 @@
> >                  *out_log = error;
> >                  return false;
> >              }
> >  
> > +            if (staticVertUse && itrFrag->staticUse) {
> 
> if above also has itrFlag->staticUse, so fold into:
> 
> if (itrFlag->staticUse) {
>   if (!definedInVertShader) {
>     nsPrintfCString error("Varying `%s` has static-use in the frag"	
>                           " shader, but is undeclared in the vert"	
>                           " shader.", itrFrag->name.c_str());
>     *out_log = error;	
>     return false;	
>   }
> 
>   if (staticVertUse) {
>     staticUseVaryingList.push_back({itrFrag->type,
>                                     (int)itrFrag->elementCount()});  }	
>   }
> }

That's fair.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: hotfix-reformat-branch.diff

r=kamidphish

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/509dbdfe3323
https://hg.mozilla.org/mozilla-central/rev/d768961b5362
https://hg.mozilla.org/mozilla-central/rev/acf73f2ea7d9
https://hg.mozilla.org/mozilla-central/rev/d4cfb9e9de85