[Mesa-dev] [PATCH v4] mesa/teximage: accept ASTC formats for 3D texture specification

[Chad Versace]

On Thu 13 Aug 2015, Nanley Chery wrote:
> From: Nanley Chery <nanley.g.chery at intel.com>
>
> The ASTC spec was revised as follows:
>
>    Revision 2, April 28, 2015 - added CompressedTex{Sub,}Image3D to
>    commands accepting ASTC format tokens in the New Tokens section [...].
>
> Support only exists in the HDR submode:
>
>    Add a second new column "3D Tex." which is empty for all non-ASTC
>    formats. If only the LDR profile is supported by the implementation,
>    this column is also empty for all ASTC formats. If both the LDR and HDR
>    profiles are supported only, this column is checked for all ASTC
>    formats.
>
> LDR-only systems should generate an INVALID_OPERATION error when
> attempting to call CompressedTexImage3D with the TEXTURE_3D target.
>
> v2. return the proper error for LDR-only systems.
> v3. update is_astc_format().
> v4. use _mesa_is_astc_format().
>
> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> ---
>  src/mesa/main/teximage.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index f372059..a8c4eff 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -1829,6 +1829,8 @@ _mesa_target_can_be_compressed(const struct gl_context *ctx, GLenum target,
>        case GL_COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT:
>           return ctx->Extensions.ARB_texture_compression_bptc;
>        default:
> +         if (_mesa_is_astc_format(intFormat))
> +            return ctx->Extensions.KHR_texture_compression_astc_hdr;
>           return GL_FALSE;
>        }
>     default:
> @@ -2330,6 +2332,13 @@ texture_error_check( struct gl_context *ctx,
>     return GL_FALSE;
>  }
>
> +static inline bool
> +is_astc_format(const struct gl_context *ctx, GLenum internalFormat)
> +{
> +      return ctx->Extensions.KHR_texture_compression_astc_ldr &&
> +             _mesa_is_astc_format(internalFormat);
> +}
> +
>
>  /**
>   * Error checking for glCompressedTexImage[123]D().
> @@ -2358,7 +2367,18 @@ compressed_texture_error_check(struct gl_context *ctx, GLint dimensions,
>         *     CompressedTexImage3D will generate an INVALID_OPERATION error if
>         *     target is not TEXTURE_2D_ARRAY."
>         */
> -      error = _mesa_is_desktop_gl(ctx) ? GL_INVALID_ENUM : GL_INVALID_OPERATION;
> +      /* From the KHR_texture_compression_astc_hdr spec:
> +       *
> +       *     'An INVALID_OPERATION error is generated by CompressedTexImage3D
> +       *      if <internalformat> is TEXTURE_CUBE_MAP_ARRAY and the
> +       *      "Cube Map Array" column of table 8.19 is *not* checked, or if
> +       *      <internalformat> is TEXTURE_3D and the "3D Tex." column of table
> +       *      8.19 is *not* checked'
> +       *
> +       * The instances of <internalformat> above should say <target>.
> +       */
> +      error = _mesa_is_desktop_gl(ctx) && !is_astc_format(ctx, internalFormat) ?
> +              GL_INVALID_ENUM : GL_INVALID_OPERATION;

Pre-patch, this block of code worried me. Post-patch, it worries me
a little bit more.

Pre-patch, the comments insist that specific errors must be emitted for
specific combinations of formats (ETC2) and texture targets, but the code that
selects the error code never checks for ETC2 nor inspects the texture target.
Moreoever, the code also inspects the GL context flavor, but the comment says
nothing about that. (I assume the code infers "if ETC2 then GLES").

The total situation is worse, though: Throughout Mesa, the same comment and
logic is duplicated at some of the calls to _mesa_target_can_be_compressed()
but missing at others.

I can't confirm that your patch is correct, because I can't confirm that
the original code is correct. And I can't confirm that you've updated
all the calls to _mesa_target_can_be_compressed() that require updating.

I suggest that, before this patch, you add a patch that:

  * Adds a new out parameter to _mesa_target_can_be_compressed that returns the
    correct GL error, if any.

    GLboolean
    _mesa_target_can_be_compressed(const struct gl_context *ctx,
                                    GLenum target,
                                    GLenum intFormat, GLenum *error);

  * And moves the comments that explain the special case for ETC2 into
     _mesa_target_can_be_compressed().