Use the same number for n_tex_coord_attribs in all linked shaders

On GLES2, we need to specify an array size for the texture coord
varying array. Previously this size would be decided in one of the
following ways:

- For generated vertex shaders it is always the number of layers in
  the pipeline.

- For generated fragment shaders it is the highest sampled texture
  unit in the pipeline or the number of attributes supplied by the
  primitive, whichever is higher.

- For user shaders it is usually the number of attributes supplied by
  the primitive. However, if the application tries to compile the
  shader and query the result before using it, it will always be at
  least 4.

These shaders can quite easily end up with different values for the
declaration which makes it fail to link. This patch changes it so that
all of the shaders are generated with the maximum of the number of
texture attributes supplied by the primitive and the number of layers
in the pipeline. If this value changes then the shaders are
regenerated, including user shaders. That way all of the shaders will
always have the same value.

https://bugzilla.gnome.org/show_bug.cgi?id=662184

Reviewed-by: Robert Bragg <robert@linux.intel.com>

7 files changed, 64 insertions, 50 deletions

diff --git a/cogl/cogl-pipeline-fragend-glsl.c b/cogl/cogl-pipeline-fragend-glsl.c
index 47282368..488070be 100644
--- a/cogl/cogl-pipeline-fragend-glsl.c
+++ b/cogl/cogl-pipeline-fragend-glsl.c
@@ -103,6 +103,11 @@ typedef struct
      program changes then we may need to redecide whether to generate
      a shader at all */
   unsigned int user_program_age;
+
+  /* The number of tex coord attributes that the shader was generated
+     for. If this changes on GLES2 then we need to regenerate the
+     shader */
+  int n_tex_coord_attribs;
 } CoglPipelineShaderState;
 
 static CoglUserDataKey shader_state_key;
