[Mesa-dev] [PATCH 2/3] mesa: helper for checking renderbuffer sample count

[Brian Paul]

On 02/17/2013 03:35 AM, Chris Forbes wrote:
> Pulls the checking of the sample count into a helper function, and
> extends the existing logic to include the interactions with both
> ARB_texture_multisample and ARB_internalformat_query.
>
> _mesa_check_sample_count() checks a desired sample count against a
> a combination of target/internalformat, and returns the error enum
> to be produced, if any. Unfortunately the conditions are messy and the
> errors vary:
>
> On p205 of the GL3.1 spec:
>
>     "... or if samples is greater than MAX_SAMPLES, then the error
>     INVALID_VALUE is generated."
>
> Or with ARB_texture_multisample (or GL3.2):
>
>     "... or if<samples>  is greater than the value of MAX_SAMPLES, then
>     the error INVALID_VALUE is generated.
>     If<internalformat>  is a signed or unsigned integer format and
>     <samples>  is greater than the value of MAX_INTEGER_SAMPLES, then the
>     error INVALID_OPERATION is generated.
>
> Or with ARB_internalformat_query (or GL4.2):
>
>     "If<samples>  is greater than the maximum number of samples supported
>     for<internalformat>  then the error INVALID_OPERATION is generated
>     (see GetInternalformativ in section 6.X)."
>
> Signed-off-by: Chris Forbes<chrisf at ijw.co.nz>
> ---
>   src/mesa/main/fbobject.c | 43 +++++++++++++++++++++++++++++++++++++++----
>   src/mesa/main/fbobject.h |  4 ++++
>   2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index c89e728..50339c2 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1449,6 +1449,35 @@ invalidate_rb(GLuint key, void *data, void *userData)
>   }
>
>

There should be a comment on this function explaining what it does and 
what it returns.


> +GLenum
> +_mesa_check_sample_count(struct gl_context *ctx, GLenum target,
> +                         GLenum internalFormat, GLsizei samples)

Can ctx be const-qualified?  It looks like the context shouldn't be 
changed by this function.


> +{
> +   /* If ARB_internalformat_query is supported, then treat its highest returned sample
> +    * count as the absolute maximum for this format; it is allowed to exceed MAX_SAMPLES.
> +    */
> +   if (ctx->Extensions.ARB_internalformat_query) {
> +      GLint buffer[16];
> +      int count = ctx->Driver.QuerySamplesForFormat(ctx, target, internalFormat, buffer);
> +      int limit = count ? buffer[0] : -1;
> +
> +      return (samples>  limit) ? GL_INVALID_OPERATION : GL_NO_ERROR;
> +   }
> +
> +   /* If ARB_texture_multisample is supported, we have separate limits for
> +    * integer formats.
> +    */
> +
> +   if (ctx->Extensions.ARB_texture_multisample) {
> +      if (_mesa_is_enum_format_integer(internalFormat))
> +         return samples>  ctx->Const.MaxIntegerSamples ? GL_INVALID_OPERATION : GL_NO_ERROR;
> +   }
> +
> +   /* No more specific limit is available, so just use MAX_SAMPLES */
> +   return samples>  ctx->Const.MaxSamples ? GL_INVALID_VALUE : GL_NO_ERROR;
> +}
> +
> +
>   /** sentinal value, see below */
>   #define NO_SAMPLES 1000
>
> @@ -1509,10 +1538,16 @@ renderbuffer_storage(GLenum target, GLenum internalFormat,
>         /* NumSamples == 0 indicates non-multisampling */
>         samples = 0;
>      }
> -   else if (samples>  (GLsizei) ctx->Const.MaxSamples) {
> -      /* note: driver may choose to use more samples than what's requested */
> -      _mesa_error(ctx, GL_INVALID_VALUE, "%s(samples)", func);
> -      return;
> +
> +   {  /* check the sample count;
> +       * note: driver may choose to use more samples than what's requested
> +       */
> +      GLenum sample_count_error = _mesa_check_sample_count(ctx, target,
> +            internalFormat, samples);
> +      if (sample_count_error != GL_NO_ERROR) {
> +         _mesa_error(ctx, sample_count_error, "%s(samples)", func);
> +         return;
> +      }

Minor style nit: I'd simply declare sample_count_error at the top of 
the function and get rid of the {}-block.


>      }
>
>      rb = ctx->CurrentRenderbuffer;
> diff --git a/src/mesa/main/fbobject.h b/src/mesa/main/fbobject.h
> index 9207f59..9adee3a 100644
> --- a/src/mesa/main/fbobject.h
> +++ b/src/mesa/main/fbobject.h
> @@ -123,6 +123,10 @@ _mesa_is_legal_color_format(const struct gl_context *ctx, GLenum baseFormat);
>   extern GLenum
>   _mesa_base_fbo_format(struct gl_context *ctx, GLenum internalFormat);
>
> +extern GLenum
> +_mesa_check_sample_count(struct gl_context *ctx, GLenum target,
> +                   GLenum internalFormat, GLsizei samples);
> +
>   extern GLboolean GLAPIENTRY
>   _mesa_IsRenderbuffer(GLuint renderbuffer);
>