On Fri, Aug 21, 2015 at 2:12 PM, Chad Versace <chad.vers...@intel.com>
wrote:
> On Wed 19 Aug 2015, Nanley Chery wrote:
> > From: Nanley Chery <nanley.g.ch...@intel.com>
> >
> > 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 <nanley.g.ch...@intel.com>
> > ---
> > 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.
>
> >
> > 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
>
> _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.
>
> > + 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);
>
> I think the test should also assert that comps >= 1.
>
> 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.
> > + }
> > +
> > + }
> > +}
> > +
> > +/**
> > + * 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)
>
> 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)
>
> > +
> > + 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
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
