[Mesa-dev] [PATCH v3] mesa/formats: refactor by removing compressed formats

[Chad Versace]

On Wed 19 Aug 2015, Nanley Chery wrote:
> From: Nanley Chery <nanley.g.chery at intel.com>
> 
> All compressed formats return GL_FALSE and there isn't any evidence to
> support that this behaviour would change. Remove all switch cases for
> compressed formats.
> 
> v2. Since the exhaustive switch is removed, add a gtest to ensure
>     all formats are handled.
> v3. Ensure that GL_NO_ERROR is set before returning.
> 
> Cc: Brian Paul <brianp at vmware.com>
> Cc: Chad Versace <chad.versace at intel.com>
> Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> ---
>  src/mesa/drivers/dri/i915/intel_pixel_read.c |  2 +-
>  src/mesa/drivers/dri/i915/intel_tex_image.c  |  2 +-
>  src/mesa/main/formats.c                      | 58 ++++++----------------------
>  src/mesa/main/formats.h                      |  2 +-
>  src/mesa/main/readpix.c                      |  2 +-
>  src/mesa/main/tests/mesa_formats.cpp         |  7 +++-
>  src/mesa/main/texgetimage.c                  |  2 +-
>  src/mesa/main/texstore.c                     |  2 +-
>  src/mesa/state_tracker/st_cb_readpixels.c    |  2 +-
>  src/mesa/state_tracker/st_cb_texture.c       |  6 +--
>  src/mesa/state_tracker/st_format.c           |  2 +-
>  src/mesa/swrast/s_drawpix.c                  |  2 +-
>  12 files changed, 29 insertions(+), 60 deletions(-)
> 
> diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
> index e58b917..1f221f1 100644
> --- a/src/mesa/main/formats.c
> +++ b/src/mesa/main/formats.c
> @@ -1432,15 +1432,19 @@ _mesa_uncompressed_format_to_type_and_comps(mesa_format format,
>  GLboolean
>  _mesa_format_matches_format_and_type(mesa_format mesa_format,
>  				     GLenum format, GLenum type,
> -                                     GLboolean swapBytes)
> +                            GLboolean swapBytes, GLenum *error)

The parameter alignment looks wrong here. Maybe it's just my email app,
though.

Please document the function's error behavior: what errors does it and
emit and when.

>  {
>     const GLboolean littleEndian = _mesa_little_endian();
> +   if (error)
> +      *error = GL_NO_ERROR;
>  
>     /* Note: When reading a GL format/type combination, the format lists channel
>      * assignments from most significant channel in the type to least
>      * significant.  A type with _REV indicates that the assignments are
>      * swapped, so they are listed from least significant to most significant.
>      *
> +    * Compressed formats will fall through and return GL_FALSE.
> +    *
>      * For sanity, please keep this switch statement ordered the same as the
>      * enums in formats.h.
>      */



> @@ -2024,8 +1983,13 @@ _mesa_format_matches_format_and_type(mesa_format mesa_format,
>     case MESA_FORMAT_B8G8R8X8_SRGB:
>     case MESA_FORMAT_X8R8G8B8_SRGB:
>        return GL_FALSE;
> +   default:
> +      if (!_mesa_is_format_compressed(format)) {
> +         assert(0);
> +         if (error)
> +            *error = GL_INVALID_ENUM;
> +      }
>     }

The pattern in the default case...

    if (!stuff) {
        assert(0);
        if (error)
            *error = GL_INVALID_ENUM;
    }

would be better phrased as this equivalent code:

    assert(stuff);
    if (error)
        *error = GL_INVALID_ENUM;
    
> -
>     return GL_FALSE;
>  }
>  
> diff --git a/src/mesa/main/formats.h b/src/mesa/main/formats.h
> index e08247e..0634b78 100644
> --- a/src/mesa/main/formats.h
> +++ b/src/mesa/main/formats.h
> @@ -675,7 +675,7 @@ _mesa_format_has_color_component(mesa_format format, int component);
>  GLboolean
>  _mesa_format_matches_format_and_type(mesa_format mesa_format,
>  				     GLenum format, GLenum type,
> -                                     GLboolean swapBytes);
> +                            GLboolean swapBytes, GLenum *error);
>  
>  #ifdef __cplusplus
>  }



> diff --git a/src/mesa/main/tests/mesa_formats.cpp b/src/mesa/main/tests/mesa_formats.cpp
> index 7bf9147..dfc7d0e 100644
> --- a/src/mesa/main/tests/mesa_formats.cpp
> +++ b/src/mesa/main/tests/mesa_formats.cpp
> @@ -49,11 +49,16 @@ TEST(MesaFormatsTest, FormatTypeAndComps)
>         */
>        if (!_mesa_is_format_compressed(f)) {
>           GLenum datatype = 0;
> +         GLenum error = 0;
>           GLuint comps = 0;
> -         _mesa_uncompressed_format_to_type_and_comps(f, &datatype, &comps);
>  
>           /* If the datatype is zero, the format was not handled */
> +         _mesa_uncompressed_format_to_type_and_comps(f, &datatype, &comps);
>           ASSERT_NE(datatype, (GLenum)0);
> +
> +         /* If the error is INVALID_ENUM, the format was not handled */
> +         _mesa_format_matches_format_and_type(f, f, datatype, GL_FALSE, &error);
> +         ASSERT_NE(error, (GLenum)GL_INVALID_ENUM);

Two comments on the new check:

    - It is more robust to check that no error was returned
      rather than checking that a specific error was not returned. That
      is, this is more robust:

        ASSERT_EQ(error, (GLenum)GL_NO_ERROR)

    - One of the first two arguments to
      _mesa_uncompressed_format_to_type_and_comps() is wrong. The patch
      passes the same value for each argument, but parameters have
      different types.


> 2.5.0

Git 2.5.0? Are you building Git from source? Fedora 22 is still shipping
git-2.4.3.