<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Aug 21, 2015 at 2:12 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">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>> > > We currently check that our format info table is sane during context > initialization in debug builds. Perform this check during > `make check` instead. This enables format testing in release builds > and removes the requirement of an exhuastive switch for > _mesa_uncompressed_format_to_type_and_comps(). > > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > --- > src/mesa/main/context.c | 4 - > src/mesa/main/formats.c | 157 ++--------------------------------- > src/mesa/main/tests/Makefile.am | 1 + > src/mesa/main/tests/mesa_formats.cpp | 130 +++++++++++++++++++++++++++++ > 4 files changed, 137 insertions(+), 155 deletions(-) > create mode 100644 src/mesa/main/tests/mesa_formats.cpp Overall the patch looks good. I found a few small problems, though. See below. <div><div class="h5"> > > diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c > index 7451e5a..6a40c0a 100644 > --- a/src/mesa/main/context.c > +++ b/src/mesa/main/context.c > @@ -402,10 +402,6 @@ one_time_init( struct gl_context *ctx ) > PACKAGE_VERSION, __DATE__, __TIME__); > } > #endif > - > -#ifdef DEBUG > - _mesa_test_formats(); > -#endif > } > > /* per-API one-time init */ > diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c > index 24c16ee..e58b917 100644 > --- a/src/mesa/main/formats.c > +++ b/src/mesa/main/formats.c > @@ -81,6 +81,7 @@ static const struct gl_format_info * > _mesa_get_format_info(mesa_format format) > { > const struct gl_format_info *info = &format_info[format]; > + STATIC_ASSERT(ARRAY_SIZE(format_info) == MESA_FORMAT_COUNT); > assert(info->Name == format); > return info; > } > @@ -869,118 +870,6 @@ _mesa_format_row_stride(mesa_format format, GLsizei width) > } > > > -/** > - * Debug/test: check that all uncompressed formats are handled in the > - * _mesa_uncompressed_format_to_type_and_comps() function. When new pixel > - * formats are added to Mesa, that function needs to be updated. > - * This is a no-op after the first call. > - */ > -static void > -check_format_to_type_and_comps(void) > -{ > - mesa_format f; > - > - for (f = MESA_FORMAT_NONE + 1; f < MESA_FORMAT_COUNT; f++) { > - GLenum datatype = 0; > - GLuint comps = 0; > - /* This function will emit a problem/warning if the format is > - * not handled. > - */ > - if (!_mesa_is_format_compressed(f)) > - _mesa_uncompressed_format_to_type_and_comps(f, &datatype, &comps); > - } > -} > - > -/** > - * Do sanity checking of the format info table. > - */ > -void > -_mesa_test_formats(void) > -{ > - GLuint i; > - > - STATIC_ASSERT(ARRAY_SIZE(format_info) == MESA_FORMAT_COUNT); > - > - for (i = 0; i < MESA_FORMAT_COUNT; i++) { > - const struct gl_format_info *info = _mesa_get_format_info(i); > - assert(info); > - > - assert(info->Name == i); > - > - if (info->Name == MESA_FORMAT_NONE) > - continue; > - > - if (info->BlockWidth == 1 && info->BlockHeight == 1) { > - if (info->RedBits > 0) { > - GLuint t = info->RedBits + info->GreenBits > - + info->BlueBits + info->AlphaBits; > - assert(t / 8 <= info->BytesPerBlock); > - (void) t; > - } > - } > - > - assert(info->DataType == GL_UNSIGNED_NORMALIZED || > - info->DataType == GL_SIGNED_NORMALIZED || > - info->DataType == GL_UNSIGNED_INT || > - info->DataType == GL_INT || > - info->DataType == GL_FLOAT || > - /* Z32_FLOAT_X24S8 has DataType of GL_NONE */ > - info->DataType == GL_NONE); > - > - if (info->BaseFormat == GL_RGB) { > - assert(info->RedBits > 0); > - assert(info->GreenBits > 0); > - assert(info->BlueBits > 0); > - assert(info->AlphaBits == 0); > - assert(info->LuminanceBits == 0); > - assert(info->IntensityBits == 0); > - } > - else if (info->BaseFormat == GL_RGBA) { > - assert(info->RedBits > 0); > - assert(info->GreenBits > 0); > - assert(info->BlueBits > 0); > - assert(info->AlphaBits > 0); > - assert(info->LuminanceBits == 0); > - assert(info->IntensityBits == 0); > - } > - else if (info->BaseFormat == GL_RG) { > - assert(info->RedBits > 0); > - assert(info->GreenBits > 0); > - assert(info->BlueBits == 0); > - assert(info->AlphaBits == 0); > - assert(info->LuminanceBits == 0); > - assert(info->IntensityBits == 0); > - } > - else if (info->BaseFormat == GL_RED) { > - assert(info->RedBits > 0); > - assert(info->GreenBits == 0); > - assert(info->BlueBits == 0); > - assert(info->AlphaBits == 0); > - assert(info->LuminanceBits == 0); > - assert(info->IntensityBits == 0); > - } > - else if (info->BaseFormat == GL_LUMINANCE) { > - assert(info->RedBits == 0); > - assert(info->GreenBits == 0); > - assert(info->BlueBits == 0); > - assert(info->AlphaBits == 0); > - assert(info->LuminanceBits > 0); > - assert(info->IntensityBits == 0); > - } > - else if (info->BaseFormat == GL_INTENSITY) { > - assert(info->RedBits == 0); > - assert(info->GreenBits == 0); > - assert(info->BlueBits == 0); > - assert(info->AlphaBits == 0); > - assert(info->LuminanceBits == 0); > - assert(info->IntensityBits > 0); > - } > - } > - > - check_format_to_type_and_comps(); > -} > - > - > > /** > * Return datatype and number of components per texel for the given > @@ -1518,48 +1407,14 @@ _mesa_uncompressed_format_to_type_and_comps(mesa_format format, > case MESA_FORMAT_COUNT: > assert(0); > return; > - > - case MESA_FORMAT_RGB_FXT1: > - case MESA_FORMAT_RGBA_FXT1: > - case MESA_FORMAT_RGB_DXT1: > - case MESA_FORMAT_RGBA_DXT1: > - case MESA_FORMAT_RGBA_DXT3: > - case MESA_FORMAT_RGBA_DXT5: > - case MESA_FORMAT_SRGB_DXT1: > - case MESA_FORMAT_SRGBA_DXT1: > - case MESA_FORMAT_SRGBA_DXT3: > - case MESA_FORMAT_SRGBA_DXT5: > - case MESA_FORMAT_R_RGTC1_UNORM: > - case MESA_FORMAT_R_RGTC1_SNORM: > - case MESA_FORMAT_RG_RGTC2_UNORM: > - case MESA_FORMAT_RG_RGTC2_SNORM: > - case MESA_FORMAT_L_LATC1_UNORM: > - case MESA_FORMAT_L_LATC1_SNORM: > - case MESA_FORMAT_LA_LATC2_UNORM: > - case MESA_FORMAT_LA_LATC2_SNORM: > - case MESA_FORMAT_ETC1_RGB8: > - case MESA_FORMAT_ETC2_RGB8: > - case MESA_FORMAT_ETC2_SRGB8: > - case MESA_FORMAT_ETC2_RGBA8_EAC: > - case MESA_FORMAT_ETC2_SRGB8_ALPHA8_EAC: > - case MESA_FORMAT_ETC2_R11_EAC: > - case MESA_FORMAT_ETC2_RG11_EAC: > - case MESA_FORMAT_ETC2_SIGNED_R11_EAC: > - case MESA_FORMAT_ETC2_SIGNED_RG11_EAC: > - case MESA_FORMAT_ETC2_RGB8_PUNCHTHROUGH_ALPHA1: > - case MESA_FORMAT_ETC2_SRGB8_PUNCHTHROUGH_ALPHA1: > - case MESA_FORMAT_BPTC_RGBA_UNORM: > - case MESA_FORMAT_BPTC_SRGB_ALPHA_UNORM: > - case MESA_FORMAT_BPTC_RGB_SIGNED_FLOAT: > - case MESA_FORMAT_BPTC_RGB_UNSIGNED_FLOAT: > - assert(_mesa_is_format_compressed(format)); > - case MESA_FORMAT_NONE: > - /* For debug builds, warn if any formats are not handled */ > -#ifdef DEBUG > default: > -#endif > +/* For debug builds, warn if any formats are not handled */ > +#ifdef DEBUG > _mesa_problem(NULL, "bad format %s in _mesa_uncompressed_format_to_type_and_comps", > _mesa_get_format_name(format)); > +#endif </div></div>_mesa_problem() should be called unconditionally here. I looked at a few dozen calls to _mesa_problem(), and I could find no precedent for guarding it with #ifdef DEBUG. <div><div class="h5"> > + assert(format == MESA_FORMAT_NONE || > + _mesa_is_format_compressed(format)); > *datatype = 0; > *comps = 1; > } > diff --git a/src/mesa/main/tests/Makefile.am b/src/mesa/main/tests/Makefile.am > index 251474d..9467f3b 100644 > --- a/src/mesa/main/tests/Makefile.am > +++ b/src/mesa/main/tests/Makefile.am > @@ -27,6 +27,7 @@ AM_CPPFLAGS += -DHAVE_SHARED_GLAPI > > main_test_SOURCES += \ > dispatch_sanity.cpp \ > + mesa_formats.cpp \ > program_state_string.cpp > > main_test_LDADD += \ > diff --git a/src/mesa/main/tests/mesa_formats.cpp b/src/mesa/main/tests/mesa_formats.cpp > new file mode 100644 > index 0000000..7bf9147 > --- /dev/null > +++ b/src/mesa/main/tests/mesa_formats.cpp > @@ -0,0 +1,130 @@ > +/* > + * Copyright © 2015 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > + > +/** > + * \name mesa_formats.cpp > + * > + * Verify that all mesa formats are handled in certain functions and that > + * the format info table is sane. > + * > + */ > + > +#include <gtest/gtest.h> > + > +#include "main/formats.h" > +#include "main/glformats.h" > + > +/** > + * Debug/test: check that all uncompressed formats are handled in the > + * _mesa_uncompressed_format_to_type_and_comps() function. When new pixel > + * formats are added to Mesa, that function needs to be updated. > + */ > +TEST(MesaFormatsTest, FormatTypeAndComps) > +{ > + for (int fi = MESA_FORMAT_NONE + 1; fi < MESA_FORMAT_COUNT; ++fi) { > + mesa_format f = (mesa_format) fi; > + > + /* This function will emit a problem/warning if the format is > + * not handled. > + */ > + if (!_mesa_is_format_compressed(f)) { > + GLenum datatype = 0; > + GLuint comps = 0; > + _mesa_uncompressed_format_to_type_and_comps(f, &datatype, &comps); > + > + /* If the datatype is zero, the format was not handled */ > + ASSERT_NE(datatype, (GLenum)0); </div></div>I think the test should also assert that comps >= 1. <div><div class="h5"> </div></div></blockquote><div>In the failure scenario for this function, comps will be set equal to 1. It can also be set equal to 1 for certain valid formats as well.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + } > + > + } > +} > + > +/** > + * Do sanity checking of the format info table. > + */ > +TEST(MesaFormatsTest, FormatSanity) > +{ > + for (int fi = 0; fi < MESA_FORMAT_COUNT; ++fi) { > + mesa_format f = (mesa_format) fi; > + GLenum datatype = _mesa_get_format_datatype(f); > + GLint r = _mesa_get_format_bits(f, GL_RED_BITS); > + GLint g = _mesa_get_format_bits(f, GL_GREEN_BITS); > + GLint b = _mesa_get_format_bits(f, GL_BLUE_BITS); > + GLint a = _mesa_get_format_bits(f, GL_ALPHA_BITS); > + GLint l = _mesa_get_format_bits(f, GL_TEXTURE_LUMINANCE_SIZE); > + GLint i = _mesa_get_format_bits(f, GL_TEXTURE_INTENSITY_SIZE); > + > + /* Note: Z32_FLOAT_X24S8 has datatype of GL_NONE */ > + ASSERT_TRUE(datatype == GL_NONE || > + datatype == GL_UNSIGNED_NORMALIZED || > + datatype == GL_SIGNED_NORMALIZED || > + datatype == GL_UNSIGNED_INT || > + datatype == GL_INT || > + datatype == GL_FLOAT); > + > + if (r > 0 && !_mesa_is_format_compressed(f)) { > + GLint bytes = _mesa_get_format_bytes(f); > + ASSERT_LE((r+g+b+a) / 8, bytes); > + } > + > +/* Determines if the base format has a channel [rgba] or property [li]. > + * > indicates existance > + * == indicates non-existance > + */ > +#define HAS_PROP(rop,gop,bop,aop,lop,iop) \ > +do { \ > + ASSERT_TRUE(r rop 0); \ > + ASSERT_TRUE(g gop 0); \ > + ASSERT_TRUE(b bop 0); \ > + ASSERT_TRUE(a aop 0); \ > + ASSERT_TRUE(l lop 0); \ > + ASSERT_TRUE(i iop 0); \ > +} while(0) </div></div>Please indent the macro as if it were part of the function body. Like this: ---#define HAS_PROP(rop,gop,bop,aop,lop,iop) \ ------do { \ ---------ASSERT_TRUE(r rop 0); \ ---------ASSERT_TRUE(g gop 0); \ ---------ASSERT_TRUE(b bop 0); \ ---------ASSERT_TRUE(a aop 0); \ ---------ASSERT_TRUE(l lop 0); \ ---------ASSERT_TRUE(i iop 0); \ ------} while(0) <div class="HOEnZb"><div class="h5"> > + > + switch (_mesa_get_format_base_format(f)) { > + case GL_RGBA: > + HAS_PROP(>,>,>,>,==,==); > + break; > + case GL_RGB: > + HAS_PROP(>,>,>,==,==,==); > + break; > + case GL_RG: > + HAS_PROP(>,>,==,==,==,==); > + break; > + case GL_RED: > + HAS_PROP(>,==,==,==,==,==); > + break; > + case GL_LUMINANCE: > + HAS_PROP(==,==,==,==,>,==); > + break; > + case GL_INTENSITY: > + HAS_PROP(==,==,==,==,==,>); > + break; > + default: > + break; > + } > + > +#undef HAS_PROP > + > + } > +} > -- > 2.5.0 > </div></div></blockquote></div> </div></div>