[Mesa-dev] [PATCH] mesa: fix ARB_copy_image internal format check
Ilia Mirkin
imirkin at alum.mit.edu
Sat Mar 7 10:18:48 PST 2015
[+jekstrand, who wrote the initial ARB_copy_image support and knows a
thing or two about texture formats]
On Sat, Mar 7, 2015 at 4:42 AM, 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. Additionally,
> the current check allows compressed textures of the same block size to be used,
> which is in violation of the spec.
>
> Additionally, add a fixme for another check against TexFormat.
> ---
> src/mesa/main/copyimage.c | 127 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/src/mesa/main/copyimage.c b/src/mesa/main/copyimage.c
> index 455929d..dd0b8ea 100644
> --- a/src/mesa/main/copyimage.c
> +++ b/src/mesa/main/copyimage.c
> @@ -33,6 +33,51 @@
> #include "texobj.h"
> #include "fbobject.h"
> #include "textureview.h"
> +#include "glformats.h"
> +
> +enum mesa_block_class {
> + BLOCK_CLASS_128_BITS,
> + BLOCK_CLASS_64_BITS,
> + BLOCK_CLASS_UNDEFINED
> +};
> +
> +struct internal_format_block_class_info {
> + enum mesa_block_class block_class;
> + GLenum internal_format;
> +};
> +
> +static const struct internal_format_block_class_info compressed_formats[] = {
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_RGBA_S3TC_DXT3_EXT},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT3_EXT},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_RGBA_S3TC_DXT5_EXT},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_RG_RGTC2},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_SIGNED_RG_RGTC2},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_RGBA_BPTC_UNORM},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_SRGB_ALPHA_BPTC_UNORM},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_RGB_BPTC_SIGNED_FLOAT},
> + {BLOCK_CLASS_128_BITS, GL_COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT},
> + {BLOCK_CLASS_64_BITS, GL_COMPRESSED_RGB_S3TC_DXT1_EXT},
> + {BLOCK_CLASS_64_BITS, GL_COMPRESSED_SRGB_S3TC_DXT1_EXT},
> + {BLOCK_CLASS_64_BITS, GL_COMPRESSED_RGBA_S3TC_DXT1_EXT},
> + {BLOCK_CLASS_64_BITS, GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT1_EXT},
> + {BLOCK_CLASS_64_BITS, GL_COMPRESSED_RED_RGTC1},
> + {BLOCK_CLASS_64_BITS, GL_COMPRESSED_SIGNED_RED_RGTC1}
> +};
> +
> +static const struct internal_format_block_class_info uncompressed_formats[] = {
> + {BLOCK_CLASS_128_BITS, GL_RGBA32UI},
> + {BLOCK_CLASS_128_BITS, GL_RGBA32I},
> + {BLOCK_CLASS_128_BITS, GL_RGBA32F},
> + {BLOCK_CLASS_64_BITS, GL_RGBA16F},
> + {BLOCK_CLASS_64_BITS, GL_RG32F},
> + {BLOCK_CLASS_64_BITS, GL_RGBA16UI},
> + {BLOCK_CLASS_64_BITS, GL_RG32UI},
> + {BLOCK_CLASS_64_BITS, GL_RGBA16I},
> + {BLOCK_CLASS_64_BITS, GL_RG32I},
> + {BLOCK_CLASS_64_BITS, GL_RGBA16},
> + {BLOCK_CLASS_64_BITS, GL_RGBA16_SNORM}
> +};
>
> static bool
> prepare_target(struct gl_context *ctx, GLuint name, GLenum *target, int level,
> @@ -253,6 +298,59 @@ 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)
> +{
> + GLuint i;
> + enum mesa_block_class compressedClass = BLOCK_CLASS_UNDEFINED,
> + otherClass = BLOCK_CLASS_UNDEFINED;
> +
> + if (_mesa_is_compressed_format(ctx, otherFormat)) {
> + return false;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(compressed_formats); i++) {
> + if (compressed_formats[i].internal_format == compressedFormat) {
> + compressedClass = compressed_formats[i].block_class;
> + break;
> + }
> + }
Each one of these is just a handful of options, rather than going
through the list each time, it'd be a lot more performant to have each
one just be a
switch (glformat) {
case ...
case ...
return BLOCK_CLASS_64;
case ..
case ..
return 128
default:
return undefined;
}
> +
> + for (i = 0; i < ARRAY_SIZE(uncompressed_formats); i++) {
> + if (uncompressed_formats[i].internal_format == otherFormat) {
> + otherClass = uncompressed_formats[i].block_class;
> + break;
> + }
> + }
> +
> + if (compressedClass == BLOCK_CLASS_UNDEFINED
> + || otherClass == BLOCK_CLASS_UNDEFINED)
> + return false;
> +
> + return compressedClass == otherClass;
> +}
> +
> +static bool
> +copy_format_compatible(struct gl_context *ctx,
> + GLenum srcFormat, GLenum dstFormat)
> +{
> + if (srcFormat == dstFormat) {
> + 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);
> + }
> + else {
> + return _mesa_texture_view_compatible_format(ctx,
> + srcFormat,
> + dstFormat);
>From my read of the spec, this should be
else if (ctx->Extensions.ARB_texture_view)
Or alternatively the check should be moved into
_mesa_texture_view_compatible_format. Actually that may be preferable.
The spec says:
Dependencies on OpenGL 4.3 and ARB_texture_view:
If OpenGL 4.3 or later and ARB_texture_view are not supported,
any references to view-compatible image formats should be removed.
> + }
> +}
> +
> void GLAPIENTRY
> _mesa_CopyImageSubData(GLuint srcName, GLenum srcTarget, GLint srcLevel,
> GLint srcX, GLint srcY, GLint srcZ,
> @@ -265,7 +363,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, "
> @@ -291,6 +389,8 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum
> srcTarget, GLint srcLevel,
> &dstTexObj, &dstTexImage, &tmpTexNames[1], "dst"))
> goto cleanup;
>
> + /* XXX: We can't rely on TexFormat to give us information about the texture
> + * format specified by the user. */
In theory, yes... but in practice this check only matters for
compressed formats, and those map fairly between gl and mesa formats.
(i.e. the driver would never pick a *different* compressed format for
a particular GL one, unlike with color textures where that's done all
the time).
> _mesa_get_format_block_size(srcTexImage->TexFormat, &src_bw, &src_bh);
> if ((srcX % src_bw != 0) || (srcY % src_bh != 0) ||
> (srcWidth % src_bw != 0) || (srcHeight % src_bh != 0)) {
> @@ -306,15 +406,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 +415,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_VALUE,
> + "glCopyImageSubData(internalFormat mismatch)");
INVALID_OPERATION is generated
* if either object is a texture and the texture is not complete,
* if the source and destination formats are not compatible,
* if the source and destination number of samples do not match,
* if one image is compressed and the other is uncompressed and the
block size of compressed image is not equal to the texel size
of the compressed image.
I think this should be GL_INVALID_OPERATION, not GL_INVALID_VALUE.
> + goto cleanup;
> }
>
> for (i = 0; i < srcDepth; ++i) {
> --
> 2.1.0
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list