[{"id":29567,"web_url":"https://patchwork.libcamera.org/comment/29567/","msgid":"<20240520121108.GA16345@pendragon.ideasonboard.com>","date":"2024-05-20T12:11:08","subject":"Re: [PATCH 1/2] apps: cam: Add option to configure sensor","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, May 20, 2024 at 05:31:09PM +0530, Umang Jain wrote:\n> From: Jacopo Mondi \n> \n> Add a '-f|--sensor_format' option to cam to allow forcing a sensor\n> configuration from the command line.\n> \n> As an example:\n> cam -c1 -C -S --stream pixelformat=YUYV -f width=3840,height=2160,bitDepth=10\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Paul Elder \n\nI *think* we discussed this previously, but I may be wrong.\n\nBefore we push this towards applications, I want the sensor\nconfiguration to be fully specified, including binning/skipping and the\ncrop rectangles. Our current API to configure the sensor isn't good\nenough.\n\n> ---\n> src/apps/cam/camera_session.cpp | 7 +++++\n> src/apps/cam/main.cpp | 4 +++\n> src/apps/cam/main.h | 1 +\n> src/apps/common/stream_options.cpp | 42 ++++++++++++++++++++++++++++++\n> src/apps/common/stream_options.h | 8 ++++++\n> 5 files changed, 62 insertions(+)\n> \n> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> index f13355ba..a51dcfbc 100644\n> --- a/src/apps/cam/camera_session.cpp\n> +++ b/src/apps/cam/camera_session.cpp\n> @@ -90,6 +90,13 @@ CameraSession::CameraSession(CameraManager *cm,\n> \t\treturn;\n> \t}\n> \n> +\t/* Apply a sensor configuration is requested. */\n> +\tif (SensorKeyValueParser::updateConfiguration(config.get(),\n> +\t\t\t\t\t\t options_[OptSensorFmt])) {\n> +\t\tstd::cerr << \"Failed to apply sensor configuration\" << std::endl;\n> +\t\treturn;\n> +\t}\n> +\n> \tbool strictFormats = options_.isSet(OptStrictFormats);\n> \n> #ifdef HAVE_KMS\n> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> index 4f87f200..0f751469 100644\n> --- a/src/apps/cam/main.cpp\n> +++ b/src/apps/cam/main.cpp\n> @@ -110,6 +110,7 @@ void CamApp::quit()\n> int CamApp::parseOptions(int argc, char *argv[])\n> {\n> \tStreamKeyValueParser streamKeyValue;\n> +\tSensorKeyValueParser sensorKeyValue;\n> \n> \tOptionsParser parser;\n> \tparser.addOption(OptCamera, OptionString,\n> @@ -178,6 +179,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \t\t\t \"Load a capture session configuration script from a file\",\n> \t\t\t \"script\", ArgumentRequired, \"script\", false,\n> \t\t\t OptCamera);\n> +\tparser.addOption(OptSensorFmt, &sensorKeyValue,\n> +\t\t\t \"Apply a format to the sensor\", \"sensor_format\", true,\n> +\t\t\t OptCamera);\n> \n> \toptions_ = parser.parse(argc, argv);\n> \tif (!options_.valid())\n> diff --git a/src/apps/cam/main.h b/src/apps/cam/main.h\n> index 64e6a20e..65844ac5 100644\n> --- a/src/apps/cam/main.h\n> +++ b/src/apps/cam/main.h\n> @@ -20,6 +20,7 @@ enum {\n> \tOptOrientation = 'o',\n> \tOptSDL = 'S',\n> \tOptStream = 's',\n> +\tOptSensorFmt = 'f',\n> \tOptListControls = 256,\n> \tOptStrictFormats = 257,\n> \tOptMetadata = 258,\n> diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp\n> index 99239e07..55a2d3d2 100644\n> --- a/src/apps/common/stream_options.cpp\n> +++ b/src/apps/common/stream_options.cpp\n> @@ -119,3 +119,45 @@ std::optional StreamKeyValueParser::parseRole(const KeyVa\n> \n> \treturn {};\n> }\n> +\n> +SensorKeyValueParser::SensorKeyValueParser()\n> +{\n> +\taddOption(\"bitDepth\", OptionInteger, \"Sensor format bit depth\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"width\", OptionInteger, \"Sensor frame width in pixels\",\n> +\t\t ArgumentRequired);\n> +\taddOption(\"height\", OptionInteger, \"Sensor frame height in pixels\",\n> +\t\t ArgumentRequired);\n> +}\n> +\n> +int SensorKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> +\t\t\t\t\t const OptionValue &values)\n> +{\n> +\tif (!config) {\n> +\t\tstd::cerr << \"No configuration provided\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* If no configuration values nothing to do. */\n> +\tif (values.empty())\n> +\t\treturn 0;\n> +\n> +\tconst std::vector &streamParameters = values.toArray();\n> +\tSensorConfiguration sensorConfig;\n> +\n> +\tfor (auto const &value : streamParameters) {\n> +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> +\n> +\t\tif (opts.isSet(\"width\") && opts.isSet(\"height\")) {\n> +\t\t\tsensorConfig.outputSize.width = opts[\"width\"];\n> +\t\t\tsensorConfig.outputSize.height = opts[\"height\"];\n> +\t\t}\n> +\n> +\t\tif (opts.isSet(\"bitDepth\"))\n> +\t\t\tsensorConfig.bitDepth = opts[\"bitDepth\"];\n> +\t}\n> +\n> +\tconfig->sensorConfig = sensorConfig;\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h\n> index a93f104c..f1020d8c 100644\n> --- a/src/apps/common/stream_options.h\n> +++ b/src/apps/common/stream_options.h\n> @@ -27,3 +27,11 @@ public:\n> private:\n> \tstatic std::optional parseRole(const KeyValueParser::Options &options);\n> };\n> +\n> +class SensorKeyValueParser : public KeyValueParser\n> +{\n> +public:\n> +\tSensorKeyValueParser();\n> +\tstatic int updateConfiguration(libcamera::CameraConfiguration *config,\n> +\t\t\t\t const OptionValue &values);\n> +};","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 2EE1DBDE6B\n\tfor ;\n\tMon, 20 May 2024 12:11:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4016A63481;\n\tMon, 20 May 2024 14:11: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 1437D6346B\n\tfor ;\n\tMon, 20 May 2024 14:11:17 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 553CE593;\n\tMon, 20 May 2024 14:11:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tW75T2q3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716207065;\n\tbh=NsIJcTUhvw3IrZyTWX1nl7da+WtYTtNmwBasu4g/7sM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tW75T2q3kqIN4dinGhqM/n/LI87E1LNxlyaPxatUiu8UIsat8exgoT9W8ikWUoapC\n\tIsDhFfvN6mXFw7EWAWUvi4CvrUI5KMQj+Ab+f/3orwmpZf+TS1SrTgw2fystlST2yt\n\thGNfrArSnEKKX+ViypVQwYLW2jEvEWdvcx+Tg95s=","Date":"Mon, 20 May 2024 15:11:08 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [PATCH 1/2] apps: cam: Add option to configure sensor","Message-ID":"<20240520121108.GA16345@pendragon.ideasonboard.com>","References":"<20240520120110.110067-1-umang.jain@ideasonboard.com>\n\t<20240520120110.110067-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240520120110.110067-2-umang.jain@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33709,"web_url":"https://patchwork.libcamera.org/comment/33709/","msgid":"","date":"2025-03-26T09:08:53","subject":"Re: [PATCH 1/2] apps: cam: Add option to configure sensor","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Mon, May 20, 2024 at 03:11:08PM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Mon, May 20, 2024 at 05:31:09PM +0530, Umang Jain wrote:\n> > From: Jacopo Mondi \n> > \n> > Add a '-f|--sensor_format' option to cam to allow forcing a sensor\n> > configuration from the command line.\n> > \n> > As an example:\n> > cam -c1 -C -S --stream pixelformat=YUYV -f width=3840,height=2160,bitDepth=10\n> > \n> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Paul Elder \n> \n> I *think* we discussed this previously, but I may be wrong.\n> \n> Before we push this towards applications, I want the sensor\n> configuration to be fully specified, including binning/skipping and the\n> crop rectangles. Our current API to configure the sensor isn't good\n> enough.\n\nafaict the sensor configuration already supports binning and skipping\nand crop rectangles. What else is missing?\n\n\nPaul\n\n> \n> > ---\n> > src/apps/cam/camera_session.cpp | 7 +++++\n> > src/apps/cam/main.cpp | 4 +++\n> > src/apps/cam/main.h | 1 +\n> > src/apps/common/stream_options.cpp | 42 ++++++++++++++++++++++++++++++\n> > src/apps/common/stream_options.h | 8 ++++++\n> > 5 files changed, 62 insertions(+)\n> > \n> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > index f13355ba..a51dcfbc 100644\n> > --- a/src/apps/cam/camera_session.cpp\n> > +++ b/src/apps/cam/camera_session.cpp\n> > @@ -90,6 +90,13 @@ CameraSession::CameraSession(CameraManager *cm,\n> > \t\treturn;\n> > \t}\n> > \n> > +\t/* Apply a sensor configuration is requested. */\n> > +\tif (SensorKeyValueParser::updateConfiguration(config.get(),\n> > +\t\t\t\t\t\t options_[OptSensorFmt])) {\n> > +\t\tstd::cerr << \"Failed to apply sensor configuration\" << std::endl;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > \tbool strictFormats = options_.isSet(OptStrictFormats);\n> > \n> > #ifdef HAVE_KMS\n> > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp\n> > index 4f87f200..0f751469 100644\n> > --- a/src/apps/cam/main.cpp\n> > +++ b/src/apps/cam/main.cpp\n> > @@ -110,6 +110,7 @@ void CamApp::quit()\n> > int CamApp::parseOptions(int argc, char *argv[])\n> > {\n> > \tStreamKeyValueParser streamKeyValue;\n> > +\tSensorKeyValueParser sensorKeyValue;\n> > \n> > \tOptionsParser parser;\n> > \tparser.addOption(OptCamera, OptionString,\n> > @@ -178,6 +179,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \t\t\t \"Load a capture session configuration script from a file\",\n> > \t\t\t \"script\", ArgumentRequired, \"script\", false,\n> > \t\t\t OptCamera);\n> > +\tparser.addOption(OptSensorFmt, &sensorKeyValue,\n> > +\t\t\t \"Apply a format to the sensor\", \"sensor_format\", true,\n> > +\t\t\t OptCamera);\n> > \n> > \toptions_ = parser.parse(argc, argv);\n> > \tif (!options_.valid())\n> > diff --git a/src/apps/cam/main.h b/src/apps/cam/main.h\n> > index 64e6a20e..65844ac5 100644\n> > --- a/src/apps/cam/main.h\n> > +++ b/src/apps/cam/main.h\n> > @@ -20,6 +20,7 @@ enum {\n> > \tOptOrientation = 'o',\n> > \tOptSDL = 'S',\n> > \tOptStream = 's',\n> > +\tOptSensorFmt = 'f',\n> > \tOptListControls = 256,\n> > \tOptStrictFormats = 257,\n> > \tOptMetadata = 258,\n> > diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp\n> > index 99239e07..55a2d3d2 100644\n> > --- a/src/apps/common/stream_options.cpp\n> > +++ b/src/apps/common/stream_options.cpp\n> > @@ -119,3 +119,45 @@ std::optional StreamKeyValueParser::parseRole(const KeyVa\n> > \n> > \treturn {};\n> > }\n> > +\n> > +SensorKeyValueParser::SensorKeyValueParser()\n> > +{\n> > +\taddOption(\"bitDepth\", OptionInteger, \"Sensor format bit depth\",\n> > +\t\t ArgumentRequired);\n> > +\taddOption(\"width\", OptionInteger, \"Sensor frame width in pixels\",\n> > +\t\t ArgumentRequired);\n> > +\taddOption(\"height\", OptionInteger, \"Sensor frame height in pixels\",\n> > +\t\t ArgumentRequired);\n> > +}\n> > +\n> > +int SensorKeyValueParser::updateConfiguration(CameraConfiguration *config,\n> > +\t\t\t\t\t const OptionValue &values)\n> > +{\n> > +\tif (!config) {\n> > +\t\tstd::cerr << \"No configuration provided\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\t/* If no configuration values nothing to do. */\n> > +\tif (values.empty())\n> > +\t\treturn 0;\n> > +\n> > +\tconst std::vector &streamParameters = values.toArray();\n> > +\tSensorConfiguration sensorConfig;\n> > +\n> > +\tfor (auto const &value : streamParameters) {\n> > +\t\tKeyValueParser::Options opts = value.toKeyValues();\n> > +\n> > +\t\tif (opts.isSet(\"width\") && opts.isSet(\"height\")) {\n> > +\t\t\tsensorConfig.outputSize.width = opts[\"width\"];\n> > +\t\t\tsensorConfig.outputSize.height = opts[\"height\"];\n> > +\t\t}\n> > +\n> > +\t\tif (opts.isSet(\"bitDepth\"))\n> > +\t\t\tsensorConfig.bitDepth = opts[\"bitDepth\"];\n> > +\t}\n> > +\n> > +\tconfig->sensorConfig = sensorConfig;\n> > +\n> > +\treturn 0;\n> > +}\n> > diff --git a/src/apps/common/stream_options.h b/src/apps/common/stream_options.h\n> > index a93f104c..f1020d8c 100644\n> > --- a/src/apps/common/stream_options.h\n> > +++ b/src/apps/common/stream_options.h\n> > @@ -27,3 +27,11 @@ public:\n> > private:\n> > \tstatic std::optional parseRole(const KeyValueParser::Options &options);\n> > };\n> > +\n> > +class SensorKeyValueParser : public KeyValueParser\n> > +{\n> > +public:\n> > +\tSensorKeyValueParser();\n> > +\tstatic int updateConfiguration(libcamera::CameraConfiguration *config,\n> > +\t\t\t\t const OptionValue &values);\n> > +};\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 3CA2DC3213\n\tfor ;\n\tWed, 26 Mar 2025 09:09:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA5176895B;\n\tWed, 26 Mar 2025 10:09:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57FDC68947\n\tfor ;\n\tWed, 26 Mar 2025 10:09:01 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:7402:917d:ea0c:6d4c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B959399F;\n\tWed, 26 Mar 2025 10:07:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IzzRZJnk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742980033;\n\tbh=IEWW3wlbLu3Q/ZF5wPw6Si7a1PDZYUUR5vZYMEa4MtU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IzzRZJnkAVbPxb1vKQ8KhZV/vHKX33xRs+J4VQIH+XKMyGt7t0unQjdNcCL5kV5ua\n\tFUjamIjHQQCh8RsYHULz0vIRHGldR+T/89OzPwTYUC3WmikCy7mK7z166h8Y9oSWN0\n\tYDojv5YQsVl/bfsOa2/KwshI91Zv81RB66Lhemw0=","Date":"Wed, 26 Mar 2025 18:08:53 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [PATCH 1/2] apps: cam: Add option to configure sensor","Message-ID":"","References":"<20240520120110.110067-1-umang.jain@ideasonboard.com>\n\t<20240520120110.110067-2-umang.jain@ideasonboard.com>\n\t<20240520121108.GA16345@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240520121108.GA16345@pendragon.ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]