[Mesa-dev,v2,12/23] glsl: Validate vertex emission in geometry shaders.

Submitted by Iago Toral Quiroga on June 18, 2014, 9:51 a.m.

Details

Message ID 1403085110-31168-13-git-send-email-itoral@igalia.com
State Accepted
Commit 7589683c97442239295e700cbea17e82736f1f27
Headers show

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga June 18, 2014, 9:51 a.m.
Check if non-zero streams are used. Fail to link if emitting to unsupported
streams or emitting to non-zero streams with output type other than GL_POINTS.
---
 src/glsl/linker.cpp | 148 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 134 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index 0b6a716..f8ff138 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -250,31 +250,100 @@  public:
    }
 };
 
-
 /**
- * Visitor that determines whether or not a shader uses ir_end_primitive.
+ * Visitor that determines the highest stream id to which a (geometry) shader
+ * emits vertices. It also checks whether End{Stream}Primitive is ever called.
  */
-class find_end_primitive_visitor : public ir_hierarchical_visitor {
+class find_emit_vertex_visitor : public ir_hierarchical_visitor {
 public:
-   find_end_primitive_visitor()
-      : found(false)
+   find_emit_vertex_visitor(int max_allowed)
+      : max_stream_allowed(max_allowed),
+        invalid_stream_id(0),
+        invalid_stream_id_from_emit_vertex(false),
+        end_primitive_found(false),
+        uses_non_zero_stream(false)
    {
       /* empty */
    }
 
-   virtual ir_visitor_status visit(ir_end_primitive *)
+   virtual ir_visitor_status visit_leave(ir_emit_vertex *ir)
    {
-      found = true;
-      return visit_stop;
+      int stream_id = ir->stream_id();
+
+      if (stream_id < 0) {
+         invalid_stream_id = stream_id;
+         invalid_stream_id_from_emit_vertex = true;
+         return visit_stop;
+      }
+
+      if (stream_id > max_stream_allowed) {
+         invalid_stream_id = stream_id;
+         invalid_stream_id_from_emit_vertex = true;
+         return visit_stop;
+      }
+
+      if (stream_id != 0)
+         uses_non_zero_stream = true;
+
+      return visit_continue;
    }
 
-   bool end_primitive_found()
+   virtual ir_visitor_status visit_leave(ir_end_primitive *ir)
    {
-      return found;
+      end_primitive_found = true;
+
+      int stream_id = ir->stream_id();
+
+      if (stream_id < 0) {
+         invalid_stream_id = stream_id;
+         invalid_stream_id_from_emit_vertex = false;
+         return visit_stop;
+      }
+
+      if (stream_id > max_stream_allowed) {
+         invalid_stream_id = stream_id;
+         invalid_stream_id_from_emit_vertex = false;
+         return visit_stop;
+      }
+
+      if (stream_id != 0)
+         uses_non_zero_stream = true;
+
+      return visit_continue;
+   }
+
+   bool error()
+   {
+      return invalid_stream_id != 0;
+   }
+
+   const char *error_func()
+   {
+      return invalid_stream_id_from_emit_vertex ?
+         "EmitStreamVertex" : "EndStreamPrimitive";
+   }
+
+   int error_stream()
+   {
+      return invalid_stream_id;
+   }
+
+   bool uses_streams()
+   {
+      return uses_non_zero_stream;
+   }
+
+   bool uses_end_primitive()
+   {
+      return end_primitive_found;
    }
 
 private:
-   bool found;
+   int max_stream_allowed;
+   int invalid_stream_id;
+   bool invalid_stream_id_from_emit_vertex;
+   bool end_primitive_found;
+   bool uses_non_zero_stream;
 };
 
 } /* anonymous namespace */
