[Mesa-dev] [PATCH 1/4] mesa/formats: only do type and component lookup for uncompressed formats

[Chad Versace]

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 <nanleychery at gmail.com> wrote:
> 
> >
> > On Fri, Aug 14, 2015 at 4:00 PM, Chad Versace <chad.versace at intel.com>
> > wrote:
> >
> >> On Tue 11 Aug 2015, Nanley Chery wrote:
> >> > From: Nanley Chery <nanley.g.chery at 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.ekstrand at intel.com>
> >> > Signed-off-by: Nanley Chery <nanley.g.chery at 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
> >