[PATCH] cam: Convert RGB variations to BGR

[Laurent Pinchart]

Hi Pavel,

On Sat, Apr 19, 2025 at 12:05:34PM +0200, Pavel Machek wrote:
> 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 at 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 that's the only change required, yes.

> > > +	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.

I wonder if it would make sense for the soft ISP to support other RGB
formats. CC'ing Milan and Hans to get their opinion.

-- 
Regards,

Laurent Pinchart