[Mesa-dev] [PATCH v2] mesa: add GL_RED, GL_RG support for floating point textures

[Kenneth Graunke]

On Tuesday, June 09, 2015 09:53:42 AM Tapani Pälli wrote:
> Mesa supports EXT_texture_rg and OES_texture_float. This patch adds
> support for using unsized enums GL_RED and GL_RG for floating point
> targets and writes proper checks for internalformat when format is
> GL_RED or GL_RG and type is of GL_FLOAT or GL_HALF_FLOAT.
> 
> Later, internalformat will get adjusted by adjust_for_oes_float_texture
> after these checks.
> 
> v2: simplify to check vs supported enums
> 
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90748
> ---
>  src/mesa/main/glformats.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 

This looks right to me.  A couple comments inline...

> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
> index 057a5d1..119956e 100644
> --- a/src/mesa/main/glformats.c
> +++ b/src/mesa/main/glformats.c
> @@ -2295,20 +2295,32 @@ _mesa_es3_error_check_format_and_type(const struct gl_context *ctx,
>              return GL_INVALID_OPERATION;
>           break;
>  
> +      case GL_HALF_FLOAT_OES:
>        case GL_HALF_FLOAT:
> -         if (internalFormat != GL_RG16F)
> -            return GL_INVALID_OPERATION;
> -         break;
> +      switch (internalFormat) {
> +         case GL_RG:
> +            if (!ctx->Extensions.ARB_texture_rg ||
> +                !ctx->Extensions.OES_texture_half_float)
> +               break;
> +         case GL_RG16F:
> +            return GL_NO_ERROR;
> +      }
> +
> +      return GL_INVALID_OPERATION;
>  
>        case GL_FLOAT:
>           switch (internalFormat) {
> +         case GL_RG:
> +            if (!ctx->Extensions.ARB_texture_rg ||
> +                !ctx->Extensions.OES_texture_float)
> +               break;
>           case GL_RG16F:
>           case GL_RG32F:
> +            return GL_NO_ERROR;
>              break;
>           default:
>              return GL_INVALID_OPERATION;
>           }
> -         break;

The rest of the function breaks out of the switch statement when a
format is allowed, and falls through to the default in order to return
GL_INVALID_OPERATION.  This doesn't fit that style.

I think you could instead do:

case GL_FLOAT:
   switch (internalFormat) {
   case GL_RG:
      if (!ctx->Extensions.ARB_texture_rg ||
          !ctx->Extensions.OES_texture_float)
         return GL_INVALID_OPERATION;
      break;
   case GL_RG16F:
   case GL_RG32F:
      break;
   default:
      return GL_INVALID_OPERATION;
   }

which just adds a new case and doesn't alter the rest of the control
flow.  Alternatively, you could do this, which fits the existing style
pretty well:

case GL_FLOAT:
   switch (internalFormat) {
   case GL_RG16F:
   case GL_RG32F:
      break;
   case GL_RG:
      if (ctx->Extensions.ARB_texture_rg &&
          ctx->Extensions.OES_texture_float)
         break;
      /* fallthrough */
   default:
      return GL_INVALID_OPERATION;
   }

>  
>        default:
>           return GL_INVALID_OPERATION;
> @@ -2365,19 +2377,34 @@ _mesa_es3_error_check_format_and_type(const struct gl_context *ctx,
>           break;
>  
>        case GL_HALF_FLOAT:
> -         if (internalFormat != GL_R16F)
> -            return GL_INVALID_OPERATION;
> -         break;
> +      case GL_HALF_FLOAT_OES:
> +
> +         switch (internalFormat) {
> +         case GL_RG:
> +         case GL_RED:
> +            if (!ctx->Extensions.ARB_texture_rg ||
> +                !ctx->Extensions.OES_texture_half_float)
> +               break;
> +         case GL_R16F:
> +            return GL_NO_ERROR;
> +         }
> +
> +         return GL_INVALID_OPERATION;
>  
>        case GL_FLOAT:
> +
>           switch (internalFormat) {
> +         case GL_RED:
> +            if (!ctx->Extensions.ARB_texture_rg ||
> +                !ctx->Extensions.OES_texture_float)
> +               break;
>           case GL_R16F:
>           case GL_R32F:
> +            return GL_NO_ERROR;
>              break;
>           default:
>              return GL_INVALID_OPERATION;
>           }
> -         break;
>  
>        default:
>           return GL_INVALID_OPERATION;
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150609/047240c8/attachment.sig>