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

[Laurent Pinchart]

Hi Umang,

On Tue, Jul 12, 2022 at 01:07:52PM +0530, Umang Jain wrote:
> On 7/12/22 03:26, Laurent Pinchart wrote:
> > 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
> 
> Me too :(
> 
> >> + *
> >> + * 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 ?
> 
> So, to be honest I am bit uncomfortable with writing to stdout really.
> 
> The descriptor is open always, so I guess any other process can write() 
> to it? Won't it corrupt the packed data if that happens, as it write 
> interleaved output data to the file.

File descriptors are per process, so no other process can write to
stdout of ipu3-pack.

> So, at least from the starting point, I prefer to fflush()ing the stdout 
> so any buffered data is cleared off to the console, before we start 
> using it for our purpose? Does it makes sense, I might probably have 
> missed something..

When outputting to stdout, ipu3-pack will likely be used with something
similar as

	ipu3-pack frame.bin - >> frames-packed.bin

Flushing stdout for the ipu3-pack process will make no difference,
anything that this program writes to stdout will be redirected to the
file.

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