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

[Laurent Pinchart]

Hi Umang,

On Mon, Jul 11, 2022 at 02:47:44PM +0530, Umang Jain wrote:
> On 7/10/22 03:44, Laurent Pinchart wrote:
> > 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
> 
> This will require working with another script but it's minuscule so I'm 
> fine with that.
> 
> I'll update the patch to input one file only and handle the "-" as output.
> 
> Do you want me to create a separate bash script as above as a patch? Do 
> you want to have that iin utils/ tree to be merged?

I wasn't thinking about adding a script for this. We may want to update
the ipu3-process.sh script to use ipu3-pack with a loop similar to the
above, but that's a separate issue.

> >> 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.
> 
> 
> How so? if ret has a positive value, we have -
> 
>          return ret ? 1 : 0;
> 
> at the end of main(). Returning 1 from main() is a unsuccessful 
> execution right?

main() has

	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;
		}
	}

so the loop will not stop if packInputFile() fails with a positive
return value.

I expect this issue to go away as you'll remove the loop in the next
version :-)

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