<div dir="ltr">Thanks for pointing out these errors. I'll fix them in the next version. <div class="gmail_extra"> <div class="gmail_quote">On Fri, Aug 21, 2015 at 2:34 PM, Chad Versace <<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed 19 Aug 2015, Nanley Chery wrote: > From: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > 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 <<a href="mailto:brianp@vmware.com">brianp@vmware.com</a>> > Cc: Chad Versace <<a href="mailto:chad.versace@intel.com">chad.versace@intel.com</a>> > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > --- > 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(-) > </div></div>> 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. </blockquote></div>I'm running Arch Linux.</div></div>