[Mesa-dev] [RFC] [PATCH 1/1] mesa: setup_glsl_generate_mipmap(): meta program compile failed

[Kenneth Graunke]

On 10/03/2012 05:59 PM, Oliver McFadden wrote:
> Discovered while attempting to run GLBenchMark 2.5 with test
> GLB25_TriangleTexVertexLitTestC24Z16 on an ES2.0 context.

Ouch :( Nice catch.

> I have changed the `if' condition in setup_glsl_generate_mipmap() such
> that it is consistent throughout the function.  This required swapping
> the code blocks of the `if-else' statement.

This kind of change is really tangential to the purpose of your patch: 
fixing glGenerateMipmaps on ES2.  It makes it much harder to follow, as 
the reader has to separate out the unnecessary code motion (that looks 
like changes to the shader programs) from the actual bug fix.

Consistency is nice though.  Ideally, we'd prefer two patches: one to 
fix the bug, and another to refactor/move code for consistency.

> Signed-off-by: Oliver McFadden <oliver.mcfadden at linux.intel.com>
> CC: Brian Paul <brianp at vmware.com>

Sounds like a candidate for 9.0.

> ---
> I seem to have a habit of getting these GL context version/GLSL version checks
> not quite correct, so I'd like to have a review/idiot-check here.  :-)
>
>   src/mesa/drivers/common/meta.c |   48 ++++++++++++++++++++-------------------
>   1 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
> index d0bb5e0..6381841 100644
> --- a/src/mesa/drivers/common/meta.c
> +++ b/src/mesa/drivers/common/meta.c
> @@ -3074,44 +3074,45 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>      const char *extension_mode;
>      GLuint vs, fs;
>
> -   if (ctx->Const.GLSLVersion < 130) {
> +   if ((_mesa_is_desktop_gl(ctx) && ctx->Const.GLSLVersion >= 130) ||
> +       _mesa_is_gles3(ctx)) {
>         vs_source =
> -         "attribute vec2 position;\n"
> -         "attribute vec3 textureCoords;\n"
> -         "varying vec3 texCoords;\n"
> +         "#version 130\n"

This won't work for ES 3.  It doesn't support #version 130, only 
"#version 300 es" or the backwards compatible "#version 100".

Letting it fall through to the old-style program *should* work, since 
without ES should interpret shaders without a #version line as 100 (an 
ES 2.0 shader).

You are correct that ES 2 wants the "attribute", "varying", and 
old-style "texture2D" functions.  I believe changing it to:

_mesa_is_desktop_gl(ctx) && ctx->Const.GLSLVersion >= 130

or if you want to leave the conditionals the same:

ctx->API == API_OPENGLES2 || ctx->Const.GLSLVersion < 130

would be correct.

> +         "in vec2 position;\n"
> +         "in vec3 textureCoords;\n"
> +         "out vec3 texCoords;\n"
>            "void main()\n"
>            "{\n"
>            "   texCoords = textureCoords;\n"
>            "   gl_Position = vec4(position, 0.0, 1.0);\n"
>            "}\n";
>         fs_template =
> -         "#extension GL_EXT_texture_array : %s\n"
> +         "#version 130\n"
>            "uniform %s texSampler;\n"
> -         "varying vec3 texCoords;\n"
> +         "in vec3 texCoords;\n"
> +         "out %s out_color;\n"
> +         "\n"
>            "void main()\n"
>            "{\n"
> -         "   gl_FragColor = %s(texSampler, %s);\n"
> +         "   out_color = texture(texSampler, %s);\n"
>            "}\n";
>      } else {
>         vs_source =
> -         "#version 130\n"
> -         "in vec2 position;\n"
> -         "in vec3 textureCoords;\n"
> -         "out vec3 texCoords;\n"
> +         "attribute vec2 position;\n"
> +         "attribute vec3 textureCoords;\n"
> +         "varying vec3 texCoords;\n"
>            "void main()\n"
>            "{\n"
>            "   texCoords = textureCoords;\n"
>            "   gl_Position = vec4(position, 0.0, 1.0);\n"
>            "}\n";
>         fs_template =
> -         "#version 130\n"
> +         "#extension GL_EXT_texture_array : %s\n"
>            "uniform %s texSampler;\n"
> -         "in vec3 texCoords;\n"
> -         "out %s out_color;\n"
> -         "\n"
> +         "varying vec3 texCoords;\n"
>            "void main()\n"
>            "{\n"
> -         "   out_color = texture(texSampler, %s);\n"
> +         "   gl_FragColor = %s(texSampler, %s);\n"
>            "}\n";
>       }
>
> @@ -3143,7 +3144,13 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>
>      mem_ctx = ralloc_context(NULL);
>
> -   if (ctx->Const.GLSLVersion < 130) {
> +   if ((_mesa_is_desktop_gl(ctx) && ctx->Const.GLSLVersion >= 130) ||
> +       _mesa_is_gles3(ctx)) {
> +      fs_source = ralloc_asprintf(mem_ctx, fs_template,
> +                                  sampler->type, "vec4",
> +                                  sampler->texcoords);
> +   }
> +   else {
>         extension_mode = ((target == GL_TEXTURE_1D_ARRAY) ||
>                           (target == GL_TEXTURE_2D_ARRAY)) ?
>                          "require" : "disable";
> @@ -3152,11 +3159,6 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>                                     extension_mode, sampler->type,
>                                     sampler->func, sampler->texcoords);
>      }
> -   else {
> -      fs_source = ralloc_asprintf(mem_ctx, fs_template,
> -                                  sampler->type, "vec4",
> -                                  sampler->texcoords);
> -   }
>
>      vs = compile_shader_with_debug(ctx, GL_VERTEX_SHADER, vs_source);
>      fs = compile_shader_with_debug(ctx, GL_FRAGMENT_SHADER, fs_source);
> @@ -3175,7 +3177,7 @@ setup_glsl_generate_mipmap(struct gl_context *ctx,
>      ralloc_free(mem_ctx);
>
>      if ((_mesa_is_desktop_gl(ctx) && ctx->Const.GLSLVersion >= 130) ||
> -       _mesa_is_gles3(ctx)){
> +       _mesa_is_gles3(ctx)) {
>         vs = compile_shader_with_debug(ctx, GL_VERTEX_SHADER, vs_int_source);
>         fs = compile_shader_with_debug(ctx, GL_FRAGMENT_SHADER, fs_int_source);

I'll note here that ES 3 is broken for integer textures, since (again) 
it doesn't support #version 130.  However, we can fix that separately or 
even defer that until later as part of the ongoing ES 3 work.