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

[Umang Jain]

Hi Laurent,

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


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.

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

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