[{"id":23820,"web_url":"https://patchwork.libcamera.org/comment/23820/","msgid":"","date":"2022-07-11T21:56:34","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 Mon, Jul 11, 2022 at 04:28:35PM +0530, Umang Jain via libcamera-devel wrote:\n> Provide a 10-bit bayer packing utility for the unpacked data produced\n> by ipu3-unpack.\n> \n> \tUsage: ipu3-unpack input-file output-file\n> \n> The output-file can be \"-\" to unpack the data to stdout.\n\ns/unpack/pack/\n\n> The output file generated by ipu3-pack can be directly fed to IMGU\n> for streaming.\n> \n> Signed-off-by: Umang Jain \n> ---\n> utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++\n> utils/ipu3/meson.build | 1 +\n> 2 files changed, 101 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..328e3cd6\n> --- /dev/null\n> +++ b/utils/ipu3/ipu3-pack.c\n> @@ -0,0 +1,100 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer\n\nI've commented on the usage message in v1 but overlooked this:\n\n * Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n\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> +\n> +static void usage(const char *argv0)\n> +{\n> +\tprintf(\"Usage:%s input-file output-file('-' to use stdout)\\n\", basename(argv0));\n> +\tprintf(\"Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\\n\");\n\n\tprintf(\"Usage: %s input-file output-file\\n\", basename(argv0));\n\tprintf(\"Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\\n\");\n\tprintf(\"If the output-file '-', output data will be written to standard output\\n\");\n\n> +}\n> +\n> +int main(int argc, char *argv[])\n> +{\n> +\tint in_fd;\n> +\tint out_fd;\n> +\tint ret;\n> +\n> +\tif (argc != 3) {\n> +\t\tusage(argv[0]);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tin_fd = open(argv[1], O_RDONLY);\n> +\tif (in_fd == -1) {\n> +\t\tfprintf(stderr, \"Failed to open input file '%s': %s\\n\",\n> +\t\t\targv[1], strerror(errno));\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tif (strcmp(argv[2], \"-\") == 0) {\n> +\t\tout_fd = STDOUT_FILENO;\n> +\t\tfflush(stdout);\n\nIs fflush() needed ?\n\n> +\t} else {\n> +\t\tout_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644);\n> +\t\tif (out_fd == -1) {\n> +\t\t\tfprintf(stderr, \"Failed to open output file '%s': %s\\n\",\n> +\t\t\t\targv[2], strerror(errno));\n\n\t\t\tclose(in_fd);\n\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t}\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\tint bytes_to_read = sizeof(in_data);\n\ns/int/size_t/\n\nBut I'd use sizeof(in_data) directly below. Adding a variable adds a\nlevel of indirection, which makes the code more difficult to read.\n\n> +\t\tret = read(in_fd, in_data, bytes_to_read);\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\tgoto done;\n> +\t\t}\n> +\n> +\t\tif (ret < bytes_to_read) {\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> +\t\t\tbreak;\n\nI would either 'goto done' here or 'break' in the places where you 'goto\ndone' for consistency.\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t}\n> +\n> +\t\tfor (i = 0; i < 30; ++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> +\t\t}\n> +\n> +\t\tout_data[30] = (in_data[24] >> 0) & 0xff;\n> +\t\tout_data[31] = (in_data[24] >> 8) & 0x03;\n> +\n> +\t\tret = write(out_fd, out_data, sizeof(out_data));\n> +\t\tif (ret < 0) {\n> +\t\t\tfprintf(stderr, \"Failed to write output data: %s\\n\",\n> +\t\t\t\tstrerror(errno));\n> +\t\t\tgoto done;\n> +\t\t}\n> +\t}\n> +\n> +done:\n> +\tclose(in_fd);\n> +\tif (out_fd != STDOUT_FILENO)\n> +\t\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 36E50BD1F1\n\tfor ;\n\tMon, 11 Jul 2022 21:57:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A3A262752;\n\tMon, 11 Jul 2022 23:57:04 +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 2FFCE60401\n\tfor ;\n\tMon, 11 Jul 2022 23:57:03 +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 91912326;\n\tMon, 11 Jul 2022 23:57:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657576624;\n\tbh=w1T/HHtiDLYcTffUFVqAQgyxygSaFp9uTJf3qSuJOSQ=;\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=S1yqdFuA0RUmnjYMqEhWreSSV8wJbsRhc7pNZyRMKPoWSwY7/z0e7/Y4To0PVLHES\n\tKBUXSqwro0L+np3vgqwswgbqIBEfsA0g4mIRasN6OrE48zKlmn0Z12RWZEm9xldqVu\n\tOgSWXk19nh3pCM3/BeA0ASOTFgXFVCk1f0pzK/gPOXly87rZgDR9tgutBpihuC37LV\n\tDXRQKBJl9m8NLq671Hk262RE/YUW0vYP5Yp8/Y9Sa/E3w7cfkbynsofqaMltX9FP4j\n\tTczO4akFU7+iscHYElA2r+ejgSzonVl/U+n8AKOZujTYSpsdLyGWjSIsI+jjy6J0Ys\n\tOPSCeRWkKHNhg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657576622;\n\tbh=w1T/HHtiDLYcTffUFVqAQgyxygSaFp9uTJf3qSuJOSQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g9na6enyV/sM+lXG5+Srd7/tZ0Os0n98X9c4we099aewoq5ZYo5C0HlOcx3fG5tCZ\n\tjdg3khMswgxeRx5vSGssSNMafyiSsciIMQxdOnpfRE1HA+THZONM4+Gg3zG+dVA02Z\n\tLdyly91YSQcPObeOAdgh3fo/lTbQqwrmytrrbBgo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"g9na6eny\"; dkim-atps=neutral","Date":"Tue, 12 Jul 2022 00:56:34 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220711105835.48254-1-umang.jain@ideasonboard.com>\n\t<20220711105835.48254-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220711105835.48254-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":23827,"web_url":"https://patchwork.libcamera.org/comment/23827/","msgid":"","date":"2022-07-12T07:37:52","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/12/22 03:26, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Jul 11, 2022 at 04:28:35PM +0530, Umang Jain via libcamera-devel wrote:\n>> Provide a 10-bit bayer packing utility for the unpacked data produced\n>> by ipu3-unpack.\n>>\n>> \tUsage: ipu3-unpack input-file output-file\n>>\n>> The output-file can be \"-\" to unpack the data to stdout.\n> s/unpack/pack/\n>\n>> The output file generated by ipu3-pack can be directly fed to IMGU\n>> for streaming.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++\n>> utils/ipu3/meson.build | 1 +\n>> 2 files changed, 101 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..328e3cd6\n>> --- /dev/null\n>> +++ b/utils/ipu3/ipu3-pack.c\n>> @@ -0,0 +1,100 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer\n> I've commented on the usage message in v1 but overlooked this:\n>\n> * Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n\n\nMe too :(\n\n>\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>> +\n>> +static void usage(const char *argv0)\n>> +{\n>> +\tprintf(\"Usage:%s input-file output-file('-' to use stdout)\\n\", basename(argv0));\n>> +\tprintf(\"Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\\n\");\n> \tprintf(\"Usage: %s input-file output-file\\n\", basename(argv0));\n> \tprintf(\"Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\\n\");\n> \tprintf(\"If the output-file '-', output data will be written to standard output\\n\");\n>\n>> +}\n>> +\n>> +int main(int argc, char *argv[])\n>> +{\n>> +\tint in_fd;\n>> +\tint out_fd;\n>> +\tint ret;\n>> +\n>> +\tif (argc != 3) {\n>> +\t\tusage(argv[0]);\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> +\tin_fd = open(argv[1], O_RDONLY);\n>> +\tif (in_fd == -1) {\n>> +\t\tfprintf(stderr, \"Failed to open input file '%s': %s\\n\",\n>> +\t\t\targv[1], strerror(errno));\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> +\tif (strcmp(argv[2], \"-\") == 0) {\n>> +\t\tout_fd = STDOUT_FILENO;\n>> +\t\tfflush(stdout);\n> Is fflush() needed ?\n\n\nSo, to be honest I am bit uncomfortable with writing to stdout really.\n\nThe descriptor is open always, so I guess any other process can write() \nto it? Won't it corrupt the packed data if that happens, as it write \ninterleaved output data to the file.\n\nSo, at least from the starting point, I prefer to fflush()ing the stdout \nso any buffered data is cleared off to the console, before we start \nusing it for our purpose? Does it makes sense, I might probably have \nmissed something..\n\n>\n>> +\t} else {\n>> +\t\tout_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644);\n>> +\t\tif (out_fd == -1) {\n>> +\t\t\tfprintf(stderr, \"Failed to open output file '%s': %s\\n\",\n>> +\t\t\t\targv[2], strerror(errno));\n> \t\t\tclose(in_fd);\n>\n>> +\t\t\treturn 1;\n>> +\t\t}\n>> +\t}\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\tint bytes_to_read = sizeof(in_data);\n> s/int/size_t/\n>\n> But I'd use sizeof(in_data) directly below. Adding a variable adds a\n> level of indirection, which makes the code more difficult to read.\n>\n>> +\t\tret = read(in_fd, in_data, bytes_to_read);\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\tgoto done;\n>> +\t\t}\n>> +\n>> +\t\tif (ret < bytes_to_read) {\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>> +\t\t\tbreak;\n> I would either 'goto done' here or 'break' in the places where you 'goto\n> done' for consistency.\n>\n> Reviewed-by: Laurent Pinchart \n>\n>> +\t\t}\n>> +\n>> +\t\tfor (i = 0; i < 30; ++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>> +\t\t}\n>> +\n>> +\t\tout_data[30] = (in_data[24] >> 0) & 0xff;\n>> +\t\tout_data[31] = (in_data[24] >> 8) & 0x03;\n>> +\n>> +\t\tret = write(out_fd, out_data, sizeof(out_data));\n>> +\t\tif (ret < 0) {\n>> +\t\t\tfprintf(stderr, \"Failed to write output data: %s\\n\",\n>> +\t\t\t\tstrerror(errno));\n>> +\t\t\tgoto done;\n>> +\t\t}\n>> +\t}\n>> +\n>> +done:\n>> +\tclose(in_fd);\n>> +\tif (out_fd != STDOUT_FILENO)\n>> +\t\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 8FE26BD1F1\n\tfor ;\n\tTue, 12 Jul 2022 07:37:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D067C63312;\n\tTue, 12 Jul 2022 09:37:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18B0C6330E\n\tfor ;\n\tTue, 12 Jul 2022 09:37:57 +0200 (CEST)","from [IPV6:2401:4900:1f3f:dcbd:bc88:bb0e:d3d8:8610] (unknown\n\t[IPv6:2401:4900:1f3f:dcbd:bc88:bb0e:d3d8:8610])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2DDE530B;\n\tTue, 12 Jul 2022 09:37:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657611478;\n\tbh=YlsK9yANsDzL3jIMaLxUlKRPJlCvktrVWm7zpantCb4=;\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=dmHr8ZGY8n8JuN77ObMxBXct+4CUyAxPzYphy/rk1GH05HjQ6FM0R/aaQRqOswUW9\n\tPT+BAJdg3GG+a+Mp3ROWD0cdx3zsmp82kUGb7jmQ22FEll/XU3ieA3BT2Ai31KToWl\n\tW6XR5ttgkAY9CQFzbqyW7NXhKtU/pIba1oLhGFkcKKpOz1Cu355cXXGz0mO2YpP2s5\n\tPxc5NDSrJCr0fRM5U9Xn9UJXQlBYuMF/KH/dOPfWPyCQqNzncpJImUjz2u2GQ31OmU\n\tzHEQvNx7+fAmwJdYbSl3hOWWpTMBw9TxSl4SspTiB7dg6lpLAH5L1G0kXo4FxAY/Bz\n\tDQ8p6SrjlxJyw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657611476;\n\tbh=YlsK9yANsDzL3jIMaLxUlKRPJlCvktrVWm7zpantCb4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=gv4URWrechn7CkwdEU0qtuRHk/QPX41xF6le6rS7320B1MgY5YqGxp8y1wIpRm+0f\n\tP9M055Nb3aw0YMCn6O/iQhcxU1JQ5Z0YJKYcxn2yo/nYR9K0bOz2xb3rAuB71J+SeS\n\tXMqaq3RZHsRDBk1B23eBkDTgu/5QDDtBcnorW2tU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gv4URWre\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 12 Jul 2022 13:07:52 +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":"<20220711105835.48254-1-umang.jain@ideasonboard.com>\n\t<20220711105835.48254-3-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":23832,"web_url":"https://patchwork.libcamera.org/comment/23832/","msgid":"","date":"2022-07-12T10:19:46","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 Tue, Jul 12, 2022 at 01:07:52PM +0530, Umang Jain wrote:\n> On 7/12/22 03:26, Laurent Pinchart wrote:\n> > On Mon, Jul 11, 2022 at 04:28:35PM +0530, Umang Jain via libcamera-devel wrote:\n> >> Provide a 10-bit bayer packing utility for the unpacked data produced\n> >> by ipu3-unpack.\n> >>\n> >> \tUsage: ipu3-unpack input-file output-file\n> >>\n> >> The output-file can be \"-\" to unpack the data to stdout.\n> > s/unpack/pack/\n> >\n> >> The output file generated by ipu3-pack can be directly fed to IMGU\n> >> for streaming.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> utils/ipu3/ipu3-pack.c | 100 +++++++++++++++++++++++++++++++++++++++++\n> >> utils/ipu3/meson.build | 1 +\n> >> 2 files changed, 101 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..328e3cd6\n> >> --- /dev/null\n> >> +++ b/utils/ipu3/ipu3-pack.c\n> >> @@ -0,0 +1,100 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * ipu3-pack - Pack IPU3 raw Bayer format to 10-bit Bayer\n> >\n> > I've commented on the usage message in v1 but overlooked this:\n> >\n> > * Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\n> \n> Me too :(\n> \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> >> +\n> >> +static void usage(const char *argv0)\n> >> +{\n> >> +\tprintf(\"Usage:%s input-file output-file('-' to use stdout)\\n\", basename(argv0));\n> >> +\tprintf(\"Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\\n\");\n> >\n> > \tprintf(\"Usage: %s input-file output-file\\n\", basename(argv0));\n> > \tprintf(\"Convert unpacked RAW10 Bayer data to the IPU3 packed Bayer formats\\n\");\n> > \tprintf(\"If the output-file '-', output data will be written to standard output\\n\");\n> >\n> >> +}\n> >> +\n> >> +int main(int argc, char *argv[])\n> >> +{\n> >> +\tint in_fd;\n> >> +\tint out_fd;\n> >> +\tint ret;\n> >> +\n> >> +\tif (argc != 3) {\n> >> +\t\tusage(argv[0]);\n> >> +\t\treturn 1;\n> >> +\t}\n> >> +\n> >> +\tin_fd = open(argv[1], O_RDONLY);\n> >> +\tif (in_fd == -1) {\n> >> +\t\tfprintf(stderr, \"Failed to open input file '%s': %s\\n\",\n> >> +\t\t\targv[1], strerror(errno));\n> >> +\t\treturn 1;\n> >> +\t}\n> >> +\n> >> +\tif (strcmp(argv[2], \"-\") == 0) {\n> >> +\t\tout_fd = STDOUT_FILENO;\n> >> +\t\tfflush(stdout);\n> >\n> > Is fflush() needed ?\n> \n> So, to be honest I am bit uncomfortable with writing to stdout really.\n> \n> The descriptor is open always, so I guess any other process can write() \n> to it? Won't it corrupt the packed data if that happens, as it write \n> interleaved output data to the file.\n\nFile descriptors are per process, so no other process can write to\nstdout of ipu3-pack.\n\n> So, at least from the starting point, I prefer to fflush()ing the stdout \n> so any buffered data is cleared off to the console, before we start \n> using it for our purpose? Does it makes sense, I might probably have \n> missed something..\n\nWhen outputting to stdout, ipu3-pack will likely be used with something\nsimilar as\n\n\tipu3-pack frame.bin - >> frames-packed.bin\n\nFlushing stdout for the ipu3-pack process will make no difference,\nanything that this program writes to stdout will be redirected to the\nfile.\n\n> >> +\t} else {\n> >> +\t\tout_fd = open(argv[2], O_WRONLY | O_TRUNC | O_CREAT, 0644);\n> >> +\t\tif (out_fd == -1) {\n> >> +\t\t\tfprintf(stderr, \"Failed to open output file '%s': %s\\n\",\n> >> +\t\t\t\targv[2], strerror(errno));\n> > \t\t\tclose(in_fd);\n> >\n> >> +\t\t\treturn 1;\n> >> +\t\t}\n> >> +\t}\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\tint bytes_to_read = sizeof(in_data);\n> > s/int/size_t/\n> >\n> > But I'd use sizeof(in_data) directly below. Adding a variable adds a\n> > level of indirection, which makes the code more difficult to read.\n> >\n> >> +\t\tret = read(in_fd, in_data, bytes_to_read);\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\tgoto done;\n> >> +\t\t}\n> >> +\n> >> +\t\tif (ret < bytes_to_read) {\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> >> +\t\t\tbreak;\n> > I would either 'goto done' here or 'break' in the places where you 'goto\n> > done' for consistency.\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> >> +\t\t}\n> >> +\n> >> +\t\tfor (i = 0; i < 30; ++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> >> +\t\t}\n> >> +\n> >> +\t\tout_data[30] = (in_data[24] >> 0) & 0xff;\n> >> +\t\tout_data[31] = (in_data[24] >> 8) & 0x03;\n> >> +\n> >> +\t\tret = write(out_fd, out_data, sizeof(out_data));\n> >> +\t\tif (ret < 0) {\n> >> +\t\t\tfprintf(stderr, \"Failed to write output data: %s\\n\",\n> >> +\t\t\t\tstrerror(errno));\n> >> +\t\t\tgoto done;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +done:\n> >> +\tclose(in_fd);\n> >> +\tif (out_fd != STDOUT_FILENO)\n> >> +\t\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 50F27BE173\n\tfor ;\n\tTue, 12 Jul 2022 10:20:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A3936330F;\n\tTue, 12 Jul 2022 12:20:17 +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 942E860402\n\tfor ;\n\tTue, 12 Jul 2022 12:20:15 +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 E652130B;\n\tTue, 12 Jul 2022 12:20:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657621217;\n\tbh=10fxEaMLQb1kJ9wtjbVvC+mXQpPP/Cv3bRB6gXl7nUo=;\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=HCXwIkdoG3Zr2W/19dmLn5WNKndU9lGThZEgHLQxs+iZmpPriv77qQ9uk6EVHdcnF\n\t89p5ds4STD7fhvbmgQiNlv9rGxw61IRacEudak6laDVR7Yb2rDwTpJjnOvfEQknBgt\n\tselmYGULgUpItI5S4ANfuvuXwtkalsgqw7D6kgVSn+piM3GlDYal9BUo/ojdKqnJjN\n\tMrq2ABb13Zw1ycZtLO82vg1pdzNEb0w8h3QRxsMyISGhcqUk6SSe5TjOTK5c6Nur00\n\t+OJkhsXJqtNcNZdv5kDNYvOBcTS2DcjXntv9D90piDgzehaYnuaXPDMSao3a4QkbE6\n\t7C/6rHJVF05sQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657621215;\n\tbh=10fxEaMLQb1kJ9wtjbVvC+mXQpPP/Cv3bRB6gXl7nUo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JUMUq/sfGIjX/a164wPrFKQyXUwM3taPhdnGpbB1uVR35sfKTJK+mJ3FzXILCBTcE\n\tuMHEQJS/r0U36kOsaDLM/G4aoewB8kz1yBod3GQ1CnEWtj2a3e7rr8HabH4HzxGoL4\n\tywF0KPOgYUatWIr6KNuuB+pVXGuPP6U2TrWcGUms="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"JUMUq/sf\"; dkim-atps=neutral","Date":"Tue, 12 Jul 2022 13:19:46 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220711105835.48254-1-umang.jain@ideasonboard.com>\n\t<20220711105835.48254-3-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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\" "}}]