Closed Bug 1128044 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: lukebenes, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted],webgl-conformance, webgl-angle)

Attachments

(5 files)

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
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Whiteboard: [gfx-noted] → [gfx-noted],webgl-conformance, webgl-angle
Blocks: webgl-1.0.2
Whiteboard: [gfx-noted],webgl-conformance, webgl-angle → [gfx-noted],webgl-conformance, webgl-angle
Assignee: nobody → jgilbert
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8576271 - Flags: review?(dglastonbury)
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?
Attachment #8576271 - Flags: review?(dglastonbury) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d768961b5362

Actually caught by warnings-as-errors!

Retroactive review from Dan.
Attachment #8576412 - Flags: review?(dglastonbury)
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cfb9e9de85

Another one. I checked that this compiled on my linux box.
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()});  }	
  }
}
Attachment #8576412 - Flags: review?(dglastonbury) → review+
(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.
r=kamidphish
Attachment #8576893 - Flags: review+
You need to log in before you can comment on or make changes to this bug.