Varying packing algorithm seems to be buggy

[GoogleCodeExporter]

What steps will reproduce the problem?

Create a shader program with the following varyings:

varying vec4 v_TexCoord[3];
varying vec4 v_FrontColor;
varying vec4 v_BackColor;
varying vec4 v_FrontSecondaryColor;
varying vec4 v_BackSecondaryColor;
varying float v_ClipDistance[5];
varying float v_FogFactor;

You can open the example html attached to this bug report with Google Chrome. 
Have a look at the messages in the Javascript console.

What is the expected output? What do you see instead?

I expect the shader program to link successfully. Instead, I get the following 
error message:

"Could not pack varying v_FogFactor"

MAX_VARYING_VECTORS is 12.

According to the packing rules for varyings as specified in Appendix A, section 
7 of the GLSL ES spec 1.0.17, the varyings should be packed like this:

 1: a a a a (a = v_TexCoord[0])
 2: b b b b (b = v_TexCoord[0])
 3: c c c c (c = v_TexCoord[0])
 4: d d d d (d = v_FrontColor)
 5: e e e e (e = v_BackColor)
 6: f f f f (f = v_FrontSecondaryColor)
 7: g g g g (g = v_BackSecondaryColor)
 8: h m x x (h = v_ClipDistance[0], m = v_FogFactor, x = unused)
 9: i x x x (i = v_ClipDistance[1], x = unused)
10: j x x x (j = v_ClipDistance[2], x = unused)
11: k x x x (k = v_ClipDistance[3], x = unused)
12: l x x x (l = v_ClipDistance[4], x = unused)

As you can see, packing the given varyings into 12 slots is possible (actually, 
there is plenty of space available for a single float like v_FogFactor). 
Nevertheless, ANGLE fails.

What version of the product are you using? On what operating system?

Google Chrome Version 37.0.2062.102 m on Windows 7.

Please provide any additional information below.

The bug seems to be located in 
"angle/src/libGLESv2/renderer/d3d/DynamicHLSL.cpp" in the function 
"packVarying" starting at line 173. The code currently looks like this:

...
int column = 0;

for (int x = 0; x < 4; x++)
{
  if (space[x] >= registers && space[x] < space[column])
  {
    column = x;
  }
}

if (space[column] >= registers)
...

Using that code, putting single floats fails as soon as column 0 is completely 
filled, even though there is still space available in other columns (because 
"space[x] < space[column]" will always fail in this case). A correct version of 
the code would look like this:

...
int column = -1;

for (int x = 0; x < 4; x++)
{
  if (space[x] >= registers && (column < 0 || space[x] < space[column]))
  {
    column = x;
  }
}

if (column >= 0)
...

Original issue reported on code.google.com by pdae...@gmail.com on 2 Sep 2014 at 3:57

Attachments:

• varyingbug.html

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Sorry, there are typos in line 2 and 3 of my table of packed varyings. The 
correct version is:

 1: a a a a (a = v_TexCoord[0])
 2: b b b b (b = v_TexCoord[1])
 3: c c c c (c = v_TexCoord[2])
 4: d d d d (d = v_FrontColor)
 5: e e e e (e = v_BackColor)
 6: f f f f (f = v_FrontSecondaryColor)
 7: g g g g (g = v_BackSecondaryColor)
 8: h m x x (h = v_ClipDistance[0], m = v_FogFactor, x = unused)
 9: i x x x (i = v_ClipDistance[1], x = unused)
10: j x x x (j = v_ClipDistance[2], x = unused)
11: k x x x (k = v_ClipDistance[3], x = unused)
12: l x x x (l = v_ClipDistance[4], x = unused)

Original comment by pdae...@gmail.com on 2 Sep 2014 at 4:02

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

See https://chromium-review.googlesource.com/#/c/214108/

I think the above CL will fix a lot of the varying packing issues.

Original comment by jmad...@chromium.org on 17 Sep 2014 at 2:37

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

It seems that the CL above does not work in D3D9. I'm not sure at the moment if 
it's a regression or simply doesn't address the packing problems in D3D9.

Original comment by jmad...@chromium.org on 1 Oct 2014 at 8:18

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

My change was designed to fix the D3D11 packing issues. I didn't know that D3D9 
was broken (since the tests weren't running against D3D9 at the time). It is 
likely that my change simply didn't fix the D3D9 packing issues.

Original comment by aukin...@microsoft.com on 1 Oct 2014 at 8:23

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

It seems Shader Model 3 does not allow TEXCOORDs higher than 16, so there's a 
chance we would have broken something that might have been working previously 
-- not sure exactly if such a case exists.

Original comment by jmad...@chromium.org on 1 Oct 2014 at 9:11

• Added labels: ****
• Removed labels: ****