[Mesa-dev] [PATCH v3] mesa: improve ARB_copy_image internal format compat check
Jason Ekstrand
jason at jlekstrand.net
Sat Mar 7 15:05:26 PST 2015
On Sat, Mar 7, 2015 at 2:10 PM, Sean Burke <leftmostcat at gmail.com> wrote:
> The memory layout of compatible internal formats may differ in bytes per
> block, so TexFormat is not a reliable measure of compatibility. For
> example,
> GL_RGB8 and GL_RGB8UI are compatible formats, but GL_RGB8 may be laid out
> in
> memory as B8G8R8X8. If GL_RGB8UI has a 3 byte-per-block memory layout, the
> existing compatibility check will fail.
>
> Additionally, the current check allows any two compressed textures which
> share
> block size to be used, which is in violation of the spec.
>
Rather than simply saying "which is in violation of the spec", perhaps it
would be better to say "whereas the spec gives an explicit table of
compatible formats". I think the original check actually worked because
mesa doesn't support anything not in that table. However, doing it
explicitly is probably better.
>
> v2: Use a switch instead of array iteration for block class and show the
> correct GL error when internal formats are mismatched.
> v3: Include spec citations for new compatibility checks, rearrange check
> order to ensure that compressed, view-compatible formats return the
> correct result, and make style fixes. Original commit message amended
> for clarity.
> ---
> src/mesa/main/copyimage.c | 148
> +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 127 insertions(+), 21 deletions(-)
>
> diff --git a/src/mesa/main/copyimage.c b/src/mesa/main/copyimage.c
> index 455929d..9fc9c82 100644
> --- a/src/mesa/main/copyimage.c
> +++ b/src/mesa/main/copyimage.c
> @@ -33,6 +33,12 @@
> #include "texobj.h"
> #include "fbobject.h"
> #include "textureview.h"
> +#include "glformats.h"
> +
> +enum mesa_block_class {
> + BLOCK_CLASS_128_BITS,
> + BLOCK_CLASS_64_BITS
> +};
>
> static bool
> prepare_target(struct gl_context *ctx, GLuint name, GLenum *target, int
> level,
> @@ -253,6 +259,121 @@ check_region_bounds(struct gl_context *ctx,
> struct gl_texture_image *tex_image,
> return true;
> }
>
> +static bool
> +compressed_format_compatible(struct gl_context *ctx,
> + GLenum compressedFormat, GLenum otherFormat)
> +{
> + enum mesa_block_class compressedClass, otherClass;
> +
> + /* Two view-incompatible compressed formats are never compatible. */
> + if (_mesa_is_compressed_format(ctx, otherFormat)) {
> + return false;
> + }
> +
> + /* Table 4.X.1 (Compatible internal formats for copying between
> compressed
> + * and uncompressed internal formats)
> + *
> ------------------------------------------------------------------------
> + * | Texel / | Uncompressed |
> |
> + * | Block | internal format | Compressed internal format
> |
> + * | size | |
> |
> + *
> ------------------------------------------------------------------------
> + * | 128-bit | RGBA32UI, | COMPRESSED_RGBA_S3TC_DXT3_EXT,
> |
> + * | | RGBA32I, |
> COMPRESSED_SRGB_ALPHA_S3TC_DXT3_EXT,|
> + * | | RGBA32F | COMPRESSED_RGBA_S3TC_DXT5_EXT,
> |
> + * | | |
> COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT,|
> + * | | | COMPRESSED_RG_RGTC2,
> |
> + * | | | COMPRESSED_SIGNED_RG_RGTC2,
> |
> + * | | | COMPRESSED_RGBA_BPTC_UNORM,
> |
> + * | | |
> COMPRESSED_SRGB_ALPHA_BPTC_UNORM, |
> + * | | |
> COMPRESSED_RGB_BPTC_SIGNED_FLOAT, |
> + * | | |
> COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT |
> + *
> ------------------------------------------------------------------------
> + * | 64-bit | RGBA16F, RG32F, | COMPRESSED_RGB_S3TC_DXT1_EXT,
> |
> + * | | RGBA16UI, RG32UI, | COMPRESSED_SRGB_S3TC_DXT1_EXT,
> |
> + * | | RGBA16I, RG32I, | COMPRESSED_RGBA_S3TC_DXT1_EXT,
> |
> + * | | RGBA16, RGBA16_SNORM |
> COMPRESSED_SRGB_ALPHA_S3TC_DXT1_EXT,|
> + * | | | COMPRESSED_RED_RGTC1,
> |
> + * | | | COMPRESSED_SIGNED_RED_RGTC1
> |
> + *
> ------------------------------------------------------------------------
> + */
>
Spec references should cite a spec version and page number or, if pulled
form the extension spec say as much. For instance:
>From the OpenGL specification version 4.4 core, p. 125:
blah
That way the reader knows exactly what document to go look at. Also, spec
citations are usually done as a block quite where the quoted text is
indented another 3 spaces from the rest of the comment.
> +
> + switch (compressedFormat) {
> + case GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
> + case GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT3_EXT:
> + case GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
> + case GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT:
> + case GL_COMPRESSED_RG_RGTC2:
> + case GL_COMPRESSED_SIGNED_RG_RGTC2:
> + case GL_COMPRESSED_RGBA_BPTC_UNORM:
> + case GL_COMPRESSED_SRGB_ALPHA_BPTC_UNORM:
> + case GL_COMPRESSED_RGB_BPTC_SIGNED_FLOAT:
> + case GL_COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT:
> + compressedClass = BLOCK_CLASS_128_BITS;
> + break;
> + case GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
> + case GL_COMPRESSED_SRGB_S3TC_DXT1_EXT:
> + case GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
> + case GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT1_EXT:
> + case GL_COMPRESSED_RED_RGTC1:
> + case GL_COMPRESSED_SIGNED_RED_RGTC1:
> + compressedClass = BLOCK_CLASS_64_BITS;
> + break;
> + default:
> + return false;
> + }
> +
> + switch (otherFormat) {
> + case GL_RGBA32UI:
> + case GL_RGBA32I:
> + case GL_RGBA32F:
> + otherClass = BLOCK_CLASS_128_BITS;
> + break;
> + case GL_RGBA16F:
> + case GL_RG32F:
> + case GL_RGBA16UI:
> + case GL_RG32UI:
> + case GL_RGBA16I:
> + case GL_RG32I:
> + case GL_RGBA16:
> + case GL_RGBA16_SNORM:
> + otherClass = BLOCK_CLASS_64_BITS;
> + break;
> + default:
> + return false;
> + }
> +
> + return compressedClass == otherClass;
> +}
> +
> +static bool
> +copy_format_compatible(struct gl_context *ctx,
> + GLenum srcFormat, GLenum dstFormat)
> +{
> + /*
>
+ * For the purposes of CopyImageSubData, two internal formats
> + * are considered compatible if any of the following conditions are
> + * met:
> + * * the formats are the same,
> + * * the formats are considered compatible according to the
> + * compatibility rules used for texture views as defined in
> + * section 3.9.X. In particular, if both internal formats are listed
> + * in the same entry of Table 3.X.2, they are considered compatible,
> or
> + * * one format is compressed and the other is uncompressed and
> + * Table 4.X.1 lists the two formats in the same row.
> + */
>
Same here.
With those comments fixed,
Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>
> +
> + if (_mesa_texture_view_compatible_format(ctx, srcFormat, dstFormat)) {
> + /* Also checks if formats are equal. */
> + return true;
> + } else if (_mesa_is_compressed_format(ctx, srcFormat)) {
> + return compressed_format_compatible(ctx, srcFormat, dstFormat);
> + } else if (_mesa_is_compressed_format(ctx, dstFormat)) {
> + return compressed_format_compatible(ctx, dstFormat, srcFormat);
> + }
> +
> + return false;
> +}
> +
> void GLAPIENTRY
> _mesa_CopyImageSubData(GLuint srcName, GLenum srcTarget, GLint srcLevel,
> GLint srcX, GLint srcY, GLint srcZ,
> @@ -265,7 +386,7 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum
> srcTarget, GLint srcLevel,
> struct gl_texture_object *srcTexObj, *dstTexObj;
> struct gl_texture_image *srcTexImage, *dstTexImage;
> GLuint src_bw, src_bh, dst_bw, dst_bh;
> - int i, srcNewZ, dstNewZ, Bpt;
> + int i, srcNewZ, dstNewZ;
>
> if (MESA_VERBOSE & VERBOSE_API)
> _mesa_debug(ctx, "glCopyImageSubData(%u, %s, %d, %d, %d, %d, "
> @@ -306,15 +427,6 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum
> srcTarget, GLint srcLevel,
> goto cleanup;
> }
>
> - /* Very simple sanity check. This is sufficient if one of the textures
> - * is compressed. */
> - Bpt = _mesa_get_format_bytes(srcTexImage->TexFormat);
> - if (_mesa_get_format_bytes(dstTexImage->TexFormat) != Bpt) {
> - _mesa_error(ctx, GL_INVALID_VALUE,
> - "glCopyImageSubData(internalFormat mismatch)");
> - goto cleanup;
> - }
> -
> if (!check_region_bounds(ctx, srcTexImage, srcX, srcY, srcZ,
> srcWidth, srcHeight, srcDepth, "src"))
> goto cleanup;
> @@ -324,17 +436,11 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum
> srcTarget, GLint srcLevel,
> (srcHeight / src_bh) * dst_bh, srcDepth,
> "dst"))
> goto cleanup;
>
> - if (_mesa_is_format_compressed(srcTexImage->TexFormat)) {
> - /* XXX: Technically, we should probaby do some more specific
> checking
> - * here. However, this should be sufficient for all compressed
> - * formats that mesa supports since it is a direct memory copy.
> - */
> - } else if (_mesa_is_format_compressed(dstTexImage->TexFormat)) {
> - } else if (_mesa_texture_view_compatible_format(ctx,
> -
> srcTexImage->InternalFormat,
> -
> dstTexImage->InternalFormat)) {
> - } else {
> - return; /* Error logged by _mesa_texture_view_compatible_format */
> + if (!copy_format_compatible(ctx, srcTexImage->InternalFormat,
> + dstTexImage->InternalFormat)) {
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> + "glCopyImageSubData(internalFormat mismatch)");
> + goto cleanup;
> }
>
> for (i = 0; i < srcDepth; ++i) {
> --
> 2.1.0
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150307/3835f754/attachment-0001.html>
More information about the mesa-dev
mailing list