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

[Barnabás Pőcze]

2025. 04. 28. 11:14 keltezéssel, Laurent Pinchart írta:
> 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 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);
>>>
>>
>> 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.

Please see the latest version: https://patchwork.libcamera.org/patch/23305/


> 
>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>>>
>>>>>>     	default:
>>>>>>     		std::cerr << "Unsupported pixel format "
>>>>>>     			  << cfg.pixelFormat.toString() << std::endl;
>