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

16579 diff mbox series

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.

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

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

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

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

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

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