[libcamera-devel] [PATCH 2/2] utils: ipu3-pack: Provide a 10-bit bayer packing utility

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Sat, Jul 09, 2022 at 03:58:38AM +0530, Umang Jain via libcamera-devel wrote:
> Provide a 10-bit bayer packing utility for the unpacked data produced
> by ipu3-unpack. Single or multiple input unpacked files can be packed
> and concatenated together in a single output file.

I think I'd rather stick to the unix philosophy of small tools that
focus on a single purpose, and only process a single input file. Scripts
calling this can easily iterate over multiple input files and
concatenate the results. To make that easier, you may want to treat the
special value "-" for the output file as meaning stdout, so a script
could do

	for file in files ; do
		ipu3-pack $file - >> pack.bin
	done

> Use-case where this is useful is streaming IMGU with different
> test patterns on each frame. The output file generated by
> ipu3-pack can be directly fed to IMGU.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  utils/ipu3/ipu3-pack.c | 129 +++++++++++++++++++++++++++++++++++++++++
>  utils/ipu3/meson.build |   1 +
>  2 files changed, 130 insertions(+)
>  create mode 100644 utils/ipu3/ipu3-pack.c
> 
> diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c
> new file mode 100644
> index 00000000..48193f74
> --- /dev/null
> +++ b/utils/ipu3/ipu3-pack.c
> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer
> + *
> + * Copyright 2022 Umang Jain <umang.jain at ideasonboard.com>
> + */
> +#define _GNU_SOURCE
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +static void usage(const char *argv0)
> +{
> +	printf("Usage: %s input-file(s) -o output-file\n", basename(argv0));
> +	printf("Pack the IPU3 raw Bayer format to 10-bit Bayer\n");

Maybe

"Converted unpacked RAW10 Bayer data to the IPU3 packed Bayer format"

> +}
> +
> +int packInputFile(int in_fd, int out_fd)
> +{
> +	int ret = 0;

No need to initialize ret to 0.

> +
> +	while (1) {
> +		uint16_t in_data[25];
> +		uint8_t out_data[32];
> +		unsigned int i;
> +
> +		ret = read(in_fd, in_data, 50);

		ret = read(in_fd, in_data, sizeof in_data);

> +		if (ret == -1) {
> +			fprintf(stderr, "Failed to read input data: %s\n",
> +				strerror(errno));
> +			break;
> +		}
> +
> +		if (ret < 50) {

		if (ret < sizeof in_data) {

> +			if (ret != 0)
> +				fprintf(stderr, "%u bytes of stray data at end of input\n",
> +					ret);

This will return a positive value is not enough data was ready, which
will be treated by main() as success.

> +			break;
> +		}
> +
> +		for (i = 0; i < 32; ++i) {
> +			unsigned int index = (i * 8) / 10;
> +			unsigned int msb_shift = (i * 8) % 10;
> +			unsigned int lsb_shift = 10 - msb_shift;
> +
> +			out_data[i] = ((in_data[index] >> msb_shift) & 0xff)
> +				    | ((in_data[index+1] << lsb_shift) & 0xff);

When i will be equal to 30 or 31, index will be 24, so you'll have an
out of bound access of in_data here. Stopping the iteration at i < 30
and handling the last two bytes as a special case are likely the easiest
solution.

Also, this won't work on big-endian platforms, but I suppose that's fine
as the IPU3 is Intel-specific and there's little use for this utility on
a different platform.

> +		}
> +
> +		/* 6 MSB are padding on the 32nd byte. */
> +		out_data[31] = out_data[31] & 0x03;
> +
> +		ret = write(out_fd, out_data, 32);

		ret = write(out_fd, out_data, sizeof out_data);

> +		if (ret < -1) {

Looks like this should be "ret < 0" or "ret == -1".

> +			fprintf(stderr, "Failed to write output data: %s\n",
> +				strerror(errno));
> +			return ret;
> +		}
> +	}
> +
> +	return ret;
> +}

Missing blank line.

> +int main(int argc, char *argv[])
> +{
> +	int input_files = argc - 3;
> +	int in_fds[input_files];
> +	int out_fd = -1;
> +	int j = 1;
> +	int ret;
> +	bool oFlag = false;
> +
> +	if (argc < 4) {
> +		usage(argv[0]);
> +		return 1;
> +	}
> +
> +	while (j < argc) {
> +		if (strcmp(argv[j], "-o") == 0) {
> +			oFlag = true;
> +			break;
> +		}
> +
> +		int fd = open(argv[j], O_RDONLY);
> +		if (fd == -1) {
> +			fprintf(stderr, "Failed to open input file '%s': %s\n",
> +				argv[j], strerror(errno));
> +			goto done;
> +		}
> +
> +		in_fds[j - 1] = fd;
> +		j++;
> +	}
> +
> +	if (!oFlag) {
> +		fprintf(stderr, "Missing option '-o' for output file\n");
> +		usage(argv[0]);
> +		goto done;
> +	}
> +
> +	out_fd = open(argv[++j], O_WRONLY | O_TRUNC | O_CREAT, 0644);
> +	if (out_fd == -1) {
> +		fprintf(stderr, "Failed to open output file '%s': %s\n",
> +			argv[j], strerror(errno));
> +		goto done;
> +	}
> +
> +	for (j = 0; j < input_files; j++) {
> +		ret = packInputFile(in_fds[j], out_fd);
> +		if (ret < 0) {
> +			fprintf(stderr, "Error packing input file '%s'\n",
> +				argv[j + 1]);
> +			goto done;
> +		}
> +	}
> +
> +done:
> +	for (j = 0; j < input_files; j++)
> +		close(in_fds[j]);
> +	close(out_fd);
> +
> +	return ret ? 1 : 0;
> +}
> diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build
> index 88049f58..c92cc658 100644
> --- a/utils/ipu3/meson.build
> +++ b/utils/ipu3/meson.build
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
> +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c')
>  ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')

-- 
Regards,

Laurent Pinchart