On Sat, Mar 7, 2015 at 12:16 PM, Sean Burke <leftmost...@gmail.com> wrote:
> > > On Sat, Mar 7, 2015 at 12:06 PM, Jason Ekstrand <ja...@jlekstrand.net> > wrote: > >> >> On Mar 7, 2015 10:57 AM, "Sean Burke" <leftmost...@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@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> >> >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev