[Piglit] [PATCH] Add tests for gl_ClipDistance and gl_ClipVertex.

[Paul Berry]

On 16 August 2011 16:37, Ian Romanick <idr at freedesktop.org> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 08/16/2011 03:00 PM, Paul Berry wrote:
>> On 16 August 2011 13:45, Ian Romanick <idr at freedesktop.org> wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 08/09/2011 02:55 PM, Paul Berry wrote:
>>>> +void main()
>>>> +{
>>>> +  gl_Position = gl_Vertex;
>>>> +  gl_FrontColor = (gl_ClipDistance.length() >= 3) ? vec4(0.0, 1.0, 0.0, 1.0)
>>>> +                                                  : vec4(1.0, 0.0, 0.0, 1.0);
>>>
>>> This should actually generate an error.  Page 26 (page 32 of the PDF) of
>>> the GLSL 1.30 spec says:
>>>
>>>    "The length method cannot be called on an array that has not been
>>>    explicitly sized."
>>
>> Yes, you're right.  I discovered this when starting to work on Mesa's
>> gl_ClipDistance implementation, sadly not until after committing this
>> patch.  Unfortunately, since you can't find out the size of an array
>> using .length(), I can't think of a way to verify using Piglit that
>> implicit array sizing of gl_ClipDistance works correctly, other than
>> the indirect approach of using gl_ClipDistance in a shader (without
>> explicitly sizing it) and verifying that clipping takes effect.  Can
>> you think of a more direct test?  Since we already have a test that
>> uses gl_ClipDistance without sizing it, and verifies that clipping
>> takes effect, I think the best thing to do is to just delete this
>> test.  But I'm open to alternatives if you have a better idea.
>
> Right.  The only methods that you have available to probe the size of an
> implicitly sized array will affect the size of that array.
>
> However, since there are things that you cannot do with an implicitly
> sized array (e.g., use .length() or a non-constant index), we should
> have negative compile tests for those cases.

I assume you mean for clip distance in specific (and not for
implicitly sized arrays in general which I really hope we already have
Piglit tests for).  I started to write a paragraph proclaiming that to
be busywork, but then I thought about what I've observed from the
nVidia Linux driver: it predeclares the array with a provisional size
of 8, and allows you to redeclare it without error--that's a clear
violation of the spec and something that my current Piglit tests don't
detect.  So I've talked myself into it :)

>
>>> In this case gl_ClipDistance is implicitly sized after the call to
>>> length.  We /should/ have similar tests for gl_TexCoord.  It behaves in
>>> a similar manner.
>>>
>>>> +  gl_ClipDistance[2] = 1.0;
>>>> +}
>>>> +
>>>> +[fragment shader]
>>>> +#version 130
>>>> +void main()
>>>> +{
>>>> +  gl_FragColor = gl_Color;
>>>> +}
>>>> +
>>>> +[test]
>>>> +draw rect -1 -1 2 2
>>>> +probe all rgba 0.0 1.0 0.0 1.0
>>>> diff --git a/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-sizeable-to-max.shader_test b/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-sizeable-to-max.shader_test
>>>> new file mode 100644
>>>> index 0000000..accd639
>>>> --- /dev/null
>>>> +++ b/tests/spec/glsl-1.30/execution/clipping/vs-clip-distance-sizeable-to-max.shader_test
>>>> @@ -0,0 +1,45 @@
>>>> +# From the GLSL 1.30 spec section 7.1 (Vertex Shader Special
>>>> +# Variables):
>>>> +#
>>>> +#   The gl_ClipDistance array is predeclared as unsized and must be
>>>> +#   sized by the shader either redeclaring it with a size or indexing
>>>> +#   it only with integral constant expressions. This needs to size the
>>>> +#   array to include all the clip planes that are enabled via the
>>>> +#   OpenGL API; if the size does not include all enabled planes,
>>>> +#   results are undefined. The size can be at most
>>>> +#   gl_MaxClipDistances. The number of varying components (see
>>>> +#   gl_MaxVaryingComponents) consumed by gl_ClipDistance will match
>>>> +#   the size of the array, no matter how many planes are enabled. The
>>>> +#   shader must also set all values in gl_ClipDistance that have been
>>>> +#   enabled via the OpenGL API, or results are undefined. Values
>>>> +#   written into gl_ClipDistance for planes that are not enabled have
>>>> +#   no effect.
>>>> +#
>>>> +# This test checks that the size of gl_ClipDistance can be set to
>>>> +# gl_MaxClipDistances without error, and that this actually causes the
>>>> +# size of the array to be set properly.
>>>> +
>>>> +[require]
>>>> +GLSL >= 1.30
>>>> +
>>>> +[vertex shader]
>>>> +#version 130
>>>> +out float gl_ClipDistance[gl_MaxClipDistances];
>>>
>>> We should also test that
>>>
>>> out float gl_ClipDistance[gl_MaxClipDistances + 1];
>>>
>>> generates an error.  I see there's a similar test for accesses beyond
>>> gl_MaxClipDistances when gl_ClipDistance is implicitly sized.
>>
>> Actually the test that I think you're referring to
>> (clip-distance-not-sizeable-above-max.c) checks that an error occurs
>> when there is an access beyond gl_MaxClipDistances and gl_ClipDistance
>> is _explicitly_ sized to gl_MaxClipDistances + 1.  But your point is
>
> I missed that on my first review.  If we're going to hit most of the
> combinations, we should hit all of them.  In particular, we should
> expect compilation errors when:
>
>  - explicit size <= gl_MaxClipDistances, access >= explicit size
>  - explicit size > gl_MaxClipDistances
>  - implicit size > gl_MaxClipDistances
>
> I thought the various C code tests hit the first two.

