[libcamera-devel] [PATCH v2] qcam: dng_writer: Add support for IPU3 Bayer formats

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Jun 11, 2020 at 03:45:36AM +0200, Niklas Söderlund wrote:
> Add support for the Bayer formats produced on the IPU3. The format uses
> a memory layout that is hard to repack and keep the 10-bit sample size,
> therefore scale the samples to 16-bit when creating the scanlines.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/qcam/dng_writer.cpp | 121 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
> 
> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> index cbd8bed3e6d02269..330b169af25b7ac7 100644
> --- a/src/qcam/dng_writer.cpp
> +++ b/src/qcam/dng_writer.cpp
> @@ -82,6 +82,103 @@ void thumbScanlineSBGGRxxP(const FormatInfo &info, void *output,
>  	}
>  }
>  
> +void packScanlineIPU3(void *output, const void *input, unsigned int width)
> +{
> +	const uint8_t *in = static_cast<const uint8_t *>(input);
> +	uint16_t *out = static_cast<uint16_t *>(output);
> +
> +	/*
> +	 * Upscale the 10-bit format to 16-bit as it's not trivial to pack it
> +	 * as 10-bit without gaps.
> +	 *
> +	 * \todo Improve packing to keep the 10-bit sample size.
> +	 */

I think we could use an intermediate line buffer. When we'll address
that, not now :-)

