[Mesa-dev] [PATCH v3] mesa/formats: refactor by removing compressed formats

[Nanley Chery]

Thanks for pointing out these errors. I'll fix them in the next version.

On Fri, Aug 21, 2015 at 2:34 PM, Chad Versace <chad.versace at intel.com>
wrote:

> On Wed 19 Aug 2015, Nanley Chery wrote:
> > From: Nanley Chery <nanley.g.chery at intel.com>
> >
> > All compressed formats return GL_FALSE and there isn't any evidence to
> > support that this behaviour would change. Remove all switch cases for
> > compressed formats.
> >
> > v2. Since the exhaustive switch is removed, add a gtest to ensure
> >     all formats are handled.
> > v3. Ensure that GL_NO_ERROR is set before returning.
> >
> > Cc: Brian Paul <brianp at vmware.com>
> > Cc: Chad Versace <chad.versace at intel.com>
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/mesa/drivers/dri/i915/intel_pixel_read.c |  2 +-
> >  src/mesa/drivers/dri/i915/intel_tex_image.c  |  2 +-
> >  src/mesa/main/formats.c                      | 58
> ++++++----------------------
> >  src/mesa/main/formats.h                      |  2 +-
> >  src/mesa/main/readpix.c                      |  2 +-
> >  src/mesa/main/tests/mesa_formats.cpp         |  7 +++-
> >  src/mesa/main/texgetimage.c                  |  2 +-
> >  src/mesa/main/texstore.c                     |  2 +-
> >  src/mesa/state_tracker/st_cb_readpixels.c    |  2 +-
> >  src/mesa/state_tracker/st_cb_texture.c       |  6 +--
> >  src/mesa/state_tracker/st_format.c           |  2 +-
> >  src/mesa/swrast/s_drawpix.c                  |  2 +-
> >  12 files changed, 29 insertions(+), 60 deletions(-)
> >
> > diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
> > index e58b917..1f221f1 100644
> > --- a/src/mesa/main/formats.c
> > +++ b/src/mesa/main/formats.c
> > @@ -1432,15 +1432,19 @@
> _mesa_uncompressed_format_to_type_and_comps(mesa_format format,
> >  GLboolean
> >  _mesa_format_matches_format_and_type(mesa_format mesa_format,
> >                                    GLenum format, GLenum type,
> > -                                     GLboolean swapBytes)
> > +                            GLboolean swapBytes, GLenum *error)
>
> The parameter alignment looks wrong here. Maybe it's just my email app,
> though.
>
> Please document the function's error behavior: what errors does it and
> emit and when.
>
> >  {
> >     const GLboolean littleEndian = _mesa_little_endian();
> > +   if (error)
> > +      *error = GL_NO_ERROR;
> >
> >     /* Note: When reading a GL format/type combination, the format lists
> channel
> >      * assignments from most significant channel in the type to least
> >      * significant.  A type with _REV indicates that the assignments are
> >      * swapped, so they are listed from least significant to most
> significant.
> >      *
> > +    * Compressed formats will fall through and return GL_FALSE.
> > +    *
> >      * For sanity, please keep this switch statement ordered the same as
> the
> >      * enums in formats.h.
> >      */
>
>
>
> > @@ -2024,8 +1983,13 @@ _mesa_format_matches_format_and_type(mesa_format
> mesa_format,
> >     case MESA_FORMAT_B8G8R8X8_SRGB:
> >     case MESA_FORMAT_X8R8G8B8_SRGB:
> >        return GL_FALSE;
> > +   default:
> > +      if (!_mesa_is_format_compressed(format)) {
> > +         assert(0);
> > +         if (error)
> > +            *error = GL_INVALID_ENUM;
> > +      }
> >     }
>
> The pattern in the default case...
>
>     if (!stuff) {
>         assert(0);
>         if (error)
>             *error = GL_INVALID_ENUM;
>     }
>
> would be better phrased as this equivalent code:
>
>     assert(stuff);
>     if (error)
>         *error = GL_INVALID_ENUM;
>
> > -
> >     return GL_FALSE;
> >  }
> >
> > diff --git a/src/mesa/main/formats.h b/src/mesa/main/formats.h
> > index e08247e..0634b78 100644
> > --- a/src/mesa/main/formats.h
> > +++ b/src/mesa/main/formats.h
> > @@ -675,7 +675,7 @@ _mesa_format_has_color_component(mesa_format format,
> int component);
> >  GLboolean
> >  _mesa_format_matches_format_and_type(mesa_format mesa_format,
> >                                    GLenum format, GLenum type,
> > -                                     GLboolean swapBytes);
> > +                            GLboolean swapBytes, GLenum *error);
> >
> >  #ifdef __cplusplus
> >  }
>
>
>
> > diff --git a/src/mesa/main/tests/mesa_formats.cpp
> b/src/mesa/main/tests/mesa_formats.cpp
> > index 7bf9147..dfc7d0e 100644
> > --- a/src/mesa/main/tests/mesa_formats.cpp
> > +++ b/src/mesa/main/tests/mesa_formats.cpp
> > @@ -49,11 +49,16 @@ TEST(MesaFormatsTest, FormatTypeAndComps)
> >         */
> >        if (!_mesa_is_format_compressed(f)) {
> >           GLenum datatype = 0;
> > +         GLenum error = 0;
> >           GLuint comps = 0;
> > -         _mesa_uncompressed_format_to_type_and_comps(f, &datatype,
> &comps);
> >
> >           /* If the datatype is zero, the format was not handled */
> > +         _mesa_uncompressed_format_to_type_and_comps(f, &datatype,
> &comps);
> >           ASSERT_NE(datatype, (GLenum)0);
> > +
> > +         /* If the error is INVALID_ENUM, the format was not handled */
> > +         _mesa_format_matches_format_and_type(f, f, datatype, GL_FALSE,
> &error);
> > +         ASSERT_NE(error, (GLenum)GL_INVALID_ENUM);
>
> Two comments on the new check:
>
>     - It is more robust to check that no error was returned
>       rather than checking that a specific error was not returned. That
>       is, this is more robust:
>
>         ASSERT_EQ(error, (GLenum)GL_NO_ERROR)
>
>     - One of the first two arguments to
>       _mesa_uncompressed_format_to_type_and_comps() is wrong. The patch
>       passes the same value for each argument, but parameters have
>       different types.
>
>
> > 2.5.0
>
> Git 2.5.0? Are you building Git from source? Fedora 22 is still shipping
> git-2.4.3.
>
I'm running Arch Linux.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150824/d1c7e4e3/attachment-0001.html>