cam: Convert RGB variations to BGR
diff mbox series

Message ID Z/t0DbvYXAjjRmVB@duo.ucw.cz
State New
Headers show
Series
  • cam: Convert RGB variations to BGR
Related show

Commit Message

Pavel Machek April 13, 2025, 8:21 a.m. UTC 
swIsp with libcamera produces ARGB data by default, which needs
conversion during write. Implement it and similar conversions.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

Comments

Laurent Pinchart April 13, 2025, 11:53 p.m. UTC | #1 
Hi Pavel,

Thank you for the patch.

On Sun, Apr 13, 2025 at 10:21:33AM +0200, Pavel Machek wrote:
> swIsp with libcamera produces ARGB data by default, which needs
> conversion during write. Implement it and similar conversions.
>     
> Signed-off-by: Pavel Machek <pavel@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;
> +	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

We use C-style coments.

> +	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
> +	// 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]);
> +		}

I'm curious, what's the performance impact of the conversion ?

> +		output.write(rowBuffer.data(), rowLength);
>  		if (!output) {
>  			std::cerr << "Failed to write image data at row " << y << std::endl;
>  			return -EIO;
>  		}
>  	}
> -
>  	return 0;
>  }
>
Pavel Machek April 19, 2025, 10:05 a.m. UTC | #2 
Hi!

> Thank you for the patch.
> 
> On Sun, Apr 13, 2025 at 10:21:33AM +0200, Pavel Machek wrote:
> > swIsp with libcamera produces ARGB data by default, which needs
> > conversion during write. Implement it and similar conversions.
> >     
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>

> > +	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
> 
> We use C-style coments.

Can you adjust that while applying?

> > +	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
> > +	// 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]);
> > +		}
> 
> I'm curious, what's the performance impact of the conversion ?

I hacked Librem 5 a lot to get libcamera working, and previous config
was not working at all, so ... I can't easily benchmark that.

So, this one is not free as it does one more copy of image
data. Compared to swIsp, it is going to be lost in the noise.

There's easy optimalization in the original case not to do writes per
row if width == stride. There are other options, but they will
complicate the code.

Best regards,
								Pavel

Patch
diff mbox series 

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;
+	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
+	// 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]);
+		}
+		output.write(rowBuffer.data(), rowLength);
 		if (!output) {
 			std::cerr << "Failed to write image data at row " << y << std::endl;
 			return -EIO;
 		}
 	}
-
 	return 0;
 }