On Wed, Oct 03, 2012 at 09:31:39PM -0700, Kenneth Graunke wrote:
> 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.
Okay. I'll generate two patches as described above.
>
> > Signed-off-by: Oliver McFadden <oliver.mcfad...@linux.intel.com>
> > CC: Brian Paul <bri...@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.
Okay, thanks. Indeed I have a habit of fscking up these API/GLSL
version checks.
>
> > + "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.
I will make this a separate patch, last in the series so that we can
choose to apply it or not.
Thanks for the feedback!
--
Oliver McFadden.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
