<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Mar 7, 2015 at 12:16 PM, Sean Burke <span dir="ltr"><<a href="mailto:leftmostcat@gmail.com" target="_blank">leftmostcat@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Sat, Mar 7, 2015 at 12:06 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br></span><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><p dir="ltr"><br>
On Mar 7, 2015 10:57 AM, "Sean Burke" <<a href="mailto:leftmostcat@gmail.com" target="_blank">leftmostcat@gmail.com</a>> wrote:<br>
><br>
> The memory layout of compatible internal formats may differ in bytes per<br>
> block, so TexFormat is not a reliable measure of compatibility. Additionally,<br>
> the current check allows compressed textures of the same block size to be used,<br>
> which is in violation of the spec.</p>
</span><p dir="ltr">Could you please be more specific about exactly when the old code fails.</p></blockquote></span><div><div>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.<br><br></div><span class="">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.<br></span></div></div></div></div></blockquote><div><br></div><div>That's what I figured.  Please put the above two paragraphs (or their equivalent) in the commit message.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span class=""></span><span class="">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.<br></span></div></div></div></div></blockquote><div><br></div><div>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. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span class=""></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>
<p dir="ltr">> v2: Use a switch instead of array iteration for block class and show the</p><div><div class="h5"><br>
>     correct GL error when internal formats are mismatched.<br>
> ---<br>
>  src/mesa/main/copyimage.c | 112 +++++++++++++++++++++++++++++++++++++---------<br>
>  1 file changed, 91 insertions(+), 21 deletions(-)<br>
><br>
> diff --git a/src/mesa/main/copyimage.c b/src/mesa/main/copyimage.c<br>
> index 455929d..eaedaa6 100644<br>
> --- a/src/mesa/main/copyimage.c<br>
> +++ b/src/mesa/main/copyimage.c<br>
> @@ -33,6 +33,12 @@<br>
>  #include "texobj.h"<br>
>  #include "fbobject.h"<br>
>  #include "textureview.h"<br>
> +#include "glformats.h"<br>
> +<br>
> +enum mesa_block_class {<br>
> +   BLOCK_CLASS_128_BITS,<br>
> +   BLOCK_CLASS_64_BITS<br>
> +};<br>
><br>
>  static bool<br>
>  prepare_target(struct gl_context *ctx, GLuint name, GLenum *target, int level,<br>
> @@ -253,6 +259,85 @@ check_region_bounds(struct gl_context *ctx,<br>
> struct gl_texture_image *tex_image,<br>
>     return true;<br>
>  }<br>
><br>
> +static bool<br>
> +compressed_format_compatible(struct gl_context *ctx,<br>
> +                             GLenum compressedFormat, GLenum otherFormat)<br>
> +{<br>
> +   enum mesa_block_class compressedClass, otherClass;<br>
> +<br>
> +   /* Two different compressed formats are never compatible. */<br>
> +   if (_mesa_is_compressed_format(ctx, otherFormat)) {<br>
> +      return false;<br>
> +   }<br>
> +<br>
> +   switch (compressedFormat) {<br>
> +      case GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:<br>
> +      case GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT3_EXT:<br>
> +      case GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:<br>
> +      case GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT:<br>
> +      case GL_COMPRESSED_RG_RGTC2:<br>
> +      case GL_COMPRESSED_SIGNED_RG_RGTC2:<br>
> +      case GL_COMPRESSED_RGBA_BPTC_UNORM:<br>
> +      case GL_COMPRESSED_SRGB_ALPHA_BPTC_UNORM:<br>
> +      case GL_COMPRESSED_RGB_BPTC_SIGNED_FLOAT:<br>
> +      case GL_COMPRESSED_RGB_BPTC_UNSIGNED_FLOAT:<br>
> +         compressedClass = BLOCK_CLASS_128_BITS;<br>
> +         break;<br>
> +      case GL_COMPRESSED_RGB_S3TC_DXT1_EXT:<br>
> +      case GL_COMPRESSED_SRGB_S3TC_DXT1_EXT:<br>
> +      case GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:<br>
> +      case GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT1_EXT:<br>
> +      case GL_COMPRESSED_RED_RGTC1:<br>
> +      case GL_COMPRESSED_SIGNED_RED_RGTC1:<br>
> +         compressedClass = BLOCK_CLASS_64_BITS;<br>
> +         break;<br>
> +      default:<br>
> +         return false;<br>
> +   }<br>
> +<br>
> +   switch (otherFormat) {<br>
> +      case GL_RGBA32UI:<br>
> +      case GL_RGBA32I:<br>
> +      case GL_RGBA32F:<br>
> +         otherClass = BLOCK_CLASS_128_BITS;<br>
> +         break;<br>
> +      case GL_RGBA16F:<br>
> +      case GL_RG32F:<br>
> +      case GL_RGBA16UI:<br>
> +      case GL_RG32UI:<br>
> +      case GL_RGBA16I:<br>
> +      case GL_RG32I:<br>
> +      case GL_RGBA16:<br>
> +      case GL_RGBA16_SNORM:<br>
> +         otherClass = BLOCK_CLASS_64_BITS;<br>
> +         break;<br>
> +      default:<br>
> +         return false;<br>
> +   }<br>
> +<br>
> +   return compressedClass == otherClass;<br></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><div><div class="h5">
> +}</div></div><p></p>
</div></div><div><div class="h5"><p dir="ltr">For compressed formats this should be the same as doing a bits-per-block check.  Why do we need all this?</p></div></div></blockquote><div><div>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.<br></div></div></div></div></div></blockquote><div><br></div><div>That's probably reasonable. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<p dir="ltr"></p><div><div>> +static bool<div><div class="h5"><br>
> +copy_format_compatible(struct gl_context *ctx,<br>
> +                                GLenum srcFormat, GLenum dstFormat)<br>
> +{<br>
> +   if (srcFormat == dstFormat) {<br>
> +      return true;<br>
> +   }<br>
> +   else if (_mesa_is_compressed_format(ctx, srcFormat)) {<br>
> +      return compressed_format_compatible(ctx, srcFormat, dstFormat);<br>
> +   }<br>
> +   else if (_mesa_is_compressed_format(ctx, dstFormat)) {<br>
> +      return compressed_format_compatible(ctx, dstFormat, srcFormat);<br>
> +   }<br>
> +   else {<br>
> +      return _mesa_texture_view_compatible_format(ctx,<br>
> +                                                  srcFormat,<br>
> +                                                  dstFormat);<br></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div><div>The else should go on the same line with the "}".  Same for the two "else if"s above.<br><br></div><div>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.<br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><div><div class="h5">
> +   }<br>
> +}<br>
> +<br>
>  void GLAPIENTRY<br>
>  _mesa_CopyImageSubData(GLuint srcName, GLenum srcTarget, GLint srcLevel,<br>
>                         GLint srcX, GLint srcY, GLint srcZ,<br>
> @@ -265,7 +350,7 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum<br>
> srcTarget, GLint srcLevel,<br>
>     struct gl_texture_object *srcTexObj, *dstTexObj;<br>
>     struct gl_texture_image *srcTexImage, *dstTexImage;<br>
>     GLuint src_bw, src_bh, dst_bw, dst_bh;<br>
> -   int i, srcNewZ, dstNewZ, Bpt;<br>
> +   int i, srcNewZ, dstNewZ;<br>
><br>
>     if (MESA_VERBOSE & VERBOSE_API)<br>
>        _mesa_debug(ctx, "glCopyImageSubData(%u, %s, %d, %d, %d, %d, "<br>
> @@ -306,15 +391,6 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum<br>
> srcTarget, GLint srcLevel,<br>
>        goto cleanup;<br>
>     }<br>
><br>
> -   /* Very simple sanity check.  This is sufficient if one of the textures<br>
> -    * is compressed. */<br>
> -   Bpt = _mesa_get_format_bytes(srcTexImage->TexFormat);<br>
> -   if (_mesa_get_format_bytes(dstTexImage->TexFormat) != Bpt) {<br>
> -      _mesa_error(ctx, GL_INVALID_VALUE,<br>
> -                  "glCopyImageSubData(internalFormat mismatch)");<br>
> -      goto cleanup;<br>
> -   }<br>
> -<br>
>     if (!check_region_bounds(ctx, srcTexImage, srcX, srcY, srcZ,<br>
>                              srcWidth, srcHeight, srcDepth, "src"))<br>
>        goto cleanup;<br>
> @@ -324,17 +400,11 @@ _mesa_CopyImageSubData(GLuint srcName, GLenum<br>
> srcTarget, GLint srcLevel,<br>
>                              (srcHeight / src_bh) * dst_bh, srcDepth, "dst"))<br>
>        goto cleanup;<br>
><br>
> -   if (_mesa_is_format_compressed(srcTexImage->TexFormat)) {<br>
> -      /* XXX: Technically, we should probaby do some more specific checking<br>
> -       * here.  However, this should be sufficient for all compressed<br>
> -       * formats that mesa supports since it is a direct memory copy.<br>
> -       */<br>
> -   } else if (_mesa_is_format_compressed(dstTexImage->TexFormat)) {<br>
> -   } else if (_mesa_texture_view_compatible_format(ctx,<br>
> -                                                   srcTexImage->InternalFormat,<br>
> -<br>
> dstTexImage->InternalFormat)) {<br>
> -   } else {<br>
> -      return; /* Error logged by _mesa_texture_view_compatible_format */<br>
> +   if (!copy_format_compatible(ctx, srcTexImage->InternalFormat,<br>
> +                               dstTexImage->InternalFormat)) {<br>
> +      _mesa_error(ctx, GL_INVALID_OPERATION,<br>
> +                  "glCopyImageSubData(internalFormat mismatch)");<br>
> +      goto cleanup;<br>
>     }<br>
><br>
>     for (i = 0; i < srcDepth; ++i) {<br>
> --<br>
> 2.1.0<br></div></div></div></div><div><div class="h5">
> _______________________________________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
<p></p>
</div></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div>