Ok, on further reflection I think I will add tests expecting errors in
these 5 cases:

- explicit size = gl_MaxClipDistances, access element gl_MaxClipDistances
- explicit size = 3, access element 3
- explicit size = gl_MaxClipDistances+1, access element 0
- explicit size = gl_MaxClipDistances+1, do not access any elements*
- implicitly sized, access element gl_MaxClipDistances

(*this seems worth testing as a separate case because I know it
doesn't produce the proper error on the nVidia Linux driver)

>> 2. clip-distance-not-sizeable-above-max.c needs to pass regardless of
>> whether the error occurs at compile time or link time, because some
>> implementations (e.g. nVidia) produce the error at link time, while
>> others (Mesa, with the code that I'm in the process of writing)
>> produce it at compile time.
>
> Oh bother.  I really *HATE* that the GLSL spec doesn't say when errors
> are supposed to be generated.  The particular issue is that an array may
> be "implicitly" sized in one compilation unit and explicitly sized in
> another.  The error in these two vertex shaders can only generated at
> link time:
>
> #version 120
> float a[];
> float foo(float x) { return a[37] * x; }
>
> #version 120
> float foo(float);
> float a[5];
>
> void main() { gl_Position = gl_Vertex * foo(3.14159); }

Yeah, considering your example, it doesn't surprise me that some
implementations seem to postpone all array size-related error checking
to link time.  It sounds like all 5 of the cases I promised to test
above are going to have to allow for the possibility that the error
might occur either at link time or at compile time.  Given the present
capabilities of Piglit, that means I'm going to have to write 5 very
similar-looking tests in C (actually 10, since gl_ClipDistance is
accessible from both vertex and fragment shaders).  I'm really not
looking forward to that.

I've noticed that glslparsertest tries linking any shader that it
successfully compiles, but doesn't pay any attention to whether the
link succeeded.  How about if I add a "--check-link" option to
glslparsertest, which causes it to treat link errors the same as
compile errors?  That would let me write these 10 test cases as quick
and easy glslparsertest tests instead of separate C programs.

>
> Again, we have the same issue with gl_TexCoord.  It might be worth
> cleaning up the few gl_TexCoord tests
> (test/shaders/glsl-link-varyings-*.shader_tests,
> tests/shaders/glsl-link-varying-TexCoord.shader_test,
> tests/shaders/glsl-texcoord-array-2.shader_test, and
> tests/glslparsertest/glsl2/unsized-array-non-const-index.vert) to be
> more like these tests.  *shrug*

Yeah, *shrug* is kind of my reaction too.  Since gl_ClipDistance is
the feature I'm focusing on right now, cleaning up a bunch of
gl_TexCoord tests in the process is not a terribly exciting prospect.
In Mesa, at least, I think most of the corner case behaviors we're
talking about for gl_ClipDistance are going to exercise the same code
as they exercise for gl_TexCoord, so I'm more inclined to let sleeping
dogs lie.  I'll change my tune, though, if in order to implement these
gl_ClipDistance behaviors I have to do a bunch of refactoring that
affects gl_TextCoord.  In that case it would be worth testing
gl_TextCoord thoroughly to make sure the refactoring doesn't regress
it.