On 3 January 2012 16:35, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5">On 12/29/2011 09:16 AM, Paul Berry wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote> </div></div> Here are the results on an AMD system: [idr@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! </blockquote><div> 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] :) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> PIGLIT: {'result': 'fail' } It would be interesting to see how this test fares on OS X.<div class="im"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Personally, I'm swayed by the arguments in favor but I would like to hear what others think. </blockquote> </div> 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. </blockquote><div> 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. </div></div>