[Mesa-dev] Geometry shader update, and a course correction

[Paul Berry]

(TL;DR: geometry shaders are humming along, but because of a hitch I've run
into, I'm going to change gears and implement GLSL 1.50-style geometry
shaders first rather than ARB_geometry_shader4 functionality.  This means
some piglit tests will need to be rewritten).

Hello all--

As some of you may have noticed, I've spent the last several weeks writing
a number of geometry shader tests and fixinging my "gs" branch (on
https://github.com/stereotype441/mesa.git).  I'm happy to report that it's
working better than ever: I now have the following features working in
geometry shaders on Intel drivers:

- Texturing.
- Built-in uniform variables.
- Interactions with ARB_shader_texture_lod, ARB_shading_language_packing,
ARB_texture_cube_map_array, ARB_texture_multisample, ARB_texture_rectangle,
ARB_uniform_buffer_object, and EXT_texture_array.
- Mixing user-defined geometry shaders with fixed-function fragment shaders.
- gl_PointSizeIn, gl_PrimitiveIDIn, gl_PrimitiveID, gl_ClipVertex,
gl_ClipDistanceIn, and gl_ClipDistance (including several important corner
cases of gl_ClipDistance that were previously broken, such as bulk
assignment).
- Color clamping.

I've done all of this work using the ARB_geometry_shader4 version of
geometry shaders; my intention was to get that working first, then
introduce additional code to support GLSL 1.50-style geometry shaders.
This was partly motivated by the fact that Bryan Cain's initial work on
geometry shaders was done using ARB_geometry_shader4, and partly motivated
by the fact that it's our standard development practice in Mesa to develop
features as extensions first, and then only support a core feature once all
of the extensions it includes have been finished.  However, I've now run
into hitch that's making me want to change course and implement GLSL-1.50
style geometry shaders first.  It's a bit of a long story so buckle up:

In GLSL 1.50, the way you specify the input primitive type for a geometry
shader is using a layout qualifier in the shader source code, so it is
known at compile time.  Whereas in ARB_geometry_shader4, the way you
specify the input primitive type is using glProgramParameteriARB() prior to
linking.  This means that the input primitive type isn't known at compile
time.

That's a problem because ARB_geometry_shader4 provides for a read-only
built-in constant called gl_VerticesIn, which is the number of vertices in
each geometry shader input primitive (e.g. if the input primitive type is
"triangles", gl_VerticesIn is 3).  Mesa does not currently support
constants whose value is determined at link time, since constant folding,
determination of explicit array sizes, and a lot of constant-related error
checking happen at compile time.  To work around this, the "gs" branch
currently treats gl_VerticesIn as a varying input during compilation, and
then at link time replaces any references to it with the appropriate
constant value.

That in turn creates a problem because it prevents declarations like this
from working:

varying in float foo[gl_VerticesIn];

since array sizes must be integral-constant expressions.  To work around
this, the "gs" branch currently modifies the parser so that the above
declaration is treated as equivalent to an unsized array, i.e.:

varying in float foo[];

Similar changes are made for 2D inputs such as gl_TexCoordIn.

That in turn is a problem because it prevents code like this from working:

varying in float foo[gl_VerticesIn];
varying out float bar;
void main()
{
  for (int i = 0; i < gl_VerticesIn; i++) {
    bar = foo[i]; /* problem */
    EmitVertex();
  }
}

since the GLSL spec stipulates that it is only legal to index into an
unsized array using an integral constant expression.  To work around that,
this stipulation is suspended when ARB_geometry_shader4 is in use.

At this point, we have three layers of workarounds being applied because of
one crazy feature in ARB_geometry_shader4 that doesn't exist in GLSL 1.50
style geometry shaders.  Some of these workarounds have unintended
consequences that aren't easy to fix.  For example, the following shader is
accepted, even though it should be rejected:

varing in float foo[];
varying out float bar;
void main()
{
  for (int i = 0; i < gl_VerticesIn; i++) {
    bar = foo[i]; /* illegal: i must be a constant because foo is unsized */
    EmitVertex();
  }
}

Conversely, code like this fails, even though it is allowed by
ARB_geometry_shader4:

varying in float foo[gl_VerticesIn + 1 - 1];

And so does this:

int main()
{
  float tmp[gl_VerticesIn];
}

Note that none of these problems applies to GLSL 1.50 style geometry
shaders.

I'm not comfortable with this situation.  I would far prefer to remove the
workarounds, and instead have Mesa postpone compilation of
ARB_geometry_shader4-style geometry shaders until link time, when the value
of gl_VerticesIn is known.  That would make the code cleaner, and eliminate
all of the corner cases that are currently broken, at the expense of making
certain compile errors reported at link time rather than compile time
(competing implementations, such as Nvidia's proprietary driver, have this
behaviour already).

However, that's going to be a fair amount of effort, and it's effort that
doesn't really push us towards GL 3.2 (which is the primary reason I'm
interested in working on geometry shaders to begin with).  So instead, I'm
going to shift gears and modify my "gs" branch so that it implements GLSL
1.50 style geometry shaders rather than ARB_geometry_shader4 style geometry
shaders.  Once GLSL 1.50 is working and committed upstream, I'll consider
working on ARB_geometry_shader4 again, if there is interest.

An additional advantage of shifting gears to GLSL 1.50 style geometry
shaders is that we no longer have to worry about interactions between
geometry shaders and compatibility-only features (since Mesa currently only
implements the core profile of GL 3.1 and above).  It also means that we
don't have to worry about subtle bugs creeping in because of
ARB_geometry_shader4's use of 2D arrays (a lot of code in src/glsl assumes
that arrays are always 1D, and I haven't yet convinced myself that this
assumption has been removed when necessary for geometry shaders).  Finally,
it's possible that a high enough percentage of the users of geometry
shaders will do so through via GLSL 1.50, so it may never be necessary to
implement ARB_geometry_shader4 at all.

One impact of this course change is that many of the existing piglit tests
which validate geometry shaders currently use ARB_geometry_shader4.  We'll
need to write counterparts to these tests that use GLSL 1.50.  Intel has a
few interns working for us this summer--I think I may try to talk them into
doing that work :)

I've taken a snapshot of the current state of my "gs" branch and called it
"gs-arb", so if we later find out this was a bad idea, we can switch back
to plan A without much penalty.

I believe this is the right way to go, but I'm open to any comments anyone
has on this plan.  Thanks in advance!

Paul
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130719/185313a4/attachment-0001.html>