[libcamera-devel] [PATCH] cam: sdl: Use uint32_t in place of SDL_PixelFormatEnum

[Eric Curtin]

I have a similar opinion to Laurent on this, I prefer
SDL_PixelFormatEnum because it's more descriptive, but happy the way
this patch is also.

Reviewed-by: Eric Curtin <ecurtin at redhat.com>

Is mise le meas/Regards,

Eric Curtin

On Fri, 15 Jul 2022 at 09:38, Jacopo Mondi via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Kieran
>
> On Fri, Jul 15, 2022 at 09:22:15AM +0100, Kieran Bingham wrote:
> > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:24:23)
> > > The SDL_PixelFormatEnum type has been introduced in libsdl by
> > > 1a4c0d4e17e6 ("Fixed bug 4377 - SDL_PIXELFORMAT enum is anonymous, which
> > > prevents its use in a templated function") which is only available after
> > > release 2.0.10 of the library.
> > >
> > > Debian 10 ships libsdl at version 2.0.9 and building cam with sdl
> > > support there fails with error:
> > > ./src/cam/sdl_texture.h:27:8: error: ‘SDL_PixelFormatEnum’ does not name
> > > a type; did you mean ‘SDL_PixelFormat’?
> > >
> > > Fix that by using the base type uint32_t in place of
> > > SDL_PixelFormatEnum.
> >
> > Is there scope to fix this by checking the version of the header being
> > compiled and defining SDL_PixelFormatEnum instead?
> >
> > I would expect something in src/cam/sdl_texture.h such as:
> >
> > #if !SDL_VERSION_ATLEAST(2.0.10)
> > typedef uint32_t SDL_PixelFormatEnum;
> > #endif
> >
>
> I don't see any benefit to be honest.
>
> The library itself uses uint32 in it's public API
>
> /**
>  * Convert one of the enumerated pixel formats to a bpp value and RGBA masks.
>  *
>  * \param format one of the SDL_PixelFormatEnum values
>  * \param bpp a bits per pixel value; usually 15, 16, or 32
>  * \param Rmask a pointer filled in with the red mask for the format
>  * \param Gmask a pointer filled in with the green mask for the format
>  * \param Bmask a pointer filled in with the blue mask for the format
>  * \param Amask a pointer filled in with the alpha mask for the format
>  * \returns SDL_TRUE on success or SDL_FALSE if the conversion wasn't
>  *          possible; call SDL_GetError() for more information.
>  *
>  * \since This function is available since SDL 2.0.0.
>  *
>  * \sa SDL_MasksToPixelFormatEnum
>  */
> extern DECLSPEC SDL_bool SDLCALL SDL_PixelFormatEnumToMasks(Uint32 format,
>                                                             int *bpp,
>                                                             Uint32 * Rmask,
>                                                             Uint32 * Gmask,
>                                                             Uint32 * Bmask,
>                                                             Uint32 * Amask);
>
>
> Looking at commit history libsdl is also progressively removing the
> "Enum" suffix from their defined types, I would not tie ourselves to
> such moving parts for no benefit.
>
>
>
> >
> > --
> > Kieran
> >
> >
> > > Reported-by: https://buildbot.libcamera.org/#/builders/6/builds/355
> > > Fixes: 11554a259f4e ("cam: sdl_sink: Add SDL sink with initial YUYV support")
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/cam/sdl_texture.cpp | 2 +-
> > >  src/cam/sdl_texture.h   | 4 ++--
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/cam/sdl_texture.cpp b/src/cam/sdl_texture.cpp
> > > index 2ca2add2f00c..02a8ff28b669 100644
> > > --- a/src/cam/sdl_texture.cpp
> > > +++ b/src/cam/sdl_texture.cpp
> > > @@ -9,7 +9,7 @@
> > >
> > >  #include <iostream>
> > >
> > > -SDLTexture::SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > +SDLTexture::SDLTexture(const SDL_Rect &rect, uint32_t pixelFormat,
> > >                        const int pitch)
> > >         : ptr_(nullptr), rect_(rect), pixelFormat_(pixelFormat), pitch_(pitch)
> > >  {
> > > diff --git a/src/cam/sdl_texture.h b/src/cam/sdl_texture.h
> > > index 9097479846f7..2275b4e605d9 100644
> > > --- a/src/cam/sdl_texture.h
> > > +++ b/src/cam/sdl_texture.h
> > > @@ -14,7 +14,7 @@
> > >  class SDLTexture
> > >  {
> > >  public:
> > > -       SDLTexture(const SDL_Rect &rect, SDL_PixelFormatEnum pixelFormat,
> > > +       SDLTexture(const SDL_Rect &rect, uint32_t pixelFormat,
> > >                    const int pitch);
> > >         virtual ~SDLTexture();
> > >         int create(SDL_Renderer *renderer);
> > > @@ -24,6 +24,6 @@ public:
> > >  protected:
> > >         SDL_Texture *ptr_;
> > >         const SDL_Rect rect_;
> > > -       const SDL_PixelFormatEnum pixelFormat_;
> > > +       const uint32_t pixelFormat_;
> > >         const int pitch_;
> > >  };
> > > --
> > > 2.36.1
> > >
>