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

[Umang Jain]

Hi Laurent,

On 7/10/22 03:44, Laurent Pinchart wrote:
> 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


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?


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

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