On Mon 17 Aug 2015, Nanley Chery wrote: > Cc'ing Brian. > > I'm planning to make a patch to implement the first (3-step) solution.
Sounds good to me. [reproducing in-person conversation for mesa-dev] For the functions touched by patches 1/4 and 2/4, Mesa verifies (pre-patch) that each function handles all values of enum mesa_format correctly. It does that today as a compile-time check employing an exhaustive switch. And I feel that we need to continue having such a build-time check. So, as long you preserve that exhaustive format verification at build-time (whether through compiler errors or through a make-check target), then your approach is good with me pending Brian's approval. (Because Brian wrote some of the verification code in the neighborhood of your patches). > > On Mon, Aug 17, 2015 at 3:45 PM, Nanley Chery <nanleych...@gmail.com> wrote: > > > > > On Fri, Aug 14, 2015 at 4:00 PM, Chad Versace <chad.vers...@intel.com> > > wrote: > > > >> On Tue 11 Aug 2015, Nanley Chery wrote: > >> > From: Nanley Chery <nanley.g.ch...@intel.com> > >> > > >> > Only uncompressed formats have a non-void type and actual components > >> per pixel. > >> > Rename _mesa_format_to_type_and_comps to > >> > _mesa_uncompressed_format_to_type_and_comps and require callers to > >> check if > >> > the format is not compressed. > >> > > >> > Cc: Jason Ekstrand <jason.ekstr...@intel.com> > >> > Signed-off-by: Nanley Chery <nanley.g.ch...@intel.com> > >> > --- > >> > src/mesa/main/format_utils.c | 2 +- > >> > src/mesa/main/formats.c | 55 > >> ++++++++------------------------------------ > >> > src/mesa/main/formats.h | 2 +- > >> > src/mesa/main/mipmap.c | 2 +- > >> > 4 files changed, 12 insertions(+), 49 deletions(-) > >> > >> I like several patches in this series, but I don't like this one. It > >> produces a lot of compiler warnings. > >> > >> The problem is that the switch in _mesa_format_to_type_and_comps() is > >> intended to be an exhaustive switch. Removing the compressed format > >> cases causes a waterfall of compiler warnings like this: > >> > >> main/formats.c:1458:4: warning: enumeration value > >> 'MESA_FORMAT_LA_LATC2_UNORM' not handled in switch [-Wswitch] > >> main/formats.c:1458:4: warning: enumeration value > >> 'MESA_FORMAT_LA_LATC2_SNORM' not handled in switch [-Wswitch] > >> main/formats.c:1458:4: warning: enumeration value > >> 'MESA_FORMAT_ETC1_RGB8' not handled in switch [-Wswitch] > >> main/formats.c:1458:4: warning: enumeration value > >> 'MESA_FORMAT_ETC2_RGB8' not handled in switch [-Wswitch] > >> main/formats.c:1458:4: warning: enumeration value > >> 'MESA_FORMAT_ETC2_SRGB8' not handled in switch [-Wswitch] > >> main/formats.c:1458:4: warning: enumeration value > >> 'MESA_FORMAT_ETC2_RGBA8_EAC' not handled in switch [-Wswitch] > >> main/formats.c:1458:4: warning: enumeration value > >> 'MESA_FORMAT_ETC2_SRGB8_ALPHA8_EAC' not handled in switch [-Wswitch] > >> > >> I think you can salvage the patch by following the hint in this deleted > >> hunk... > >> > >> > - 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: > >> > - /* XXX generate error instead? */ > >> > - *datatype = GL_UNSIGNED_BYTE; > >> > - *comps = 0; > >> > - return; > >> > - > >> > >> In other words, rename the function to > >> _mesa_uncompressed_format_to_type_and_comps(); keep all the format > >> cases; and add an assertion or _mesa_problem() for the compressed cases. > >> > > > > I was hoping to avoid having to add cases to the switch for compressed > > textures because they are all invalid inputs. Nonetheless, your proposed > > solution is a good one. > > > > I do have some alternative ideas that I'd like to share. After a little > > more investigation, it seems that we have redundant checking that new > > formats have their datatype and components assigned. The first check is at > > compile time; the compiler checks that every enum has a case. The second > > check is at run-time for debug builds; in check_format_to_type_and_comps(), > > we pass every format through _mesa_uncompressed_format_to_type_and_comps() > > during context init (but don't do anything when an error is emitted). Both > > checks serve to remind the developer to add a case to the switch whenever a > > new format is added to the mesa_format enum. > > > > We could remove this redundancy and only update the cases for the formats > > that matter (uncompressed formats) by doing the following: > > 1. in _mesa_uncompressed_format_to_type_and_comps: unconditionally add the > > default case with an assertion that what has fallen through was either a > > compressed format or MESA_FORMAT_NONE. > > 2. in check_format_to_type_and_comps(): assert that the datatype value is > > greater than 0 to ensure the test fails in an error state. > > 3. in src/mesa/main/tests/ : make _mesa_test_formats() a test we run > > during make check. > > > > We could also remove the redundant checking (but keep adding unnecessary > > compressed cases by doing the following: > > 1. keep the compressed cases as suggested > > 2. delete check_format_to_type_and_comps() (since the compiler does this > > for us). > > > > Nanley > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
