[PATCH] cam: Convert RGB variations to BGR

[Barnabás Pőcze]

Hi


2025. 04. 13. 10:21 keltezéssel, Pavel Machek írta:
> swIsp with libcamera produces ARGB data by default, which needs
> conversion during write. Implement it and similar conversions.
>      
> Signed-off-by: Pavel Machek <pavel at ucw.cz>
> 
> diff --git a/src/apps/common/ppm_writer.cpp b/src/apps/common/ppm_writer.cpp
> index 368de8bf..ddbd3263 100644
> --- a/src/apps/common/ppm_writer.cpp
> +++ b/src/apps/common/ppm_writer.cpp
> @@ -20,8 +20,56 @@ int PPMWriter::write(const char *filename,
>   		     const StreamConfiguration &config,
>   		     const Span<uint8_t> &data)
>   {
> -	if (config.pixelFormat != formats::BGR888) {
> -		std::cerr << "Only BGR888 output pixel format is supported ("
> +	int r_pos, g_pos, b_pos, bpp;

I think the "pos" variables should probably be camelCase.
And please add an empty line after them.


> +	switch (config.pixelFormat) {
> +	case libcamera::formats::R8:
> +		r_pos = 0;
> +		g_pos = 0;
> +		b_pos = 0;
> +		bpp = 1;
> +		break;
> +	case libcamera::formats::RGB888:
> +		r_pos = 2;
> +		g_pos = 1;
> +		b_pos = 0;
> +		bpp = 3;
> +		break;
> +	case libcamera::formats::BGR888:
> +		r_pos = 0;
> +		g_pos = 1;
> +		b_pos = 2;
> +		bpp = 3;
> +		break;
> +	case libcamera::formats::ARGB8888:
> +	case libcamera::formats::XRGB8888:
> +		r_pos = 2;
> +		g_pos = 1;
> +		b_pos = 0;
> +		bpp = 4;
> +		break;
> +	case libcamera::formats::RGBA8888:
> +	case libcamera::formats::RGBX8888:
> +		r_pos = 3;
> +		g_pos = 2;
> +		b_pos = 1;
> +		bpp = 4;
> +		break;
> +	case libcamera::formats::ABGR8888:
> +	case libcamera::formats::XBGR8888:
> +		r_pos = 0;
> +		g_pos = 1;
> +		b_pos = 2;
> +		bpp = 4;
> +		break;
> +	case libcamera::formats::BGRA8888:
> +	case libcamera::formats::BGRX8888:
> +		r_pos = 1;
> +		g_pos = 2;
> +		b_pos = 3;
> +		bpp = 4;
> +		break;
> +	default:
> +		std::cerr << "Only RGB output pixel formats are supported ("
>   			  << config.pixelFormat << " requested)" << std::endl;
>   		return -EINVAL;
>   	}
> @@ -41,14 +89,35 @@ int PPMWriter::write(const char *filename,
>   	}
>   
>   	const unsigned int rowLength = config.size.width * 3;
> -	const char *row = reinterpret_cast<const char *>(data.data());
> -	for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) {
> -		output.write(row, rowLength);
> +	// Output without any conversion will be slightly faster
> +	if (config.pixelFormat == formats::BGR888) {
> +		const char *row = reinterpret_cast<const char *>(data.data());
> +		for (unsigned int y = 0; y < config.size.height; y++, row += config.stride) {
> +			output.write(row, rowLength);
> +			if (!output) {
> +				std::cerr << "Failed to write image data at row " << y << std::endl;
> +				return -EIO;
> +			}
> +		}
> +		return 0;
> +	}
> +
> +	const unsigned int inputRowBytes = config.stride; // should be >= width * 4

What is the reasoning for saving this into a separate variable here? But not above in
the `BGR888` case? And not `config.size`?

And why "should be >= width * 4"?


> +	// Create a temporary buffer to hold one output row.
> +	std::vector<char> rowBuffer(rowLength);
> +	const uint8_t *row = reinterpret_cast<const uint8_t *>(data.data());
> +	for (unsigned int y = 0; y < config.size.height; y++, row += inputRowBytes) {
> +		for (unsigned int x = 0; x < config.size.width; x++) {
> +			const uint8_t *pixel = row + x * bpp;
> +			rowBuffer[x * 3 + 0] = static_cast<char>(pixel[r_pos]);
> +			rowBuffer[x * 3 + 1] = static_cast<char>(pixel[g_pos]);
> +			rowBuffer[x * 3 + 2] = static_cast<char>(pixel[b_pos]);
> +		}

Empty line here please.


> +		output.write(rowBuffer.data(), rowLength);
>   		if (!output) {
>   			std::cerr << "Failed to write image data at row " << y << std::endl;
>   			return -EIO;
>   		}
>   	}
> -

Don't remove this.


>   	return 0;
>   }
> 


As for performance, I have briefly taken a look at the x86 assembly of the inner
loop, it looks pretty acceptable to me. Apart from the fact that the vector's
operator[] adds three branches because meson sets `_GLIBCXX_ASSERTIONS=1` unless
`b_ndebug=true`. So that is not too good, but it is probably not too significant.

But whether or not the soft ISP will support other formats, I think it's useful to
support multiple formats here, even at the cost of conversion. Especially since PPM
is not a container or such, so if the user explicitly selects it, I think they
expect that anything coming out of the camera will be converted appropriately.

In addition, it appears to me that qcam already has a sizable set of format
conversions implemented. I wonder if those could be used somehow.


Regards,
Barnabás Pőcze