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

[Tapani Pälli]

On 06/09/2015 11:25 AM, Kenneth Graunke wrote:
> 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;
>     }

OK, thanks for checking this. I'll send another version following the style.


>>
>>         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;
>>