| Message ID | 20250425104703.805170-5-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-04-25 11:47:03) > With the newly introduced `SDLTexture1Plane` it is easy to handle > any single-plane format that has an SDL equivalent. So use it for > more YUV and RGB formats. > > The mapping of RGB formats is not entirely straightforward because > `SDL_PIXELFORMAT_ZZZ...888...` defines a format where the order of > the components is endian dependent, while libcamera's `ZZZ...888...` > formats are derived from the matching DRM formats, and the RGB formats > in question are defined to be little-endian there. So the > endian-independent `SDL_PIXELFORMAT_{ZZZ24,ZZZZ32}` are used. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/apps/cam/sdl_sink.cpp | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp > index b295675dc..2edbb523d 100644 > --- a/src/apps/cam/sdl_sink.cpp > +++ b/src/apps/cam/sdl_sink.cpp > @@ -77,6 +77,42 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > case libcamera::formats::YUYV: > texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YUY2, cfg.stride); > break; > + case libcamera::formats::UYVY: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_UYVY, cfg.stride); > + break; > + case libcamera::formats::YVYU: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YVYU, cfg.stride); > + break; > + case libcamera::formats::ARGB8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRA32, cfg.stride); > + break; > + case libcamera::formats::XRGB8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRX32, cfg.stride); > + break; > + case libcamera::formats::RGBA8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ABGR32, cfg.stride); > + break; > + case libcamera::formats::RGBX8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XBGR32, cfg.stride); > + break; > + case libcamera::formats::ABGR8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBA32, cfg.stride); > + break; > + case libcamera::formats::XBGR8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBX32, cfg.stride); > + break; > + case libcamera::formats::BGRA8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ARGB32, cfg.stride); > + break; > + case libcamera::formats::BGRX8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XRGB32, cfg.stride); > + break; > + case libcamera::formats::RGB888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGR24, cfg.stride); > + break; > + case libcamera::formats::BGR888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGB24, cfg.stride); > + break; all of those are in the ordering I would expect: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > default: > std::cerr << "Unsupported pixel format " > << cfg.pixelFormat.toString() << std::endl; > -- > 2.49.0 >
Hi Barnabás, Thank you for the patch. On Fri, Apr 25, 2025 at 12:47:03PM +0200, Barnabás Pőcze wrote: > With the newly introduced `SDLTexture1Plane` it is easy to handle > any single-plane format that has an SDL equivalent. So use it for > more YUV and RGB formats. > > The mapping of RGB formats is not entirely straightforward because > `SDL_PIXELFORMAT_ZZZ...888...` defines a format where the order of > the components is endian dependent, while libcamera's `ZZZ...888...` > formats are derived from the matching DRM formats, and the RGB formats > in question are defined to be little-endian there. So the > endian-independent `SDL_PIXELFORMAT_{ZZZ24,ZZZZ32}` are used. Format mapping is always painful :-( > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/apps/cam/sdl_sink.cpp | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp > index b295675dc..2edbb523d 100644 > --- a/src/apps/cam/sdl_sink.cpp > +++ b/src/apps/cam/sdl_sink.cpp > @@ -77,6 +77,42 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > case libcamera::formats::YUYV: > texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YUY2, cfg.stride); > break; > + case libcamera::formats::UYVY: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_UYVY, cfg.stride); > + break; > + case libcamera::formats::YVYU: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YVYU, cfg.stride); > + break; > + case libcamera::formats::ARGB8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRA32, cfg.stride); > + break; > + case libcamera::formats::XRGB8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRX32, cfg.stride); > + break; > + case libcamera::formats::RGBA8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ABGR32, cfg.stride); > + break; > + case libcamera::formats::RGBX8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XBGR32, cfg.stride); > + break; > + case libcamera::formats::ABGR8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBA32, cfg.stride); > + break; > + case libcamera::formats::XBGR8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBX32, cfg.stride); > + break; > + case libcamera::formats::BGRA8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ARGB32, cfg.stride); > + break; > + case libcamera::formats::BGRX8888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XRGB32, cfg.stride); > + break; > + case libcamera::formats::RGB888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGR24, cfg.stride); > + break; > + case libcamera::formats::BGR888: > + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGB24, cfg.stride); > + break; I'm tempted to avoid all the constructor calls by instead adding a sdlFormat variable and filling it in the switch, and then creating the texture after the switch. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > default: > std::cerr << "Unsupported pixel format " > << cfg.pixelFormat.toString() << std::endl;
Hi 2025. 04. 25. 14:45 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Fri, Apr 25, 2025 at 12:47:03PM +0200, Barnabás Pőcze wrote: >> With the newly introduced `SDLTexture1Plane` it is easy to handle >> any single-plane format that has an SDL equivalent. So use it for >> more YUV and RGB formats. >> >> The mapping of RGB formats is not entirely straightforward because >> `SDL_PIXELFORMAT_ZZZ...888...` defines a format where the order of >> the components is endian dependent, while libcamera's `ZZZ...888...` >> formats are derived from the matching DRM formats, and the RGB formats >> in question are defined to be little-endian there. So the >> endian-independent `SDL_PIXELFORMAT_{ZZZ24,ZZZZ32}` are used. > > Format mapping is always painful :-( > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/apps/cam/sdl_sink.cpp | 36 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp >> index b295675dc..2edbb523d 100644 >> --- a/src/apps/cam/sdl_sink.cpp >> +++ b/src/apps/cam/sdl_sink.cpp >> @@ -77,6 +77,42 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) >> case libcamera::formats::YUYV: >> texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YUY2, cfg.stride); >> break; >> + case libcamera::formats::UYVY: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_UYVY, cfg.stride); >> + break; >> + case libcamera::formats::YVYU: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YVYU, cfg.stride); >> + break; >> + case libcamera::formats::ARGB8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRA32, cfg.stride); >> + break; >> + case libcamera::formats::XRGB8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRX32, cfg.stride); >> + break; >> + case libcamera::formats::RGBA8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ABGR32, cfg.stride); >> + break; >> + case libcamera::formats::RGBX8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XBGR32, cfg.stride); >> + break; >> + case libcamera::formats::ABGR8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBA32, cfg.stride); >> + break; >> + case libcamera::formats::XBGR8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBX32, cfg.stride); >> + break; >> + case libcamera::formats::BGRA8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ARGB32, cfg.stride); >> + break; >> + case libcamera::formats::BGRX8888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XRGB32, cfg.stride); >> + break; >> + case libcamera::formats::RGB888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGR24, cfg.stride); >> + break; >> + case libcamera::formats::BGR888: >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGB24, cfg.stride); >> + break; > > I'm tempted to avoid all the constructor calls by instead adding a > sdlFormat variable and filling it in the switch, and then creating the > texture after the switch. Yes, indeed, and I had thought about that for a bit, but there is also SDLTextureNV12 (conditionally), and I didn't really see a good way to reconcile the two. Regards, Barnabás Pőcze > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> default: >> std::cerr << "Unsupported pixel format " >> << cfg.pixelFormat.toString() << std::endl; >
On Fri, Apr 25, 2025 at 02:58:08PM +0200, Barnabás Pőcze wrote: > Hi > > > 2025. 04. 25. 14:45 keltezéssel, Laurent Pinchart írta: > > Hi Barnabás, > > > > Thank you for the patch. > > > > On Fri, Apr 25, 2025 at 12:47:03PM +0200, Barnabás Pőcze wrote: > >> With the newly introduced `SDLTexture1Plane` it is easy to handle > >> any single-plane format that has an SDL equivalent. So use it for > >> more YUV and RGB formats. > >> > >> The mapping of RGB formats is not entirely straightforward because > >> `SDL_PIXELFORMAT_ZZZ...888...` defines a format where the order of > >> the components is endian dependent, while libcamera's `ZZZ...888...` > >> formats are derived from the matching DRM formats, and the RGB formats > >> in question are defined to be little-endian there. So the > >> endian-independent `SDL_PIXELFORMAT_{ZZZ24,ZZZZ32}` are used. > > > > Format mapping is always painful :-( > > > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/apps/cam/sdl_sink.cpp | 36 ++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 36 insertions(+) > >> > >> diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp > >> index b295675dc..2edbb523d 100644 > >> --- a/src/apps/cam/sdl_sink.cpp > >> +++ b/src/apps/cam/sdl_sink.cpp > >> @@ -77,6 +77,42 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > >> case libcamera::formats::YUYV: > >> texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YUY2, cfg.stride); > >> break; > >> + case libcamera::formats::UYVY: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_UYVY, cfg.stride); > >> + break; > >> + case libcamera::formats::YVYU: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YVYU, cfg.stride); > >> + break; > >> + case libcamera::formats::ARGB8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRA32, cfg.stride); > >> + break; > >> + case libcamera::formats::XRGB8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRX32, cfg.stride); > >> + break; > >> + case libcamera::formats::RGBA8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ABGR32, cfg.stride); > >> + break; > >> + case libcamera::formats::RGBX8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XBGR32, cfg.stride); > >> + break; > >> + case libcamera::formats::ABGR8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBA32, cfg.stride); > >> + break; > >> + case libcamera::formats::XBGR8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBX32, cfg.stride); > >> + break; > >> + case libcamera::formats::BGRA8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ARGB32, cfg.stride); > >> + break; > >> + case libcamera::formats::BGRX8888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XRGB32, cfg.stride); > >> + break; > >> + case libcamera::formats::RGB888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGR24, cfg.stride); > >> + break; > >> + case libcamera::formats::BGR888: > >> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGB24, cfg.stride); > >> + break; > > > > I'm tempted to avoid all the constructor calls by instead adding a > > sdlFormat variable and filling it in the switch, and then creating the > > texture after the switch. > > Yes, indeed, and I had thought about that for a bit, but there is also > SDLTextureNV12 (conditionally), and I didn't really see a good way to > reconcile the two. switch (cfg.pixelFormat) { #ifdef HAVE_LIBJPEG case libcamera::formats::MJPEG: texture_ = std::make_unique<SDLTextureMJPG>(rect_); break; #endif #if SDL_VERSION_ATLEAST(2, 0, 16) case libcamera::formats::NV12: texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); break; #endif case libcamera::formats::YUYV: sdlFormat = SDL_PIXELFORMAT_YUY2; break; case libcamera::formats::UYVY: sdlFormat = SDL_PIXELFORMAT_UYVY; break; ... default: std::cerr << "Unsupported pixel format " << cfg.pixelFormat.toString() << std::endl; return -EINVAL; }; if (sdlFormat) texture_ = std::make_unique<SDLTexture1Plane>(rect_, sdlFormat, cfg.stride); > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> default: > >> std::cerr << "Unsupported pixel format " > >> << cfg.pixelFormat.toString() << std::endl;
Hi 2025. 04. 25. 16:41 keltezéssel, Laurent Pinchart írta: > On Fri, Apr 25, 2025 at 02:58:08PM +0200, Barnabás Pőcze wrote: >> Hi >> >> >> 2025. 04. 25. 14:45 keltezéssel, Laurent Pinchart írta: >>> Hi Barnabás, >>> >>> Thank you for the patch. >>> >>> On Fri, Apr 25, 2025 at 12:47:03PM +0200, Barnabás Pőcze wrote: >>>> With the newly introduced `SDLTexture1Plane` it is easy to handle >>>> any single-plane format that has an SDL equivalent. So use it for >>>> more YUV and RGB formats. >>>> >>>> The mapping of RGB formats is not entirely straightforward because >>>> `SDL_PIXELFORMAT_ZZZ...888...` defines a format where the order of >>>> the components is endian dependent, while libcamera's `ZZZ...888...` >>>> formats are derived from the matching DRM formats, and the RGB formats >>>> in question are defined to be little-endian there. So the >>>> endian-independent `SDL_PIXELFORMAT_{ZZZ24,ZZZZ32}` are used. >>> >>> Format mapping is always painful :-( >>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/apps/cam/sdl_sink.cpp | 36 ++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 36 insertions(+) >>>> >>>> diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp >>>> index b295675dc..2edbb523d 100644 >>>> --- a/src/apps/cam/sdl_sink.cpp >>>> +++ b/src/apps/cam/sdl_sink.cpp >>>> @@ -77,6 +77,42 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) >>>> case libcamera::formats::YUYV: >>>> texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YUY2, cfg.stride); >>>> break; >>>> + case libcamera::formats::UYVY: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_UYVY, cfg.stride); >>>> + break; >>>> + case libcamera::formats::YVYU: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YVYU, cfg.stride); >>>> + break; >>>> + case libcamera::formats::ARGB8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRA32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::XRGB8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRX32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::RGBA8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ABGR32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::RGBX8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XBGR32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::ABGR8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBA32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::XBGR8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBX32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::BGRA8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ARGB32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::BGRX8888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XRGB32, cfg.stride); >>>> + break; >>>> + case libcamera::formats::RGB888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGR24, cfg.stride); >>>> + break; >>>> + case libcamera::formats::BGR888: >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGB24, cfg.stride); >>>> + break; >>> >>> I'm tempted to avoid all the constructor calls by instead adding a >>> sdlFormat variable and filling it in the switch, and then creating the >>> texture after the switch. >> >> Yes, indeed, and I had thought about that for a bit, but there is also >> SDLTextureNV12 (conditionally), and I didn't really see a good way to >> reconcile the two. > > > switch (cfg.pixelFormat) { > #ifdef HAVE_LIBJPEG > case libcamera::formats::MJPEG: > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > break; > #endif > #if SDL_VERSION_ATLEAST(2, 0, 16) > case libcamera::formats::NV12: > texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); > break; > #endif > case libcamera::formats::YUYV: > sdlFormat = SDL_PIXELFORMAT_YUY2; > break; > case libcamera::formats::UYVY: > sdlFormat = SDL_PIXELFORMAT_UYVY; > break; > ... > default: > std::cerr << "Unsupported pixel format " > << cfg.pixelFormat.toString() << std::endl; > return -EINVAL; > }; > > if (sdlFormat) > texture_ = std::make_unique<SDLTexture1Plane>(rect_, sdlFormat, cfg.stride); > I have looked at something like this, but I am not a fan at all. :( Regards, Barnabás Pőcze >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> default: >>>> std::cerr << "Unsupported pixel format " >>>> << cfg.pixelFormat.toString() << std::endl; >
On Mon, Apr 28, 2025 at 10:52:39AM +0200, Barnabás Pőcze wrote: > 2025. 04. 25. 16:41 keltezéssel, Laurent Pinchart írta: > > On Fri, Apr 25, 2025 at 02:58:08PM +0200, Barnabás Pőcze wrote: > >> 2025. 04. 25. 14:45 keltezéssel, Laurent Pinchart írta: > >>> On Fri, Apr 25, 2025 at 12:47:03PM +0200, Barnabás Pőcze wrote: > >>>> With the newly introduced `SDLTexture1Plane` it is easy to handle > >>>> any single-plane format that has an SDL equivalent. So use it for > >>>> more YUV and RGB formats. > >>>> > >>>> The mapping of RGB formats is not entirely straightforward because > >>>> `SDL_PIXELFORMAT_ZZZ...888...` defines a format where the order of > >>>> the components is endian dependent, while libcamera's `ZZZ...888...` > >>>> formats are derived from the matching DRM formats, and the RGB formats > >>>> in question are defined to be little-endian there. So the > >>>> endian-independent `SDL_PIXELFORMAT_{ZZZ24,ZZZZ32}` are used. > >>> > >>> Format mapping is always painful :-( > >>> > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>> --- > >>>> src/apps/cam/sdl_sink.cpp | 36 ++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 36 insertions(+) > >>>> > >>>> diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp > >>>> index b295675dc..2edbb523d 100644 > >>>> --- a/src/apps/cam/sdl_sink.cpp > >>>> +++ b/src/apps/cam/sdl_sink.cpp > >>>> @@ -77,6 +77,42 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > >>>> case libcamera::formats::YUYV: > >>>> texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YUY2, cfg.stride); > >>>> break; > >>>> + case libcamera::formats::UYVY: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_UYVY, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::YVYU: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YVYU, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::ARGB8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRA32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::XRGB8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRX32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::RGBA8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ABGR32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::RGBX8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XBGR32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::ABGR8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBA32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::XBGR8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBX32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::BGRA8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ARGB32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::BGRX8888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XRGB32, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::RGB888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGR24, cfg.stride); > >>>> + break; > >>>> + case libcamera::formats::BGR888: > >>>> + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGB24, cfg.stride); > >>>> + break; > >>> > >>> I'm tempted to avoid all the constructor calls by instead adding a > >>> sdlFormat variable and filling it in the switch, and then creating the > >>> texture after the switch. > >> > >> Yes, indeed, and I had thought about that for a bit, but there is also > >> SDLTextureNV12 (conditionally), and I didn't really see a good way to > >> reconcile the two. > > > > > > switch (cfg.pixelFormat) { > > #ifdef HAVE_LIBJPEG > > case libcamera::formats::MJPEG: > > texture_ = std::make_unique<SDLTextureMJPG>(rect_); > > break; > > #endif > > #if SDL_VERSION_ATLEAST(2, 0, 16) > > case libcamera::formats::NV12: > > texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); > > break; > > #endif > > case libcamera::formats::YUYV: > > sdlFormat = SDL_PIXELFORMAT_YUY2; > > break; > > case libcamera::formats::UYVY: > > sdlFormat = SDL_PIXELFORMAT_UYVY; > > break; > > ... > > default: > > std::cerr << "Unsupported pixel format " > > << cfg.pixelFormat.toString() << std::endl; > > return -EINVAL; > > }; > > > > if (sdlFormat) > > texture_ = std::make_unique<SDLTexture1Plane>(rect_, sdlFormat, cfg.stride); > > > > I have looked at something like this, but I am not a fan at all. :( I understand why and I probably have the same feeling. I still think it's a bit better than duplicating the calls though. > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>>> default: > >>>> std::cerr << "Unsupported pixel format " > >>>> << cfg.pixelFormat.toString() << std::endl;
diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp index b295675dc..2edbb523d 100644 --- a/src/apps/cam/sdl_sink.cpp +++ b/src/apps/cam/sdl_sink.cpp @@ -77,6 +77,42 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) case libcamera::formats::YUYV: texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YUY2, cfg.stride); break; + case libcamera::formats::UYVY: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_UYVY, cfg.stride); + break; + case libcamera::formats::YVYU: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_YVYU, cfg.stride); + break; + case libcamera::formats::ARGB8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRA32, cfg.stride); + break; + case libcamera::formats::XRGB8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGRX32, cfg.stride); + break; + case libcamera::formats::RGBA8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ABGR32, cfg.stride); + break; + case libcamera::formats::RGBX8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XBGR32, cfg.stride); + break; + case libcamera::formats::ABGR8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBA32, cfg.stride); + break; + case libcamera::formats::XBGR8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGBX32, cfg.stride); + break; + case libcamera::formats::BGRA8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_ARGB32, cfg.stride); + break; + case libcamera::formats::BGRX8888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_XRGB32, cfg.stride); + break; + case libcamera::formats::RGB888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_BGR24, cfg.stride); + break; + case libcamera::formats::BGR888: + texture_ = std::make_unique<SDLTexture1Plane>(rect_, SDL_PIXELFORMAT_RGB24, cfg.stride); + break; default: std::cerr << "Unsupported pixel format " << cfg.pixelFormat.toString() << std::endl;
With the newly introduced `SDLTexture1Plane` it is easy to handle any single-plane format that has an SDL equivalent. So use it for more YUV and RGB formats. The mapping of RGB formats is not entirely straightforward because `SDL_PIXELFORMAT_ZZZ...888...` defines a format where the order of the components is endian dependent, while libcamera's `ZZZ...888...` formats are derived from the matching DRM formats, and the RGB formats in question are defined to be little-endian there. So the endian-independent `SDL_PIXELFORMAT_{ZZZ24,ZZZZ32}` are used. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/apps/cam/sdl_sink.cpp | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)