[Mesa-dev] [PATCH] gles2: a stub implementation for GL_EXT_discard_framebuffer

[Ian Romanick]

On 02/11/2013 02:38 PM, Eric Anholt wrote:
> Tapani Pälli <tapani.palli at intel.com> writes:
>
>> This patch implements a stub for GL_EXT_discard_framebuffer with
>> required checks listed by the extension specification. This extension
>> is required by GLBenchmark 2.5 when compiled with OpenGL ES 2.0
>> as the rendering backend.
>
> Are there piglit tests for this coming?
>
>> ---
>>   src/mapi/glapi/gen/es_EXT.xml                 | 13 +++++++++
>>   src/mesa/drivers/dri/intel/intel_extensions.c |  1 +
>>   src/mesa/main/dd.h                            |  4 ++-
>>   src/mesa/main/extensions.c                    |  1 +
>>   src/mesa/main/fbobject.c                      | 40 +++++++++++++++++++++++++++
>>   src/mesa/main/fbobject.h                      |  4 +++
>>   src/mesa/main/mtypes.h                        |  1 +
>>   src/mesa/main/tests/dispatch_sanity.cpp       |  1 +
>>   8 files changed, 64 insertions(+), 1 deletion(-)
>
>> diff --git a/src/mesa/drivers/dri/intel/intel_extensions.c b/src/mesa/drivers/dri/intel/intel_extensions.c
>> index bf5e2b5..a182139 100755
>> --- a/src/mesa/drivers/dri/intel/intel_extensions.c
>> +++ b/src/mesa/drivers/dri/intel/intel_extensions.c
>> @@ -65,6 +65,7 @@ intelInitExtensions(struct gl_context *ctx)
>>      ctx->Extensions.EXT_blend_equation_separate = true;
>>      ctx->Extensions.EXT_blend_func_separate = true;
>>      ctx->Extensions.EXT_blend_minmax = true;
>> +   ctx->Extensions.EXT_discard_framebuffer = true;
>>      ctx->Extensions.EXT_framebuffer_blit = true;
>>      ctx->Extensions.EXT_framebuffer_object = true;
>>      ctx->Extensions.EXT_fog_coord = true;
>
>> diff --git a/src/mesa/main/extensions.c b/src/mesa/main/extensions.c
>> index 04435e0..f7bd6f2 100644
>> --- a/src/mesa/main/extensions.c
>> +++ b/src/mesa/main/extensions.c
>> @@ -168,6 +168,7 @@ static const struct extension extension_table[] = {
>>      { "GL_EXT_blend_color",                         o(EXT_blend_color),                         GLL,            1995 },
>>      { "GL_EXT_blend_equation_separate",             o(EXT_blend_equation_separate),             GL,             2003 },
>>      { "GL_EXT_blend_func_separate",                 o(EXT_blend_func_separate),                 GLL,            1999 },
>> +   { "GL_EXT_discard_framebuffer",                 o(EXT_discard_framebuffer),                             ES2, 2009 },
>>      { "GL_EXT_blend_minmax",                        o(EXT_blend_minmax),                        GLL | ES1 | ES2, 1995 },
>>      { "GL_EXT_blend_subtract",                      o(dummy_true),                              GLL,            1995 },
>>      { "GL_EXT_clip_volume_hint",                    o(EXT_clip_volume_hint),                    GL,             1996 },
>
> Is there any reason not to just expose this stub extension everywhere,
> since it requires nothing from the driver author?

Since it requires OES_framebuffer_object (and nothing more?), it could 
use the same bit.  Also, the spec says it can be exposed on ES1.

>> +
>> +void GLAPIENTRY
>> +_mesa_DiscardFramebufferEXT(GLenum target, GLsizei numAttachments,
>> +                            const GLenum *attachments)
>> +{
>> +   struct gl_framebuffer *fb;
>> +   GLint i;
>> +
>> +   GET_CURRENT_CONTEXT(ctx);
>> +
>> +   if (target != GL_FRAMEBUFFER) {
>> +      _mesa_error(ctx, GL_INVALID_ENUM, "%s(target)", __func__);
>> +      return;
>> +   }
>> +
>> +   if (numAttachments < 0) {
>> +      _mesa_error(ctx, GL_INVALID_VALUE,
>> +                  "%s(numAttachments < 0)", __func__);
>> +      return;
>> +   }
>> +
>> +   for(i = 0; i < numAttachments; i++) {
>> +      switch (attachments[i]) {
>> +         case GL_COLOR:
>> +         case GL_DEPTH:
>> +         case GL_STENCIL:
>> +         case GL_COLOR_ATTACHMENT0:
>> +         case GL_DEPTH_ATTACHMENT:
>> +         case GL_STENCIL_ATTACHMENT:
>> +         break;
>
> break should be indented, like return is.  Usually "case" statements are
> not indented from the switch though.
>
>> +         default:
>> +            _mesa_error(ctx, GL_INVALID_ENUM, "%s(attachment)", __func__);
>> +            return;
>> +      }
>> +   }
>
> This looks like not the right error behavior -- you're supposed to
> reject non-(COLOR/DEPTH/STENCIL) on _mesa_is_winsys_fbo() and
> non-(COLOR_ATT0/DEPTH_ATT/STENCIL_ATT) otherwise.