<div dir="ltr">Cc'ing Brian.<div><br></div><div>I'm planning to make a patch to implement the first (3-step) solution.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 17, 2015 at 3:45 PM, Nanley Chery <span dir="ltr"><<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 14, 2015 at 4:00 PM, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Tue 11 Aug 2015, Nanley Chery wrote:<br> > From: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com" target="_blank">nanley.g.chery@intel.com</a>><br> ><br> > Only uncompressed formats have a non-void type and actual components per pixel.<br> > Rename _mesa_format_to_type_and_comps to<br> > _mesa_uncompressed_format_to_type_and_comps and require callers to check if<br> > the format is not compressed.<br> ><br> > Cc: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>><br> > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com" target="_blank">nanley.g.chery@intel.com</a>><br> > ---<br> > src/mesa/main/format_utils.c | 2 +-<br> > src/mesa/main/formats.c | 55 ++++++++------------------------------------<br> > src/mesa/main/formats.h | 2 +-<br> > src/mesa/main/mipmap.c | 2 +-<br> > 4 files changed, 12 insertions(+), 49 deletions(-)<br> <br> </span>I like several patches in this series, but I don't like this one. It<br> produces a lot of compiler warnings.<br> <br> The problem is that the switch in _mesa_format_to_type_and_comps() is<br> intended to be an exhaustive switch. Removing the compressed format<br> cases causes a waterfall of compiler warnings like this:<br> <br> main/formats.c:1458:4: warning: enumeration value 'MESA_FORMAT_LA_LATC2_UNORM' not handled in switch [-Wswitch]<br> main/formats.c:1458:4: warning: enumeration value 'MESA_FORMAT_LA_LATC2_SNORM' not handled in switch [-Wswitch]<br> main/formats.c:1458:4: warning: enumeration value 'MESA_FORMAT_ETC1_RGB8' not handled in switch [-Wswitch]<br> main/formats.c:1458:4: warning: enumeration value 'MESA_FORMAT_ETC2_RGB8' not handled in switch [-Wswitch]<br> main/formats.c:1458:4: warning: enumeration value 'MESA_FORMAT_ETC2_SRGB8' not handled in switch [-Wswitch]<br> main/formats.c:1458:4: warning: enumeration value 'MESA_FORMAT_ETC2_RGBA8_EAC' not handled in switch [-Wswitch]<br> main/formats.c:1458:4: warning: enumeration value 'MESA_FORMAT_ETC2_SRGB8_ALPHA8_EAC' not handled in switch [-Wswitch]<br> <br> I think you can salvage the patch by following the hint in this deleted<br> hunk...<br> <div><div><br> > - case MESA_FORMAT_RGB_FXT1:<br> > - case MESA_FORMAT_RGBA_FXT1:<br> > - case MESA_FORMAT_RGB_DXT1:<br> > - case MESA_FORMAT_RGBA_DXT1:<br> > - case MESA_FORMAT_RGBA_DXT3:<br> > - case MESA_FORMAT_RGBA_DXT5:<br> > - case MESA_FORMAT_SRGB_DXT1:<br> > - case MESA_FORMAT_SRGBA_DXT1:<br> > - case MESA_FORMAT_SRGBA_DXT3:<br> > - case MESA_FORMAT_SRGBA_DXT5:<br> > - case MESA_FORMAT_R_RGTC1_UNORM:<br> > - case MESA_FORMAT_R_RGTC1_SNORM:<br> > - case MESA_FORMAT_RG_RGTC2_UNORM:<br> > - case MESA_FORMAT_RG_RGTC2_SNORM:<br> > - case MESA_FORMAT_L_LATC1_UNORM:<br> > - case MESA_FORMAT_L_LATC1_SNORM:<br> > - case MESA_FORMAT_LA_LATC2_UNORM:<br> > - case MESA_FORMAT_LA_LATC2_SNORM:<br> > - case MESA_FORMAT_ETC1_RGB8:<br> > - case MESA_FORMAT_ETC2_RGB8:<br> > - case MESA_FORMAT_ETC2_SRGB8:<br> > - case MESA_FORMAT_ETC2_RGBA8_EAC:<br> > - case MESA_FORMAT_ETC2_SRGB8_ALPHA8_EAC:<br> > - case MESA_FORMAT_ETC2_R11_EAC:<br> > - case MESA_FORMAT_ETC2_RG11_EAC:<br> > - case MESA_FORMAT_ETC2_SIGNED_R11_EAC:<br> > - case MESA_FORMAT_ETC2_SIGNED_RG11_EAC:<br> > - case MESA_FORMAT_ETC2_RGB8_PUNCHTHROUGH_ALPHA1:<br> > - case MESA_FORMAT_ETC2_SRGB8_PUNCHTHROUGH_ALPHA1:<br> > - case MESA_FORMAT_BPTC_RGBA_UNORM:<br> > - case MESA_FORMAT_BPTC_SRGB_ALPHA_UNORM:<br> > - case MESA_FORMAT_BPTC_RGB_SIGNED_FLOAT:<br> > - case MESA_FORMAT_BPTC_RGB_UNSIGNED_FLOAT:<br> > - /* XXX generate error instead? */<br> > - *datatype = GL_UNSIGNED_BYTE;<br> > - *comps = 0;<br> > - return;<br> > -<br> <br> </div></div>In other words, rename the function to<br> _mesa_uncompressed_format_to_type_and_comps(); keep all the format<br> cases; and add an assertion or _mesa_problem() for the compressed cases.<br> </blockquote></div><br></div></div></div><div class="gmail_extra">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. </div><div class="gmail_extra"><br></div><div class="gmail_extra">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. </div><div class="gmail_extra"><br></div><div class="gmail_extra">We could remove this redundancy and only update the cases for the formats that matter (uncompressed formats) by doing the following:</div><div class="gmail_extra">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. </div><div class="gmail_extra">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. </div><div class="gmail_extra">3. in src/mesa/main/tests/ : make _mesa_test_formats() a test we run during make check. </div><div class="gmail_extra"><br></div><div class="gmail_extra">We could also remove the redundant checking (but keep adding unnecessary compressed cases by doing the following:</div><div class="gmail_extra">1. keep the compressed cases as suggested</div><div class="gmail_extra">2. delete check_format_to_type_and_comps() (since the compiler does this for us).</div><span class="HOEnZb"><font color="#888888"><div class="gmail_extra"><br></div><div class="gmail_extra">Nanley </div></font></span></div> </blockquote></div><br></div>