> +	unsigned int x = 0;
> +	while (true) {
> +		for (unsigned int i = 0; i < 6; i++) {
> +			*out++ = (in[1] & 0x03) << 14 | (in[0] & 0xff) << 6;
> +			if (++x >= width)
> +				break;
> +
> +			*out++ = (in[2] & 0x0f) << 12 | (in[1] & 0xfc) << 4;
> +			if (++x >= width)
> +				break;
> +
> +			*out++ = (in[3] & 0x3f) << 10 | (in[2] & 0xf0) << 2;
> +			if (++x >= width)
> +				break;
> +
> +			*out++ = (in[4] & 0xff) <<  8 | (in[3] & 0xc0) << 0;
> +			if (++x >= width)
> +				break;
> +
> +			in += 5;
> +		}
> +
> +		*out++ = (in[1] & 0x03) << 14 | (in[0] & 0xff) << 6;

You will execute this even if the above for loop stopped due to EOL
being reached. You could replace all the break by return to fix that.

What an awful format... I wonder how they handle it at the hardware
level, I suppose there's an efficient mechanism.

> +		if (++x >= width)
> +			break;
> +
> +		in += 2;
> +	}
> +}
> +
> +void thumbScanlineIPU3(const FormatInfo &info, void *output,
> +		       const void *input, unsigned int width,
> +		       unsigned int stride)
> +{
> +	uint8_t *out = static_cast<uint8_t *>(output);
> +
> +	for (unsigned int x = 0; x < width; x++) {
> +		unsigned int pixel = x * 16;
> +		unsigned int block = pixel / 25;
> +		unsigned int pixelInBlock = pixel - block * 25;
> +
> +		/*
> +		 * If the pixel is the last in the block cheat a little and
> +		 * move one pixel backward to avoid reading between two blocks
> +		 * and having to deal with the padding bits.
> +		 */
> +		if (pixelInBlock == 24)
> +			pixelInBlock--;

I think that's fair enough. I wonder if we could switch (pixelInBlock)
below instead of switch (pixelInBlock % 4), in order to handle case 24
explicitly, but if it would require any additional complexity, I think
we can live with this hack.

> +
> +		const uint8_t *in = static_cast<const uint8_t *>(input)
> +			+ block * 32 + (pixelInBlock / 4) * 5;

Aligning + under = ?

> +
> +		uint16_t val1, val2, val3, val4;
> +		switch (pixelInBlock % 4) {
> +		case 0:
> +			val1 = (in[1] & 0x03) << 14 | (in[0] & 0xff) << 6;
> +			val2 = (in[2] & 0x0f) << 12 | (in[1] & 0xfc) << 4;
> +			val3 = (in[stride + 1] & 0x03) << 14 | (in[stride + 0] & 0xff) << 6;
> +			val4 = (in[stride + 2] & 0x0f) << 12 | (in[stride + 1] & 0xfc) << 4;
> +			break;
> +		case 1:
> +			val1 = (in[2] & 0x0f) << 12 | (in[1] & 0xfc) << 4;
> +			val2 = (in[3] & 0x3f) << 10 | (in[2] & 0xf0) << 2;
> +			val3 = (in[stride + 2] & 0x0f) << 12 | (in[stride + 1] & 0xfc) << 4;
> +			val4 = (in[stride + 3] & 0x3f) << 10 | (in[stride + 2] & 0xf0) << 2;
> +			break;
> +		case 2:
> +			val1 = (in[3] & 0x3f) << 10 | (in[2] & 0xf0) << 2;
> +			val2 = (in[4] & 0xff) <<  8 | (in[3] & 0xc0) << 0;
> +			val3 = (in[stride + 3] & 0x3f) << 10 | (in[stride + 2] & 0xf0) << 2;
> +			val4 = (in[stride + 4] & 0xff) <<  8 | (in[stride + 3] & 0xc0) << 0;
> +			break;
> +		case 3:
> +			val1 = (in[4] & 0xff) <<  8 | (in[3] & 0xc0) << 0;
> +			val2 = (in[6] & 0x03) << 14 | (in[5] & 0xff) << 6;
> +			val3 = (in[stride + 4] & 0xff) <<  8 | (in[stride + 3] & 0xc0) << 0;
> +			val4 = (in[stride + 6] & 0x03) << 14 | (in[stride + 5] & 0xff) << 6;
> +		}
> +
> +		uint8_t value = (val1 + val2 + val3 + val4) >> 8;

As val[1234] are 16-bit values, shouldn't you shift right by 10 to
convert to 8-bit and average the four values ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		*out++ = value;
> +		*out++ = value;
> +		*out++ = value;
> +	}
> +}
> +
>  static const std::map<PixelFormat, FormatInfo> formatInfo = {
>  	{ PixelFormat(DRM_FORMAT_SBGGR10, MIPI_FORMAT_MOD_CSI2_PACKED), {
>  		.bitsPerSample = 10,
> @@ -131,6 +228,30 @@ static const std::map<PixelFormat, FormatInfo> formatInfo = {
>  		.packScanline = packScanlineSBGGR12P,
>  		.thumbScanline = thumbScanlineSBGGRxxP,
>  	} },
> +	{ PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED), {
> +		.bitsPerSample = 16,
> +		.pattern = { CFAPatternBlue, CFAPatternGreen, CFAPatternGreen, CFAPatternRed },
> +		.packScanline = packScanlineIPU3,
> +		.thumbScanline = thumbScanlineIPU3,
> +	} },
> +	{ PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED), {
> +		.bitsPerSample = 16,
> +		.pattern = { CFAPatternGreen, CFAPatternBlue, CFAPatternRed, CFAPatternGreen },
> +		.packScanline = packScanlineIPU3,
> +		.thumbScanline = thumbScanlineIPU3,
> +	} },
> +	{ PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED), {
> +		.bitsPerSample = 16,
> +		.pattern = { CFAPatternGreen, CFAPatternRed, CFAPatternBlue, CFAPatternGreen },
> +		.packScanline = packScanlineIPU3,
> +		.thumbScanline = thumbScanlineIPU3,
> +	} },
> +	{ PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED), {
> +		.bitsPerSample = 16,
> +		.pattern = { CFAPatternRed, CFAPatternGreen, CFAPatternGreen, CFAPatternBlue },
> +		.packScanline = packScanlineIPU3,
> +		.thumbScanline = thumbScanlineIPU3,
> +	} },
>  };
>  
>  int DNGWriter::write(const char *filename, const Camera *camera,

-- 
Regards,

Laurent Pinchart