@@ -268,9 +273,14 @@ _cogl_pipeline_fragend_glsl_start (CoglPipeline *pipeline,
     {
       /* If we already have a valid GLSL shader then we don't need to
          generate a new one. However if there's a user program and it
-         has changed since the last link then we do need a new shader */
-      if (user_program == NULL ||
-          shader_state->user_program_age == user_program->age)
+         has changed since the last link then we do need a new
+         shader. If the number of tex coord attribs changes on GLES2
+         then we need to regenerate the shader with a different boiler
+         plate */
+      if ((user_program == NULL ||
+           shader_state->user_program_age == user_program->age)
+          && (ctx->driver != COGL_DRIVER_GLES2 ||
+              shader_state->n_tex_coord_attribs == n_tex_coord_attribs))
         return TRUE;
 
       /* We need to recreate the shader so destroy the existing one */
@@ -285,6 +295,8 @@ _cogl_pipeline_fragend_glsl_start (CoglPipeline *pipeline,
   if (user_program)
     shader_state->user_program_age = user_program->age;
 
+  shader_state->n_tex_coord_attribs = n_tex_coord_attribs;
+
   /* If the user program contains a fragment shader then we don't need
      to generate one */
   if (user_program &&
@@ -873,8 +885,6 @@ _cogl_pipeline_fragend_glsl_end (CoglPipeline *pipeline,
       GLint lengths[2];
       GLint compile_status;
       GLuint shader;
-      int n_tex_coord_attribs = 0;
-      int i, n_layers;
 
       COGL_STATIC_COUNTER (fragend_glsl_compile_counter,
                            "glsl fragment compile counter",
@@ -921,15 +931,9 @@ _cogl_pipeline_fragend_glsl_end (CoglPipeline *pipeline,
       lengths[1] = shader_state->source->len;
       source_strings[1] = shader_state->source->str;
 
-      /* Find the highest texture unit that is sampled to pass as the
-         number of texture coordinate attributes */
-      n_layers = cogl_pipeline_get_n_layers (pipeline);
-      for (i = 0; i < n_layers; i++)
-        if (shader_state->unit_state[i].sampled)
-          n_tex_coord_attribs = i + 1;
-
       _cogl_shader_set_source_with_boilerplate (shader, GL_FRAGMENT_SHADER,
-                                                n_tex_coord_attribs,
+                                                shader_state
+                                                ->n_tex_coord_attribs,
                                                 2, /* count */
                                                 source_strings, lengths);
 
diff --git a/cogl/cogl-pipeline-opengl.c b/cogl/cogl-pipeline-opengl.c
index cc9ca1b6..92d75ea2 100644
--- a/cogl/cogl-pipeline-opengl.c
+++ b/cogl/cogl-pipeline-opengl.c
@@ -1200,6 +1200,16 @@ _cogl_pipeline_flush_gl_state (CoglPipeline *pipeline,
   else
     layer_differences = NULL;
 
+  /* Make sure we generate the texture coordinate array to be at least
+     the number of layers. This is important because the vertend will
+     try to pass along the corresponding varying for each layer
+     regardless of whether the fragment shader is actually using
+     it. Also it is possible that the application is assuming that if
+     the attribute isn't passed then it will default to 0,0. This is
+     what test-cogl-primitive does */
+  if (n_layers > n_tex_coord_attribs)
+    n_tex_coord_attribs = n_layers;
+
   /* First flush everything that's the same regardless of which
    * pipeline backend is being used...
    *
@@ -1300,7 +1310,8 @@ _cogl_pipeline_flush_gl_state (CoglPipeline *pipeline,
        * scratch buffers here... */
       if (G_UNLIKELY (!vertend->start (pipeline,
                                        n_layers,
-                                       pipelines_difference)))
+                                       pipelines_difference,
+                                       n_tex_coord_attribs)))
         continue;
 
       state.vertend = vertend;
diff --git a/cogl/cogl-pipeline-private.h b/cogl/cogl-pipeline-private.h
index b8965c29..57f3503e 100644
--- a/cogl/cogl-pipeline-private.h
+++ b/cogl/cogl-pipeline-private.h
@@ -542,7 +542,8 @@ typedef struct _CoglPipelineVertend
 {
   gboolean (*start) (CoglPipeline *pipeline,
                      int n_layers,
-                     unsigned long pipelines_difference);
+                     unsigned long pipelines_difference,
+                     int n_tex_coord_attribs);
   gboolean (*add_layer) (CoglPipeline *pipeline,
                          CoglPipelineLayer *layer,
                          unsigned long layers_difference);
diff --git a/cogl/cogl-pipeline-progend-glsl.c b/cogl/cogl-pipeline-progend-glsl.c
index c82bc76f..0d83ed60 100644
--- a/cogl/cogl-pipeline-progend-glsl.c
+++ b/cogl/cogl-pipeline-progend-glsl.c
@@ -106,7 +106,7 @@ typedef struct
    * array of texture coordinate varyings, but to know how to emit the
    * declaration we need to know how many texture coordinate
    * attributes are in use.  The boilerplate also needs to be changed
-   * if this increases. */
+   * if this changes. */
   int n_tex_coord_attribs;
 
 #ifdef HAVE_COGL_GLES2
@@ -620,11 +620,12 @@ _cogl_pipeline_progend_glsl_end (CoglPipeline *pipeline,
    * need to relink
    *
    * Also if the number of texture coordinate attributes in use has
-   * increased, then delete the program so we can prepend a new
+   * changed, then delete the program so we can prepend a new
    * _cogl_tex_coord[] varying array declaration. */
-  if (program_state->program && user_program &&
-      (user_program->age != program_state->user_program_age ||
-       n_tex_coord_attribs > program_state->n_tex_coord_attribs))
+  if ((program_state->program && user_program &&
+       user_program->age != program_state->user_program_age) ||
+      (ctx->driver == COGL_DRIVER_GLES2 &&
+       n_tex_coord_attribs != program_state->n_tex_coord_attribs))
     {
       GE( ctx, glDeleteProgram (program_state->program) );
       program_state->program = 0;
@@ -640,23 +641,6 @@ _cogl_pipeline_progend_glsl_end (CoglPipeline *pipeline,
       /* Attach all of the shader from the user program */
       if (user_program)
         {
-          if (program_state->n_tex_coord_attribs > n_tex_coord_attribs)
-            n_tex_coord_attribs = program_state->n_tex_coord_attribs;
-
-#ifdef HAVE_COGL_GLES2
-          /* Find the largest count of texture coordinate attributes
-           * associated with each of the shaders so we can ensure a consistent
-           * _cogl_tex_coord[] array declaration across all of the shaders.*/
-          if (ctx->driver == COGL_DRIVER_GLES2 &&
-              user_program)
-            for (l = user_program->attached_shaders; l; l = l->next)
-              {
-                CoglShader *shader = l->data;
-                n_tex_coord_attribs = MAX (shader->n_tex_coord_attribs,
-                                           n_tex_coord_attribs);
-              }
-#endif
-
           for (l = user_program->attached_shaders; l; l = l->next)
             {
               CoglShader *shader = l->data;
diff --git a/cogl/cogl-pipeline-vertend-fixed.c b/cogl/cogl-pipeline-vertend-fixed.c
index 3275a068..477b2af9 100644
--- a/cogl/cogl-pipeline-vertend-fixed.c
+++ b/cogl/cogl-pipeline-vertend-fixed.c
@@ -46,7 +46,8 @@ const CoglPipelineVertend _cogl_pipeline_fixed_vertend;
 static gboolean
 _cogl_pipeline_vertend_fixed_start (CoglPipeline *pipeline,
                                     int n_layers,
-                                    unsigned long pipelines_difference)
+                                    unsigned long pipelines_difference,
+                                    int n_tex_coord_attribs)
 {
   CoglProgram *user_program;
 
diff --git a/cogl/cogl-pipeline-vertend-glsl.c b/cogl/cogl-pipeline-vertend-glsl.c
index a3a460d3..34655737 100644
--- a/cogl/cogl-pipeline-vertend-glsl.c
+++ b/cogl/cogl-pipeline-vertend-glsl.c
@@ -56,6 +56,11 @@ typedef struct
      program changes then we may need to redecide whether to generate
      a shader at all */
   unsigned int user_program_age;
+
+  /* The number of tex coord attributes that the shader was generated
+     for. If this changes on GLES2 then we need to regenerate the
+     shader */
+  int n_tex_coord_attribs;
 } CoglPipelineShaderState;
 
 static CoglUserDataKey shader_state_key;
@@ -126,7 +131,8 @@ _cogl_pipeline_vertend_glsl_get_shader (CoglPipeline *pipeline)
 static gboolean
 _cogl_pipeline_vertend_glsl_start (CoglPipeline *pipeline,
                                    int n_layers,
-                                   unsigned long pipelines_difference)
+                                   unsigned long pipelines_difference,
+                                   int n_tex_coord_attribs)
 {
   CoglPipelineShaderState *shader_state;
   CoglPipeline *template_pipeline = NULL;
@@ -203,9 +209,14 @@ _cogl_pipeline_vertend_glsl_start (CoglPipeline *pipeline,
     {
       /* If we already have a valid GLSL shader then we don't need to
          generate a new one. However if there's a user program and it
-         has changed since the last link then we do need a new shader */
-      if (user_program == NULL ||
-          shader_state->user_program_age == user_program->age)
+         has changed since the last link then we do need a new
+         shader. If the number of tex coord attribs changes on GLES2
+         then we need to regenerate the shader with a different boiler
+         plate */
+      if ((user_program == NULL ||
+           shader_state->user_program_age == user_program->age)
+          && (ctx->driver != COGL_DRIVER_GLES2 ||
+              shader_state->n_tex_coord_attribs == n_tex_coord_attribs))
         return TRUE;
 
       /* We need to recreate the shader so destroy the existing one */
@@ -220,6 +231,8 @@ _cogl_pipeline_vertend_glsl_start (CoglPipeline *pipeline,
   if (user_program)
     shader_state->user_program_age = user_program->age;
 
+  shader_state->n_tex_coord_attribs = n_tex_coord_attribs;
+
   /* If the user program contains a vertex shader then we don't need
      to generate one */
   if (user_program &&
@@ -336,7 +349,6 @@ _cogl_pipeline_vertend_glsl_end (CoglPipeline *pipeline,
       GLint lengths[2];
       GLint compile_status;
       GLuint shader;
-      int n_layers;
 
       COGL_STATIC_COUNTER (vertend_glsl_compile_counter,
                            "glsl vertex compile counter",
@@ -359,10 +371,9 @@ _cogl_pipeline_vertend_glsl_end (CoglPipeline *pipeline,
       lengths[1] = shader_state->source->len;
       source_strings[1] = shader_state->source->str;
 
-      n_layers = cogl_pipeline_get_n_layers (pipeline);
-
       _cogl_shader_set_source_with_boilerplate (shader, GL_VERTEX_SHADER,
-                                                n_layers,
+                                                shader_state
+                                                ->n_tex_coord_attribs,
                                                 2, /* count */
                                                 source_strings, lengths);
 
diff --git a/cogl/cogl-shader.c b/cogl/cogl-shader.c
index b94f23d4..72d95416 100644
--- a/cogl/cogl-shader.c
+++ b/cogl/cogl-shader.c
@@ -342,7 +342,7 @@ _cogl_shader_compile_real (CoglHandle handle,
 #ifdef HAVE_COGL_GLES2
           &&
           (ctx->driver != COGL_DRIVER_GLES2 ||
-           shader->n_tex_coord_attribs >= n_tex_coord_attribs)
+           shader->n_tex_coord_attribs == n_tex_coord_attribs)
 #endif
          )
         return;
@@ -424,9 +424,11 @@ cogl_shader_get_info_log (CoglHandle handle)
        * Here we force an early compile if the user is interested in
        * log information to increase the chance that the log will be
        * useful! We have to guess the number of texture coordinate
-       * attributes that may be used (normally less than 4) since that
-       * affects the boilerplate.
-       */
+       * attributes that may be used since that affects the
+       * boilerplate. We use four so that the shader will still
+       * compile if the user is using more than one
+       * layer. Unfortunately this is likely to end up causing it to
+       * be compiled again when we know the actual number of layers */
       if (!shader->gl_handle)
         _cogl_shader_compile_real (handle, 4);