Thanks for pointing out these errors. I'll fix them in the next version.
On Fri, Aug 21, 2015 at 2:34 PM, Chad Versace <chad.vers...@intel.com>
wrote:
> 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.
>
I'm running Arch Linux.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
