[Mesa-dev] [PATCH v2] mesa: fix ARB_copy_image internal format check
Jason Ekstrand
jason at jlekstrand.net
Sat Mar 7 13:31:01 PST 2015
On Sat, Mar 7, 2015 at 12:16 PM, Sean Burke <leftmostcat at gmail.com> wrote:
>
>
> On Sat, Mar 7, 2015 at 12:06 PM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
>
>>
>> On Mar 7, 2015 10:57 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.
>>
>> Could you please be more specific about exactly when the old code fails.
>>
> One example I was finding in running piglit tests was that
> GL_INVALID_VALUE was being set when the formats to be converted were
> GL_RGB8 and GL_RGB8UI. TexFormat refers specifically to the actual memory
> layout which mesa has chosen for the texture, not to the internal format
> specified by the user, and there's no guarantee that the bytes-per-block of
> the memory layout matches the internal format. GL_RGB8 was being backed by
> B8G8R8X8, even though its internal format is still 3 bytes.
>
> Additionally, the previous code indicated that textures with two different
> compressed formats were compatible if block size was identical. According
> to the spec, this is incorrect. Different texture formats are only
> compatible if they're compatible texture view-wise or if one is compressed
> and the other is uncompressed and they are listed in the given table.
>
That's what I figured. Please put the above two paragraphs (or their
equivalent) in the commit message.
> In writing this out, I've discovered that I do have a bug in
> copy_format_compatible() (view compatibility should be checked second,
> immediately after checking if the formats are equal, to account for
> view-compatible compressed formats), which I'll correct with a v3.
>
Yeah, that will follow the spec better. I don't know that it is actually
going to have a different result but it doesn't hurt to follow the spec
more closely. It would also be good to put a spec citation in the
copy_format_compatible() function.
> > v2: Use a switch instead of array iteration for block class and show the
>>
>> > correct GL error when internal formats are mismatched.
>> > ---
>> > src/mesa/main/copyimage.c | 112
>> +++++++++++++++++++++++++++++++++++++---------
>> > 1 file changed, 91 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/src/mesa/main/copyimage.c b/src/mesa/main/copyimage.c
>> > index 455929d..eaedaa6 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,85 @@ 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 different compressed formats are never compatible. */
>> > + if (_mesa_is_compressed_format(ctx, otherFormat)) {
>> > + return false;
>> > + }
>> > +
>> > + 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;
>>
>
Or you could just return "compressedClass == BLOCK_CLASS_N_bITS" in the
switch statement. Either works so I don't care if you like it the way it
is.
> > +}
>>
>> For compressed formats this should be the same as doing a bits-per-block
>> check. Why do we need all this?
>>
> Given that the spec gives an exhaustive list of all compatible
> compressed/uncompressed formats, rather than indicating that it's valid for
> any compressed format of a given block size, I feel it's best to be
> specific. A similar approach is taken for texture view compatibility, so it
> makes sense to me that we would take the same exhaustive approach here.
>
That's probably reasonable.
> > +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);
>>
>
The else should go on the same line with the "}". Same for the two "else
if"s above.
All in all, this looks pretty good. There's enough of a delta that I'd
like to see the v3 before giving an R-B, but one should be coming.
--Jason
> > + }
>> > +}
>> > +
>> > void GLAPIENTRY
>> > _mesa_CopyImageSubData(GLuint srcName, GLenum srcTarget, GLint
>> srcLevel,
>> > GLint srcX, GLint srcY, GLint srcZ,
>> > @@ -265,7 +350,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 +391,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 +400,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
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150307/85aebbb8/attachment-0001.html>
More information about the mesa-dev
mailing list