On Wed 19 Aug 2015, Nanley Chery wrote:
> From: Nanley Chery <nanley.g.ch...@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 <bri...@vmware.com>
> Cc: Chad Versace <chad.vers...@intel.com>
> Signed-off-by: Nanley Chery <nanley.g.ch...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
