[Mesa-dev] [PATCH v3] mesa: GL_EXT_texture_norm16 extension plumbing

Tapani Pälli tapani.palli at intel.com
Sat Apr 21 10:29:59 UTC 2018


On 21.04.2018 10:25, Tapani Pälli wrote:
> 
> 
> On 20.04.2018 19:15, Ilia Mirkin wrote:
>> On Fri, Apr 20, 2018 at 11:33 AM, Tapani Pälli 
>> <tapani.palli at intel.com> wrote:
>>> Patch enables use of short and unsigned short data for texture uploads,
>>> rendering and reading of framebuffers within the restrictions specified
>>> in GL_EXT_texture_norm16 spec.
>>>
>>> Patch also enables those 16bit format layout qualifiers listed in
>>> GL_NV_image_formats that depend on EXT_texture_norm16.
>>>
>>> v2: expose extension with dummy_true
>>>      fix layout qualifier map changes (Ilia Mirkin)
>>> v3: use _mesa_has_EXT_texture_norm16, other fixes
>>>      and cleanup (Ilia Mirkin)
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> ---
>>>
>>> Note, Piglit test "ext_texture_norm16-render" passes with these changes.
>>>
>>>   src/compiler/glsl/glsl_parser.yy | 12 ++++----
>>>   src/mesa/main/extensions_table.h |  1 +
>>>   src/mesa/main/genmipmap.c        |  2 +-
>>>   src/mesa/main/glformats.c        | 61 
>>> +++++++++++++++++++++++++++++++++++++++-
>>>   src/mesa/main/glformats.h        |  3 +-
>>>   src/mesa/main/readpix.c          | 14 +++++++--
>>>   src/mesa/main/shaderimage.c      |  7 ++---
>>>   7 files changed, 85 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/glsl_parser.yy 
>>> b/src/compiler/glsl/glsl_parser.yy
>>> index e5ea41d4df..b4951a258a 100644
>>> --- a/src/compiler/glsl/glsl_parser.yy
>>> +++ b/src/compiler/glsl/glsl_parser.yy
>>> @@ -1340,18 +1340,18 @@ layout_qualifier_id:
>>>                  { "r32i", GL_R32I, GLSL_TYPE_INT, 130, 310, false },
>>>                  { "r16i", GL_R16I, GLSL_TYPE_INT, 130, 0, true },
>>>                  { "r8i", GL_R8I, GLSL_TYPE_INT, 130, 0, true },
>>> -               { "rgba16", GL_RGBA16, GLSL_TYPE_FLOAT, 130, 0, false },
>>> +               { "rgba16", GL_RGBA16, GLSL_TYPE_FLOAT, 130, 0, true },
>>>                  { "rgb10_a2", GL_RGB10_A2, GLSL_TYPE_FLOAT, 130, 0, 
>>> true },
>>>                  { "rgba8", GL_RGBA8, GLSL_TYPE_FLOAT, 130, 310, 
>>> false },
>>> -               { "rg16", GL_RG16, GLSL_TYPE_FLOAT, 130, 0, false },
>>> +               { "rg16", GL_RG16, GLSL_TYPE_FLOAT, 130, 0, true },
>>>                  { "rg8", GL_RG8, GLSL_TYPE_FLOAT, 130, 0, true },
>>> -               { "r16", GL_R16, GLSL_TYPE_FLOAT, 130, 0, false },
>>> +               { "r16", GL_R16, GLSL_TYPE_FLOAT, 130, 0, true },
>>>                  { "r8", GL_R8, GLSL_TYPE_FLOAT, 130, 0, true },
>>> -               { "rgba16_snorm", GL_RGBA16_SNORM, GLSL_TYPE_FLOAT, 
>>> 130, 0, false },
>>> +               { "rgba16_snorm", GL_RGBA16_SNORM, GLSL_TYPE_FLOAT, 
>>> 130, 0, true },
>>>                  { "rgba8_snorm", GL_RGBA8_SNORM, GLSL_TYPE_FLOAT, 
>>> 130, 310, false },
>>> -               { "rg16_snorm", GL_RG16_SNORM, GLSL_TYPE_FLOAT, 130, 
>>> 0, false },
>>> +               { "rg16_snorm", GL_RG16_SNORM, GLSL_TYPE_FLOAT, 130, 
>>> 0, true },
>>>                  { "rg8_snorm", GL_RG8_SNORM, GLSL_TYPE_FLOAT, 130, 
>>> 0, true },
>>> -               { "r16_snorm", GL_R16_SNORM, GLSL_TYPE_FLOAT, 130, 0, 
>>> false },
>>> +               { "r16_snorm", GL_R16_SNORM, GLSL_TYPE_FLOAT, 130, 0, 
>>> true },
>>>                  { "r8_snorm", GL_R8_SNORM, GLSL_TYPE_FLOAT, 130, 0, 
>>> true }
>>>               };
>>>
>>> diff --git a/src/mesa/main/extensions_table.h 
>>> b/src/mesa/main/extensions_table.h
>>> index 492f7c3d20..aec17750d5 100644
>>> --- a/src/mesa/main/extensions_table.h
>>> +++ b/src/mesa/main/extensions_table.h
>>> @@ -283,6 +283,7 @@ EXT(EXT_texture_format_BGRA8888             , 
>>> dummy_true
>>>   EXT(EXT_texture_integer                     , 
>>> EXT_texture_integer                    , GLL, GLC,  x ,  x , 2006)
>>>   EXT(EXT_texture_lod_bias                    , 
>>> dummy_true                             , GLL,  x , ES1,  x , 1999)
>>>   EXT(EXT_texture_mirror_clamp                , 
>>> EXT_texture_mirror_clamp               , GLL, GLC,  x ,  x , 2004)
>>> +EXT(EXT_texture_norm16                      , 
>>> dummy_true                             ,  x ,  x ,  x ,  31, 2014)
>>>   EXT(EXT_texture_object                      , 
>>> dummy_true                             , GLL,  x ,  x ,  x , 1995)
>>>   EXT(EXT_texture_rectangle                   , 
>>> NV_texture_rectangle                   , GLL,  x ,  x ,  x , 2004)
>>>   EXT(EXT_texture_rg                          , 
>>> ARB_texture_rg                         ,  x ,  x ,  x , ES2, 2011)
>>> diff --git a/src/mesa/main/genmipmap.c b/src/mesa/main/genmipmap.c
>>> index 488c32f810..fd20ea2806 100644
>>> --- a/src/mesa/main/genmipmap.c
>>> +++ b/src/mesa/main/genmipmap.c
>>> @@ -93,7 +93,7 @@ 
>>> _mesa_is_valid_generate_texture_mipmap_internalformat(struct 
>>> gl_context *ctx,
>>>                internalformat == GL_LUMINANCE_ALPHA ||
>>>                internalformat == GL_LUMINANCE || internalformat == 
>>> GL_ALPHA ||
>>>                internalformat == GL_BGRA_EXT ||
>>> -             (_mesa_is_es3_color_renderable(internalformat) &&
>>> +             (_mesa_is_es3_color_renderable(ctx, internalformat) &&
>>>                 _mesa_is_es3_texture_filterable(ctx, internalformat));
>>>      }
>>>
>>> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
>>> index 1e797c24c2..5473ec02df 100644
>>> --- a/src/mesa/main/glformats.c
>>> +++ b/src/mesa/main/glformats.c
>>> @@ -2857,6 +2857,17 @@ _mesa_es3_error_check_format_and_type(const 
>>> struct gl_context *ctx,
>>>               return GL_INVALID_OPERATION;
>>>            break;
>>>
>>> +      case GL_UNSIGNED_SHORT:
>>> +         if (!_mesa_has_EXT_texture_norm16(ctx) || internalFormat != 
>>> GL_RGBA16)
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>> +      case GL_SHORT:
>>> +         if (!_mesa_has_EXT_texture_norm16(ctx) ||
>>> +             internalFormat != GL_RGBA16_SNORM)
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>>         case GL_UNSIGNED_SHORT_4_4_4_4:
>>>            switch (internalFormat) {
>>>            case GL_RGBA:
>>> @@ -2984,6 +2995,17 @@ _mesa_es3_error_check_format_and_type(const 
>>> struct gl_context *ctx,
>>>               return GL_INVALID_OPERATION;
>>>            break;
>>>
>>> +      case GL_UNSIGNED_SHORT:
>>> +         if (!_mesa_has_EXT_texture_norm16(ctx) || internalFormat != 
>>> GL_RGB16)
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>> +      case GL_SHORT:
>>> +         if (!_mesa_has_EXT_texture_norm16(ctx) ||
>>> +             internalFormat != GL_RGB16_SNORM)
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>>         case GL_UNSIGNED_SHORT_5_6_5:
>>>            switch (internalFormat) {
>>>            case GL_RGB:
>>> @@ -3115,6 +3137,17 @@ _mesa_es3_error_check_format_and_type(const 
>>> struct gl_context *ctx,
>>>               return GL_INVALID_OPERATION;
>>>            break;
>>>
>>> +      case GL_UNSIGNED_SHORT:
>>> +         if (!_mesa_has_EXT_texture_norm16(ctx) || internalFormat != 
>>> GL_RG16)
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>> +      case GL_SHORT:
>>> +         if (!_mesa_has_EXT_texture_norm16(ctx) ||
>>> +             internalFormat != GL_RG16_SNORM)
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>>         case GL_HALF_FLOAT:
>>>         case GL_HALF_FLOAT_OES:
>>>            switch (internalFormat) {
>>> @@ -3205,6 +3238,16 @@ _mesa_es3_error_check_format_and_type(const 
>>> struct gl_context *ctx,
>>>               return GL_INVALID_OPERATION;
>>>            break;
>>>
>>> +      case GL_UNSIGNED_SHORT:
>>> +         if (ctx->Version < 31 || internalFormat != GL_R16)
>>
>> !_mesa_has_EXT_texture_norm16(ctx) (and below)
> 
> will fix
> 
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>> +      case GL_SHORT:
>>> +         if (ctx->Version < 31 || internalFormat != GL_R16_SNORM)
>>> +            return GL_INVALID_OPERATION;
>>> +         break;
>>> +
>>>         case GL_HALF_FLOAT:
>>>         case GL_HALF_FLOAT_OES:
>>>            switch (internalFormat) {
>>> @@ -3704,7 +3747,8 @@ _mesa_tex_format_from_format_and_type(const 
>>> struct gl_context *ctx,
>>>    * is marked "Color Renderable" in Table 8.10 of the ES 3.2 
>>> specification.
>>>    */
>>>   bool
>>> -_mesa_is_es3_color_renderable(GLenum internal_format)
>>> +_mesa_is_es3_color_renderable(const struct gl_context *ctx,
>>> +                              GLenum internal_format)
>>>   {
>>>      switch (internal_format) {
>>>      case GL_R8:
>>> @@ -3743,6 +3787,11 @@ _mesa_is_es3_color_renderable(GLenum 
>>> internal_format)
>>>      case GL_RGBA32I:
>>>      case GL_RGBA32UI:
>>>         return true;
>>> +   case GL_R16:
>>> +   case GL_RG16:
>>> +   case GL_RGBA16:
>>> +      if (_mesa_has_EXT_texture_norm16(ctx))
>>> +         return true;
>>>      default:
>>>         return false;
>>>      }
>>> @@ -3778,6 +3827,16 @@ _mesa_is_es3_texture_filterable(const struct 
>>> gl_context *ctx,
>>>      case GL_R11F_G11F_B10F:
>>>      case GL_RGB9_E5:
>>>         return true;
>>> +   case GL_R16:
>>> +   case GL_R16_SNORM:
>>> +   case GL_RG16:
>>> +   case GL_RG16_SNORM:
>>> +   case GL_RGB16:
>>> +   case GL_RGB16_SNORM:
>>> +   case GL_RGBA16:
>>> +   case GL_RGBA16_SNORM:
>>> +      if (_mesa_has_EXT_texture_norm16(ctx))
>>> +         return true;
>>
>> Here and above, you fall through if it's false. Is that desirable?
> 
> yes, then we will return false like should happen without the extension

I'll change these to return _mesa_has_EXT_texture_norm16(ctx)!

Thanks for your patience on the review, I've been typing this while 
traveling so there's been quite a bit of silly mistakes :/


>>>      case GL_R32F:
>>>      case GL_RG32F:
>>>      case GL_RGB32F:
>>> diff --git a/src/mesa/main/glformats.h b/src/mesa/main/glformats.h
>>> index 844f1e270c..5a21317159 100644
>>> --- a/src/mesa/main/glformats.h
>>> +++ b/src/mesa/main/glformats.h
>>> @@ -155,7 +155,8 @@ _mesa_tex_format_from_format_and_type(const 
>>> struct gl_context *ctx,
>>>                                         GLenum gl_format, GLenum type);
>>>
>>>   extern bool
>>> -_mesa_is_es3_color_renderable(GLenum internal_format);
>>> +_mesa_is_es3_color_renderable(const struct gl_context *ctx,
>>> +                              GLenum internal_format);
>>>
>>>   extern bool
>>>   _mesa_is_es3_texture_filterable(const struct gl_context *ctx,
>>> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
>>> index 6ce340ddf9..ff0647327e 100644
>>> --- a/src/mesa/main/readpix.c
>>> +++ b/src/mesa/main/readpix.c
>>> @@ -901,7 +901,7 @@ _mesa_readpixels(struct gl_context *ctx,
>>>
>>>
>>>   static GLenum
>>> -read_pixels_es3_error_check(GLenum format, GLenum type,
>>> +read_pixels_es3_error_check(struct gl_context *ctx, GLenum format, 
>>> GLenum type,
>>>                               const struct gl_renderbuffer *rb)
>>>   {
>>>      const GLenum internalFormat = rb->InternalFormat;
>>> @@ -927,6 +927,16 @@ read_pixels_es3_error_check(GLenum format, 
>>> GLenum type,
>>>            return GL_NO_ERROR;
>>>         if (internalFormat == GL_RGB10_A2UI && type == GL_UNSIGNED_BYTE)
>>>            return GL_NO_ERROR;
>>> +      if (type == GL_UNSIGNED_SHORT) {
>>> +         switch (internalFormat) {
>>> +         case GL_R16:
>>> +         case GL_RG16:
>>> +         case GL_RGB16:
>>> +         case GL_RGBA16:
>>> +         if (_mesa_has_EXT_texture_norm16(ctx))
>>
>> This should be indented in. (i.e. the if should be indented, the case
>> should remain as-is.)
> 
> will fix
> 
>> With these minor fixes, this is
>>
>> Acked-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>
>> (I haven't done a thorough review of what all this should affect, but
>> the changes that you do have seem fine.)
> 
> Thanks, it is about glTeximage2D, glGenerateMipmap, glReadPixels, 
> rendering to fbo with these formats and GL_EXT_copy_image. Those are 
> tested by the Piglit test I've sent. Parser changes get tested by 
> NV_image_formats tests in Piglit.
> 
> 
>>> +            return GL_NO_ERROR;
>>> +         }
>>> +      }
>>>         break;
>>>      case GL_BGRA:
>>>         /* GL_EXT_read_format_bgra */
>>> @@ -1049,7 +1059,7 @@ read_pixels(GLint x, GLint y, GLsizei width, 
>>> GLsizei height, GLenum format,
>>>                  }
>>>               }
>>>            } else {
>>> -            err = read_pixels_es3_error_check(format, type, rb);
>>> +            err = read_pixels_es3_error_check(ctx, format, type, rb);
>>>            }
>>>
>>>            if (err != GL_NO_ERROR) {
>>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
>>> index 596eadd4f8..ac4985d8b7 100644
>>> --- a/src/mesa/main/shaderimage.c
>>> +++ b/src/mesa/main/shaderimage.c
>>> @@ -430,9 +430,8 @@ _mesa_is_shader_image_format_supported(const 
>>> struct gl_context *ctx,
>>>       * ARB_shader_image_load_store extension, c.f. table 3.21 of the 
>>> OpenGL 4.2
>>>       * specification.
>>>       *
>>> -    * These can be supported by GLES 3.1 with GL_NV_image_formats &
>>> -    * GL_EXT_texture_norm16 extensions but we don't have support for 
>>> the
>>> -    * latter in Mesa yet.
>>> +    * Following formats are supported by GLES 3.1 with 
>>> GL_NV_image_formats &
>>> +    * GL_EXT_texture_norm16 extensions.
>>>       */
>>>      case GL_RGBA16:
>>>      case GL_RGBA16_SNORM:
>>> @@ -440,7 +439,7 @@ _mesa_is_shader_image_format_supported(const 
>>> struct gl_context *ctx,
>>>      case GL_RG16_SNORM:
>>>      case GL_R16:
>>>      case GL_R16_SNORM:
>>> -      return _mesa_is_desktop_gl(ctx);
>>> +      return (_mesa_is_desktop_gl(ctx) || 
>>> _mesa_has_EXT_texture_norm16(ctx));
>>>
>>>      default:
>>>         return false;
>>> -- 
>>> 2.13.6
>>>


More information about the mesa-dev mailing list