@@ -551,10 +620,58 @@  validate_geometry_shader_executable(struct gl_shader_program *prog,
 
    analyze_clip_usage(prog, shader, &prog->Geom.UsesClipDistance,
                       &prog->Geom.ClipDistanceArraySize);
+}
+
+/**
+ * Check if geometry shaders emit to non-zero streams and do corresponding
+ * validations.
+ */
+static void
+validate_geometry_shader_emissions(struct gl_context *ctx,
+                                   struct gl_shader_program *prog)
+{
+   if (prog->_LinkedShaders[MESA_SHADER_GEOMETRY] != NULL) {
+      find_emit_vertex_visitor emit_vertex(ctx->Const.MaxVertexStreams - 1);
+      emit_vertex.run(prog->_LinkedShaders[MESA_SHADER_GEOMETRY]->ir);
+      if (emit_vertex.error()) {
+         linker_error(prog, "Invalid call %s(%d). Accepted values for the "
+                      "stream parameter are in the range [0, %d].",
+                      emit_vertex.error_func(),
+                      emit_vertex.error_stream(),
+                      ctx->Const.MaxVertexStreams - 1);
+      }
+      prog->Geom.UsesStreams = emit_vertex.uses_streams();
+      prog->Geom.UsesEndPrimitive = emit_vertex.uses_end_primitive();
 
-   find_end_primitive_visitor end_primitive;
-   end_primitive.run(shader->ir);
-   prog->Geom.UsesEndPrimitive = end_primitive.end_primitive_found();
+      /* From the ARB_gpu_shader5 spec:
+       *
+       *   "Multiple vertex streams are supported only if the output primitive
+       *    type is declared to be "points".  A program will fail to link if it
+       *    contains a geometry shader calling EmitStreamVertex() or
+       *    EndStreamPrimitive() if its output primitive type is not "points".
+       *
+       * However, in the same spec:
+       *
+       *   "The function EmitVertex() is equivalent to calling EmitStreamVertex()
+       *    with <stream> set to zero."
+       *
+       * And:
+       *
+       *   "The function EndPrimitive() is equivalent to calling
+       *    EndStreamPrimitive() with <stream> set to zero."
+       *
+       * Since we can call EmitVertex() and EndPrimitive() when we output
+       * primitives other than points, calling EmitStreamVertex(0) or
+       * EmitEndPrimitive(0) should not produce errors. This it also what Nvidia
+       * does. Currently we only set prog->Geom.UsesStreams to TRUE when
+       * EmitStreamVertex() or EmitEndPrimitive() are called with a non-zero
+       * stream.
+       */
+      if (prog->Geom.UsesStreams && prog->Geom.OutputType != GL_POINTS) {
+         linker_error(prog, "EmitStreamVertex(n) and EndStreamPrimitive(n) "
+                      "with n>0 requires point output");
+      }
+   }
 }
 
 
@@ -2556,6 +2673,9 @@  link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
 	 ;
    }
 
