[Mesa-dev] RFC [PATCH 0/7] Fix transform feedback of builtin "varyings".

[Paul Berry]

On 3 January 2012 16:35, Ian Romanick <idr at freedesktop.org> wrote:

> On 12/29/2011 09:16 AM, Paul Berry wrote:
>
>> This patch series allows transform feedback to work properly on the
>> built-in vertex shader output variables gl_PointSize, gl_ClipVertex,
>> and gl_ClipDistance.  gl_PointSize and gl_ClipVertex were broken due
>> to bugs in the i965 driver, and were trivial to fix--those are fixed
>> in patches 1 and 2.
>>
>> gl_ClipDistance was harder to fix, since on i965 its 8 floats are
>> packed into 2 vec4s, so the linker has to tell the back-end to select
>> a single component of one of the vec4's for streaming out.  This
>> required changing both core mesa and the back-ends, and adding a new
>> field to gl_transform_feedback_info.  However, the work seems worth it
>> because it lays some of the groundwork we will need when we get around
>> to packing varyings.  Patch 3 adds the new field, patches 4-5 cause
>> the back-ends to use it, and patches 6-7 update the linker to populate
>> it correctly for gl_ClipDistance.
>>
>> I'm putting this patch series out as an RFC partly because I want to
>> find out if the new field in gl_transform_feedback_info makes sense
>> for other driver back-ends, and partly because it is not 100% clear
>> from the spec whether transform feedback is intended to work on all
>> vertex shader outputs, or just the "varyings" (the ones that are
>> interpolated across the surface of a primitive).  Here are the
>> arguments I can see for and against going through with this patch
>> series:
>>
>> Arguments against: (1) The GL 3.0 spec says that "The varying
>> variables specified in<varyings>  can be either built-in varying
>> variables (beginning with 'gl_') or user-defined ones".  But it also
>> explicitly states that gl_Position is not a varying variable.  And
>> GLSL 1.20 lists gl_Position, gl_PointSize, and gl_ClipVertex in
>> section 7.1 ("Vertex Shader Special Variables") rather than section
>> 7.6 ("Varying Variables").  It seems clear that there was an intention
>> to distinguish between "varyings" and other vertex shader outputs, and
>> transform feedback is defined to work on varyings.  (2) In all
>> likelihood, most code that uses transform feedback uses it on
>> user-defined varyings anyhow, so fixing these built-in variables is
>> unlikely to make much difference.  (3) Making transform feedback work
>> on gl_ClipDistance adds a lot of complication for the benefit of a
>> tiny corner case.
>>
>> Arguments in favor: (1) Because of transform feedback's intended use
>> and its position in the pipeline, the distinction between varyings and
>> other vertex shader outputs is irrelevant; in all likelihood the spec
>> writers intended for it to work on all vertex shader outputs.  (2) The
>> very use of the term "varying" (and hence, this distinction) has
>> largely disappeared from the standard as of GLSL 1.30.  (3) nVidia's
>> proprietary Linux driver supports transform feedback of all vertex
>> shader outputs (except gl_ClipVertex, which has many other bugs), so
>> it's conceivable that some code in the wild relies on this feature.
>> (4) Fixing transform feedback of gl_ClipVertex provides a nice
>> opportunity to prepare for the changes we will have to make to
>> transform feedback in order to support varying packing.
>>
>
> Here are the results on an AMD system:
>
> [idr at oscar piglit]$ for i in gl_Color gl_SecondaryColor gl_TexCoord
> gl_FogFragCoord gl_Position gl_PointSize gl_ClipVertex gl_ClipDistance ; do
> echo $i; bin/ext_transform_feedback-**builtin-varyings -auto $i; echo;
> done
> gl_Color
> PIGLIT: {'result': 'pass' }
>
> gl_SecondaryColor
> PIGLIT: {'result': 'pass' }
>
> gl_TexCoord
> PIGLIT: {'result': 'pass' }
>
> gl_FogFragCoord
> PIGLIT: {'result': 'pass' }
>
> gl_Position
> PIGLIT: {'result': 'pass' }
>
> gl_PointSize
> Failed to link: A variable name specified with the
> TransformFeedbackVaryings is not declared as an output in the geometry or
> vertex shader.
>
> PIGLIT: {'result': 'fail' }
>
> gl_ClipVertex
> PIGLIT: {'result': 'pass' }
>
> gl_ClipDistance
> Failed to link: Vertex shader(s) failed to link.
> gl_ClipDistance[2] was not declared as an output in current vertex shader!
>

Heh, sounds like we're not the only ones with a lowering pass that converts
gl_ClipDistance from a float[8] to a vec4[2] :)


>
> PIGLIT: {'result': 'fail' }
>
> It would be interesting to see how this test fares on OS X.
>
>
>  Personally, I'm swayed by the arguments in favor but I would like to
>> hear what others think.
>>
>
> I searched a bit through old meeting notes and mailing discussions from
> the OpenGL 3.0 time frame.  It seems to me that your suspicion that "the
> spec writers intended for it to work on all vertex shader outputs" is
> almost certainly correct.
>
> I would consider the failing cases to just be bugs in those other
> implementations.
>

Ok, sounds good.  I'm going to consider this an "acked-by" if that's all
right with you.  I haven't heard many comments on this patch series, but
given that it fixes the tests and there hasn't been any negative feedback,
I think I'm going to consider it good.  I'll plan on pushing the patches at
the end of the day tomorrow unless I hear other comments.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120103/ef1db341/attachment.htm>