[PATCH v2 4/4] apps: cam: sdl_sink: Support more single-plane formats

[Laurent Pinchart]

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 at 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 at ideasonboard.com>
> > 
> >>   	default:
> >>   		std::cerr << "Unsupported pixel format "
> >>   			  << cfg.pixelFormat.toString() << std::endl;

-- 
Regards,

Laurent Pinchart