+   /* Check and validate stream emissions in geometry shaders */
+   validate_geometry_shader_emissions(ctx, prog);
+
    /* Mark all generic shader inputs and outputs as unpaired. */
    for (unsigned i = MESA_SHADER_VERTEX; i <= MESA_SHADER_FRAGMENT; i++) {
       if (prog->_LinkedShaders[i] != NULL) {

Comments

On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote:
> Check if non-zero streams are used. Fail to link if emitting to unsupported
> streams or emitting to non-zero streams with output type other than GL_POINTS.
> ---
>  src/glsl/linker.cpp | 148 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 134 insertions(+), 14 deletions(-)
> 
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 0b6a716..f8ff138 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -250,31 +250,100 @@ public:
>     }
>  };
>  
> -
>  /**
> - * Visitor that determines whether or not a shader uses ir_end_primitive.
> + * Visitor that determines the highest stream id to which a (geometry) shader
> + * emits vertices. It also checks whether End{Stream}Primitive is ever called.
>   */
> -class find_end_primitive_visitor : public ir_hierarchical_visitor {
> +class find_emit_vertex_visitor : public ir_hierarchical_visitor {
>  public:
> -   find_end_primitive_visitor()
> -      : found(false)
> +   find_emit_vertex_visitor(int max_allowed)
> +      : max_stream_allowed(max_allowed),
> +        invalid_stream_id(0),
> +        invalid_stream_id_from_emit_vertex(false),
> +        end_primitive_found(false),
> +        uses_non_zero_stream(false)
>     {
>        /* empty */
>     }
>  
> -   virtual ir_visitor_status visit(ir_end_primitive *)
> +   virtual ir_visitor_status visit_leave(ir_emit_vertex *ir)
>     {
> -      found = true;
> -      return visit_stop;
> +      int stream_id = ir->stream_id();
> +
> +      if (stream_id < 0) {
> +         invalid_stream_id = stream_id;
> +         invalid_stream_id_from_emit_vertex = true;
> +         return visit_stop;
> +      }
> +
> +      if (stream_id > max_stream_allowed) {
> +         invalid_stream_id = stream_id;
> +         invalid_stream_id_from_emit_vertex = true;
> +         return visit_stop;
> +      }
> +
> +      if (stream_id != 0)
> +         uses_non_zero_stream = true;
> +
> +      return visit_continue;
>     }
>  
> -   bool end_primitive_found()
> +   virtual ir_visitor_status visit_leave(ir_end_primitive *ir)
>     {
> -      return found;
> +      end_primitive_found = true;
> +
> +      int stream_id = ir->stream_id();
> +
> +      if (stream_id < 0) {
> +         invalid_stream_id = stream_id;
> +         invalid_stream_id_from_emit_vertex = false;
> +         return visit_stop;
> +      }
> +
> +      if (stream_id > max_stream_allowed) {
> +         invalid_stream_id = stream_id;
> +         invalid_stream_id_from_emit_vertex = false;
> +         return visit_stop;
> +      }
> +
> +      if (stream_id != 0)
> +         uses_non_zero_stream = true;
> +
> +      return visit_continue;
> +   }
> +
> +   bool error()
> +   {
> +      return invalid_stream_id != 0;
> +   }
> +
> +   const char *error_func()
> +   {
> +      return invalid_stream_id_from_emit_vertex ?
> +         "EmitStreamVertex" : "EndStreamPrimitive";
> +   }
> +
> +   int error_stream()
> +   {
> +      return invalid_stream_id;
> +   }
> +
> +   bool uses_streams()
> +   {
> +      return uses_non_zero_stream;
> +   }
> +
> +   bool uses_end_primitive()
> +   {
> +      return end_primitive_found;
>     }
>  
>  private:
> -   bool found;
> +   int max_stream_allowed;
> +   int invalid_stream_id;
> +   bool invalid_stream_id_from_emit_vertex;
> +   bool end_primitive_found;
> +   bool uses_non_zero_stream;
>  };
>  
>  } /* anonymous namespace */
> @@ -551,10 +620,58 @@ validate_geometry_shader_executable(struct gl_shader_program *prog,
>  
>     analyze_clip_usage(prog, shader, &prog->Geom.UsesClipDistance,
>                        &prog->Geom.ClipDistanceArraySize);
> +}
> +
> +/**
> + * Check if geometry shaders emit to non-zero streams and do corresponding
> + * validations.
> + */
> +static void
> +validate_geometry_shader_emissions(struct gl_context *ctx,
> +                                   struct gl_shader_program *prog)
> +{
> +   if (prog->_LinkedShaders[MESA_SHADER_GEOMETRY] != NULL) {
> +      find_emit_vertex_visitor emit_vertex(ctx->Const.MaxVertexStreams - 1);
> +      emit_vertex.run(prog->_LinkedShaders[MESA_SHADER_GEOMETRY]->ir);
> +      if (emit_vertex.error()) {
> +         linker_error(prog, "Invalid call %s(%d). Accepted values for the "
> +                      "stream parameter are in the range [0, %d].",
> +                      emit_vertex.error_func(),
> +                      emit_vertex.error_stream(),
> +                      ctx->Const.MaxVertexStreams - 1);
> +      }
> +      prog->Geom.UsesStreams = emit_vertex.uses_streams();
> +      prog->Geom.UsesEndPrimitive = emit_vertex.uses_end_primitive();
>  
> -   find_end_primitive_visitor end_primitive;
> -   end_primitive.run(shader->ir);
> -   prog->Geom.UsesEndPrimitive = end_primitive.end_primitive_found();
> +      /* From the ARB_gpu_shader5 spec:
> +       *
> +       *   "Multiple vertex streams are supported only if the output primitive
> +       *    type is declared to be "points".  A program will fail to link if it
> +       *    contains a geometry shader calling EmitStreamVertex() or
> +       *    EndStreamPrimitive() if its output primitive type is not "points".
> +       *
> +       * However, in the same spec:
> +       *
> +       *   "The function EmitVertex() is equivalent to calling EmitStreamVertex()
> +       *    with <stream> set to zero."
> +       *
> +       * And:
> +       *
> +       *   "The function EndPrimitive() is equivalent to calling
> +       *    EndStreamPrimitive() with <stream> set to zero."
> +       *
> +       * Since we can call EmitVertex() and EndPrimitive() when we output
> +       * primitives other than points, calling EmitStreamVertex(0) or
> +       * EmitEndPrimitive(0) should not produce errors. This it also what Nvidia
> +       * does. Currently we only set prog->Geom.UsesStreams to TRUE when
> +       * EmitStreamVertex() or EmitEndPrimitive() are called with a non-zero
> +       * stream.

Does AMD also behave this way?  If so, I can submit a spec bug to make
these explicitly legal.  Otherwise, I think we should not allow it.  We
probably ought to check Apple and the Intel Windows driver too...

> +       */
> +      if (prog->Geom.UsesStreams && prog->Geom.OutputType != GL_POINTS) {
> +         linker_error(prog, "EmitStreamVertex(n) and EndStreamPrimitive(n) "
> +                      "with n>0 requires point output");
> +      }
> +   }
>  }
>  
>  
> @@ -2556,6 +2673,9 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>  	 ;
>     }
>  
> +   /* Check and validate stream emissions in geometry shaders */
> +   validate_geometry_shader_emissions(ctx, prog);
> +
>     /* Mark all generic shader inputs and outputs as unpaired. */
>     for (unsigned i = MESA_SHADER_VERTEX; i <= MESA_SHADER_FRAGMENT; i++) {
>        if (prog->_LinkedShaders[i] != NULL) {
>
On Wed, 2014-06-18 at 13:38 -0700, Ian Romanick wrote:
> On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote:
> > Check if non-zero streams are used. Fail to link if emitting to unsupported
> > streams or emitting to non-zero streams with output type other than GL_POINTS.
> > ---
> >  src/glsl/linker.cpp | 148 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 134 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > index 0b6a716..f8ff138 100644
> > --- a/src/glsl/linker.cpp
> > +++ b/src/glsl/linker.cpp
> > @@ -250,31 +250,100 @@ public:
> >     }
> >  };
> >  
> > -
> >  /**
> > - * Visitor that determines whether or not a shader uses ir_end_primitive.
> > + * Visitor that determines the highest stream id to which a (geometry) shader
> > + * emits vertices. It also checks whether End{Stream}Primitive is ever called.
> >   */
> > -class find_end_primitive_visitor : public ir_hierarchical_visitor {
> > +class find_emit_vertex_visitor : public ir_hierarchical_visitor {
> >  public:
> > -   find_end_primitive_visitor()
> > -      : found(false)
> > +   find_emit_vertex_visitor(int max_allowed)
> > +      : max_stream_allowed(max_allowed),
> > +        invalid_stream_id(0),
> > +        invalid_stream_id_from_emit_vertex(false),
> > +        end_primitive_found(false),
> > +        uses_non_zero_stream(false)
> >     {
> >        /* empty */
> >     }
> >  
> > -   virtual ir_visitor_status visit(ir_end_primitive *)
> > +   virtual ir_visitor_status visit_leave(ir_emit_vertex *ir)
> >     {
> > -      found = true;
> > -      return visit_stop;
> > +      int stream_id = ir->stream_id();
> > +
> > +      if (stream_id < 0) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = true;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id > max_stream_allowed) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = true;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id != 0)
> > +         uses_non_zero_stream = true;
> > +
> > +      return visit_continue;
> >     }
> >  
> > -   bool end_primitive_found()
> > +   virtual ir_visitor_status visit_leave(ir_end_primitive *ir)
> >     {
> > -      return found;
> > +      end_primitive_found = true;
> > +
> > +      int stream_id = ir->stream_id();
> > +
> > +      if (stream_id < 0) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = false;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id > max_stream_allowed) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = false;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id != 0)
> > +         uses_non_zero_stream = true;
> > +
> > +      return visit_continue;
> > +   }
> > +
> > +   bool error()
> > +   {
> > +      return invalid_stream_id != 0;
> > +   }
> > +
> > +   const char *error_func()
> > +   {
> > +      return invalid_stream_id_from_emit_vertex ?
> > +         "EmitStreamVertex" : "EndStreamPrimitive";
> > +   }
> > +
> > +   int error_stream()
> > +   {
> > +      return invalid_stream_id;
> > +   }
> > +
> > +   bool uses_streams()
> > +   {
> > +      return uses_non_zero_stream;
> > +   }
> > +
> > +   bool uses_end_primitive()
> > +   {
> > +      return end_primitive_found;
> >     }
> >  
> >  private:
> > -   bool found;
> > +   int max_stream_allowed;
> > +   int invalid_stream_id;
> > +   bool invalid_stream_id_from_emit_vertex;
> > +   bool end_primitive_found;
> > +   bool uses_non_zero_stream;
> >  };
> >  
> >  } /* anonymous namespace */
> > @@ -551,10 +620,58 @@ validate_geometry_shader_executable(struct gl_shader_program *prog,
> >  
> >     analyze_clip_usage(prog, shader, &prog->Geom.UsesClipDistance,
> >                        &prog->Geom.ClipDistanceArraySize);
> > +}
> > +
> > +/**
> > + * Check if geometry shaders emit to non-zero streams and do corresponding
> > + * validations.
> > + */
> > +static void
> > +validate_geometry_shader_emissions(struct gl_context *ctx,
> > +                                   struct gl_shader_program *prog)
> > +{
> > +   if (prog->_LinkedShaders[MESA_SHADER_GEOMETRY] != NULL) {
> > +      find_emit_vertex_visitor emit_vertex(ctx->Const.MaxVertexStreams - 1);
> > +      emit_vertex.run(prog->_LinkedShaders[MESA_SHADER_GEOMETRY]->ir);
> > +      if (emit_vertex.error()) {
> > +         linker_error(prog, "Invalid call %s(%d). Accepted values for the "
> > +                      "stream parameter are in the range [0, %d].",
> > +                      emit_vertex.error_func(),
> > +                      emit_vertex.error_stream(),
> > +                      ctx->Const.MaxVertexStreams - 1);
> > +      }
> > +      prog->Geom.UsesStreams = emit_vertex.uses_streams();
> > +      prog->Geom.UsesEndPrimitive = emit_vertex.uses_end_primitive();
> >  
> > -   find_end_primitive_visitor end_primitive;
> > -   end_primitive.run(shader->ir);
> > -   prog->Geom.UsesEndPrimitive = end_primitive.end_primitive_found();
> > +      /* From the ARB_gpu_shader5 spec:
> > +       *
> > +       *   "Multiple vertex streams are supported only if the output primitive
> > +       *    type is declared to be "points".  A program will fail to link if it
> > +       *    contains a geometry shader calling EmitStreamVertex() or
> > +       *    EndStreamPrimitive() if its output primitive type is not "points".
> > +       *
> > +       * However, in the same spec:
> > +       *
> > +       *   "The function EmitVertex() is equivalent to calling EmitStreamVertex()
> > +       *    with <stream> set to zero."
> > +       *
> > +       * And:
> > +       *
> > +       *   "The function EndPrimitive() is equivalent to calling
> > +       *    EndStreamPrimitive() with <stream> set to zero."
> > +       *
> > +       * Since we can call EmitVertex() and EndPrimitive() when we output
> > +       * primitives other than points, calling EmitStreamVertex(0) or
> > +       * EmitEndPrimitive(0) should not produce errors. This it also what Nvidia
> > +       * does. Currently we only set prog->Geom.UsesStreams to TRUE when
> > +       * EmitStreamVertex() or EmitEndPrimitive() are called with a non-zero
> > +       * stream.
> 
> Does AMD also behave this way?  If so, I can submit a spec bug to make
> these explicitly legal.  Otherwise, I think we should not allow it.  We
> probably ought to check Apple and the Intel Windows driver too...

I am not sure that I have access to all of these systems, but let me
see.

FWIW, the spec also says this:

"The function EmitVertex() is equivalent to calling EmitStreamVertex()
with <stream> set to zero."

And they can't be equivalent if ones goes through checks that the other
does not...

> > +       */
> > +      if (prog->Geom.UsesStreams && prog->Geom.OutputType != GL_POINTS) {
> > +         linker_error(prog, "EmitStreamVertex(n) and EndStreamPrimitive(n) "
> > +                      "with n>0 requires point output");
> > +      }
> > +   }
> >  }
> >  
> >  
> > @@ -2556,6 +2673,9 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> >  	 ;
> >     }
> >  
> > +   /* Check and validate stream emissions in geometry shaders */
> > +   validate_geometry_shader_emissions(ctx, prog);
> > +
> >     /* Mark all generic shader inputs and outputs as unpaired. */
> >     for (unsigned i = MESA_SHADER_VERTEX; i <= MESA_SHADER_FRAGMENT; i++) {
> >        if (prog->_LinkedShaders[i] != NULL) {
> > 
> 
>
On Wed, 2014-06-18 at 13:38 -0700, Ian Romanick wrote:
> On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote:
(...)
> > +      /* From the ARB_gpu_shader5 spec:
> > +       *
> > +       *   "Multiple vertex streams are supported only if the output primitive
> > +       *    type is declared to be "points".  A program will fail to link if it
> > +       *    contains a geometry shader calling EmitStreamVertex() or
> > +       *    EndStreamPrimitive() if its output primitive type is not "points".
> > +       *
> > +       * However, in the same spec:
> > +       *
> > +       *   "The function EmitVertex() is equivalent to calling EmitStreamVertex()
> > +       *    with <stream> set to zero."
> > +       *
> > +       * And:
> > +       *
> > +       *   "The function EndPrimitive() is equivalent to calling
> > +       *    EndStreamPrimitive() with <stream> set to zero."
> > +       *
> > +       * Since we can call EmitVertex() and EndPrimitive() when we output
> > +       * primitives other than points, calling EmitStreamVertex(0) or
> > +       * EmitEndPrimitive(0) should not produce errors. This it also what Nvidia
> > +       * does. Currently we only set prog->Geom.UsesStreams to TRUE when
> > +       * EmitStreamVertex() or EmitEndPrimitive() are called with a non-zero
> > +       * stream.
> 
> Does AMD also behave this way?  If so, I can submit a spec bug to make
> these explicitly legal.  Otherwise, I think we should not allow it.  We
> probably ought to check Apple and the Intel Windows driver too...

I could test this on the proprietary AMD Linux driver:

A geometry shader with output triangle_strip using EmitStreamVertex(0)
and EmitEndPrimitive(0) works fine: no link errors, correct output.

A geometry shader with output triangle_strip using EmitStreamVertex(1)
does not produce a link error (which is wrong by the spec) and produces
incorrect output (blank screen).

A geometry shader with output triangle_strip using EmitStreamVertex(0)
that also calls EmitStreamPrimitive(1) does not produce a link error
(which is wrong by the spec) and produces correct output (the end stream
primitive on stream 1 is really a no-op since there are no vertices
emitted on that stream).

So I'd say that AMD is not respecting the spec too much here, but it
looks like using EmitStreamVertex(0) or EndStreamPrimitive(0) with
output types other than points works on both Nvidia and AMD proprietary
drivers in the end.

So Ian, what do you think we should do?

Iago
On Friday, June 20, 2014 08:51:14 AM Iago Toral wrote:
> On Wed, 2014-06-18 at 13:38 -0700, Ian Romanick wrote:
> > On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote:
> (...)
> > > +      /* From the ARB_gpu_shader5 spec:
> > > +       *
> > > +       *   "Multiple vertex streams are supported only if the output 
primitive
> > > +       *    type is declared to be "points".  A program will fail to 
link if it
> > > +       *    contains a geometry shader calling EmitStreamVertex() or
> > > +       *    EndStreamPrimitive() if its output primitive type is not 
"points".
> > > +       *
> > > +       * However, in the same spec:
> > > +       *
> > > +       *   "The function EmitVertex() is equivalent to calling 
EmitStreamVertex()
> > > +       *    with <stream> set to zero."
> > > +       *
> > > +       * And:
> > > +       *
> > > +       *   "The function EndPrimitive() is equivalent to calling
> > > +       *    EndStreamPrimitive() with <stream> set to zero."
> > > +       *
> > > +       * Since we can call EmitVertex() and EndPrimitive() when we 
output
> > > +       * primitives other than points, calling EmitStreamVertex(0) or
> > > +       * EmitEndPrimitive(0) should not produce errors. This it also 
what Nvidia
> > > +       * does. Currently we only set prog->Geom.UsesStreams to TRUE 
when
> > > +       * EmitStreamVertex() or EmitEndPrimitive() are called with a 
non-zero
> > > +       * stream.
> > 
> > Does AMD also behave this way?  If so, I can submit a spec bug to make
> > these explicitly legal.  Otherwise, I think we should not allow it.  We
> > probably ought to check Apple and the Intel Windows driver too...
> 
> I could test this on the proprietary AMD Linux driver:
> 
> A geometry shader with output triangle_strip using EmitStreamVertex(0)
> and EmitEndPrimitive(0) works fine: no link errors, correct output.
> 
> A geometry shader with output triangle_strip using EmitStreamVertex(1)
> does not produce a link error (which is wrong by the spec) and produces
> incorrect output (blank screen).
> 
> A geometry shader with output triangle_strip using EmitStreamVertex(0)
> that also calls EmitStreamPrimitive(1) does not produce a link error
> (which is wrong by the spec) and produces correct output (the end stream
> primitive on stream 1 is really a no-op since there are no vertices
> emitted on that stream).
> 
> So I'd say that AMD is not respecting the spec too much here, but it
> looks like using EmitStreamVertex(0) or EndStreamPrimitive(0) with
> output types other than points works on both Nvidia and AMD proprietary
> drivers in the end.
> 
> So Ian, what do you think we should do?
> 
> Iago

For what it's worth, AMD also supports writing lines and triangles to multiple 
streams, not just points:

http://www.opengl.org/registry/specs/AMD/transform_feedback3_lines_triangles.txt

--Ken