[{"id":31517,"web_url":"https://patchwork.libcamera.org/comment/31517/","msgid":"<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>","date":"2024-10-02T07:14:38","subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Paul\n\nOn Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:\n> Add support to the cam application for loading the camera configuration\n> from a capture script. These are not expected to be written by hand, but\n\nWouldn't it be useful to write camera configurations by hand ? It\ndoesn't seem to require any special handling compared to what you have\nalready done here, right ?\n\n> rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment\n> variable.\n>\n> If any configuration options are specified by command line parameters,\n> those will take precedence.\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/apps/cam/camera_session.cpp | 22 +++--\n> src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++\n> src/apps/cam/capture_script.h | 8 ++\n> 3 files changed, 184 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> index edc49b875450..6f1d8b58870f 100644\n> --- a/src/apps/cam/camera_session.cpp\n> +++ b/src/apps/cam/camera_session.cpp\n> @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,\n> \t\treturn;\n> \t}\n>\n> +\tif (options_.isSet(OptCaptureScript)) {\n> +\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> +\t\tscript_ = std::make_unique(camera_, scriptName);\n> +\t\tif (!script_->valid()) {\n> +\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> +\t\t\t\t << \"'\" << std::endl;\n> +\t\t\treturn;\n> +\t\t}\n> +\n\nWhat about adding a comment here to record that options specified on\nthe command line take precendece over options coming from the\nconfiguration file ?\n\n> +\t\tscript_->populateConfiguration(config.get());\n> +\t}\n> +\n> \tif (options_.isSet(OptOrientation)) {\n> \t\tstd::string orientOpt = options_[OptOrientation].toString();\n> \t\tstatic const std::map orientations{\n> @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,\n> \t}\n> #endif\n>\n> -\tif (options_.isSet(OptCaptureScript)) {\n> -\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> -\t\tscript_ = std::make_unique(camera_, scriptName);\n> -\t\tif (!script_->valid()) {\n> -\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> -\t\t\t\t << \"'\" << std::endl;\n> -\t\t\treturn;\n> -\t\t}\n> -\t}\n> -\n> \tswitch (config->validate()) {\n> \tcase CameraConfiguration::Valid:\n> \t\tbreak;\n> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> index fc1dfa75f2d4..7f166f45657e 100644\n> --- a/src/apps/cam/capture_script.cpp\n> +++ b/src/apps/cam/capture_script.cpp\n> @@ -7,6 +7,7 @@\n>\n> #include \"capture_script.h\"\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)\n> \t\t\tret = parseFrames();\n> \t\t\tif (ret)\n> \t\t\t\treturn ret;\n> +\t\t} else if (section == \"configuration\") {\n> +\t\t\tret = parseConfiguration();\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> \t\t} else {\n> \t\t\tstd::cerr << \"Unsupported section '\" << section << \"'\"\n> \t\t\t\t << std::endl;\n> @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> \treturn 0;\n> }\n>\n> +int CaptureScript::parseOrientation(EventPtr event)\n> +{\n> +\tstatic const std::map orientations{\n> +\t\t{ \"Rotate0\", libcamera::Orientation::Rotate0 },\n> +\t\t{ \"Rotate0Mirror\", libcamera::Orientation::Rotate0Mirror },\n> +\t\t{ \"Rotate180\", libcamera::Orientation::Rotate180 },\n> +\t\t{ \"Rotate180Mirror\", libcamera::Orientation::Rotate180Mirror },\n> +\t\t{ \"Rotate90Mirror\", libcamera::Orientation::Rotate90Mirror },\n> +\t\t{ \"Rotate270\", libcamera::Orientation::Rotate270 },\n> +\t\t{ \"Rotate270Mirror\", libcamera::Orientation::Rotate270Mirror },\n> +\t\t{ \"Rotate90\", libcamera::Orientation::Rotate90 },\n> +\t};\n> +\n> +\tstd::string orientation = eventScalarValue(event);\n> +\n> +\tauto it = orientations.find(orientation);\n> +\tif (it == orientations.end()) {\n> +\t\tstd::cerr << \"Invalid orientation '\" << orientation\n> +\t\t\t << \"' in capture script\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\torientation_ = it->second;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CaptureScript::parseStream(EventPtr event, unsigned int index)\n> +{\n> +\tif (!checkEvent(event, YAML_MAPPING_START_EVENT))\n> +\t\treturn -EINVAL;\n> +\n> +\tStreamConfiguration config;\n> +\twhile (1) {\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> +\t\t\tbreak;\n> +\n> +\t\tstd::string key = eventScalarValue(event);\n> +\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> +\t\t\tbreak;\n\nisn't this an error condition ? If you get a MAPPING_END after a key\nisn't the scrip malformed ?\n\n> +\n> +\t\tstd::string value = eventScalarValue(event);\n> +\n> +\t\tif (key == \"pixelFormat\") {\n> +\t\t\tconfig.pixelFormat = libcamera::PixelFormat::fromString(value);\n\nShould this be validated ?\n\n> +\t\t} else if (key == \"size\") {\n> +\t\t\tunsigned int split = value.find(\"x\");\n> +\t\t\tif (split == std::string::npos) {\n> +\t\t\t\tstd::cerr << \"Invalid size '\" << value\n> +\t\t\t\t\t << \"' in stream configuration \"\n> +\t\t\t\t\t << index << std::endl;\n> +\t\t\t}\n> +\n> +\t\t\tstd::string width = value.substr(0, split);\n> +\t\t\tstd::string height = value.substr(split + 1);\n> +\t\t\tconfig.size = Size(std::stoi(width), std::stoi(height));\n> +\t\t} else if (key == \"stride\") {\n> +\t\t\tconfig.stride = std::stoi(value);\n> +\t\t} else if (key == \"frameSize\") {\n> +\t\t\tconfig.frameSize = std::stoi(value);\n\nDo we allow to specify the frame size ? What's the purpose ? Also, we\nalready have stride so it can be calculated from there ?\n\n> +\t\t} else if (key == \"bufferCount\") {\n> +\t\t\tconfig.bufferCount = std::stoi(value);\n> +\t\t} else if (key == \"colorSpace\") {\n> +\t\t\tconfig.colorSpace = libcamera::ColorSpace::fromString(value);\n> +\t\t} else {\n> +\t\t\tstd::cerr << \"Unknown key-value pair '\"\n> +\t\t\t\t << key << \"': '\" << value\n> +\t\t\t\t << \"' in stream configuration \"\n> +\t\t\t\t << index << std::endl;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n\nAre there mandatory fields to be specified ? Should we check that all\nthe above have been populated in the script ? I'm concerned that a\nscript might specify only a partial set of configurations. Ofc if you\nconsider this to be only generated by a dump, that's not an issue ;)\n\n> +\n> +\tstreamConfigs_.push_back(config);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CaptureScript::parseStreams(EventPtr event)\n> +{\n> +\tif (!checkEvent(event, YAML_SEQUENCE_START_EVENT))\n> +\t\treturn -EINVAL;\n> +\n> +\tunsigned int index = 0;\n> +\twhile (1) {\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> +\t\t\treturn 0;\n> +\n> +\t\tif (event->type == YAML_MAPPING_START_EVENT) {\n> +\t\t\tparseStream(std::move(event), index++);\n> +\t\t\tcontinue;\n> +\t\t} else {\n> +\t\t\tstd::cerr << \"UNKNOWN TYPE\" << std::endl;\n\nno need to scream :)\n\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CaptureScript::parseConfiguration()\n> +{\n> +\tint ret;\n> +\n> +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> +\tif (!event)\n> +\t\treturn -EINVAL;\n> +\n> +\twhile (1) {\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> +\t\t\tbreak;\n> +\n> +\t\tstd::string key = eventScalarValue(event);\n> +\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> +\t\t\tbreak;\n\nSame question, a mapping_end event after a key is an error ?\n\nThanks\n j\n\n> +\n> +\t\t/* TODO Load sensor configuration */\n> +\t\tif (key == \"orientation\") {\n> +\t\t\tret = parseOrientation(std::move(event));\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t} else if (key == \"streams\") {\n> +\t\t\tret = parseStreams(std::move(event));\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const\n> +{\n> +\tif (!configuration)\n> +\t\treturn;\n> +\n> +\tconfiguration->orientation = orientation_;\n> +\n> +\tfor (unsigned int i = 0; i < streamConfigs_.size(); i++)\n> +\t\t(*configuration)[i] = streamConfigs_[i];\n> +}\n> +\n> std::string CaptureScript::parseScalar()\n> {\n> \tEventPtr event = nextEvent(YAML_SCALAR_EVENT);\n> diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h\n> index 294b920363ba..4ba862d742cf 100644\n> --- a/src/apps/cam/capture_script.h\n> +++ b/src/apps/cam/capture_script.h\n> @@ -26,6 +26,7 @@ public:\n>\n> \tconst libcamera::ControlList &frameControls(unsigned int frame);\n>\n> +\tvoid populateConfiguration(libcamera::CameraConfiguration *configuration) const;\n> private:\n> \tstruct EventDeleter {\n> \t\tvoid operator()(yaml_event_t *event) const\n> @@ -43,6 +44,9 @@ private:\n> \tunsigned int loop_;\n> \tbool valid_;\n>\n> +\tlibcamera::Orientation orientation_;\n> +\tstd::vector streamConfigs_;\n> +\n> \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> \tbool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;\n> \tstatic std::string eventScalarValue(const EventPtr &event);\n> @@ -55,6 +59,10 @@ private:\n> \tint parseFrames();\n> \tint parseFrame(EventPtr event);\n> \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> +\tint parseConfiguration();\n> +\tint parseOrientation(EventPtr event);\n> +\tint parseStreams(EventPtr event);\n> +\tint parseStream(EventPtr event, unsigned int index);\n>\n> \tlibcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,\n> \t\t\t\t\t\t const std::string repr);\n> --\n> 2.39.2\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 73B13C3257\n\tfor ;\n\tWed, 2 Oct 2024 07:14:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7261D6351F;\n\tWed, 2 Oct 2024 09:14:45 +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 B558D60534\n\tfor ;\n\tWed, 2 Oct 2024 09:14:43 +0200 (CEST)","from ideasonboard.com (unknown [5.77.88.238])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 089A85A5;\n\tWed, 2 Oct 2024 09:13:10 +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=\"UFcPOLXu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727853191;\n\tbh=me1XnOb0v/WriPMZ0/l6O0d8P33gwG0IbSuma8+EoGU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UFcPOLXuOIVDapVJionmhkNi4Wte/LYswT/RuO5STnq0XrksjhC6OAuGB/5oEDdrh\n\tvLVGd5O/kaRU+LFAwWVG7vk3AIv6XNntNGePBaGBSefa5z7XpMi4BQImDxFLr5ZlT2\n\t8frAoSw9r/N8l13ODC5IiV/xjEpD7Cma5J+SK9D0=","Date":"Wed, 2 Oct 2024 09:14:38 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","Message-ID":"<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240930170907.1404844-3-paul.elder@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":31521,"web_url":"https://patchwork.libcamera.org/comment/31521/","msgid":"","date":"2024-10-02T10:35:21","subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:\n> Hi Paul\n> \n> On Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:\n> > Add support to the cam application for loading the camera configuration\n> > from a capture script. These are not expected to be written by hand, but\n> \n> Wouldn't it be useful to write camera configurations by hand ? It\n> doesn't seem to require any special handling compared to what you have\n> already done here, right ?\n\nI didn't expect it to happen but you certainly can. I suppose I can\nremove this to remove confusion?\n\n> \n> > rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment\n> > variable.\n> >\n> > If any configuration options are specified by command line parameters,\n> > those will take precedence.\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/apps/cam/camera_session.cpp | 22 +++--\n> > src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++\n> > src/apps/cam/capture_script.h | 8 ++\n> > 3 files changed, 184 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > index edc49b875450..6f1d8b58870f 100644\n> > --- a/src/apps/cam/camera_session.cpp\n> > +++ b/src/apps/cam/camera_session.cpp\n> > @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,\n> > \t\treturn;\n> > \t}\n> >\n> > +\tif (options_.isSet(OptCaptureScript)) {\n> > +\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > +\t\tscript_ = std::make_unique(camera_, scriptName);\n> > +\t\tif (!script_->valid()) {\n> > +\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > +\t\t\t\t << \"'\" << std::endl;\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> \n> What about adding a comment here to record that options specified on\n> the command line take precendece over options coming from the\n> configuration file ?\n\nGood idea.\n\n> \n> > +\t\tscript_->populateConfiguration(config.get());\n> > +\t}\n> > +\n> > \tif (options_.isSet(OptOrientation)) {\n> > \t\tstd::string orientOpt = options_[OptOrientation].toString();\n> > \t\tstatic const std::map orientations{\n> > @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,\n> > \t}\n> > #endif\n> >\n> > -\tif (options_.isSet(OptCaptureScript)) {\n> > -\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > -\t\tscript_ = std::make_unique(camera_, scriptName);\n> > -\t\tif (!script_->valid()) {\n> > -\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > -\t\t\t\t << \"'\" << std::endl;\n> > -\t\t\treturn;\n> > -\t\t}\n> > -\t}\n> > -\n> > \tswitch (config->validate()) {\n> > \tcase CameraConfiguration::Valid:\n> > \t\tbreak;\n> > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> > index fc1dfa75f2d4..7f166f45657e 100644\n> > --- a/src/apps/cam/capture_script.cpp\n> > +++ b/src/apps/cam/capture_script.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \"capture_script.h\"\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)\n> > \t\t\tret = parseFrames();\n> > \t\t\tif (ret)\n> > \t\t\t\treturn ret;\n> > +\t\t} else if (section == \"configuration\") {\n> > +\t\t\tret = parseConfiguration();\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > \t\t} else {\n> > \t\t\tstd::cerr << \"Unsupported section '\" << section << \"'\"\n> > \t\t\t\t << std::endl;\n> > @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > \treturn 0;\n> > }\n> >\n> > +int CaptureScript::parseOrientation(EventPtr event)\n> > +{\n> > +\tstatic const std::map orientations{\n> > +\t\t{ \"Rotate0\", libcamera::Orientation::Rotate0 },\n> > +\t\t{ \"Rotate0Mirror\", libcamera::Orientation::Rotate0Mirror },\n> > +\t\t{ \"Rotate180\", libcamera::Orientation::Rotate180 },\n> > +\t\t{ \"Rotate180Mirror\", libcamera::Orientation::Rotate180Mirror },\n> > +\t\t{ \"Rotate90Mirror\", libcamera::Orientation::Rotate90Mirror },\n> > +\t\t{ \"Rotate270\", libcamera::Orientation::Rotate270 },\n> > +\t\t{ \"Rotate270Mirror\", libcamera::Orientation::Rotate270Mirror },\n> > +\t\t{ \"Rotate90\", libcamera::Orientation::Rotate90 },\n> > +\t};\n> > +\n> > +\tstd::string orientation = eventScalarValue(event);\n> > +\n> > +\tauto it = orientations.find(orientation);\n> > +\tif (it == orientations.end()) {\n> > +\t\tstd::cerr << \"Invalid orientation '\" << orientation\n> > +\t\t\t << \"' in capture script\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\torientation_ = it->second;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CaptureScript::parseStream(EventPtr event, unsigned int index)\n> > +{\n> > +\tif (!checkEvent(event, YAML_MAPPING_START_EVENT))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tStreamConfiguration config;\n> > +\twhile (1) {\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tstd::string key = eventScalarValue(event);\n> > +\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > +\t\t\tbreak;\n> \n> isn't this an error condition ? If you get a MAPPING_END after a key\n> isn't the scrip malformed ?\n\nI'm not sure how it shows up in the yaml parser, but afaiu it's possible\nhave a key with no value. We do that in tuning files. (Not that this is\na tuning file, but speaking from a yaml-correctness perspective).\n\n> \n> > +\n> > +\t\tstd::string value = eventScalarValue(event);\n> > +\n> > +\t\tif (key == \"pixelFormat\") {\n> > +\t\t\tconfig.pixelFormat = libcamera::PixelFormat::fromString(value);\n> \n> Should this be validated ?\n\nIt should be fine not to. Camera::configure() will adjust it if it's not\nvalid.\n\n> \n> > +\t\t} else if (key == \"size\") {\n> > +\t\t\tunsigned int split = value.find(\"x\");\n> > +\t\t\tif (split == std::string::npos) {\n> > +\t\t\t\tstd::cerr << \"Invalid size '\" << value\n> > +\t\t\t\t\t << \"' in stream configuration \"\n> > +\t\t\t\t\t << index << std::endl;\n> > +\t\t\t}\n> > +\n> > +\t\t\tstd::string width = value.substr(0, split);\n> > +\t\t\tstd::string height = value.substr(split + 1);\n> > +\t\t\tconfig.size = Size(std::stoi(width), std::stoi(height));\n> > +\t\t} else if (key == \"stride\") {\n> > +\t\t\tconfig.stride = std::stoi(value);\n> > +\t\t} else if (key == \"frameSize\") {\n> > +\t\t\tconfig.frameSize = std::stoi(value);\n> \n> Do we allow to specify the frame size ? What's the purpose ? Also, we\n> already have stride so it can be calculated from there ?\n\nThe purpose was to support all fields from the dump. If the\nconfiguration comes from a dump, you'll have all fields. If the\nconfiguration is handwritten (which I didn't actually expect in the\nfirst place but is technically possible), then you can just skip the\nfield, or if its wrong it'll just get adjusted by Camera::configure().\nSo either way it should be fine.\n\n> \n> > +\t\t} else if (key == \"bufferCount\") {\n> > +\t\t\tconfig.bufferCount = std::stoi(value);\n> > +\t\t} else if (key == \"colorSpace\") {\n> > +\t\t\tconfig.colorSpace = libcamera::ColorSpace::fromString(value);\n> > +\t\t} else {\n> > +\t\t\tstd::cerr << \"Unknown key-value pair '\"\n> > +\t\t\t\t << key << \"': '\" << value\n> > +\t\t\t\t << \"' in stream configuration \"\n> > +\t\t\t\t << index << std::endl;\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t}\n> \n> Are there mandatory fields to be specified ? Should we check that all\n> the above have been populated in the script ? I'm concerned that a\n> script might specify only a partial set of configurations. Ofc if you\n> consider this to be only generated by a dump, that's not an issue ;)\n\nSee above.\n\n> \n> > +\n> > +\tstreamConfigs_.push_back(config);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CaptureScript::parseStreams(EventPtr event)\n> > +{\n> > +\tif (!checkEvent(event, YAML_SEQUENCE_START_EVENT))\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tunsigned int index = 0;\n> > +\twhile (1) {\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > +\t\t\treturn 0;\n> > +\n> > +\t\tif (event->type == YAML_MAPPING_START_EVENT) {\n> > +\t\t\tparseStream(std::move(event), index++);\n> > +\t\t\tcontinue;\n> > +\t\t} else {\n> > +\t\t\tstd::cerr << \"UNKNOWN TYPE\" << std::endl;\n> \n> no need to scream :)\n\nOops :p Leftover debugging...\n\n\nThanks,\n\nPaul\n\n> \n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CaptureScript::parseConfiguration()\n> > +{\n> > +\tint ret;\n> > +\n> > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > +\tif (!event)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\twhile (1) {\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tstd::string key = eventScalarValue(event);\n> > +\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > +\t\t\tbreak;\n> \n> Same question, a mapping_end event after a key is an error ?\n> \n> Thanks\n> j\n> \n> > +\n> > +\t\t/* TODO Load sensor configuration */\n> > +\t\tif (key == \"orientation\") {\n> > +\t\t\tret = parseOrientation(std::move(event));\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t} else if (key == \"streams\") {\n> > +\t\t\tret = parseStreams(std::move(event));\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const\n> > +{\n> > +\tif (!configuration)\n> > +\t\treturn;\n> > +\n> > +\tconfiguration->orientation = orientation_;\n> > +\n> > +\tfor (unsigned int i = 0; i < streamConfigs_.size(); i++)\n> > +\t\t(*configuration)[i] = streamConfigs_[i];\n> > +}\n> > +\n> > std::string CaptureScript::parseScalar()\n> > {\n> > \tEventPtr event = nextEvent(YAML_SCALAR_EVENT);\n> > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h\n> > index 294b920363ba..4ba862d742cf 100644\n> > --- a/src/apps/cam/capture_script.h\n> > +++ b/src/apps/cam/capture_script.h\n> > @@ -26,6 +26,7 @@ public:\n> >\n> > \tconst libcamera::ControlList &frameControls(unsigned int frame);\n> >\n> > +\tvoid populateConfiguration(libcamera::CameraConfiguration *configuration) const;\n> > private:\n> > \tstruct EventDeleter {\n> > \t\tvoid operator()(yaml_event_t *event) const\n> > @@ -43,6 +44,9 @@ private:\n> > \tunsigned int loop_;\n> > \tbool valid_;\n> >\n> > +\tlibcamera::Orientation orientation_;\n> > +\tstd::vector streamConfigs_;\n> > +\n> > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > \tbool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;\n> > \tstatic std::string eventScalarValue(const EventPtr &event);\n> > @@ -55,6 +59,10 @@ private:\n> > \tint parseFrames();\n> > \tint parseFrame(EventPtr event);\n> > \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> > +\tint parseConfiguration();\n> > +\tint parseOrientation(EventPtr event);\n> > +\tint parseStreams(EventPtr event);\n> > +\tint parseStream(EventPtr event, unsigned int index);\n> >\n> > \tlibcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,\n> > \t\t\t\t\t\t const std::string repr);\n> > --\n> > 2.39.2\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 4B002C3257\n\tfor ;\n\tWed, 2 Oct 2024 10:35:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEF5B6351F;\n\tWed, 2 Oct 2024 12:35:30 +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 EE16663510\n\tfor ;\n\tWed, 2 Oct 2024 12:35:28 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6ef4:b42e:3cf6:d89b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3DD393E6;\n\tWed, 2 Oct 2024 12:33:55 +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=\"M1GPkQr9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727865236;\n\tbh=inD2s0xDYF61PIzT2JtnnrDp4XXN8xAybEe2BqpYkTQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M1GPkQr95CrwkM0CYs0fLu4tPDcZD0Bn3EWqZfzM4kOcbHMnzvzCHfbR+3ypSZiMl\n\tLh7hAX+Q0anaTzQQaaiM/1sdNl5Wm9+k/PJjkWdCY+xki4w1VO2w6LHgfGFp09+q4i\n\tLCS00pY408gkmC8OP5aDfkYR0rWIKo5xnyjpZjQU=","Date":"Wed, 2 Oct 2024 19:35:21 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","Message-ID":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-3-paul.elder@ideasonboard.com>\n\t<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>","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":31537,"web_url":"https://patchwork.libcamera.org/comment/31537/","msgid":"<20241002232650.GA17537@pendragon.ideasonboard.com>","date":"2024-10-02T23:26:50","subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Oct 02, 2024 at 07:35:21PM +0900, Paul Elder wrote:\n> On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:\n> > > Add support to the cam application for loading the camera configuration\n> > > from a capture script. These are not expected to be written by hand, but\n> > \n> > Wouldn't it be useful to write camera configurations by hand ? It\n> > doesn't seem to require any special handling compared to what you have\n> > already done here, right ?\n> \n> I didn't expect it to happen but you certainly can. I suppose I can\n> remove this to remove confusion?\n\nYou could then replace the last sentence with \"These can be written\nmanually, or dumped from a capture session via the ...\".\n\n> > > rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment\n> > > variable.\n> > >\n> > > If any configuration options are specified by command line parameters,\n> > > those will take precedence.\n> > >\n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/apps/cam/camera_session.cpp | 22 +++--\n> > > src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++\n> > > src/apps/cam/capture_script.h | 8 ++\n> > > 3 files changed, 184 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > index edc49b875450..6f1d8b58870f 100644\n> > > --- a/src/apps/cam/camera_session.cpp\n> > > +++ b/src/apps/cam/camera_session.cpp\n> > > @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > \t\treturn;\n> > > \t}\n> > >\n> > > +\tif (options_.isSet(OptCaptureScript)) {\n> > > +\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > > +\t\tscript_ = std::make_unique(camera_, scriptName);\n> > > +\t\tif (!script_->valid()) {\n> > > +\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > > +\t\t\t\t << \"'\" << std::endl;\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > \n> > What about adding a comment here to record that options specified on\n> > the command line take precendece over options coming from the\n> > configuration file ?\n> \n> Good idea.\n\nIt would also explain why you're moving this block of code.\n\n\t/*\n\t * Parse the capture script first to populate the configuration, and let\n\t * command line arguments take precendence.\n\t */\n\nThis should ideally also be mentioned in the help text.\n\n> > > +\t\tscript_->populateConfiguration(config.get());\n> > > +\t}\n> > > +\n> > > \tif (options_.isSet(OptOrientation)) {\n> > > \t\tstd::string orientOpt = options_[OptOrientation].toString();\n> > > \t\tstatic const std::map orientations{\n> > > @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > \t}\n> > > #endif\n> > >\n> > > -\tif (options_.isSet(OptCaptureScript)) {\n> > > -\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > > -\t\tscript_ = std::make_unique(camera_, scriptName);\n> > > -\t\tif (!script_->valid()) {\n> > > -\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > > -\t\t\t\t << \"'\" << std::endl;\n> > > -\t\t\treturn;\n> > > -\t\t}\n> > > -\t}\n> > > -\n> > > \tswitch (config->validate()) {\n> > > \tcase CameraConfiguration::Valid:\n> > > \t\tbreak;\n> > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> > > index fc1dfa75f2d4..7f166f45657e 100644\n> > > --- a/src/apps/cam/capture_script.cpp\n> > > +++ b/src/apps/cam/capture_script.cpp\n> > > @@ -7,6 +7,7 @@\n> > >\n> > > #include \"capture_script.h\"\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)\n> > > \t\t\tret = parseFrames();\n> > > \t\t\tif (ret)\n> > > \t\t\t\treturn ret;\n> > > +\t\t} else if (section == \"configuration\") {\n> > > +\t\t\tret = parseConfiguration();\n> > > +\t\t\tif (ret)\n> > > +\t\t\t\treturn ret;\n\nI'd move this before \"frames\" to reflect the expected order in the\ncapture script.\n\n> > > \t\t} else {\n> > > \t\t\tstd::cerr << \"Unsupported section '\" << section << \"'\"\n> > > \t\t\t\t << std::endl;\n> > > @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > > \treturn 0;\n> > > }\n> > >\n> > > +int CaptureScript::parseOrientation(EventPtr event)\n> > > +{\n> > > +\tstatic const std::map orientations{\n> > > +\t\t{ \"Rotate0\", libcamera::Orientation::Rotate0 },\n> > > +\t\t{ \"Rotate0Mirror\", libcamera::Orientation::Rotate0Mirror },\n> > > +\t\t{ \"Rotate180\", libcamera::Orientation::Rotate180 },\n> > > +\t\t{ \"Rotate180Mirror\", libcamera::Orientation::Rotate180Mirror },\n> > > +\t\t{ \"Rotate90Mirror\", libcamera::Orientation::Rotate90Mirror },\n> > > +\t\t{ \"Rotate270\", libcamera::Orientation::Rotate270 },\n> > > +\t\t{ \"Rotate270Mirror\", libcamera::Orientation::Rotate270Mirror },\n> > > +\t\t{ \"Rotate90\", libcamera::Orientation::Rotate90 },\n> > > +\t};\n> > > +\n> > > +\tstd::string orientation = eventScalarValue(event);\n\nIf you move this line to the caller, this function could be a shared\nhelper, there's similar code in CameraSession::CameraSession(). The\nhelper could be a global function in a new utils.cpp file in the same\ndirectory.\n\n> > > +\n> > > +\tauto it = orientations.find(orientation);\n> > > +\tif (it == orientations.end()) {\n> > > +\t\tstd::cerr << \"Invalid orientation '\" << orientation\n> > > +\t\t\t << \"' in capture script\" << std::endl;\n\nThe error message would also need to move to the caller, or drop the \"in\ncapture script\" part.\n\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\torientation_ = it->second;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CaptureScript::parseStream(EventPtr event, unsigned int index)\n> > > +{\n> > > +\tif (!checkEvent(event, YAML_MAPPING_START_EVENT))\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tStreamConfiguration config;\n> > > +\twhile (1) {\n> > > +\t\tevent = nextEvent();\n> > > +\t\tif (!event)\n> > > +\t\t\treturn -EINVAL;\n\nAdd a blank line here for consistency with existing code.\n\n> > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tstd::string key = eventScalarValue(event);\n> > > +\n> > > +\t\tevent = nextEvent();\n> > > +\t\tif (!event)\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > +\t\t\tbreak;\n> > \n> > isn't this an error condition ? If you get a MAPPING_END after a key\n> > isn't the scrip malformed ?\n> \n> I'm not sure how it shows up in the yaml parser, but afaiu it's possible\n> have a key with no value. We do that in tuning files. (Not that this is\n> a tuning file, but speaking from a yaml-correctness perspective).\n> \n> > > +\n> > > +\t\tstd::string value = eventScalarValue(event);\n\nI think you can replace this with\n\n\t\tstd::string key = eventScalarValue(event);\n\n\t\tstd::string value = parseScalar();\n\t\tif (value.empty())\n\t\t\treturn -EINVAL;\n\nas the value must be present.\n\n> > > +\n> > > +\t\tif (key == \"pixelFormat\") {\n> > > +\t\t\tconfig.pixelFormat = libcamera::PixelFormat::fromString(value);\n> > \n> > Should this be validated ?\n> \n> It should be fine not to. Camera::configure() will adjust it if it's not\n> valid.\n> \n> > > +\t\t} else if (key == \"size\") {\n> > > +\t\t\tunsigned int split = value.find(\"x\");\n> > > +\t\t\tif (split == std::string::npos) {\n> > > +\t\t\t\tstd::cerr << \"Invalid size '\" << value\n> > > +\t\t\t\t\t << \"' in stream configuration \"\n> > > +\t\t\t\t\t << index << std::endl;\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tstd::string width = value.substr(0, split);\n> > > +\t\t\tstd::string height = value.substr(split + 1);\n> > > +\t\t\tconfig.size = Size(std::stoi(width), std::stoi(height));\n\nWe specify width and height independently on the command line, would it\nmake sense to do the same in yaml ? It would simplify the code here.\n\n> > > +\t\t} else if (key == \"stride\") {\n> > > +\t\t\tconfig.stride = std::stoi(value);\n> > > +\t\t} else if (key == \"frameSize\") {\n> > > +\t\t\tconfig.frameSize = std::stoi(value);\n> > \n> > Do we allow to specify the frame size ? What's the purpose ? Also, we\n> > already have stride so it can be calculated from there ?\n> \n> The purpose was to support all fields from the dump. If the\n> configuration comes from a dump, you'll have all fields. If the\n> configuration is handwritten (which I didn't actually expect in the\n> first place but is technically possible), then you can just skip the\n> field, or if its wrong it'll just get adjusted by Camera::configure().\n> So either way it should be fine.\n> \n> > > +\t\t} else if (key == \"bufferCount\") {\n> > > +\t\t\tconfig.bufferCount = std::stoi(value);\n> > > +\t\t} else if (key == \"colorSpace\") {\n> > > +\t\t\tconfig.colorSpace = libcamera::ColorSpace::fromString(value);\n> > > +\t\t} else {\n> > > +\t\t\tstd::cerr << \"Unknown key-value pair '\"\n> > > +\t\t\t\t << key << \"': '\" << value\n> > > +\t\t\t\t << \"' in stream configuration \"\n> > > +\t\t\t\t << index << std::endl;\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\t}\n> > \n> > Are there mandatory fields to be specified ? Should we check that all\n> > the above have been populated in the script ? I'm concerned that a\n> > script might specify only a partial set of configurations. Ofc if you\n> > consider this to be only generated by a dump, that's not an issue ;)\n> \n> See above.\n> \n> > > +\n> > > +\tstreamConfigs_.push_back(config);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CaptureScript::parseStreams(EventPtr event)\n> > > +{\n> > > +\tif (!checkEvent(event, YAML_SEQUENCE_START_EVENT))\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tunsigned int index = 0;\n\nAdd a blank line.\n\n> > > +\twhile (1) {\n> > > +\t\tevent = nextEvent();\n> > > +\t\tif (!event)\n> > > +\t\t\treturn -EINVAL;\n\nAdd a blank line here too.\n\n> > > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > > +\t\t\treturn 0;\n> > > +\n> > > +\t\tif (event->type == YAML_MAPPING_START_EVENT) {\n> > > +\t\t\tparseStream(std::move(event), index++);\n\nMissing error handling.\n\n> > > +\t\t\tcontinue;\n\nYou can drop the continue.\n\n> > > +\t\t} else {\n> > > +\t\t\tstd::cerr << \"UNKNOWN TYPE\" << std::endl;\n> > \n> > no need to scream :)\n> \n> Oops :p Leftover debugging...\n> \n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n\nparseProperties() implements something similar with\n\n while (1) {\n if (event->type == YAML_SEQUENCE_END_EVENT)\n return 0;\n\n int ret = parseProperty();\n if (ret)\n return ret;\n\n event = nextEvent();\n if (!event)\n return -EINVAL;\n }\n\nand parseProperty() starts with\n\n EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n if (!event)\n return -EINVAL;\n\nshould you do the same here for consistency ?\n\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CaptureScript::parseConfiguration()\n> > > +{\n> > > +\tint ret;\n> > > +\n> > > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > +\tif (!event)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\twhile (1) {\n> > > +\t\tevent = nextEvent();\n> > > +\t\tif (!event)\n> > > +\t\t\treturn -EINVAL;\n\nAdd a blank line here too.\n\n> > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tstd::string key = eventScalarValue(event);\n> > > +\n> > > +\t\tevent = nextEvent();\n> > > +\t\tif (!event)\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > +\t\t\tbreak;\n> > \n> > Same question, a mapping_end event after a key is an error ?\n> > \n> > > +\n> > > +\t\t/* TODO Load sensor configuration */\n> > > +\t\tif (key == \"orientation\") {\n> > > +\t\t\tret = parseOrientation(std::move(event));\n> > > +\t\t\tif (ret)\n> > > +\t\t\t\treturn ret;\n> > > +\t\t} else if (key == \"streams\") {\n> > > +\t\t\tret = parseStreams(std::move(event));\n> > > +\t\t\tif (ret)\n> > > +\t\t\t\treturn ret;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const\n> > > +{\n> > > +\tif (!configuration)\n> > > +\t\treturn;\n\nIs this a valid use case ? If not you can turn the argument into a\nreference, and drop this check.\n\n> > > +\n> > > +\tconfiguration->orientation = orientation_;\n> > > +\n> > > +\tfor (unsigned int i = 0; i < streamConfigs_.size(); i++)\n> > > +\t\t(*configuration)[i] = streamConfigs_[i];\n> > > +}\n> > > +\n> > > std::string CaptureScript::parseScalar()\n> > > {\n> > > \tEventPtr event = nextEvent(YAML_SCALAR_EVENT);\n> > > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h\n> > > index 294b920363ba..4ba862d742cf 100644\n> > > --- a/src/apps/cam/capture_script.h\n> > > +++ b/src/apps/cam/capture_script.h\n> > > @@ -26,6 +26,7 @@ public:\n> > >\n> > > \tconst libcamera::ControlList &frameControls(unsigned int frame);\n> > >\n> > > +\tvoid populateConfiguration(libcamera::CameraConfiguration *configuration) const;\n\nMissing blank line.\n\n> > > private:\n> > > \tstruct EventDeleter {\n> > > \t\tvoid operator()(yaml_event_t *event) const\n> > > @@ -43,6 +44,9 @@ private:\n> > > \tunsigned int loop_;\n> > > \tbool valid_;\n> > >\n> > > +\tlibcamera::Orientation orientation_;\n> > > +\tstd::vector streamConfigs_;\n> > > +\n> > > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > \tbool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;\n> > > \tstatic std::string eventScalarValue(const EventPtr &event);\n> > > @@ -55,6 +59,10 @@ private:\n> > > \tint parseFrames();\n> > > \tint parseFrame(EventPtr event);\n> > > \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> > > +\tint parseConfiguration();\n> > > +\tint parseOrientation(EventPtr event);\n> > > +\tint parseStreams(EventPtr event);\n> > > +\tint parseStream(EventPtr event, unsigned int index);\n\nMaybe also order the functions (here and in the .cpp file) in the order\nin which the elements are expected to appear in the yaml file.\n\n> > >\n> > > \tlibcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,\n> > > \t\t\t\t\t\t const std::string repr);","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 908AABD80A\n\tfor ;\n\tWed, 2 Oct 2024 23:26:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 803CE63525;\n\tThu, 3 Oct 2024 01:26:56 +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 BD6F960553\n\tfor ;\n\tThu, 3 Oct 2024 01:26:54 +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 A781E55A;\n\tThu, 3 Oct 2024 01:25:21 +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=\"pgEg/q+K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727911521;\n\tbh=kOxmwzwjg4H22DQD5GvSJgw6kSfwcECXTG5FdFSjyMc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pgEg/q+KehmVF8IENKj9y7BlJNGPab1uOPIyzRsYeOxrI5gWrKS7oQ0X6wcihVjJc\n\t+ejBP1m82yZ5fbRtsXtyya2MGPDrG2MnYSgvL/k0r3EOUSO/eDbKwc4oGG2dXOX78V\n\teKeRnn0ODPlMibNVu6uMZYfYoSuTnmbP+QaH3kEg=","Date":"Thu, 3 Oct 2024 02:26:50 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","Message-ID":"<20241002232650.GA17537@pendragon.ideasonboard.com>","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-3-paul.elder@ideasonboard.com>\n\t<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":31595,"web_url":"https://patchwork.libcamera.org/comment/31595/","msgid":"","date":"2024-10-04T12:05:29","subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Oct 03, 2024 at 02:26:50AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Wed, Oct 02, 2024 at 07:35:21PM +0900, Paul Elder wrote:\n> > On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:\n> > > On Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:\n> > > > Add support to the cam application for loading the camera configuration\n> > > > from a capture script. These are not expected to be written by hand, but\n> > > \n> > > Wouldn't it be useful to write camera configurations by hand ? It\n> > > doesn't seem to require any special handling compared to what you have\n> > > already done here, right ?\n> > \n> > I didn't expect it to happen but you certainly can. I suppose I can\n> > remove this to remove confusion?\n> \n> You could then replace the last sentence with \"These can be written\n> manually, or dumped from a capture session via the ...\".\n> \n> > > > rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment\n> > > > variable.\n> > > >\n> > > > If any configuration options are specified by command line parameters,\n> > > > those will take precedence.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > > ---\n> > > > src/apps/cam/camera_session.cpp | 22 +++--\n> > > > src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++\n> > > > src/apps/cam/capture_script.h | 8 ++\n> > > > 3 files changed, 184 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > > index edc49b875450..6f1d8b58870f 100644\n> > > > --- a/src/apps/cam/camera_session.cpp\n> > > > +++ b/src/apps/cam/camera_session.cpp\n> > > > @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > \t\treturn;\n> > > > \t}\n> > > >\n> > > > +\tif (options_.isSet(OptCaptureScript)) {\n> > > > +\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > > > +\t\tscript_ = std::make_unique(camera_, scriptName);\n> > > > +\t\tif (!script_->valid()) {\n> > > > +\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > > > +\t\t\t\t << \"'\" << std::endl;\n> > > > +\t\t\treturn;\n> > > > +\t\t}\n> > > > +\n> > > \n> > > What about adding a comment here to record that options specified on\n> > > the command line take precendece over options coming from the\n> > > configuration file ?\n> > \n> > Good idea.\n> \n> It would also explain why you're moving this block of code.\n> \n> \t/*\n> \t * Parse the capture script first to populate the configuration, and let\n> \t * command line arguments take precendence.\n> \t */\n> \n> This should ideally also be mentioned in the help text.\n> \n> > > > +\t\tscript_->populateConfiguration(config.get());\n> > > > +\t}\n> > > > +\n> > > > \tif (options_.isSet(OptOrientation)) {\n> > > > \t\tstd::string orientOpt = options_[OptOrientation].toString();\n> > > > \t\tstatic const std::map orientations{\n> > > > @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > \t}\n> > > > #endif\n> > > >\n> > > > -\tif (options_.isSet(OptCaptureScript)) {\n> > > > -\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > > > -\t\tscript_ = std::make_unique(camera_, scriptName);\n> > > > -\t\tif (!script_->valid()) {\n> > > > -\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > > > -\t\t\t\t << \"'\" << std::endl;\n> > > > -\t\t\treturn;\n> > > > -\t\t}\n> > > > -\t}\n> > > > -\n> > > > \tswitch (config->validate()) {\n> > > > \tcase CameraConfiguration::Valid:\n> > > > \t\tbreak;\n> > > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> > > > index fc1dfa75f2d4..7f166f45657e 100644\n> > > > --- a/src/apps/cam/capture_script.cpp\n> > > > +++ b/src/apps/cam/capture_script.cpp\n> > > > @@ -7,6 +7,7 @@\n> > > >\n> > > > #include \"capture_script.h\"\n> > > >\n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)\n> > > > \t\t\tret = parseFrames();\n> > > > \t\t\tif (ret)\n> > > > \t\t\t\treturn ret;\n> > > > +\t\t} else if (section == \"configuration\") {\n> > > > +\t\t\tret = parseConfiguration();\n> > > > +\t\t\tif (ret)\n> > > > +\t\t\t\treturn ret;\n> \n> I'd move this before \"frames\" to reflect the expected order in the\n> capture script.\n> \n> > > > \t\t} else {\n> > > > \t\t\tstd::cerr << \"Unsupported section '\" << section << \"'\"\n> > > > \t\t\t\t << std::endl;\n> > > > @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +int CaptureScript::parseOrientation(EventPtr event)\n> > > > +{\n> > > > +\tstatic const std::map orientations{\n> > > > +\t\t{ \"Rotate0\", libcamera::Orientation::Rotate0 },\n> > > > +\t\t{ \"Rotate0Mirror\", libcamera::Orientation::Rotate0Mirror },\n> > > > +\t\t{ \"Rotate180\", libcamera::Orientation::Rotate180 },\n> > > > +\t\t{ \"Rotate180Mirror\", libcamera::Orientation::Rotate180Mirror },\n> > > > +\t\t{ \"Rotate90Mirror\", libcamera::Orientation::Rotate90Mirror },\n> > > > +\t\t{ \"Rotate270\", libcamera::Orientation::Rotate270 },\n> > > > +\t\t{ \"Rotate270Mirror\", libcamera::Orientation::Rotate270Mirror },\n> > > > +\t\t{ \"Rotate90\", libcamera::Orientation::Rotate90 },\n> > > > +\t};\n> > > > +\n> > > > +\tstd::string orientation = eventScalarValue(event);\n> \n> If you move this line to the caller, this function could be a shared\n> helper, there's similar code in CameraSession::CameraSession(). The\n> helper could be a global function in a new utils.cpp file in the same\n> directory.\n\nNo, because the code in CameraSession is for parsing the command line\nparameters, which don't match the enum names. Unless you think we should\nchange the command line parameter orientation names to match the enums,\nbut that wouldn't be very nice to type.\n\n> \n> > > > +\n> > > > +\tauto it = orientations.find(orientation);\n> > > > +\tif (it == orientations.end()) {\n> > > > +\t\tstd::cerr << \"Invalid orientation '\" << orientation\n> > > > +\t\t\t << \"' in capture script\" << std::endl;\n> \n> The error message would also need to move to the caller, or drop the \"in\n> capture script\" part.\n> \n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\torientation_ = it->second;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int CaptureScript::parseStream(EventPtr event, unsigned int index)\n> > > > +{\n> > > > +\tif (!checkEvent(event, YAML_MAPPING_START_EVENT))\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\tStreamConfiguration config;\n> > > > +\twhile (1) {\n> > > > +\t\tevent = nextEvent();\n> > > > +\t\tif (!event)\n> > > > +\t\t\treturn -EINVAL;\n> \n> Add a blank line here for consistency with existing code.\n> \n> > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > +\t\t\tbreak;\n> > > > +\n> > > > +\t\tstd::string key = eventScalarValue(event);\n> > > > +\n> > > > +\t\tevent = nextEvent();\n> > > > +\t\tif (!event)\n> > > > +\t\t\treturn -EINVAL;\n> > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > +\t\t\tbreak;\n> > > \n> > > isn't this an error condition ? If you get a MAPPING_END after a key\n> > > isn't the scrip malformed ?\n> > \n> > I'm not sure how it shows up in the yaml parser, but afaiu it's possible\n> > have a key with no value. We do that in tuning files. (Not that this is\n> > a tuning file, but speaking from a yaml-correctness perspective).\n> > \n> > > > +\n> > > > +\t\tstd::string value = eventScalarValue(event);\n> \n> I think you can replace this with\n> \n> \t\tstd::string key = eventScalarValue(event);\n> \n> \t\tstd::string value = parseScalar();\n> \t\tif (value.empty())\n> \t\t\treturn -EINVAL;\n> \n> as the value must be present.\n\nack\n\n> \n> > > > +\n> > > > +\t\tif (key == \"pixelFormat\") {\n> > > > +\t\t\tconfig.pixelFormat = libcamera::PixelFormat::fromString(value);\n> > > \n> > > Should this be validated ?\n> > \n> > It should be fine not to. Camera::configure() will adjust it if it's not\n> > valid.\n> > \n> > > > +\t\t} else if (key == \"size\") {\n> > > > +\t\t\tunsigned int split = value.find(\"x\");\n> > > > +\t\t\tif (split == std::string::npos) {\n> > > > +\t\t\t\tstd::cerr << \"Invalid size '\" << value\n> > > > +\t\t\t\t\t << \"' in stream configuration \"\n> > > > +\t\t\t\t\t << index << std::endl;\n> > > > +\t\t\t}\n> > > > +\n> > > > +\t\t\tstd::string width = value.substr(0, split);\n> > > > +\t\t\tstd::string height = value.substr(split + 1);\n> > > > +\t\t\tconfig.size = Size(std::stoi(width), std::stoi(height));\n> \n> We specify width and height independently on the command line, would it\n> make sense to do the same in yaml ? It would simplify the code here.\n\nNo, I think it's fine to keep it symmetric to what is dumped, since it's\na single field of type Size, not two fields each of int.\n\n> \n> > > > +\t\t} else if (key == \"stride\") {\n> > > > +\t\t\tconfig.stride = std::stoi(value);\n> > > > +\t\t} else if (key == \"frameSize\") {\n> > > > +\t\t\tconfig.frameSize = std::stoi(value);\n> > > \n> > > Do we allow to specify the frame size ? What's the purpose ? Also, we\n> > > already have stride so it can be calculated from there ?\n> > \n> > The purpose was to support all fields from the dump. If the\n> > configuration comes from a dump, you'll have all fields. If the\n> > configuration is handwritten (which I didn't actually expect in the\n> > first place but is technically possible), then you can just skip the\n> > field, or if its wrong it'll just get adjusted by Camera::configure().\n> > So either way it should be fine.\n> > \n> > > > +\t\t} else if (key == \"bufferCount\") {\n> > > > +\t\t\tconfig.bufferCount = std::stoi(value);\n> > > > +\t\t} else if (key == \"colorSpace\") {\n> > > > +\t\t\tconfig.colorSpace = libcamera::ColorSpace::fromString(value);\n> > > > +\t\t} else {\n> > > > +\t\t\tstd::cerr << \"Unknown key-value pair '\"\n> > > > +\t\t\t\t << key << \"': '\" << value\n> > > > +\t\t\t\t << \"' in stream configuration \"\n> > > > +\t\t\t\t << index << std::endl;\n> > > > +\t\t\treturn -EINVAL;\n> > > > +\t\t}\n> > > > +\t}\n> > > \n> > > Are there mandatory fields to be specified ? Should we check that all\n> > > the above have been populated in the script ? I'm concerned that a\n> > > script might specify only a partial set of configurations. Ofc if you\n> > > consider this to be only generated by a dump, that's not an issue ;)\n> > \n> > See above.\n> > \n> > > > +\n> > > > +\tstreamConfigs_.push_back(config);\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int CaptureScript::parseStreams(EventPtr event)\n> > > > +{\n> > > > +\tif (!checkEvent(event, YAML_SEQUENCE_START_EVENT))\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\tunsigned int index = 0;\n> \n> Add a blank line.\n> \n> > > > +\twhile (1) {\n> > > > +\t\tevent = nextEvent();\n> > > > +\t\tif (!event)\n> > > > +\t\t\treturn -EINVAL;\n> \n> Add a blank line here too.\n> \n> > > > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > +\t\t\treturn 0;\n> > > > +\n> > > > +\t\tif (event->type == YAML_MAPPING_START_EVENT) {\n> > > > +\t\t\tparseStream(std::move(event), index++);\n> \n> Missing error handling.\n> \n> > > > +\t\t\tcontinue;\n> \n> You can drop the continue.\n> \n> > > > +\t\t} else {\n> > > > +\t\t\tstd::cerr << \"UNKNOWN TYPE\" << std::endl;\n> > > \n> > > no need to scream :)\n> > \n> > Oops :p Leftover debugging...\n> > \n> > > > +\t\t\treturn -EINVAL;\n> > > > +\t\t}\n> \n> parseProperties() implements something similar with\n> \n> while (1) {\n> if (event->type == YAML_SEQUENCE_END_EVENT)\n> return 0;\n> \n> int ret = parseProperty();\n> if (ret)\n> return ret;\n> \n> event = nextEvent();\n> if (!event)\n> return -EINVAL;\n> }\n> \n> and parseProperty() starts with\n> \n> EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> if (!event)\n> return -EINVAL;\n> \n> should you do the same here for consistency ?\n> \n\nparseProperties() is a \"top-level\" parsing function, at the same level\nas parseConfiguration(). Anything at levels below that takes an\nEventPtr, which is why I have it like this (also I don't really like the\nstatefulness of not passing the EventPtr but anyway).\n\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int CaptureScript::parseConfiguration()\n> > > > +{\n> > > > +\tint ret;\n> > > > +\n> > > > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > +\tif (!event)\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\twhile (1) {\n> > > > +\t\tevent = nextEvent();\n> > > > +\t\tif (!event)\n> > > > +\t\t\treturn -EINVAL;\n> \n> Add a blank line here too.\n> \n> > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > +\t\t\tbreak;\n> > > > +\n> > > > +\t\tstd::string key = eventScalarValue(event);\n> > > > +\n> > > > +\t\tevent = nextEvent();\n> > > > +\t\tif (!event)\n> > > > +\t\t\treturn -EINVAL;\n> > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > +\t\t\tbreak;\n> > > \n> > > Same question, a mapping_end event after a key is an error ?\n> > > \n> > > > +\n> > > > +\t\t/* TODO Load sensor configuration */\n> > > > +\t\tif (key == \"orientation\") {\n> > > > +\t\t\tret = parseOrientation(std::move(event));\n> > > > +\t\t\tif (ret)\n> > > > +\t\t\t\treturn ret;\n> > > > +\t\t} else if (key == \"streams\") {\n> > > > +\t\t\tret = parseStreams(std::move(event));\n> > > > +\t\t\tif (ret)\n> > > > +\t\t\t\treturn ret;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const\n> > > > +{\n> > > > +\tif (!configuration)\n> > > > +\t\treturn;\n> \n> Is this a valid use case ? If not you can turn the argument into a\n> reference, and drop this check.\n> \n\nIt is indeed not. I'll do that.\n\n> > > > +\n> > > > +\tconfiguration->orientation = orientation_;\n> > > > +\n> > > > +\tfor (unsigned int i = 0; i < streamConfigs_.size(); i++)\n> > > > +\t\t(*configuration)[i] = streamConfigs_[i];\n> > > > +}\n> > > > +\n> > > > std::string CaptureScript::parseScalar()\n> > > > {\n> > > > \tEventPtr event = nextEvent(YAML_SCALAR_EVENT);\n> > > > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h\n> > > > index 294b920363ba..4ba862d742cf 100644\n> > > > --- a/src/apps/cam/capture_script.h\n> > > > +++ b/src/apps/cam/capture_script.h\n> > > > @@ -26,6 +26,7 @@ public:\n> > > >\n> > > > \tconst libcamera::ControlList &frameControls(unsigned int frame);\n> > > >\n> > > > +\tvoid populateConfiguration(libcamera::CameraConfiguration *configuration) const;\n> \n> Missing blank line.\n> \n> > > > private:\n> > > > \tstruct EventDeleter {\n> > > > \t\tvoid operator()(yaml_event_t *event) const\n> > > > @@ -43,6 +44,9 @@ private:\n> > > > \tunsigned int loop_;\n> > > > \tbool valid_;\n> > > >\n> > > > +\tlibcamera::Orientation orientation_;\n> > > > +\tstd::vector streamConfigs_;\n> > > > +\n> > > > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > \tbool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;\n> > > > \tstatic std::string eventScalarValue(const EventPtr &event);\n> > > > @@ -55,6 +59,10 @@ private:\n> > > > \tint parseFrames();\n> > > > \tint parseFrame(EventPtr event);\n> > > > \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> > > > +\tint parseConfiguration();\n> > > > +\tint parseOrientation(EventPtr event);\n> > > > +\tint parseStreams(EventPtr event);\n> > > > +\tint parseStream(EventPtr event, unsigned int index);\n> \n> Maybe also order the functions (here and in the .cpp file) in the order\n> in which the elements are expected to appear in the yaml file.\n\nack\n\n\nThanks,\n\nPaul\n\n> \n> > > >\n> > > > \tlibcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,\n> > > > \t\t\t\t\t\t const std::string repr);","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 463BBC3257\n\tfor ;\n\tFri, 4 Oct 2024 12:05:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C82CC6352A;\n\tFri, 4 Oct 2024 14:05:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E3AF463512\n\tfor ;\n\tFri, 4 Oct 2024 14:05:35 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:e3ca:2180:ae9b:1941])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1CD455A;\n\tFri, 4 Oct 2024 14:04:00 +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=\"KDku4rJ7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728043442;\n\tbh=kp4g7x+z+XXX5tkGUJBOnAi6xMbFi/ycI8yjLHpvs4s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KDku4rJ7+hb9IjiNgJpcqvtfoEXK7lTG7uhrsuctr1f4tKhr16KbGOYZjnbiOzxA5\n\teVTgFcAaWMRzz73Z9yTM+HdLJpqKEO9qAvGHLVd5r3gdkZQajcVvXuNxFthVPbwkPR\n\tRtHkkeX7XPxD1nzJBHfTvKCt8qERS+Fq914SqVHw=","Date":"Fri, 4 Oct 2024 21:05:29 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","Message-ID":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-3-paul.elder@ideasonboard.com>\n\t<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>\n\t\n\t<20241002232650.GA17537@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241002232650.GA17537@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\" "}},{"id":31599,"web_url":"https://patchwork.libcamera.org/comment/31599/","msgid":"<20241007184547.GG14766@pendragon.ideasonboard.com>","date":"2024-10-07T18:45:47","subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Fri, Oct 04, 2024 at 09:05:29PM +0900, Paul Elder wrote:\n> On Thu, Oct 03, 2024 at 02:26:50AM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 02, 2024 at 07:35:21PM +0900, Paul Elder wrote:\n> > > On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:\n> > > > On Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:\n> > > > > Add support to the cam application for loading the camera configuration\n> > > > > from a capture script. These are not expected to be written by hand, but\n> > > > \n> > > > Wouldn't it be useful to write camera configurations by hand ? It\n> > > > doesn't seem to require any special handling compared to what you have\n> > > > already done here, right ?\n> > > \n> > > I didn't expect it to happen but you certainly can. I suppose I can\n> > > remove this to remove confusion?\n> > \n> > You could then replace the last sentence with \"These can be written\n> > manually, or dumped from a capture session via the ...\".\n> > \n> > > > > rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment\n> > > > > variable.\n> > > > >\n> > > > > If any configuration options are specified by command line parameters,\n> > > > > those will take precedence.\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > > ---\n> > > > > src/apps/cam/camera_session.cpp | 22 +++--\n> > > > > src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++\n> > > > > src/apps/cam/capture_script.h | 8 ++\n> > > > > 3 files changed, 184 insertions(+), 10 deletions(-)\n> > > > >\n> > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > > > index edc49b875450..6f1d8b58870f 100644\n> > > > > --- a/src/apps/cam/camera_session.cpp\n> > > > > +++ b/src/apps/cam/camera_session.cpp\n> > > > > @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > > \t\treturn;\n> > > > > \t}\n> > > > >\n> > > > > +\tif (options_.isSet(OptCaptureScript)) {\n> > > > > +\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > > > > +\t\tscript_ = std::make_unique(camera_, scriptName);\n> > > > > +\t\tif (!script_->valid()) {\n> > > > > +\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > > > > +\t\t\t\t << \"'\" << std::endl;\n> > > > > +\t\t\treturn;\n> > > > > +\t\t}\n> > > > > +\n> > > > \n> > > > What about adding a comment here to record that options specified on\n> > > > the command line take precendece over options coming from the\n> > > > configuration file ?\n> > > \n> > > Good idea.\n> > \n> > It would also explain why you're moving this block of code.\n> > \n> > \t/*\n> > \t * Parse the capture script first to populate the configuration, and let\n> > \t * command line arguments take precendence.\n> > \t */\n> > \n> > This should ideally also be mentioned in the help text.\n> > \n> > > > > +\t\tscript_->populateConfiguration(config.get());\n> > > > > +\t}\n> > > > > +\n> > > > > \tif (options_.isSet(OptOrientation)) {\n> > > > > \t\tstd::string orientOpt = options_[OptOrientation].toString();\n> > > > > \t\tstatic const std::map orientations{\n> > > > > @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > > \t}\n> > > > > #endif\n> > > > >\n> > > > > -\tif (options_.isSet(OptCaptureScript)) {\n> > > > > -\t\tstd::string scriptName = options_[OptCaptureScript].toString();\n> > > > > -\t\tscript_ = std::make_unique(camera_, scriptName);\n> > > > > -\t\tif (!script_->valid()) {\n> > > > > -\t\t\tstd::cerr << \"Invalid capture script '\" << scriptName\n> > > > > -\t\t\t\t << \"'\" << std::endl;\n> > > > > -\t\t\treturn;\n> > > > > -\t\t}\n> > > > > -\t}\n> > > > > -\n> > > > > \tswitch (config->validate()) {\n> > > > > \tcase CameraConfiguration::Valid:\n> > > > > \t\tbreak;\n> > > > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> > > > > index fc1dfa75f2d4..7f166f45657e 100644\n> > > > > --- a/src/apps/cam/capture_script.cpp\n> > > > > +++ b/src/apps/cam/capture_script.cpp\n> > > > > @@ -7,6 +7,7 @@\n> > > > >\n> > > > > #include \"capture_script.h\"\n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > \t\t\tret = parseFrames();\n> > > > > \t\t\tif (ret)\n> > > > > \t\t\t\treturn ret;\n> > > > > +\t\t} else if (section == \"configuration\") {\n> > > > > +\t\t\tret = parseConfiguration();\n> > > > > +\t\t\tif (ret)\n> > > > > +\t\t\t\treturn ret;\n> > \n> > I'd move this before \"frames\" to reflect the expected order in the\n> > capture script.\n> > \n> > > > > \t\t} else {\n> > > > > \t\t\tstd::cerr << \"Unsupported section '\" << section << \"'\"\n> > > > > \t\t\t\t << std::endl;\n> > > > > @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > > > > \treturn 0;\n> > > > > }\n> > > > >\n> > > > > +int CaptureScript::parseOrientation(EventPtr event)\n> > > > > +{\n> > > > > +\tstatic const std::map orientations{\n> > > > > +\t\t{ \"Rotate0\", libcamera::Orientation::Rotate0 },\n> > > > > +\t\t{ \"Rotate0Mirror\", libcamera::Orientation::Rotate0Mirror },\n> > > > > +\t\t{ \"Rotate180\", libcamera::Orientation::Rotate180 },\n> > > > > +\t\t{ \"Rotate180Mirror\", libcamera::Orientation::Rotate180Mirror },\n> > > > > +\t\t{ \"Rotate90Mirror\", libcamera::Orientation::Rotate90Mirror },\n> > > > > +\t\t{ \"Rotate270\", libcamera::Orientation::Rotate270 },\n> > > > > +\t\t{ \"Rotate270Mirror\", libcamera::Orientation::Rotate270Mirror },\n> > > > > +\t\t{ \"Rotate90\", libcamera::Orientation::Rotate90 },\n> > > > > +\t};\n> > > > > +\n> > > > > +\tstd::string orientation = eventScalarValue(event);\n> > \n> > If you move this line to the caller, this function could be a shared\n> > helper, there's similar code in CameraSession::CameraSession(). The\n> > helper could be a global function in a new utils.cpp file in the same\n> > directory.\n> \n> No, because the code in CameraSession is for parsing the command line\n> parameters, which don't match the enum names. Unless you think we should\n> change the command line parameter orientation names to match the enums,\n> but that wouldn't be very nice to type.\n\nI was thinking of doing it the other way around, but thinking about it\nagain, we should keep the strings in the capture script identical to the\nenumerator names.\n\nI'm in two minds about the command line arguments. Short strings are\nnicer to type, but consistency is good too.\n\n> > > > > +\n> > > > > +\tauto it = orientations.find(orientation);\n> > > > > +\tif (it == orientations.end()) {\n> > > > > +\t\tstd::cerr << \"Invalid orientation '\" << orientation\n> > > > > +\t\t\t << \"' in capture script\" << std::endl;\n> > \n> > The error message would also need to move to the caller, or drop the \"in\n> > capture script\" part.\n> > \n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\torientation_ = it->second;\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CaptureScript::parseStream(EventPtr event, unsigned int index)\n> > > > > +{\n> > > > > +\tif (!checkEvent(event, YAML_MAPPING_START_EVENT))\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\tStreamConfiguration config;\n> > > > > +\twhile (1) {\n> > > > > +\t\tevent = nextEvent();\n> > > > > +\t\tif (!event)\n> > > > > +\t\t\treturn -EINVAL;\n> > \n> > Add a blank line here for consistency with existing code.\n> > \n> > > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > > +\t\t\tbreak;\n> > > > > +\n> > > > > +\t\tstd::string key = eventScalarValue(event);\n> > > > > +\n> > > > > +\t\tevent = nextEvent();\n> > > > > +\t\tif (!event)\n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > > +\t\t\tbreak;\n> > > > \n> > > > isn't this an error condition ? If you get a MAPPING_END after a key\n> > > > isn't the scrip malformed ?\n> > > \n> > > I'm not sure how it shows up in the yaml parser, but afaiu it's possible\n> > > have a key with no value. We do that in tuning files. (Not that this is\n> > > a tuning file, but speaking from a yaml-correctness perspective).\n> > > \n> > > > > +\n> > > > > +\t\tstd::string value = eventScalarValue(event);\n> > \n> > I think you can replace this with\n> > \n> > \t\tstd::string key = eventScalarValue(event);\n> > \n> > \t\tstd::string value = parseScalar();\n> > \t\tif (value.empty())\n> > \t\t\treturn -EINVAL;\n> > \n> > as the value must be present.\n> \n> ack\n> \n> > > > > +\n> > > > > +\t\tif (key == \"pixelFormat\") {\n> > > > > +\t\t\tconfig.pixelFormat = libcamera::PixelFormat::fromString(value);\n> > > > \n> > > > Should this be validated ?\n> > > \n> > > It should be fine not to. Camera::configure() will adjust it if it's not\n> > > valid.\n> > > \n> > > > > +\t\t} else if (key == \"size\") {\n> > > > > +\t\t\tunsigned int split = value.find(\"x\");\n> > > > > +\t\t\tif (split == std::string::npos) {\n> > > > > +\t\t\t\tstd::cerr << \"Invalid size '\" << value\n> > > > > +\t\t\t\t\t << \"' in stream configuration \"\n> > > > > +\t\t\t\t\t << index << std::endl;\n\nYou're missing a return here.\n\n> > > > > +\t\t\t}\n> > > > > +\n> > > > > +\t\t\tstd::string width = value.substr(0, split);\n> > > > > +\t\t\tstd::string height = value.substr(split + 1);\n> > > > > +\t\t\tconfig.size = Size(std::stoi(width), std::stoi(height));\n> > \n> > We specify width and height independently on the command line, would it\n> > make sense to do the same in yaml ? It would simplify the code here.\n> \n> No, I think it's fine to keep it symmetric to what is dumped, since it's\n> a single field of type Size, not two fields each of int.\n\nFair point.\n\n> > > > > +\t\t} else if (key == \"stride\") {\n> > > > > +\t\t\tconfig.stride = std::stoi(value);\n> > > > > +\t\t} else if (key == \"frameSize\") {\n> > > > > +\t\t\tconfig.frameSize = std::stoi(value);\n> > > > \n> > > > Do we allow to specify the frame size ? What's the purpose ? Also, we\n> > > > already have stride so it can be calculated from there ?\n> > > \n> > > The purpose was to support all fields from the dump. If the\n> > > configuration comes from a dump, you'll have all fields. If the\n> > > configuration is handwritten (which I didn't actually expect in the\n> > > first place but is technically possible), then you can just skip the\n> > > field, or if its wrong it'll just get adjusted by Camera::configure().\n> > > So either way it should be fine.\n> > > \n> > > > > +\t\t} else if (key == \"bufferCount\") {\n> > > > > +\t\t\tconfig.bufferCount = std::stoi(value);\n> > > > > +\t\t} else if (key == \"colorSpace\") {\n> > > > > +\t\t\tconfig.colorSpace = libcamera::ColorSpace::fromString(value);\n> > > > > +\t\t} else {\n> > > > > +\t\t\tstd::cerr << \"Unknown key-value pair '\"\n> > > > > +\t\t\t\t << key << \"': '\" << value\n> > > > > +\t\t\t\t << \"' in stream configuration \"\n> > > > > +\t\t\t\t << index << std::endl;\n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > \n> > > > Are there mandatory fields to be specified ? Should we check that all\n> > > > the above have been populated in the script ? I'm concerned that a\n> > > > script might specify only a partial set of configurations. Ofc if you\n> > > > consider this to be only generated by a dump, that's not an issue ;)\n> > > \n> > > See above.\n> > > \n> > > > > +\n> > > > > +\tstreamConfigs_.push_back(config);\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CaptureScript::parseStreams(EventPtr event)\n> > > > > +{\n> > > > > +\tif (!checkEvent(event, YAML_SEQUENCE_START_EVENT))\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\tunsigned int index = 0;\n> > \n> > Add a blank line.\n> > \n> > > > > +\twhile (1) {\n> > > > > +\t\tevent = nextEvent();\n> > > > > +\t\tif (!event)\n> > > > > +\t\t\treturn -EINVAL;\n> > \n> > Add a blank line here too.\n> > \n> > > > > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > > +\t\t\treturn 0;\n> > > > > +\n> > > > > +\t\tif (event->type == YAML_MAPPING_START_EVENT) {\n> > > > > +\t\t\tparseStream(std::move(event), index++);\n> > \n> > Missing error handling.\n> > \n> > > > > +\t\t\tcontinue;\n> > \n> > You can drop the continue.\n> > \n> > > > > +\t\t} else {\n> > > > > +\t\t\tstd::cerr << \"UNKNOWN TYPE\" << std::endl;\n> > > > \n> > > > no need to scream :)\n> > > \n> > > Oops :p Leftover debugging...\n> > > \n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\t\t}\n> > \n> > parseProperties() implements something similar with\n> > \n> > while (1) {\n> > if (event->type == YAML_SEQUENCE_END_EVENT)\n> > return 0;\n> > \n> > int ret = parseProperty();\n> > if (ret)\n> > return ret;\n> > \n> > event = nextEvent();\n> > if (!event)\n> > return -EINVAL;\n> > }\n> > \n> > and parseProperty() starts with\n> > \n> > EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > if (!event)\n> > return -EINVAL;\n> > \n> > should you do the same here for consistency ?\n> \n> parseProperties() is a \"top-level\" parsing function, at the same level\n> as parseConfiguration(). Anything at levels below that takes an\n> EventPtr, which is why I have it like this (also I don't really like the\n> statefulness of not passing the EventPtr but anyway).\n\nI see it more as parseProperties() parsing a collection and\nparseProperty() parsing one element. parseStreams() and parseStream()\nare similar and I think consistency would be good.\n\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CaptureScript::parseConfiguration()\n> > > > > +{\n> > > > > +\tint ret;\n> > > > > +\n> > > > > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > +\tif (!event)\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\twhile (1) {\n> > > > > +\t\tevent = nextEvent();\n> > > > > +\t\tif (!event)\n> > > > > +\t\t\treturn -EINVAL;\n> > \n> > Add a blank line here too.\n> > \n> > > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > > +\t\t\tbreak;\n> > > > > +\n> > > > > +\t\tstd::string key = eventScalarValue(event);\n> > > > > +\n> > > > > +\t\tevent = nextEvent();\n> > > > > +\t\tif (!event)\n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\t\tif (event->type == YAML_MAPPING_END_EVENT)\n> > > > > +\t\t\tbreak;\n> > > > \n> > > > Same question, a mapping_end event after a key is an error ?\n> > > > \n> > > > > +\n> > > > > +\t\t/* TODO Load sensor configuration */\n> > > > > +\t\tif (key == \"orientation\") {\n> > > > > +\t\t\tret = parseOrientation(std::move(event));\n> > > > > +\t\t\tif (ret)\n> > > > > +\t\t\t\treturn ret;\n> > > > > +\t\t} else if (key == \"streams\") {\n> > > > > +\t\t\tret = parseStreams(std::move(event));\n> > > > > +\t\t\tif (ret)\n> > > > > +\t\t\t\treturn ret;\n> > > > > +\t\t}\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const\n> > > > > +{\n> > > > > +\tif (!configuration)\n> > > > > +\t\treturn;\n> > \n> > Is this a valid use case ? If not you can turn the argument into a\n> > reference, and drop this check.\n> \n> It is indeed not. I'll do that.\n> \n> > > > > +\n> > > > > +\tconfiguration->orientation = orientation_;\n> > > > > +\n> > > > > +\tfor (unsigned int i = 0; i < streamConfigs_.size(); i++)\n> > > > > +\t\t(*configuration)[i] = streamConfigs_[i];\n> > > > > +}\n> > > > > +\n> > > > > std::string CaptureScript::parseScalar()\n> > > > > {\n> > > > > \tEventPtr event = nextEvent(YAML_SCALAR_EVENT);\n> > > > > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h\n> > > > > index 294b920363ba..4ba862d742cf 100644\n> > > > > --- a/src/apps/cam/capture_script.h\n> > > > > +++ b/src/apps/cam/capture_script.h\n> > > > > @@ -26,6 +26,7 @@ public:\n> > > > >\n> > > > > \tconst libcamera::ControlList &frameControls(unsigned int frame);\n> > > > >\n> > > > > +\tvoid populateConfiguration(libcamera::CameraConfiguration *configuration) const;\n> > \n> > Missing blank line.\n> > \n> > > > > private:\n> > > > > \tstruct EventDeleter {\n> > > > > \t\tvoid operator()(yaml_event_t *event) const\n> > > > > @@ -43,6 +44,9 @@ private:\n> > > > > \tunsigned int loop_;\n> > > > > \tbool valid_;\n> > > > >\n> > > > > +\tlibcamera::Orientation orientation_;\n> > > > > +\tstd::vector streamConfigs_;\n> > > > > +\n> > > > > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > > \tbool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;\n> > > > > \tstatic std::string eventScalarValue(const EventPtr &event);\n> > > > > @@ -55,6 +59,10 @@ private:\n> > > > > \tint parseFrames();\n> > > > > \tint parseFrame(EventPtr event);\n> > > > > \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> > > > > +\tint parseConfiguration();\n> > > > > +\tint parseOrientation(EventPtr event);\n> > > > > +\tint parseStreams(EventPtr event);\n> > > > > +\tint parseStream(EventPtr event, unsigned int index);\n> > \n> > Maybe also order the functions (here and in the .cpp file) in the order\n> > in which the elements are expected to appear in the yaml file.\n> \n> ack\n> \n> > > > >\n> > > > > \tlibcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,\n> > > > > \t\t\t\t\t\t const std::string repr);","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 28146BD80A\n\tfor ;\n\tMon, 7 Oct 2024 18:45:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBEE76353A;\n\tMon, 7 Oct 2024 20:45:55 +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 0EEC06351F\n\tfor ;\n\tMon, 7 Oct 2024 20:45:54 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.14])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3307E792;\n\tMon, 7 Oct 2024 20:44:17 +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=\"SROIA2+i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728326657;\n\tbh=7nt9MQisu2zw31dluG3kYTWaCLXT4bRHTQK2YuBGFEI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SROIA2+iPzOTsnJLCfeN1EKPvvxfvuRJlXdRDIyBsHZW9IdSKraToGKATThuSZKZC\n\t/gPOGxa7ZzKj5Uo7tzAhKPWTETgdppebrLZsx8Sz246YHoZ0Mqo8Qi/MT7FiQOusEu\n\tIzvmDEpRI8CVTwjG01DJ/1aRhJ+cwhfkGWn82m7M=","Date":"Mon, 7 Oct 2024 21:45:47 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","Message-ID":"<20241007184547.GG14766@pendragon.ideasonboard.com>","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-3-paul.elder@ideasonboard.com>\n\t<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>\n\t\n\t<20241002232650.GA17537@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":31609,"web_url":"https://patchwork.libcamera.org/comment/31609/","msgid":"","date":"2024-10-08T07:38:03","subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Paul and reviewers,\n\nMaybe it's a bit late, but I have a question:\nThis patch adds the load of captures on libcamera app: cam,\nwhile the Android adapter hasn't got the support.\n\nI understand that it's difficult to add such a support in the\nAndroid adapter, as it's just a translation layer, and that the\ncapture file contains StreamConfiguration settings besides\nframes' controls, but have we considered loading the controls\nin the core libraries (e.g. libcamera::PipelineHandler)?\nWe might need to check if the active streams are the same or\nsimilar though.\n\nBR,\nHarvey\n\nOn Tue, Oct 8, 2024 at 2:45 AM Laurent Pinchart\n wrote:\n>\n> Hi Paul,\n>\n> On Fri, Oct 04, 2024 at 09:05:29PM +0900, Paul Elder wrote:\n> > On Thu, Oct 03, 2024 at 02:26:50AM +0300, Laurent Pinchart wrote:\n> > > On Wed, Oct 02, 2024 at 07:35:21PM +0900, Paul Elder wrote:\n> > > > On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:\n> > > > > On Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:\n> > > > > > Add support to the cam application for loading the camera configuration\n> > > > > > from a capture script. These are not expected to be written by hand, but\n> > > > >\n> > > > > Wouldn't it be useful to write camera configurations by hand ? It\n> > > > > doesn't seem to require any special handling compared to what you have\n> > > > > already done here, right ?\n> > > >\n> > > > I didn't expect it to happen but you certainly can. I suppose I can\n> > > > remove this to remove confusion?\n> > >\n> > > You could then replace the last sentence with \"These can be written\n> > > manually, or dumped from a capture session via the ...\".\n> > >\n> > > > > > rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment\n> > > > > > variable.\n> > > > > >\n> > > > > > If any configuration options are specified by command line parameters,\n> > > > > > those will take precedence.\n> > > > > >\n> > > > > > Signed-off-by: Paul Elder \n> > > > > > ---\n> > > > > > src/apps/cam/camera_session.cpp | 22 +++--\n> > > > > > src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++\n> > > > > > src/apps/cam/capture_script.h | 8 ++\n> > > > > > 3 files changed, 184 insertions(+), 10 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > > > > index edc49b875450..6f1d8b58870f 100644\n> > > > > > --- a/src/apps/cam/camera_session.cpp\n> > > > > > +++ b/src/apps/cam/camera_session.cpp\n> > > > > > @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > > > return;\n> > > > > > }\n> > > > > >\n> > > > > > + if (options_.isSet(OptCaptureScript)) {\n> > > > > > + std::string scriptName = options_[OptCaptureScript].toString();\n> > > > > > + script_ = std::make_unique(camera_, scriptName);\n> > > > > > + if (!script_->valid()) {\n> > > > > > + std::cerr << \"Invalid capture script '\" << scriptName\n> > > > > > + << \"'\" << std::endl;\n> > > > > > + return;\n> > > > > > + }\n> > > > > > +\n> > > > >\n> > > > > What about adding a comment here to record that options specified on\n> > > > > the command line take precendece over options coming from the\n> > > > > configuration file ?\n> > > >\n> > > > Good idea.\n> > >\n> > > It would also explain why you're moving this block of code.\n> > >\n> > > /*\n> > > * Parse the capture script first to populate the configuration, and let\n> > > * command line arguments take precendence.\n> > > */\n> > >\n> > > This should ideally also be mentioned in the help text.\n> > >\n> > > > > > + script_->populateConfiguration(config.get());\n> > > > > > + }\n> > > > > > +\n> > > > > > if (options_.isSet(OptOrientation)) {\n> > > > > > std::string orientOpt = options_[OptOrientation].toString();\n> > > > > > static const std::map orientations{\n> > > > > > @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > > > }\n> > > > > > #endif\n> > > > > >\n> > > > > > - if (options_.isSet(OptCaptureScript)) {\n> > > > > > - std::string scriptName = options_[OptCaptureScript].toString();\n> > > > > > - script_ = std::make_unique(camera_, scriptName);\n> > > > > > - if (!script_->valid()) {\n> > > > > > - std::cerr << \"Invalid capture script '\" << scriptName\n> > > > > > - << \"'\" << std::endl;\n> > > > > > - return;\n> > > > > > - }\n> > > > > > - }\n> > > > > > -\n> > > > > > switch (config->validate()) {\n> > > > > > case CameraConfiguration::Valid:\n> > > > > > break;\n> > > > > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> > > > > > index fc1dfa75f2d4..7f166f45657e 100644\n> > > > > > --- a/src/apps/cam/capture_script.cpp\n> > > > > > +++ b/src/apps/cam/capture_script.cpp\n> > > > > > @@ -7,6 +7,7 @@\n> > > > > >\n> > > > > > #include \"capture_script.h\"\n> > > > > >\n> > > > > > +#include \n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > > ret = parseFrames();\n> > > > > > if (ret)\n> > > > > > return ret;\n> > > > > > + } else if (section == \"configuration\") {\n> > > > > > + ret = parseConfiguration();\n> > > > > > + if (ret)\n> > > > > > + return ret;\n> > >\n> > > I'd move this before \"frames\" to reflect the expected order in the\n> > > capture script.\n> > >\n> > > > > > } else {\n> > > > > > std::cerr << \"Unsupported section '\" << section << \"'\"\n> > > > > > << std::endl;\n> > > > > > @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > +int CaptureScript::parseOrientation(EventPtr event)\n> > > > > > +{\n> > > > > > + static const std::map orientations{\n> > > > > > + { \"Rotate0\", libcamera::Orientation::Rotate0 },\n> > > > > > + { \"Rotate0Mirror\", libcamera::Orientation::Rotate0Mirror },\n> > > > > > + { \"Rotate180\", libcamera::Orientation::Rotate180 },\n> > > > > > + { \"Rotate180Mirror\", libcamera::Orientation::Rotate180Mirror },\n> > > > > > + { \"Rotate90Mirror\", libcamera::Orientation::Rotate90Mirror },\n> > > > > > + { \"Rotate270\", libcamera::Orientation::Rotate270 },\n> > > > > > + { \"Rotate270Mirror\", libcamera::Orientation::Rotate270Mirror },\n> > > > > > + { \"Rotate90\", libcamera::Orientation::Rotate90 },\n> > > > > > + };\n> > > > > > +\n> > > > > > + std::string orientation = eventScalarValue(event);\n> > >\n> > > If you move this line to the caller, this function could be a shared\n> > > helper, there's similar code in CameraSession::CameraSession(). The\n> > > helper could be a global function in a new utils.cpp file in the same\n> > > directory.\n> >\n> > No, because the code in CameraSession is for parsing the command line\n> > parameters, which don't match the enum names. Unless you think we should\n> > change the command line parameter orientation names to match the enums,\n> > but that wouldn't be very nice to type.\n>\n> I was thinking of doing it the other way around, but thinking about it\n> again, we should keep the strings in the capture script identical to the\n> enumerator names.\n>\n> I'm in two minds about the command line arguments. Short strings are\n> nicer to type, but consistency is good too.\n>\n> > > > > > +\n> > > > > > + auto it = orientations.find(orientation);\n> > > > > > + if (it == orientations.end()) {\n> > > > > > + std::cerr << \"Invalid orientation '\" << orientation\n> > > > > > + << \"' in capture script\" << std::endl;\n> > >\n> > > The error message would also need to move to the caller, or drop the \"in\n> > > capture script\" part.\n> > >\n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > > > > > +\n> > > > > > + orientation_ = it->second;\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +int CaptureScript::parseStream(EventPtr event, unsigned int index)\n> > > > > > +{\n> > > > > > + if (!checkEvent(event, YAML_MAPPING_START_EVENT))\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + StreamConfiguration config;\n> > > > > > + while (1) {\n> > > > > > + event = nextEvent();\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > >\n> > > Add a blank line here for consistency with existing code.\n> > >\n> > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > + break;\n> > > > > > +\n> > > > > > + std::string key = eventScalarValue(event);\n> > > > > > +\n> > > > > > + event = nextEvent();\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > + break;\n> > > > >\n> > > > > isn't this an error condition ? If you get a MAPPING_END after a key\n> > > > > isn't the scrip malformed ?\n> > > >\n> > > > I'm not sure how it shows up in the yaml parser, but afaiu it's possible\n> > > > have a key with no value. We do that in tuning files. (Not that this is\n> > > > a tuning file, but speaking from a yaml-correctness perspective).\n> > > >\n> > > > > > +\n> > > > > > + std::string value = eventScalarValue(event);\n> > >\n> > > I think you can replace this with\n> > >\n> > > std::string key = eventScalarValue(event);\n> > >\n> > > std::string value = parseScalar();\n> > > if (value.empty())\n> > > return -EINVAL;\n> > >\n> > > as the value must be present.\n> >\n> > ack\n> >\n> > > > > > +\n> > > > > > + if (key == \"pixelFormat\") {\n> > > > > > + config.pixelFormat = libcamera::PixelFormat::fromString(value);\n> > > > >\n> > > > > Should this be validated ?\n> > > >\n> > > > It should be fine not to. Camera::configure() will adjust it if it's not\n> > > > valid.\n> > > >\n> > > > > > + } else if (key == \"size\") {\n> > > > > > + unsigned int split = value.find(\"x\");\n> > > > > > + if (split == std::string::npos) {\n> > > > > > + std::cerr << \"Invalid size '\" << value\n> > > > > > + << \"' in stream configuration \"\n> > > > > > + << index << std::endl;\n>\n> You're missing a return here.\n>\n> > > > > > + }\n> > > > > > +\n> > > > > > + std::string width = value.substr(0, split);\n> > > > > > + std::string height = value.substr(split + 1);\n> > > > > > + config.size = Size(std::stoi(width), std::stoi(height));\n> > >\n> > > We specify width and height independently on the command line, would it\n> > > make sense to do the same in yaml ? It would simplify the code here.\n> >\n> > No, I think it's fine to keep it symmetric to what is dumped, since it's\n> > a single field of type Size, not two fields each of int.\n>\n> Fair point.\n>\n> > > > > > + } else if (key == \"stride\") {\n> > > > > > + config.stride = std::stoi(value);\n> > > > > > + } else if (key == \"frameSize\") {\n> > > > > > + config.frameSize = std::stoi(value);\n> > > > >\n> > > > > Do we allow to specify the frame size ? What's the purpose ? Also, we\n> > > > > already have stride so it can be calculated from there ?\n> > > >\n> > > > The purpose was to support all fields from the dump. If the\n> > > > configuration comes from a dump, you'll have all fields. If the\n> > > > configuration is handwritten (which I didn't actually expect in the\n> > > > first place but is technically possible), then you can just skip the\n> > > > field, or if its wrong it'll just get adjusted by Camera::configure().\n> > > > So either way it should be fine.\n> > > >\n> > > > > > + } else if (key == \"bufferCount\") {\n> > > > > > + config.bufferCount = std::stoi(value);\n> > > > > > + } else if (key == \"colorSpace\") {\n> > > > > > + config.colorSpace = libcamera::ColorSpace::fromString(value);\n> > > > > > + } else {\n> > > > > > + std::cerr << \"Unknown key-value pair '\"\n> > > > > > + << key << \"': '\" << value\n> > > > > > + << \"' in stream configuration \"\n> > > > > > + << index << std::endl;\n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > > > > > + }\n> > > > >\n> > > > > Are there mandatory fields to be specified ? Should we check that all\n> > > > > the above have been populated in the script ? I'm concerned that a\n> > > > > script might specify only a partial set of configurations. Ofc if you\n> > > > > consider this to be only generated by a dump, that's not an issue ;)\n> > > >\n> > > > See above.\n> > > >\n> > > > > > +\n> > > > > > + streamConfigs_.push_back(config);\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +int CaptureScript::parseStreams(EventPtr event)\n> > > > > > +{\n> > > > > > + if (!checkEvent(event, YAML_SEQUENCE_START_EVENT))\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + unsigned int index = 0;\n> > >\n> > > Add a blank line.\n> > >\n> > > > > > + while (1) {\n> > > > > > + event = nextEvent();\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > >\n> > > Add a blank line here too.\n> > >\n> > > > > > + if (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > > > + return 0;\n> > > > > > +\n> > > > > > + if (event->type == YAML_MAPPING_START_EVENT) {\n> > > > > > + parseStream(std::move(event), index++);\n> > >\n> > > Missing error handling.\n> > >\n> > > > > > + continue;\n> > >\n> > > You can drop the continue.\n> > >\n> > > > > > + } else {\n> > > > > > + std::cerr << \"UNKNOWN TYPE\" << std::endl;\n> > > > >\n> > > > > no need to scream :)\n> > > >\n> > > > Oops :p Leftover debugging...\n> > > >\n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > >\n> > > parseProperties() implements something similar with\n> > >\n> > > while (1) {\n> > > if (event->type == YAML_SEQUENCE_END_EVENT)\n> > > return 0;\n> > >\n> > > int ret = parseProperty();\n> > > if (ret)\n> > > return ret;\n> > >\n> > > event = nextEvent();\n> > > if (!event)\n> > > return -EINVAL;\n> > > }\n> > >\n> > > and parseProperty() starts with\n> > >\n> > > EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > if (!event)\n> > > return -EINVAL;\n> > >\n> > > should you do the same here for consistency ?\n> >\n> > parseProperties() is a \"top-level\" parsing function, at the same level\n> > as parseConfiguration(). Anything at levels below that takes an\n> > EventPtr, which is why I have it like this (also I don't really like the\n> > statefulness of not passing the EventPtr but anyway).\n>\n> I see it more as parseProperties() parsing a collection and\n> parseProperty() parsing one element. parseStreams() and parseStream()\n> are similar and I think consistency would be good.\n>\n> > > > > > + }\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +int CaptureScript::parseConfiguration()\n> > > > > > +{\n> > > > > > + int ret;\n> > > > > > +\n> > > > > > + EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + while (1) {\n> > > > > > + event = nextEvent();\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > >\n> > > Add a blank line here too.\n> > >\n> > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > + break;\n> > > > > > +\n> > > > > > + std::string key = eventScalarValue(event);\n> > > > > > +\n> > > > > > + event = nextEvent();\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > + break;\n> > > > >\n> > > > > Same question, a mapping_end event after a key is an error ?\n> > > > >\n> > > > > > +\n> > > > > > + /* TODO Load sensor configuration */\n> > > > > > + if (key == \"orientation\") {\n> > > > > > + ret = parseOrientation(std::move(event));\n> > > > > > + if (ret)\n> > > > > > + return ret;\n> > > > > > + } else if (key == \"streams\") {\n> > > > > > + ret = parseStreams(std::move(event));\n> > > > > > + if (ret)\n> > > > > > + return ret;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const\n> > > > > > +{\n> > > > > > + if (!configuration)\n> > > > > > + return;\n> > >\n> > > Is this a valid use case ? If not you can turn the argument into a\n> > > reference, and drop this check.\n> >\n> > It is indeed not. I'll do that.\n> >\n> > > > > > +\n> > > > > > + configuration->orientation = orientation_;\n> > > > > > +\n> > > > > > + for (unsigned int i = 0; i < streamConfigs_.size(); i++)\n> > > > > > + (*configuration)[i] = streamConfigs_[i];\n> > > > > > +}\n> > > > > > +\n> > > > > > std::string CaptureScript::parseScalar()\n> > > > > > {\n> > > > > > EventPtr event = nextEvent(YAML_SCALAR_EVENT);\n> > > > > > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h\n> > > > > > index 294b920363ba..4ba862d742cf 100644\n> > > > > > --- a/src/apps/cam/capture_script.h\n> > > > > > +++ b/src/apps/cam/capture_script.h\n> > > > > > @@ -26,6 +26,7 @@ public:\n> > > > > >\n> > > > > > const libcamera::ControlList &frameControls(unsigned int frame);\n> > > > > >\n> > > > > > + void populateConfiguration(libcamera::CameraConfiguration *configuration) const;\n> > >\n> > > Missing blank line.\n> > >\n> > > > > > private:\n> > > > > > struct EventDeleter {\n> > > > > > void operator()(yaml_event_t *event) const\n> > > > > > @@ -43,6 +44,9 @@ private:\n> > > > > > unsigned int loop_;\n> > > > > > bool valid_;\n> > > > > >\n> > > > > > + libcamera::Orientation orientation_;\n> > > > > > + std::vector streamConfigs_;\n> > > > > > +\n> > > > > > EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > > > bool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;\n> > > > > > static std::string eventScalarValue(const EventPtr &event);\n> > > > > > @@ -55,6 +59,10 @@ private:\n> > > > > > int parseFrames();\n> > > > > > int parseFrame(EventPtr event);\n> > > > > > int parseControl(EventPtr event, libcamera::ControlList &controls);\n> > > > > > + int parseConfiguration();\n> > > > > > + int parseOrientation(EventPtr event);\n> > > > > > + int parseStreams(EventPtr event);\n> > > > > > + int parseStream(EventPtr event, unsigned int index);\n> > >\n> > > Maybe also order the functions (here and in the .cpp file) in the order\n> > > in which the elements are expected to appear in the yaml file.\n> >\n> > ack\n> >\n> > > > > >\n> > > > > > libcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,\n> > > > > > const std::string repr);\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 4D42FBD80A\n\tfor ;\n\tTue, 8 Oct 2024 07:38:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FC5E6353A;\n\tTue, 8 Oct 2024 09:38:18 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 73E0062C8E\n\tfor ;\n\tTue, 8 Oct 2024 09:38:15 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id\n\t38308e7fff4ca-2fabe5c8c26so47364661fa.2\n\tfor ;\n\tTue, 08 Oct 2024 00:38:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"CNh+z52N\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728373095; x=1728977895;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=IlQqHSPYTmBs7UhV4ruCAgbFgZ5ABM26khQtY5ZwlHo=;\n\tb=CNh+z52No9mqdnFBRhKeni/1DJdNbjA+Nx3zagcEefIya+4kjn5SmJHK0d8uqoIe+c\n\trnftsRbndauVY13xaUuqQNIHFYigl3RDm3ExVYhydR//MMFJMf748QJkIt3urJYNjDzi\n\t5pZsTng+sx8YZjbiXgIeFWisCoVtJwDoMdLd8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728373095; x=1728977895;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=IlQqHSPYTmBs7UhV4ruCAgbFgZ5ABM26khQtY5ZwlHo=;\n\tb=INCMbnmDfNAV2Nahb+tH9ffzyYeLQGPz/Bmxwwz6WnuWcAxxKeJvE8zDUiWyr+oSU+\n\tnRcFJBO0sfv/9wYWrtCnPzDK+GYwoXVw8IeT3/LMgjST34oVLyzn8o1MqxB7yxREOaww\n\t+h8v7ie9qoBbEt7tgCCz0iecy7jc6MFOrJNCmUf5/drUF/00EE3qsvv3ogwN7OSAZjvp\n\tMnpm/XUbhBmxIL3L6QZnw7c3htQgYM3FAF/4mDenInZKT/egoKV+FPUktRdHjIp82POb\n\tu9CecKC7lsccvXo9kCsIKTUxh4386qLQlFMDK0jt2FnGYrfns0zxoFwxkWHc0FfUqG2E\n\tFewA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVylE34/X/EZmMjZ5BTExrQ2k2276z75jmmwu1iX4d2FloGz1DH1UdMwGPdAAiP7O45jxmROUyA5AeYIcXrQyc=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwwpibBm12OnPlhdEtb/1yXmWONNqTwkGoWDMCpg4z6j7bkRkfv\n\tfkYv+tLs4PRwz2y/CdkRHEuccp9S5xuMMq6DF/9nW9rPsgKcikFHrCWQSfLDwa/qXESPdleQUko\n\t/me1p9AfysRMQJMz8CXdgTiut3kseJSZ5P3dOH1JcSpmq+18=","X-Google-Smtp-Source":"AGHT+IF6mGD9VT9DOMdX+iL0V0LsAxos7q+8lVwmYWnCIZoWCkuiXPLEk9glem7LUepXQV7OyZfixXOydz/gvwNKRoE=","X-Received":"by 2002:a05:651c:19a3:b0:2f9:cf21:b9f with SMTP id\n\t38308e7fff4ca-2faf3c41fb1mr72415071fa.22.1728373094423;\n\tTue, 08 Oct 2024 00:38:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-3-paul.elder@ideasonboard.com>\n\t<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>\n\t\n\t<20241002232650.GA17537@pendragon.ideasonboard.com>\n\t\n\t<20241007184547.GG14766@pendragon.ideasonboard.com>","In-Reply-To":"<20241007184547.GG14766@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Tue, 8 Oct 2024 15:38:03 +0800","Message-ID":"","Subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","To":"Laurent Pinchart ","Cc":"Paul Elder , \n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":31701,"web_url":"https://patchwork.libcamera.org/comment/31701/","msgid":"<172857134212.877857.1024756578792190215@ping.linuxembedded.co.uk>","date":"2024-10-10T14:42:22","subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Cheng-Hao Yang (2024-10-08 08:38:03)\n> Hi Paul and reviewers,\n> \n> Maybe it's a bit late, but I have a question:\n> This patch adds the load of captures on libcamera app: cam,\n> while the Android adapter hasn't got the support.\n\nI think the capture happens in core doesn't it ? It's only the replay\nthat happens from cam... Are you looking for a way to replay capture\nscripts through the android layer?\n\nPerhaps that's something that would need to be in an android\napplication? or maybe some specific capturing from the android layer?\n\nWe shouldn't 'inject' processing into the android layer...\n\nBut you could use this implementation to capture the requests and\nconfiguration from an android applciation, and likely replay it back (on\nthe same hardware) through cam ?\n\n\n> I understand that it's difficult to add such a support in the\n> Android adapter, as it's just a translation layer, and that the\n> capture file contains StreamConfiguration settings besides\n> frames' controls, but have we considered loading the controls\n> in the core libraries (e.g. libcamera::PipelineHandler)?\n> We might need to check if the active streams are the same or\n> similar though.\n> \n> BR,\n> Harvey\n> \n> On Tue, Oct 8, 2024 at 2:45 AM Laurent Pinchart\n> wrote:\n> >\n> > Hi Paul,\n> >\n> > On Fri, Oct 04, 2024 at 09:05:29PM +0900, Paul Elder wrote:\n> > > On Thu, Oct 03, 2024 at 02:26:50AM +0300, Laurent Pinchart wrote:\n> > > > On Wed, Oct 02, 2024 at 07:35:21PM +0900, Paul Elder wrote:\n> > > > > On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:\n> > > > > > On Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:\n> > > > > > > Add support to the cam application for loading the camera configuration\n> > > > > > > from a capture script. These are not expected to be written by hand, but\n> > > > > >\n> > > > > > Wouldn't it be useful to write camera configurations by hand ? It\n> > > > > > doesn't seem to require any special handling compared to what you have\n> > > > > > already done here, right ?\n> > > > >\n> > > > > I didn't expect it to happen but you certainly can. I suppose I can\n> > > > > remove this to remove confusion?\n> > > >\n> > > > You could then replace the last sentence with \"These can be written\n> > > > manually, or dumped from a capture session via the ...\".\n> > > >\n> > > > > > > rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment\n> > > > > > > variable.\n> > > > > > >\n> > > > > > > If any configuration options are specified by command line parameters,\n> > > > > > > those will take precedence.\n> > > > > > >\n> > > > > > > Signed-off-by: Paul Elder \n> > > > > > > ---\n> > > > > > > src/apps/cam/camera_session.cpp | 22 +++--\n> > > > > > > src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++\n> > > > > > > src/apps/cam/capture_script.h | 8 ++\n> > > > > > > 3 files changed, 184 insertions(+), 10 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp\n> > > > > > > index edc49b875450..6f1d8b58870f 100644\n> > > > > > > --- a/src/apps/cam/camera_session.cpp\n> > > > > > > +++ b/src/apps/cam/camera_session.cpp\n> > > > > > > @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > > > > return;\n> > > > > > > }\n> > > > > > >\n> > > > > > > + if (options_.isSet(OptCaptureScript)) {\n> > > > > > > + std::string scriptName = options_[OptCaptureScript].toString();\n> > > > > > > + script_ = std::make_unique(camera_, scriptName);\n> > > > > > > + if (!script_->valid()) {\n> > > > > > > + std::cerr << \"Invalid capture script '\" << scriptName\n> > > > > > > + << \"'\" << std::endl;\n> > > > > > > + return;\n> > > > > > > + }\n> > > > > > > +\n> > > > > >\n> > > > > > What about adding a comment here to record that options specified on\n> > > > > > the command line take precendece over options coming from the\n> > > > > > configuration file ?\n> > > > >\n> > > > > Good idea.\n> > > >\n> > > > It would also explain why you're moving this block of code.\n> > > >\n> > > > /*\n> > > > * Parse the capture script first to populate the configuration, and let\n> > > > * command line arguments take precendence.\n> > > > */\n> > > >\n> > > > This should ideally also be mentioned in the help text.\n> > > >\n> > > > > > > + script_->populateConfiguration(config.get());\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > if (options_.isSet(OptOrientation)) {\n> > > > > > > std::string orientOpt = options_[OptOrientation].toString();\n> > > > > > > static const std::map orientations{\n> > > > > > > @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,\n> > > > > > > }\n> > > > > > > #endif\n> > > > > > >\n> > > > > > > - if (options_.isSet(OptCaptureScript)) {\n> > > > > > > - std::string scriptName = options_[OptCaptureScript].toString();\n> > > > > > > - script_ = std::make_unique(camera_, scriptName);\n> > > > > > > - if (!script_->valid()) {\n> > > > > > > - std::cerr << \"Invalid capture script '\" << scriptName\n> > > > > > > - << \"'\" << std::endl;\n> > > > > > > - return;\n> > > > > > > - }\n> > > > > > > - }\n> > > > > > > -\n> > > > > > > switch (config->validate()) {\n> > > > > > > case CameraConfiguration::Valid:\n> > > > > > > break;\n> > > > > > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> > > > > > > index fc1dfa75f2d4..7f166f45657e 100644\n> > > > > > > --- a/src/apps/cam/capture_script.cpp\n> > > > > > > +++ b/src/apps/cam/capture_script.cpp\n> > > > > > > @@ -7,6 +7,7 @@\n> > > > > > >\n> > > > > > > #include \"capture_script.h\"\n> > > > > > >\n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > > > ret = parseFrames();\n> > > > > > > if (ret)\n> > > > > > > return ret;\n> > > > > > > + } else if (section == \"configuration\") {\n> > > > > > > + ret = parseConfiguration();\n> > > > > > > + if (ret)\n> > > > > > > + return ret;\n> > > >\n> > > > I'd move this before \"frames\" to reflect the expected order in the\n> > > > capture script.\n> > > >\n> > > > > > > } else {\n> > > > > > > std::cerr << \"Unsupported section '\" << section << \"'\"\n> > > > > > > << std::endl;\n> > > > > > > @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > +int CaptureScript::parseOrientation(EventPtr event)\n> > > > > > > +{\n> > > > > > > + static const std::map orientations{\n> > > > > > > + { \"Rotate0\", libcamera::Orientation::Rotate0 },\n> > > > > > > + { \"Rotate0Mirror\", libcamera::Orientation::Rotate0Mirror },\n> > > > > > > + { \"Rotate180\", libcamera::Orientation::Rotate180 },\n> > > > > > > + { \"Rotate180Mirror\", libcamera::Orientation::Rotate180Mirror },\n> > > > > > > + { \"Rotate90Mirror\", libcamera::Orientation::Rotate90Mirror },\n> > > > > > > + { \"Rotate270\", libcamera::Orientation::Rotate270 },\n> > > > > > > + { \"Rotate270Mirror\", libcamera::Orientation::Rotate270Mirror },\n> > > > > > > + { \"Rotate90\", libcamera::Orientation::Rotate90 },\n> > > > > > > + };\n> > > > > > > +\n> > > > > > > + std::string orientation = eventScalarValue(event);\n> > > >\n> > > > If you move this line to the caller, this function could be a shared\n> > > > helper, there's similar code in CameraSession::CameraSession(). The\n> > > > helper could be a global function in a new utils.cpp file in the same\n> > > > directory.\n> > >\n> > > No, because the code in CameraSession is for parsing the command line\n> > > parameters, which don't match the enum names. Unless you think we should\n> > > change the command line parameter orientation names to match the enums,\n> > > but that wouldn't be very nice to type.\n> >\n> > I was thinking of doing it the other way around, but thinking about it\n> > again, we should keep the strings in the capture script identical to the\n> > enumerator names.\n> >\n> > I'm in two minds about the command line arguments. Short strings are\n> > nicer to type, but consistency is good too.\n> >\n> > > > > > > +\n> > > > > > > + auto it = orientations.find(orientation);\n> > > > > > > + if (it == orientations.end()) {\n> > > > > > > + std::cerr << \"Invalid orientation '\" << orientation\n> > > > > > > + << \"' in capture script\" << std::endl;\n> > > >\n> > > > The error message would also need to move to the caller, or drop the \"in\n> > > > capture script\" part.\n> > > >\n> > > > > > > + return -EINVAL;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + orientation_ = it->second;\n> > > > > > > +\n> > > > > > > + return 0;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +int CaptureScript::parseStream(EventPtr event, unsigned int index)\n> > > > > > > +{\n> > > > > > > + if (!checkEvent(event, YAML_MAPPING_START_EVENT))\n> > > > > > > + return -EINVAL;\n> > > > > > > +\n> > > > > > > + StreamConfiguration config;\n> > > > > > > + while (1) {\n> > > > > > > + event = nextEvent();\n> > > > > > > + if (!event)\n> > > > > > > + return -EINVAL;\n> > > >\n> > > > Add a blank line here for consistency with existing code.\n> > > >\n> > > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > > + break;\n> > > > > > > +\n> > > > > > > + std::string key = eventScalarValue(event);\n> > > > > > > +\n> > > > > > > + event = nextEvent();\n> > > > > > > + if (!event)\n> > > > > > > + return -EINVAL;\n> > > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > > + break;\n> > > > > >\n> > > > > > isn't this an error condition ? If you get a MAPPING_END after a key\n> > > > > > isn't the scrip malformed ?\n> > > > >\n> > > > > I'm not sure how it shows up in the yaml parser, but afaiu it's possible\n> > > > > have a key with no value. We do that in tuning files. (Not that this is\n> > > > > a tuning file, but speaking from a yaml-correctness perspective).\n> > > > >\n> > > > > > > +\n> > > > > > > + std::string value = eventScalarValue(event);\n> > > >\n> > > > I think you can replace this with\n> > > >\n> > > > std::string key = eventScalarValue(event);\n> > > >\n> > > > std::string value = parseScalar();\n> > > > if (value.empty())\n> > > > return -EINVAL;\n> > > >\n> > > > as the value must be present.\n> > >\n> > > ack\n> > >\n> > > > > > > +\n> > > > > > > + if (key == \"pixelFormat\") {\n> > > > > > > + config.pixelFormat = libcamera::PixelFormat::fromString(value);\n> > > > > >\n> > > > > > Should this be validated ?\n> > > > >\n> > > > > It should be fine not to. Camera::configure() will adjust it if it's not\n> > > > > valid.\n> > > > >\n> > > > > > > + } else if (key == \"size\") {\n> > > > > > > + unsigned int split = value.find(\"x\");\n> > > > > > > + if (split == std::string::npos) {\n> > > > > > > + std::cerr << \"Invalid size '\" << value\n> > > > > > > + << \"' in stream configuration \"\n> > > > > > > + << index << std::endl;\n> >\n> > You're missing a return here.\n> >\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + std::string width = value.substr(0, split);\n> > > > > > > + std::string height = value.substr(split + 1);\n> > > > > > > + config.size = Size(std::stoi(width), std::stoi(height));\n> > > >\n> > > > We specify width and height independently on the command line, would it\n> > > > make sense to do the same in yaml ? It would simplify the code here.\n> > >\n> > > No, I think it's fine to keep it symmetric to what is dumped, since it's\n> > > a single field of type Size, not two fields each of int.\n> >\n> > Fair point.\n> >\n> > > > > > > + } else if (key == \"stride\") {\n> > > > > > > + config.stride = std::stoi(value);\n> > > > > > > + } else if (key == \"frameSize\") {\n> > > > > > > + config.frameSize = std::stoi(value);\n> > > > > >\n> > > > > > Do we allow to specify the frame size ? What's the purpose ? Also, we\n> > > > > > already have stride so it can be calculated from there ?\n> > > > >\n> > > > > The purpose was to support all fields from the dump. If the\n> > > > > configuration comes from a dump, you'll have all fields. If the\n> > > > > configuration is handwritten (which I didn't actually expect in the\n> > > > > first place but is technically possible), then you can just skip the\n> > > > > field, or if its wrong it'll just get adjusted by Camera::configure().\n> > > > > So either way it should be fine.\n> > > > >\n> > > > > > > + } else if (key == \"bufferCount\") {\n> > > > > > > + config.bufferCount = std::stoi(value);\n> > > > > > > + } else if (key == \"colorSpace\") {\n> > > > > > > + config.colorSpace = libcamera::ColorSpace::fromString(value);\n> > > > > > > + } else {\n> > > > > > > + std::cerr << \"Unknown key-value pair '\"\n> > > > > > > + << key << \"': '\" << value\n> > > > > > > + << \"' in stream configuration \"\n> > > > > > > + << index << std::endl;\n> > > > > > > + return -EINVAL;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > >\n> > > > > > Are there mandatory fields to be specified ? Should we check that all\n> > > > > > the above have been populated in the script ? I'm concerned that a\n> > > > > > script might specify only a partial set of configurations. Ofc if you\n> > > > > > consider this to be only generated by a dump, that's not an issue ;)\n> > > > >\n> > > > > See above.\n> > > > >\n> > > > > > > +\n> > > > > > > + streamConfigs_.push_back(config);\n> > > > > > > +\n> > > > > > > + return 0;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +int CaptureScript::parseStreams(EventPtr event)\n> > > > > > > +{\n> > > > > > > + if (!checkEvent(event, YAML_SEQUENCE_START_EVENT))\n> > > > > > > + return -EINVAL;\n> > > > > > > +\n> > > > > > > + unsigned int index = 0;\n> > > >\n> > > > Add a blank line.\n> > > >\n> > > > > > > + while (1) {\n> > > > > > > + event = nextEvent();\n> > > > > > > + if (!event)\n> > > > > > > + return -EINVAL;\n> > > >\n> > > > Add a blank line here too.\n> > > >\n> > > > > > > + if (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > > > > + return 0;\n> > > > > > > +\n> > > > > > > + if (event->type == YAML_MAPPING_START_EVENT) {\n> > > > > > > + parseStream(std::move(event), index++);\n> > > >\n> > > > Missing error handling.\n> > > >\n> > > > > > > + continue;\n> > > >\n> > > > You can drop the continue.\n> > > >\n> > > > > > > + } else {\n> > > > > > > + std::cerr << \"UNKNOWN TYPE\" << std::endl;\n> > > > > >\n> > > > > > no need to scream :)\n> > > > >\n> > > > > Oops :p Leftover debugging...\n> > > > >\n> > > > > > > + return -EINVAL;\n> > > > > > > + }\n> > > >\n> > > > parseProperties() implements something similar with\n> > > >\n> > > > while (1) {\n> > > > if (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > return 0;\n> > > >\n> > > > int ret = parseProperty();\n> > > > if (ret)\n> > > > return ret;\n> > > >\n> > > > event = nextEvent();\n> > > > if (!event)\n> > > > return -EINVAL;\n> > > > }\n> > > >\n> > > > and parseProperty() starts with\n> > > >\n> > > > EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > if (!event)\n> > > > return -EINVAL;\n> > > >\n> > > > should you do the same here for consistency ?\n> > >\n> > > parseProperties() is a \"top-level\" parsing function, at the same level\n> > > as parseConfiguration(). Anything at levels below that takes an\n> > > EventPtr, which is why I have it like this (also I don't really like the\n> > > statefulness of not passing the EventPtr but anyway).\n> >\n> > I see it more as parseProperties() parsing a collection and\n> > parseProperty() parsing one element. parseStreams() and parseStream()\n> > are similar and I think consistency would be good.\n> >\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + return 0;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +int CaptureScript::parseConfiguration()\n> > > > > > > +{\n> > > > > > > + int ret;\n> > > > > > > +\n> > > > > > > + EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > > > + if (!event)\n> > > > > > > + return -EINVAL;\n> > > > > > > +\n> > > > > > > + while (1) {\n> > > > > > > + event = nextEvent();\n> > > > > > > + if (!event)\n> > > > > > > + return -EINVAL;\n> > > >\n> > > > Add a blank line here too.\n> > > >\n> > > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > > + break;\n> > > > > > > +\n> > > > > > > + std::string key = eventScalarValue(event);\n> > > > > > > +\n> > > > > > > + event = nextEvent();\n> > > > > > > + if (!event)\n> > > > > > > + return -EINVAL;\n> > > > > > > + if (event->type == YAML_MAPPING_END_EVENT)\n> > > > > > > + break;\n> > > > > >\n> > > > > > Same question, a mapping_end event after a key is an error ?\n> > > > > >\n> > > > > > > +\n> > > > > > > + /* TODO Load sensor configuration */\n> > > > > > > + if (key == \"orientation\") {\n> > > > > > > + ret = parseOrientation(std::move(event));\n> > > > > > > + if (ret)\n> > > > > > > + return ret;\n> > > > > > > + } else if (key == \"streams\") {\n> > > > > > > + ret = parseStreams(std::move(event));\n> > > > > > > + if (ret)\n> > > > > > > + return ret;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + return 0;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const\n> > > > > > > +{\n> > > > > > > + if (!configuration)\n> > > > > > > + return;\n> > > >\n> > > > Is this a valid use case ? If not you can turn the argument into a\n> > > > reference, and drop this check.\n> > >\n> > > It is indeed not. I'll do that.\n> > >\n> > > > > > > +\n> > > > > > > + configuration->orientation = orientation_;\n> > > > > > > +\n> > > > > > > + for (unsigned int i = 0; i < streamConfigs_.size(); i++)\n> > > > > > > + (*configuration)[i] = streamConfigs_[i];\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > std::string CaptureScript::parseScalar()\n> > > > > > > {\n> > > > > > > EventPtr event = nextEvent(YAML_SCALAR_EVENT);\n> > > > > > > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h\n> > > > > > > index 294b920363ba..4ba862d742cf 100644\n> > > > > > > --- a/src/apps/cam/capture_script.h\n> > > > > > > +++ b/src/apps/cam/capture_script.h\n> > > > > > > @@ -26,6 +26,7 @@ public:\n> > > > > > >\n> > > > > > > const libcamera::ControlList &frameControls(unsigned int frame);\n> > > > > > >\n> > > > > > > + void populateConfiguration(libcamera::CameraConfiguration *configuration) const;\n> > > >\n> > > > Missing blank line.\n> > > >\n> > > > > > > private:\n> > > > > > > struct EventDeleter {\n> > > > > > > void operator()(yaml_event_t *event) const\n> > > > > > > @@ -43,6 +44,9 @@ private:\n> > > > > > > unsigned int loop_;\n> > > > > > > bool valid_;\n> > > > > > >\n> > > > > > > + libcamera::Orientation orientation_;\n> > > > > > > + std::vector streamConfigs_;\n> > > > > > > +\n> > > > > > > EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > > > > bool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;\n> > > > > > > static std::string eventScalarValue(const EventPtr &event);\n> > > > > > > @@ -55,6 +59,10 @@ private:\n> > > > > > > int parseFrames();\n> > > > > > > int parseFrame(EventPtr event);\n> > > > > > > int parseControl(EventPtr event, libcamera::ControlList &controls);\n> > > > > > > + int parseConfiguration();\n> > > > > > > + int parseOrientation(EventPtr event);\n> > > > > > > + int parseStreams(EventPtr event);\n> > > > > > > + int parseStream(EventPtr event, unsigned int index);\n> > > >\n> > > > Maybe also order the functions (here and in the .cpp file) in the order\n> > > > in which the elements are expected to appear in the yaml file.\n> > >\n> > > ack\n> > >\n> > > > > > >\n> > > > > > > libcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,\n> > > > > > > const std::string repr);\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 C6B32C32EA\n\tfor ;\n\tThu, 10 Oct 2024 14:42:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DA866536B;\n\tThu, 10 Oct 2024 16:42:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92C746353A\n\tfor ;\n\tThu, 10 Oct 2024 16:42:25 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0FCD94D4;\n\tThu, 10 Oct 2024 16:40:47 +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=\"GGlNY9RL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728571247;\n\tbh=N8YeNZqL8D10SGpzOplr7NIw4F25UnLrq7ng8mjraAY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GGlNY9RLXqEIyfj0TE1to+Li5kg3ET5rNa1GEpJiD6Bkq/KioVo8yi3+4DHnsFBAC\n\t22tae+l7j7k+/Zdq+VGgVlCy9xcvjecAcYcJgjfOXlUQVilMAtwhTLVLT1KQ2dsKhm\n\tYLozIV9qV2Vd0mYAmevXsg/3UTFG1IA2KiMlUjhM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240930170907.1404844-1-paul.elder@ideasonboard.com>\n\t<20240930170907.1404844-3-paul.elder@ideasonboard.com>\n\t<5raa74ekdsjiykm7jh5sb23vzy7gk7thwzfna3ocev6bwohi7i@os5izfyyoapg>\n\t\n\t<20241002232650.GA17537@pendragon.ideasonboard.com>\n\t\n\t<20241007184547.GG14766@pendragon.ideasonboard.com>\n\t","Subject":"Re: [PATCH 2/2] apps: cam: Add support for loading configuration\n\tfrom capture script","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tJacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","To":"Cheng-Hao Yang ,\n\tLaurent Pinchart ","Date":"Thu, 10 Oct 2024 15:42:22 +0100","Message-ID":"<172857134212.877857.1024756578792190215@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]