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.
> + }
> +
> + }
> +}
> +
> +/**
> + * 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
