[{"id":23809,"web_url":"https://patchwork.libcamera.org/comment/23809/","msgid":"","date":"2022-07-09T22:14:49","subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipu3-pack: Provide a\n\t10-bit bayer packing utility","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Sat, Jul 09, 2022 at 03:58:38AM +0530, Umang Jain via libcamera-devel wrote:\n> Provide a 10-bit bayer packing utility for the unpacked data produced\n> by ipu3-unpack. Single or multiple input unpacked files can be packed\n> and concatenated together in a single output file.\n\nI think I'd rather stick to the unix philosophy of small tools that\nfocus on a single purpose, and only process a single input file. Scripts\ncalling this can easily iterate over multiple input files and\nconcatenate the results. To make that easier, you may want to treat the\nspecial value \"-\" for the output file as meaning stdout, so a script\ncould do\n\n\tfor file in files ; do\n\t\tipu3-pack $file - >> pack.bin\n\tdone\n\n> Use-case where this is useful is streaming IMGU with different\n> test patterns on each frame. The output file generated by\n> ipu3-pack can be directly fed to IMGU.\n> \n> Signed-off-by: Umang Jain \n> ---\n> utils/ipu3/ipu3-pack.c | 129 +++++++++++++++++++++++++++++++++++++++++\n> utils/ipu3/meson.build | 1 +\n> 2 files changed, 130 insertions(+)\n> create mode 100644 utils/ipu3/ipu3-pack.c\n> \n> diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c\n> new file mode 100644\n> index 00000000..48193f74\n> --- /dev/null\n> +++ b/utils/ipu3/ipu3-pack.c\n> @@ -0,0 +1,129 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer\n> + *\n> + * Copyright 2022 Umang Jain \n> + */\n> +#define _GNU_SOURCE\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +static void usage(const char *argv0)\n> +{\n> +\tprintf(\"Usage: %s input-file(s) -o output-file\\n\", basename(argv0));\n> +\tprintf(\"Pack the IPU3 raw Bayer format to 10-bit Bayer\\n\");\n\nMaybe\n\n\"Converted unpacked RAW10 Bayer data to the IPU3 packed Bayer format\"\n\n> +}\n> +\n> +int packInputFile(int in_fd, int out_fd)\n> +{\n> +\tint ret = 0;\n\nNo need to initialize ret to 0.\n\n> +\n> +\twhile (1) {\n> +\t\tuint16_t in_data[25];\n> +\t\tuint8_t out_data[32];\n> +\t\tunsigned int i;\n> +\n> +\t\tret = read(in_fd, in_data, 50);\n\n\t\tret = read(in_fd, in_data, sizeof in_data);\n\n> +\t\tif (ret == -1) {\n> +\t\t\tfprintf(stderr, \"Failed to read input data: %s\\n\",\n> +\t\t\t\tstrerror(errno));\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (ret < 50) {\n\n\t\tif (ret < sizeof in_data) {\n\n> +\t\t\tif (ret != 0)\n> +\t\t\t\tfprintf(stderr, \"%u bytes of stray data at end of input\\n\",\n> +\t\t\t\t\tret);\n\nThis will return a positive value is not enough data was ready, which\nwill be treated by main() as success.\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tfor (i = 0; i < 32; ++i) {\n> +\t\t\tunsigned int index = (i * 8) / 10;\n> +\t\t\tunsigned int msb_shift = (i * 8) % 10;\n> +\t\t\tunsigned int lsb_shift = 10 - msb_shift;\n> +\n> +\t\t\tout_data[i] = ((in_data[index] >> msb_shift) & 0xff)\n> +\t\t\t\t | ((in_data[index+1] << lsb_shift) & 0xff);\n\nWhen i will be equal to 30 or 31, index will be 24, so you'll have an\nout of bound access of in_data here. Stopping the iteration at i < 30\nand handling the last two bytes as a special case are likely the easiest\nsolution.\n\nAlso, this won't work on big-endian platforms, but I suppose that's fine\nas the IPU3 is Intel-specific and there's little use for this utility on\na different platform.\n\n> +\t\t}\n> +\n> +\t\t/* 6 MSB are padding on the 32nd byte. */\n> +\t\tout_data[31] = out_data[31] & 0x03;\n> +\n> +\t\tret = write(out_fd, out_data, 32);\n\n\t\tret = write(out_fd, out_data, sizeof out_data);\n\n> +\t\tif (ret < -1) {\n\nLooks like this should be \"ret < 0\" or \"ret == -1\".\n\n> +\t\t\tfprintf(stderr, \"Failed to write output data: %s\\n\",\n> +\t\t\t\tstrerror(errno));\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n> +\treturn ret;\n> +}\n\nMissing blank line.\n\n> +int main(int argc, char *argv[])\n> +{\n> +\tint input_files = argc - 3;\n> +\tint in_fds[input_files];\n> +\tint out_fd = -1;\n> +\tint j = 1;\n> +\tint ret;\n> +\tbool oFlag = false;\n> +\n> +\tif (argc < 4) {\n> +\t\tusage(argv[0]);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\twhile (j < argc) {\n> +\t\tif (strcmp(argv[j], \"-o\") == 0) {\n> +\t\t\toFlag = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tint fd = open(argv[j], O_RDONLY);\n> +\t\tif (fd == -1) {\n> +\t\t\tfprintf(stderr, \"Failed to open input file '%s': %s\\n\",\n> +\t\t\t\targv[j], strerror(errno));\n> +\t\t\tgoto done;\n> +\t\t}\n> +\n> +\t\tin_fds[j - 1] = fd;\n> +\t\tj++;\n> +\t}\n> +\n> +\tif (!oFlag) {\n> +\t\tfprintf(stderr, \"Missing option '-o' for output file\\n\");\n> +\t\tusage(argv[0]);\n> +\t\tgoto done;\n> +\t}\n> +\n> +\tout_fd = open(argv[++j], O_WRONLY | O_TRUNC | O_CREAT, 0644);\n> +\tif (out_fd == -1) {\n> +\t\tfprintf(stderr, \"Failed to open output file '%s': %s\\n\",\n> +\t\t\targv[j], strerror(errno));\n> +\t\tgoto done;\n> +\t}\n> +\n> +\tfor (j = 0; j < input_files; j++) {\n> +\t\tret = packInputFile(in_fds[j], out_fd);\n> +\t\tif (ret < 0) {\n> +\t\t\tfprintf(stderr, \"Error packing input file '%s'\\n\",\n> +\t\t\t\targv[j + 1]);\n> +\t\t\tgoto done;\n> +\t\t}\n> +\t}\n> +\n> +done:\n> +\tfor (j = 0; j < input_files; j++)\n> +\t\tclose(in_fds[j]);\n> +\tclose(out_fd);\n> +\n> +\treturn ret ? 1 : 0;\n> +}\n> diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build\n> index 88049f58..c92cc658 100644\n> --- a/utils/ipu3/meson.build\n> +++ b/utils/ipu3/meson.build\n> @@ -1,3 +1,4 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c')\n> ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1E02EBD1F1\n\tfor ;\n\tSat, 9 Jul 2022 22:15:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 497EF63312;\n\tSun, 10 Jul 2022 00:15:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F21E16330A\n\tfor ;\n\tSun, 10 Jul 2022 00:15:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58DE14F7;\n\tSun, 10 Jul 2022 00:15:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657404919;\n\tbh=OWJaZZZrnZY2Vzf66VKTMrjaNlNqmb/gBi19CdMcteQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=e+kl4nzGbbVE0dF4j1avHTKXHGcn6lCYEHVej8Fq4RFpFluwW9l3Q/QJy6mgrw88P\n\tKmIxUwJglNdM/lX4YbWDPl/BO47N3wmML8o3n3w0j7gsMQGn1hG/jddTRodqtvVuN2\n\ti0pwKNeOzh+fWXtmEG9pEavY2AVF9NkNfNlA5X9uqSUvvYkjD6jN6EZy5vz5rLIep7\n\tVZbnl1/W9SuzBOSCbFGMpS94J88SJ9DJKI++Gi2oSVrCuxQfR3QUo/ovNvuQaHNbFA\n\tff4lsuz+qZDX619BKE4p4awZkABY0rBn0hZ5dYbBrGyvhDHIzzgBcht60B92OQW34K\n\tGcLyjFX5TWbLA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657404916;\n\tbh=OWJaZZZrnZY2Vzf66VKTMrjaNlNqmb/gBi19CdMcteQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NqNlYdGwF8JUatlu6ne07i6ZylqJajIsJWEfmtJEBj0H67w03v2aJpXmoQC7yQO3x\n\tJSA+MU1c2GXmOiMkeWU6yzvnkY5mr0KFbRRl5e0dbzy3KxHzHmcTRgB3ILJRucH0ZU\n\t3Vgqwwmeb2BbCCfUnIUYJmmzuhcPuKDRmGiImt+8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NqNlYdGw\"; dkim-atps=neutral","Date":"Sun, 10 Jul 2022 01:14:49 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220708222838.370187-1-umang.jain@ideasonboard.com>\n\t<20220708222838.370187-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220708222838.370187-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipu3-pack: Provide a\n\t10-bit bayer packing utility","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23811,"web_url":"https://patchwork.libcamera.org/comment/23811/","msgid":"","date":"2022-07-11T09:17:44","subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipu3-pack: Provide a\n\t10-bit bayer packing utility","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 7/10/22 03:44, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Sat, Jul 09, 2022 at 03:58:38AM +0530, Umang Jain via libcamera-devel wrote:\n>> Provide a 10-bit bayer packing utility for the unpacked data produced\n>> by ipu3-unpack. Single or multiple input unpacked files can be packed\n>> and concatenated together in a single output file.\n> I think I'd rather stick to the unix philosophy of small tools that\n> focus on a single purpose, and only process a single input file. Scripts\n> calling this can easily iterate over multiple input files and\n> concatenate the results. To make that easier, you may want to treat the\n> special value \"-\" for the output file as meaning stdout, so a script\n> could do\n>\n> \tfor file in files ; do\n> \t\tipu3-pack $file - >> pack.bin\n> \tdone\n\n\nThis will require working with another script but it's minuscule so I'm \nfine with that.\n\nI'll update the patch to input one file only and handle the \"-\" as output.\n\nDo you want me to create a separate bash script as above as a patch? Do \nyou want to have that iin utils/ tree to be merged?\n\n\n>\n>> Use-case where this is useful is streaming IMGU with different\n>> test patterns on each frame. The output file generated by\n>> ipu3-pack can be directly fed to IMGU.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> utils/ipu3/ipu3-pack.c | 129 +++++++++++++++++++++++++++++++++++++++++\n>> utils/ipu3/meson.build | 1 +\n>> 2 files changed, 130 insertions(+)\n>> create mode 100644 utils/ipu3/ipu3-pack.c\n>>\n>> diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c\n>> new file mode 100644\n>> index 00000000..48193f74\n>> --- /dev/null\n>> +++ b/utils/ipu3/ipu3-pack.c\n>> @@ -0,0 +1,129 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer\n>> + *\n>> + * Copyright 2022 Umang Jain \n>> + */\n>> +#define _GNU_SOURCE\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +static void usage(const char *argv0)\n>> +{\n>> +\tprintf(\"Usage: %s input-file(s) -o output-file\\n\", basename(argv0));\n>> +\tprintf(\"Pack the IPU3 raw Bayer format to 10-bit Bayer\\n\");\n> Maybe\n>\n> \"Converted unpacked RAW10 Bayer data to the IPU3 packed Bayer format\"\n>\n>> +}\n>> +\n>> +int packInputFile(int in_fd, int out_fd)\n>> +{\n>> +\tint ret = 0;\n> No need to initialize ret to 0.\n>\n>> +\n>> +\twhile (1) {\n>> +\t\tuint16_t in_data[25];\n>> +\t\tuint8_t out_data[32];\n>> +\t\tunsigned int i;\n>> +\n>> +\t\tret = read(in_fd, in_data, 50);\n> \t\tret = read(in_fd, in_data, sizeof in_data);\n>\n>> +\t\tif (ret == -1) {\n>> +\t\t\tfprintf(stderr, \"Failed to read input data: %s\\n\",\n>> +\t\t\t\tstrerror(errno));\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tif (ret < 50) {\n> \t\tif (ret < sizeof in_data) {\n>\n>> +\t\t\tif (ret != 0)\n>> +\t\t\t\tfprintf(stderr, \"%u bytes of stray data at end of input\\n\",\n>> +\t\t\t\t\tret);\n> This will return a positive value is not enough data was ready, which\n> will be treated by main() as success.\n\n\nHow so? if ret has a positive value, we have -\n\n         return ret ? 1 : 0;\n\nat the end of main(). Returning 1 from main() is a unsuccessful \nexecution right?\n\n>\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tfor (i = 0; i < 32; ++i) {\n>> +\t\t\tunsigned int index = (i * 8) / 10;\n>> +\t\t\tunsigned int msb_shift = (i * 8) % 10;\n>> +\t\t\tunsigned int lsb_shift = 10 - msb_shift;\n>> +\n>> +\t\t\tout_data[i] = ((in_data[index] >> msb_shift) & 0xff)\n>> +\t\t\t\t | ((in_data[index+1] << lsb_shift) & 0xff);\n> When i will be equal to 30 or 31, index will be 24, so you'll have an\n> out of bound access of in_data here. Stopping the iteration at i < 30\n> and handling the last two bytes as a special case are likely the easiest\n> solution.\n>\n> Also, this won't work on big-endian platforms, but I suppose that's fine\n> as the IPU3 is Intel-specific and there's little use for this utility on\n> a different platform.\n>\n>> +\t\t}\n>> +\n>> +\t\t/* 6 MSB are padding on the 32nd byte. */\n>> +\t\tout_data[31] = out_data[31] & 0x03;\n>> +\n>> +\t\tret = write(out_fd, out_data, 32);\n> \t\tret = write(out_fd, out_data, sizeof out_data);\n>\n>> +\t\tif (ret < -1) {\n> Looks like this should be \"ret < 0\" or \"ret == -1\".\n>\n>> +\t\t\tfprintf(stderr, \"Failed to write output data: %s\\n\",\n>> +\t\t\t\tstrerror(errno));\n>> +\t\t\treturn ret;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\treturn ret;\n>> +}\n> Missing blank line.\n>\n>> +int main(int argc, char *argv[])\n>> +{\n>> +\tint input_files = argc - 3;\n>> +\tint in_fds[input_files];\n>> +\tint out_fd = -1;\n>> +\tint j = 1;\n>> +\tint ret;\n>> +\tbool oFlag = false;\n>> +\n>> +\tif (argc < 4) {\n>> +\t\tusage(argv[0]);\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> +\twhile (j < argc) {\n>> +\t\tif (strcmp(argv[j], \"-o\") == 0) {\n>> +\t\t\toFlag = true;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tint fd = open(argv[j], O_RDONLY);\n>> +\t\tif (fd == -1) {\n>> +\t\t\tfprintf(stderr, \"Failed to open input file '%s': %s\\n\",\n>> +\t\t\t\targv[j], strerror(errno));\n>> +\t\t\tgoto done;\n>> +\t\t}\n>> +\n>> +\t\tin_fds[j - 1] = fd;\n>> +\t\tj++;\n>> +\t}\n>> +\n>> +\tif (!oFlag) {\n>> +\t\tfprintf(stderr, \"Missing option '-o' for output file\\n\");\n>> +\t\tusage(argv[0]);\n>> +\t\tgoto done;\n>> +\t}\n>> +\n>> +\tout_fd = open(argv[++j], O_WRONLY | O_TRUNC | O_CREAT, 0644);\n>> +\tif (out_fd == -1) {\n>> +\t\tfprintf(stderr, \"Failed to open output file '%s': %s\\n\",\n>> +\t\t\targv[j], strerror(errno));\n>> +\t\tgoto done;\n>> +\t}\n>> +\n>> +\tfor (j = 0; j < input_files; j++) {\n>> +\t\tret = packInputFile(in_fds[j], out_fd);\n>> +\t\tif (ret < 0) {\n>> +\t\t\tfprintf(stderr, \"Error packing input file '%s'\\n\",\n>> +\t\t\t\targv[j + 1]);\n>> +\t\t\tgoto done;\n>> +\t\t}\n>> +\t}\n>> +\n>> +done:\n>> +\tfor (j = 0; j < input_files; j++)\n>> +\t\tclose(in_fds[j]);\n>> +\tclose(out_fd);\n>> +\n>> +\treturn ret ? 1 : 0;\n>> +}\n>> diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build\n>> index 88049f58..c92cc658 100644\n>> --- a/utils/ipu3/meson.build\n>> +++ b/utils/ipu3/meson.build\n>> @@ -1,3 +1,4 @@\n>> # SPDX-License-Identifier: CC0-1.0\n>> \n>> +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c')\n>> ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 54FE9BD1F1\n\tfor ;\n\tMon, 11 Jul 2022 09:17:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA89163312;\n\tMon, 11 Jul 2022 11:17:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1766260402\n\tfor ;\n\tMon, 11 Jul 2022 11:17:51 +0200 (CEST)","from [IPV6:2401:4900:1f3f:b3fb:3712:29bf:7855:376e] (unknown\n\t[IPv6:2401:4900:1f3f:b3fb:3712:29bf:7855:376e])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA6C1326;\n\tMon, 11 Jul 2022 11:17:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657531072;\n\tbh=t/Sddps/h4d62EOUXc9/mr5hKQkA7JUtQStSET0h3Fg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=IOZrlfYqh4lIae2Y0gFBaDMCEK4Rm45+4xMlxskF//WNlOjvLJJu7Q21hxUfGFf7e\n\teu2/UugClQ70sRHJEnr8I8pxVZvBgV00GtOoXPfEgeV+f96vaG3VytblpT8qSrVSfR\n\tVCtazT2UOrTxVLyLaRntt/BQhnX6umxFvNokx9DwJZVQCtkYBagOgRJgt/dlGkdDdp\n\t43ljbKId3XVz8rQDOQyb5UL8OTY8Npf6Xg9q4xZSHZ4gAR+N5q1cqzL9813FruTUzz\n\tOADEaGoEwSKWRmDOfz7jH6RofvVuqOkgEfw930N9kzkqz0OMW4go/OwhZM2BWWzGRa\n\tQVtFc0pE3P3yg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657531070;\n\tbh=t/Sddps/h4d62EOUXc9/mr5hKQkA7JUtQStSET0h3Fg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=WrVK/Ji9dI9jLgKrsxrbPJVphH5PIJeamPaCImx5FtjA/+jZxw8NKrihcdzzSjU+1\n\tM/uKHdKA3+xslwKuzRAYiT77E9OM7Ub+R/EGRp5ggsOacnAGLNcl39PStPwvZuJfIm\n\tgwoD7CnTPQ6kjdUFaGe9uCfs7tuT7hieiZvbo8zU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WrVK/Ji9\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 11 Jul 2022 14:47:44 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220708222838.370187-1-umang.jain@ideasonboard.com>\n\t<20220708222838.370187-3-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipu3-pack: Provide a\n\t10-bit bayer packing utility","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23812,"web_url":"https://patchwork.libcamera.org/comment/23812/","msgid":"","date":"2022-07-11T09:22:19","subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipu3-pack: Provide a\n\t10-bit bayer packing utility","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Jul 11, 2022 at 02:47:44PM +0530, Umang Jain wrote:\n> On 7/10/22 03:44, Laurent Pinchart wrote:\n> > On Sat, Jul 09, 2022 at 03:58:38AM +0530, Umang Jain via libcamera-devel wrote:\n> >> Provide a 10-bit bayer packing utility for the unpacked data produced\n> >> by ipu3-unpack. Single or multiple input unpacked files can be packed\n> >> and concatenated together in a single output file.\n> > I think I'd rather stick to the unix philosophy of small tools that\n> > focus on a single purpose, and only process a single input file. Scripts\n> > calling this can easily iterate over multiple input files and\n> > concatenate the results. To make that easier, you may want to treat the\n> > special value \"-\" for the output file as meaning stdout, so a script\n> > could do\n> >\n> > \tfor file in files ; do\n> > \t\tipu3-pack $file - >> pack.bin\n> > \tdone\n> \n> This will require working with another script but it's minuscule so I'm \n> fine with that.\n> \n> I'll update the patch to input one file only and handle the \"-\" as output.\n> \n> Do you want me to create a separate bash script as above as a patch? Do \n> you want to have that iin utils/ tree to be merged?\n\nI wasn't thinking about adding a script for this. We may want to update\nthe ipu3-process.sh script to use ipu3-pack with a loop similar to the\nabove, but that's a separate issue.\n\n> >> Use-case where this is useful is streaming IMGU with different\n> >> test patterns on each frame. The output file generated by\n> >> ipu3-pack can be directly fed to IMGU.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> utils/ipu3/ipu3-pack.c | 129 +++++++++++++++++++++++++++++++++++++++++\n> >> utils/ipu3/meson.build | 1 +\n> >> 2 files changed, 130 insertions(+)\n> >> create mode 100644 utils/ipu3/ipu3-pack.c\n> >>\n> >> diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c\n> >> new file mode 100644\n> >> index 00000000..48193f74\n> >> --- /dev/null\n> >> +++ b/utils/ipu3/ipu3-pack.c\n> >> @@ -0,0 +1,129 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer\n> >> + *\n> >> + * Copyright 2022 Umang Jain \n> >> + */\n> >> +#define _GNU_SOURCE\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +static void usage(const char *argv0)\n> >> +{\n> >> +\tprintf(\"Usage: %s input-file(s) -o output-file\\n\", basename(argv0));\n> >> +\tprintf(\"Pack the IPU3 raw Bayer format to 10-bit Bayer\\n\");\n> > Maybe\n> >\n> > \"Converted unpacked RAW10 Bayer data to the IPU3 packed Bayer format\"\n> >\n> >> +}\n> >> +\n> >> +int packInputFile(int in_fd, int out_fd)\n> >> +{\n> >> +\tint ret = 0;\n> > No need to initialize ret to 0.\n> >\n> >> +\n> >> +\twhile (1) {\n> >> +\t\tuint16_t in_data[25];\n> >> +\t\tuint8_t out_data[32];\n> >> +\t\tunsigned int i;\n> >> +\n> >> +\t\tret = read(in_fd, in_data, 50);\n> > \t\tret = read(in_fd, in_data, sizeof in_data);\n> >\n> >> +\t\tif (ret == -1) {\n> >> +\t\t\tfprintf(stderr, \"Failed to read input data: %s\\n\",\n> >> +\t\t\t\tstrerror(errno));\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\n> >> +\t\tif (ret < 50) {\n> > \t\tif (ret < sizeof in_data) {\n> >\n> >> +\t\t\tif (ret != 0)\n> >> +\t\t\t\tfprintf(stderr, \"%u bytes of stray data at end of input\\n\",\n> >> +\t\t\t\t\tret);\n> > \n> > This will return a positive value is not enough data was ready, which\n> > will be treated by main() as success.\n> \n> \n> How so? if ret has a positive value, we have -\n> \n>         return ret ? 1 : 0;\n> \n> at the end of main(). Returning 1 from main() is a unsuccessful \n> execution right?\n\nmain() has\n\n\tfor (j = 0; j < input_files; j++) {\n\t\tret = packInputFile(in_fds[j], out_fd);\n\t\tif (ret < 0) {\n\t\t\tfprintf(stderr, \"Error packing input file '%s'\\n\",\n\t\t\t\targv[j + 1]);\n\t\t\tgoto done;\n\t\t}\n\t}\n\nso the loop will not stop if packInputFile() fails with a positive\nreturn value.\n\nI expect this issue to go away as you'll remove the loop in the next\nversion :-)\n\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\n> >> +\t\tfor (i = 0; i < 32; ++i) {\n> >> +\t\t\tunsigned int index = (i * 8) / 10;\n> >> +\t\t\tunsigned int msb_shift = (i * 8) % 10;\n> >> +\t\t\tunsigned int lsb_shift = 10 - msb_shift;\n> >> +\n> >> +\t\t\tout_data[i] = ((in_data[index] >> msb_shift) & 0xff)\n> >> +\t\t\t\t | ((in_data[index+1] << lsb_shift) & 0xff);\n> > When i will be equal to 30 or 31, index will be 24, so you'll have an\n> > out of bound access of in_data here. Stopping the iteration at i < 30\n> > and handling the last two bytes as a special case are likely the easiest\n> > solution.\n> >\n> > Also, this won't work on big-endian platforms, but I suppose that's fine\n> > as the IPU3 is Intel-specific and there's little use for this utility on\n> > a different platform.\n> >\n> >> +\t\t}\n> >> +\n> >> +\t\t/* 6 MSB are padding on the 32nd byte. */\n> >> +\t\tout_data[31] = out_data[31] & 0x03;\n> >> +\n> >> +\t\tret = write(out_fd, out_data, 32);\n> > \t\tret = write(out_fd, out_data, sizeof out_data);\n> >\n> >> +\t\tif (ret < -1) {\n> > Looks like this should be \"ret < 0\" or \"ret == -1\".\n> >\n> >> +\t\t\tfprintf(stderr, \"Failed to write output data: %s\\n\",\n> >> +\t\t\t\tstrerror(errno));\n> >> +\t\t\treturn ret;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\treturn ret;\n> >> +}\n> > Missing blank line.\n> >\n> >> +int main(int argc, char *argv[])\n> >> +{\n> >> +\tint input_files = argc - 3;\n> >> +\tint in_fds[input_files];\n> >> +\tint out_fd = -1;\n> >> +\tint j = 1;\n> >> +\tint ret;\n> >> +\tbool oFlag = false;\n> >> +\n> >> +\tif (argc < 4) {\n> >> +\t\tusage(argv[0]);\n> >> +\t\treturn 1;\n> >> +\t}\n> >> +\n> >> +\twhile (j < argc) {\n> >> +\t\tif (strcmp(argv[j], \"-o\") == 0) {\n> >> +\t\t\toFlag = true;\n> >> +\t\t\tbreak;\n> >> +\t\t}\n> >> +\n> >> +\t\tint fd = open(argv[j], O_RDONLY);\n> >> +\t\tif (fd == -1) {\n> >> +\t\t\tfprintf(stderr, \"Failed to open input file '%s': %s\\n\",\n> >> +\t\t\t\targv[j], strerror(errno));\n> >> +\t\t\tgoto done;\n> >> +\t\t}\n> >> +\n> >> +\t\tin_fds[j - 1] = fd;\n> >> +\t\tj++;\n> >> +\t}\n> >> +\n> >> +\tif (!oFlag) {\n> >> +\t\tfprintf(stderr, \"Missing option '-o' for output file\\n\");\n> >> +\t\tusage(argv[0]);\n> >> +\t\tgoto done;\n> >> +\t}\n> >> +\n> >> +\tout_fd = open(argv[++j], O_WRONLY | O_TRUNC | O_CREAT, 0644);\n> >> +\tif (out_fd == -1) {\n> >> +\t\tfprintf(stderr, \"Failed to open output file '%s': %s\\n\",\n> >> +\t\t\targv[j], strerror(errno));\n> >> +\t\tgoto done;\n> >> +\t}\n> >> +\n> >> +\tfor (j = 0; j < input_files; j++) {\n> >> +\t\tret = packInputFile(in_fds[j], out_fd);\n> >> +\t\tif (ret < 0) {\n> >> +\t\t\tfprintf(stderr, \"Error packing input file '%s'\\n\",\n> >> +\t\t\t\targv[j + 1]);\n> >> +\t\t\tgoto done;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +done:\n> >> +\tfor (j = 0; j < input_files; j++)\n> >> +\t\tclose(in_fds[j]);\n> >> +\tclose(out_fd);\n> >> +\n> >> +\treturn ret ? 1 : 0;\n> >> +}\n> >> diff --git a/utils/ipu3/meson.build b/utils/ipu3/meson.build\n> >> index 88049f58..c92cc658 100644\n> >> --- a/utils/ipu3/meson.build\n> >> +++ b/utils/ipu3/meson.build\n> >> @@ -1,3 +1,4 @@\n> >> # SPDX-License-Identifier: CC0-1.0\n> >> \n> >> +ipu3_pack = executable('ipu3-pack', 'ipu3-pack.c')\n> >> ipu3_unpack = executable('ipu3-unpack', 'ipu3-unpack.c')","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 28AECBE173\n\tfor ;\n\tMon, 11 Jul 2022 09:22:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D39A63310;\n\tMon, 11 Jul 2022 11:22:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0599660402\n\tfor ;\n\tMon, 11 Jul 2022 11:22:47 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6ABB5440;\n\tMon, 11 Jul 2022 11:22:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657531368;\n\tbh=W8kz56shHn5FukngVv9BVLG2N1QRfEd5j7WpkCG83xc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=U+v3gXXLz8pZzYNR3ZrIctb41yaGE+KIG/1Ic2eyqEz/zie2cgVKuSD9HaB9DbuxL\n\tkr1j5BtffZqdvoiypt9anHjLxpz6tW/8fT3XBf5KNjHmvqK4TxlSvsAWGjPK3IR2Nv\n\tM1kG/zeaGV3mK26XsUKmW2l4iDfjr4wrxtlriRp/kqm42o40lZaBm26Q0XzbAZpMES\n\tKMwjjdaCfCRf6RW54rVeZJezFcamJ+FyDM14dx9xJzz3DiwkGtmxSC24yYF/YrIv6L\n\tP+FjW/TonDb8VI/F/3MSY7FZQ1fjg79wN6KgnNWqN4bW+XUFjvY8T1u/S6vzFMOSgl\n\t6mLG++YzipiGg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657531366;\n\tbh=W8kz56shHn5FukngVv9BVLG2N1QRfEd5j7WpkCG83xc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E+oo+6sayMpIykPhQ0gMfSnBZ53PE8bUhrG5AICYLom0E26I37NcvJLZOW/BbuOqX\n\trBSipXWOg47MPeXmDRb5L20S29AUC36yTZ6rQacATZdQNhCUmEnL3Rh+M/LGhTlwwx\n\tLR4nP6+Gl6evZDC4/Ojhot3B4N3P3yb24E5k1Gec="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E+oo+6sa\"; dkim-atps=neutral","Date":"Mon, 11 Jul 2022 12:22:19 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220708222838.370187-1-umang.jain@ideasonboard.com>\n\t<20220708222838.370187-3-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipu3-pack: Provide a\n\t10-bit bayer packing utility","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]