[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 Mon, Jul 11, 2022 at 04:28:35PM +0530, Umang Jain via libcamera-devel wrote:
> Provide a 10-bit bayer packing utility for the unpacked data produced
> by ipu3-unpack.
> 
> 	Usage: ipu3-unpack input-file output-file
> 
> The output-file can be "-" to unpack the data to stdout.

s/unpack/pack/

> The output file generated by ipu3-pack can be directly fed to IMGU
> for streaming.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++
>  utils/ipu3/meson.build |   1 +
>  2 files changed, 101 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..328e3cd6
> --- /dev/null
> +++ b/utils/ipu3/ipu3-pack.c
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer

I've commented on the usage message in v1 but overlooked this:

 * Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats

> + *
> + * Copyright 2022 Umang Jain <umang.jain at ideasonboard.com>
> + */
> +#define _GNU_SOURCE
> +
> +#include <errno.h>
> +#include <fcntl.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 output-file('-' to use stdout)\n", basename(argv0));
> +	printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n");

	printf("Usage: %s input-file output-file\n", basename(argv0));
	printf("Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n");
	printf("If the output-file '-', output data will be written to standard output\n");

> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int in_fd;
> +	int out_fd;
> +	int ret;
> +
> +	if (argc != 3) {
> +		usage(argv[0]);
> +		return 1;
> +	}
> +
> +	in_fd = open(argv[1], O_RDONLY);
> +	if (in_fd == -1) {
> +		fprintf(stderr, "Failed to open input file '%s': %s\n",
> +			argv[1], strerror(errno));
> +		return 1;
> +	}
> +
> +	if (strcmp(argv[2], "-") == 0) {
> +		out_fd = STDOUT_FILENO;
> +		fflush(stdout);

Is fflush() needed ?

> +	} else {
> +		out_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644);
> +		if (out_fd == -1) {
> +			fprintf(stderr, "Failed to open output file '%s': %s\n",
> +				argv[2], strerror(errno));

			close(in_fd);

> +			return 1;
> +		}
> +	}
> +
> +	while (1) {
> +		uint16_t in_data[25];
> +		uint8_t out_data[32];
> +		unsigned int i;
> +
> +		int bytes_to_read = sizeof(in_data);

s/int/size_t/

But I'd use sizeof(in_data) directly below. Adding a variable adds a
level of indirection, which makes the code more difficult to read.

> +		ret = read(in_fd, in_data, bytes_to_read);
> +		if (ret == -1) {
> +			fprintf(stderr, "Failed to read input data: %s\n",
> +				strerror(errno));
> +			goto done;
> +		}
> +
> +		if (ret < bytes_to_read) {
> +			if (ret != 0)
> +				fprintf(stderr, "%u bytes of stray data at end of input\n",
> +					ret);
> +			break;

I would either 'goto done' here or 'break' in the places where you 'goto
done' for consistency.

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

> +		}
> +
> +		for (i = 0; i < 30; ++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);
> +		}
> +
> +		out_data[30] = (in_data[24] >> 0) & 0xff;
> +		out_data[31] = (in_data[24] >> 8) & 0x03;
> +
> +		ret = write(out_fd, out_data, sizeof(out_data));
> +		if (ret < 0) {
> +			fprintf(stderr, "Failed to write output data: %s\n",
> +				strerror(errno));
> +			goto done;
> +		}
> +	}
> +
> +done:
> +	close(in_fd);
> +	if (out_fd != STDOUT_FILENO)
> +		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