[{"id":24904,"web_url":"https://patchwork.libcamera.org/comment/24904/","msgid":"<20220903001901.GB5705@pyrite.rasen.tech>","date":"2022-09-03T00:19:01","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Add to the capture script support for properties that control the script\n> execution. Script properties are specified in a 'properties' section\n> before the actual list of controls specified in the 'frames' section.\n> \n> Define a first 'loop' property that allows to repeat the frame list\n\ns/to repeat/repeating/\n\n> periodically. All the frame ids in the 'frames' section shall be smaller\n> than the loop control.\n> \n> Modify the capture script example to show usage of the 'loop' property\n> and better document the frames list while at it.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/cam/capture-script.yaml | 50 +++++++++++------------\n> src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> src/cam/capture_script.h | 3 ++\n> 3 files changed, 102 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> index 6a749bc60cf7..dbea4a9f01a7 100644\n> --- a/src/cam/capture-script.yaml\n> +++ b/src/cam/capture-script.yaml\n> @@ -5,6 +5,20 @@\n> # A capture script allows to associate a list of controls and their values\n> # to frame numbers.\n> \n> +# The script allows to define a list of frames associated with controls and\n\ns/to define/defining/\n\n> +# an optional list of properties that controls the script behaviour\n> +#\n> +# properties:\n> +# - loop: idx\n> +# Repeat the controls every 'idx' frames.\n> +#\n> +# frames:\n> +# - frameid:\n> +# Control1: value1\n> +# Control2: value2\n> +#\n> +# List of frame ids with associated a list of controls to be applied\n> +#\n> # \\todo Formally define the capture script structure with a schema\n> \n> # Notes:\n> @@ -12,35 +26,17 @@\n> # libcamera::controls:: enumeration\n> # - Controls not supported by the camera currently operated are ignored\n> # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> +# strictly minor than the loop control\n\ns/minor/less than/\n\n\nReviewed-by: Paul Elder \n\n> \n> -# Example:\n> -frames:\n> - - 1:\n> - Brightness: 0.0\n> +# Example: Turn brightness up and down every 50 frames\n> \n> - - 40:\n> - Brightness: 0.2\n> +properties:\n> + - loop: 50\n> \n> - - 80:\n> - Brightness: 0.4\n> -\n> - - 120:\n> - Brightness: 0.8\n> -\n> - - 160:\n> - Brightness: 0.4\n> -\n> - - 200:\n> - Brightness: 0.2\n> -\n> - - 240:\n> +frames:\n> + - 0:\n> Brightness: 0.0\n> \n> - - 280:\n> - Brightness: -0.2\n> -\n> - - 300:\n> - Brightness: -0.4\n> -\n> - - 340:\n> - Brightness: -0.8\n> + - 25:\n> + Brightness: 0.8\n> diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> index 5e85b3ca604c..52bf19961c17 100644\n> --- a/src/cam/capture_script.cpp\n> +++ b/src/cam/capture_script.cpp\n> @@ -15,7 +15,7 @@ using namespace libcamera;\n> \n> CaptureScript::CaptureScript(std::shared_ptr camera,\n> \t\t\t const std::string &fileName)\n> -\t: camera_(camera), valid_(false)\n> +\t: camera_(camera), loop_(0), valid_(false)\n> {\n> \tFILE *fh = fopen(fileName.c_str(), \"r\");\n> \tif (!fh) {\n> @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> const ControlList &CaptureScript::frameControls(unsigned int frame)\n> {\n> \tstatic ControlList controls{};\n> +\tunsigned int idx = frame;\n> \n> -\tauto it = frameControls_.find(frame);\n> +\t/* If we loop, repeat the controls every 'loop_' frames. */\n> +\tif (loop_)\n> +\t\tidx = frame % loop_;\n> +\n> +\tauto it = frameControls_.find(idx);\n> \tif (it == frameControls_.end())\n> \t\treturn controls;\n> \n> @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> \n> \t\tstd::string section = eventScalarValue(event);\n> \n> -\t\tif (section == \"frames\") {\n> +\t\tif (section == \"properties\") {\n> +\t\t\tret = parseProperties();\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n> +\t\t} else if (section == \"frames\") {\n> \t\t\tret = parseFrames();\n> \t\t\tif (ret)\n> \t\t\t\treturn ret;\n> @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> \t}\n> }\n> \n> +int CaptureScript::parseProperty()\n> +{\n> +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> +\tif (!event)\n> +\t\treturn -EINVAL;\n> +\n> +\tstd::string prop = parseScalar();\n> +\tif (prop.empty())\n> +\t\treturn -EINVAL;\n> +\n> +\tif (prop == \"loop\") {\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tstd::string value = eventScalarValue(event);\n> +\t\tif (value.empty())\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tloop_ = atoi(value.c_str());\n> +\t\tif (!loop_) {\n> +\t\t\tstd::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t} else {\n> +\t\tstd::cerr << \"Unsupported property: \" << prop << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tevent = nextEvent(YAML_MAPPING_END_EVENT);\n> +\tif (!event)\n> +\t\treturn -EINVAL;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CaptureScript::parseProperties()\n> +{\n> +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> +\tif (!event)\n> +\t\treturn -EINVAL;\n> +\n> +\twhile (1) {\n> +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> +\t\t\treturn 0;\n> +\n> +\t\tint ret = parseProperty();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> int CaptureScript::parseFrames()\n> {\n> \tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> \t\treturn -EINVAL;\n> \n> \tunsigned int frameId = atoi(key.c_str());\n> +\tif (loop_ && frameId >= loop_) {\n> +\t\tstd::cerr\n> +\t\t\t<< \"Frame id (\" << frameId << \") shall be smaller than\"\n> +\t\t\t<< \"loop limit (\" << loop_ << \")\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> \n> \tevent = nextEvent(YAML_MAPPING_START_EVENT);\n> \tif (!event)\n> diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> index fffe67e5a3df..7a0ddebb00b5 100644\n> --- a/src/cam/capture_script.h\n> +++ b/src/cam/capture_script.h\n> @@ -40,6 +40,7 @@ private:\n> \tstd::map frameControls_;\n> \tstd::shared_ptr camera_;\n> \tyaml_parser_t parser_;\n> +\tunsigned int loop_;\n> \tbool valid_;\n> \n> \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> @@ -49,6 +50,8 @@ private:\n> \n> \tint parseScript(FILE *script);\n> \n> +\tint parseProperties();\n> +\tint parseProperty();\n> \tint parseFrames();\n> \tint parseFrame(EventPtr event);\n> \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> -- \n> 2.37.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 CD01CC0DA4\n\tfor ;\n\tSat, 3 Sep 2022 00:19:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E16062018;\n\tSat, 3 Sep 2022 02:19:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6206661FF2\n\tfor ;\n\tSat, 3 Sep 2022 02:19:18 +0200 (CEST)","from pyrite.rasen.tech (unknown [50.228.9.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 293476DD;\n\tSat, 3 Sep 2022 02:19:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662164360;\n\tbh=6uAlMIOSLhB/AHz3d3+vNaZXT5dbClBjT5EpGbY+q6Y=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=P84cDEThplmNmsQhI7PBZ6ddiAHDnvja/Lc4WxuxDuruwFwdHPVQ7fwu7jFWXe9Bm\n\t+tux9NNSz8ZhzK1EwQprqu2QK9YjHV5nAJbRwcjjKvEOPnXTmY+1mFrw6IJCzDFqu8\n\tQ0t4lHAGCA0WPyPgWi1XxC1/FNIVWqRAuOWd51gMN039pRidiTwzk05Zf6dpeKEu9p\n\tHpEqRZZ0aXN2nWV1cEl9wf2X30YlFeKQUSXoZXUHGqEtPARqIZ21LCvcRGGxT29FaF\n\tNWrSSw+fqSOuCRm4ugeoW5lJ4M/eeaaaCJdE5aVmpKMrvBfAQC++HPQaNiW6i5muzM\n\tDQl2Xz2EMCshg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662164358;\n\tbh=6uAlMIOSLhB/AHz3d3+vNaZXT5dbClBjT5EpGbY+q6Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FeYbGYro6C8MtrjUtyyifZwzMBb7M7yCGVotDMfx9EokXvl4XSR93X8txVG+ecf4h\n\tAWKnzzZSDODZwQa3iy1DM/IGEYWoua44gh6hRSvEj8g0fNkfznZa89s0/I4ebrc+wh\n\tyRgxWxU+8xfHcN2Eg8qt5GhcwvZIa6KoOb25GK18="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FeYbGYro\"; dkim-atps=neutral","Date":"Fri, 2 Sep 2022 20:19:01 -0400","To":"Jacopo Mondi ","Message-ID":"<20220903001901.GB5705@pyrite.rasen.tech>","References":"<20220902150031.31184-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220902150031.31184-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24905,"web_url":"https://patchwork.libcamera.org/comment/24905/","msgid":"<20220903003203.GA113531@pyrite.rasen.tech>","date":"2022-09-03T00:32:03","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:\n> On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Add to the capture script support for properties that control the script\n> > execution. Script properties are specified in a 'properties' section\n> > before the actual list of controls specified in the 'frames' section.\n> > \n> > Define a first 'loop' property that allows to repeat the frame list\n> \n> s/to repeat/repeating/\n> \n> > periodically. All the frame ids in the 'frames' section shall be smaller\n> > than the loop control.\n> > \n> > Modify the capture script example to show usage of the 'loop' property\n> > and better document the frames list while at it.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/cam/capture-script.yaml | 50 +++++++++++------------\n> > src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> > src/cam/capture_script.h | 3 ++\n> > 3 files changed, 102 insertions(+), 30 deletions(-)\n> > \n> > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> > index 6a749bc60cf7..dbea4a9f01a7 100644\n> > --- a/src/cam/capture-script.yaml\n> > +++ b/src/cam/capture-script.yaml\n> > @@ -5,6 +5,20 @@\n> > # A capture script allows to associate a list of controls and their values\n> > # to frame numbers.\n> > \n> > +# The script allows to define a list of frames associated with controls and\n> \n> s/to define/defining/\n> \n> > +# an optional list of properties that controls the script behaviour\n> > +#\n> > +# properties:\n> > +# - loop: idx\n> > +# Repeat the controls every 'idx' frames.\n> > +#\n> > +# frames:\n> > +# - frameid:\n> > +# Control1: value1\n> > +# Control2: value2\n> > +#\n> > +# List of frame ids with associated a list of controls to be applied\n> > +#\n> > # \\todo Formally define the capture script structure with a schema\n> > \n> > # Notes:\n> > @@ -12,35 +26,17 @@\n> > # libcamera::controls:: enumeration\n> > # - Controls not supported by the camera currently operated are ignored\n> > # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> > +# strictly minor than the loop control\n> \n> s/minor/less than/\n\nOops:\n\ns/less than/less/\n\n\nPaul\n\n> \n> \n> Reviewed-by: Paul Elder \n> \n> > \n> > -# Example:\n> > -frames:\n> > - - 1:\n> > - Brightness: 0.0\n> > +# Example: Turn brightness up and down every 50 frames\n> > \n> > - - 40:\n> > - Brightness: 0.2\n> > +properties:\n> > + - loop: 50\n> > \n> > - - 80:\n> > - Brightness: 0.4\n> > -\n> > - - 120:\n> > - Brightness: 0.8\n> > -\n> > - - 160:\n> > - Brightness: 0.4\n> > -\n> > - - 200:\n> > - Brightness: 0.2\n> > -\n> > - - 240:\n> > +frames:\n> > + - 0:\n> > Brightness: 0.0\n> > \n> > - - 280:\n> > - Brightness: -0.2\n> > -\n> > - - 300:\n> > - Brightness: -0.4\n> > -\n> > - - 340:\n> > - Brightness: -0.8\n> > + - 25:\n> > + Brightness: 0.8\n> > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > index 5e85b3ca604c..52bf19961c17 100644\n> > --- a/src/cam/capture_script.cpp\n> > +++ b/src/cam/capture_script.cpp\n> > @@ -15,7 +15,7 @@ using namespace libcamera;\n> > \n> > CaptureScript::CaptureScript(std::shared_ptr camera,\n> > \t\t\t const std::string &fileName)\n> > -\t: camera_(camera), valid_(false)\n> > +\t: camera_(camera), loop_(0), valid_(false)\n> > {\n> > \tFILE *fh = fopen(fileName.c_str(), \"r\");\n> > \tif (!fh) {\n> > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> > const ControlList &CaptureScript::frameControls(unsigned int frame)\n> > {\n> > \tstatic ControlList controls{};\n> > +\tunsigned int idx = frame;\n> > \n> > -\tauto it = frameControls_.find(frame);\n> > +\t/* If we loop, repeat the controls every 'loop_' frames. */\n> > +\tif (loop_)\n> > +\t\tidx = frame % loop_;\n> > +\n> > +\tauto it = frameControls_.find(idx);\n> > \tif (it == frameControls_.end())\n> > \t\treturn controls;\n> > \n> > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> > \n> > \t\tstd::string section = eventScalarValue(event);\n> > \n> > -\t\tif (section == \"frames\") {\n> > +\t\tif (section == \"properties\") {\n> > +\t\t\tret = parseProperties();\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t} else if (section == \"frames\") {\n> > \t\t\tret = parseFrames();\n> > \t\t\tif (ret)\n> > \t\t\t\treturn ret;\n> > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> > \t}\n> > }\n> > \n> > +int CaptureScript::parseProperty()\n> > +{\n> > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > +\tif (!event)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tstd::string prop = parseScalar();\n> > +\tif (prop.empty())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tif (prop == \"loop\") {\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tstd::string value = eventScalarValue(event);\n> > +\t\tif (value.empty())\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tloop_ = atoi(value.c_str());\n> > +\t\tif (!loop_) {\n> > +\t\t\tstd::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t} else {\n> > +\t\tstd::cerr << \"Unsupported property: \" << prop << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tevent = nextEvent(YAML_MAPPING_END_EVENT);\n> > +\tif (!event)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CaptureScript::parseProperties()\n> > +{\n> > +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > +\tif (!event)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\twhile (1) {\n> > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > +\t\t\treturn 0;\n> > +\n> > +\t\tint ret = parseProperty();\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int CaptureScript::parseFrames()\n> > {\n> > \tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> > \t\treturn -EINVAL;\n> > \n> > \tunsigned int frameId = atoi(key.c_str());\n> > +\tif (loop_ && frameId >= loop_) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Frame id (\" << frameId << \") shall be smaller than\"\n> > +\t\t\t<< \"loop limit (\" << loop_ << \")\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > \n> > \tevent = nextEvent(YAML_MAPPING_START_EVENT);\n> > \tif (!event)\n> > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > index fffe67e5a3df..7a0ddebb00b5 100644\n> > --- a/src/cam/capture_script.h\n> > +++ b/src/cam/capture_script.h\n> > @@ -40,6 +40,7 @@ private:\n> > \tstd::map frameControls_;\n> > \tstd::shared_ptr camera_;\n> > \tyaml_parser_t parser_;\n> > +\tunsigned int loop_;\n> > \tbool valid_;\n> > \n> > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > @@ -49,6 +50,8 @@ private:\n> > \n> > \tint parseScript(FILE *script);\n> > \n> > +\tint parseProperties();\n> > +\tint parseProperty();\n> > \tint parseFrames();\n> > \tint parseFrame(EventPtr event);\n> > \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> > -- \n> > 2.37.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 61874C3272\n\tfor ;\n\tSat, 3 Sep 2022 00:32:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EA6862019;\n\tSat, 3 Sep 2022 02:32:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 424F061FF2\n\tfor ;\n\tSat, 3 Sep 2022 02:32:09 +0200 (CEST)","from pyrite.rasen.tech (unknown [50.228.9.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 41D6A6DD;\n\tSat, 3 Sep 2022 02:32:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662165131;\n\tbh=bbo6CJ2FImK/qxEUozCgaK+BpyOD3XrZeo55gesS/QA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=anhdP+lSMRw8id4lx3EfMm4Q9RVw+gTEYJppl7ET4g7RZiVeSdOOxsAwvs0mDqR8r\n\t2NLIBNzyORlLQGIyeCev3J3CjIDFFPAT+vOzyH0Mm7XdC123k9U320pnPC0YkCSmGZ\n\tLBfI23sLojlDr8dErSc+LzSOZ2cFt0ao0L1LuwgpzTW0k/jBrA6YR17iethhFI01Nr\n\tIHt9zepqPrDS9x6FFnJbLJ7jnO4wYcirO6i+DLlM6rhPNkWdVZYk79W//vdkFqpXVI\n\tDPkxYpokVfNFUjF/C0u5eXb6MLKbo7zAYdDjY0n2Xh4UZsZHt9TLgfS+O+N4WVTDj+\n\txuu9zlIRpP+0w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662165128;\n\tbh=bbo6CJ2FImK/qxEUozCgaK+BpyOD3XrZeo55gesS/QA=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=LK1xcMt3gw/1Bn4/c/GW4gQUuAArwOpt97z8A9xE53r6R8uH1jjGNUyMmN71E9vzc\n\tIm2hM8Nvw3fFQs2MCXVmWSru2+JvLtukmdTyB4QnoU2wMp93u3PzQusF/798V0sJG6\n\tV3+tMJmmuzIsiDpbKHxIFN7TZMdT5KiErLfiZl+Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LK1xcMt3\"; dkim-atps=neutral","Date":"Fri, 2 Sep 2022 20:32:03 -0400","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20220903003203.GA113531@pyrite.rasen.tech>","References":"<20220902150031.31184-1-jacopo@jmondi.org>\n\t<20220903001901.GB5705@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220903001901.GB5705@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24906,"web_url":"https://patchwork.libcamera.org/comment/24906/","msgid":"","date":"2022-09-03T00:44:01","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:\n> On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Add to the capture script support for properties that control the script\n\n\"Add support to the capture script for properties\" would be clearer.\n\n> > execution. Script properties are specified in a 'properties' section\n> > before the actual list of controls specified in the 'frames' section.\n> > \n> > Define a first 'loop' property that allows to repeat the frame list\n> \n> s/to repeat/repeating/\n> \n> > periodically. All the frame ids in the 'frames' section shall be smaller\n> > than the loop control.\n> > \n> > Modify the capture script example to show usage of the 'loop' property\n> > and better document the frames list while at it.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/cam/capture-script.yaml | 50 +++++++++++------------\n> > src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> > src/cam/capture_script.h | 3 ++\n> > 3 files changed, 102 insertions(+), 30 deletions(-)\n> > \n> > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> > index 6a749bc60cf7..dbea4a9f01a7 100644\n> > --- a/src/cam/capture-script.yaml\n> > +++ b/src/cam/capture-script.yaml\n> > @@ -5,6 +5,20 @@\n> > # A capture script allows to associate a list of controls and their values\n> > # to frame numbers.\n> > \n> > +# The script allows to define a list of frames associated with controls and\n> \n> s/to define/defining/\n> \n> > +# an optional list of properties that controls the script behaviour\n\ns/controls/control/\n\n> > +#\n> > +# properties:\n> > +# - loop: idx\n> > +# Repeat the controls every 'idx' frames.\n\nThis is confusing, it looks like the comment is part of the YAML file.\nI'd write\n\n# properties:\n# # Repeat the controls every 'idx' frames.\n# - loop: idx\n\n> > +#\n> > +# frames:\n> > +# - frameid:\n\nI'd write \"frame-number\" instead of \"frameid\" to emphasize it's a frame\nnumber, not just any identifier.\n\n> > +# Control1: value1\n> > +# Control2: value2\n> > +#\n> > +# List of frame ids with associated a list of controls to be applied\n\nSame here.\n\n> > +#\n> > # \\todo Formally define the capture script structure with a schema\n> > \n> > # Notes:\n> > @@ -12,35 +26,17 @@\n> > # libcamera::controls:: enumeration\n> > # - Controls not supported by the camera currently operated are ignored\n> > # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> > +# strictly minor than the loop control\n> \n> s/minor/less than/\n\nOr \"smaller than\" ?\n\n> Reviewed-by: Paul Elder \n> \n> > \n> > -# Example:\n> > -frames:\n> > - - 1:\n> > - Brightness: 0.0\n> > +# Example: Turn brightness up and down every 50 frames\n> > \n> > - - 40:\n> > - Brightness: 0.2\n> > +properties:\n> > + - loop: 50\n> > \n> > - - 80:\n> > - Brightness: 0.4\n> > -\n> > - - 120:\n> > - Brightness: 0.8\n> > -\n> > - - 160:\n> > - Brightness: 0.4\n> > -\n> > - - 200:\n> > - Brightness: 0.2\n> > -\n> > - - 240:\n> > +frames:\n> > + - 0:\n\nI like starting at 0 instead of 1.\n\n> > Brightness: 0.0\n> > \n> > - - 280:\n> > - Brightness: -0.2\n> > -\n> > - - 300:\n> > - Brightness: -0.4\n> > -\n> > - - 340:\n> > - Brightness: -0.8\n> > + - 25:\n> > + Brightness: 0.8\n\nIt's nice to have more than just two frames in the example script (I\nregularly use it for testing :-)). Could you instead extend it to\ncontinue with\n\n- 380:\n Brightness: -0.4\n- 380:\n Brightness: -0.2\n\nand loop at 420 ?\n\n> > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > index 5e85b3ca604c..52bf19961c17 100644\n> > --- a/src/cam/capture_script.cpp\n> > +++ b/src/cam/capture_script.cpp\n> > @@ -15,7 +15,7 @@ using namespace libcamera;\n> > \n> > CaptureScript::CaptureScript(std::shared_ptr camera,\n> > \t\t\t const std::string &fileName)\n> > -\t: camera_(camera), valid_(false)\n> > +\t: camera_(camera), loop_(0), valid_(false)\n> > {\n> > \tFILE *fh = fopen(fileName.c_str(), \"r\");\n> > \tif (!fh) {\n> > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> > const ControlList &CaptureScript::frameControls(unsigned int frame)\n> > {\n> > \tstatic ControlList controls{};\n> > +\tunsigned int idx = frame;\n> > \n> > -\tauto it = frameControls_.find(frame);\n> > +\t/* If we loop, repeat the controls every 'loop_' frames. */\n> > +\tif (loop_)\n> > +\t\tidx = frame % loop_;\n> > +\n> > +\tauto it = frameControls_.find(idx);\n> > \tif (it == frameControls_.end())\n> > \t\treturn controls;\n> > \n> > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> > \n> > \t\tstd::string section = eventScalarValue(event);\n> > \n> > -\t\tif (section == \"frames\") {\n> > +\t\tif (section == \"properties\") {\n> > +\t\t\tret = parseProperties();\n> > +\t\t\tif (ret)\n> > +\t\t\t\treturn ret;\n> > +\t\t} else if (section == \"frames\") {\n> > \t\t\tret = parseFrames();\n> > \t\t\tif (ret)\n> > \t\t\t\treturn ret;\n> > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> > \t}\n> > }\n> > \n> > +int CaptureScript::parseProperty()\n> > +{\n> > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > +\tif (!event)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tstd::string prop = parseScalar();\n> > +\tif (prop.empty())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tif (prop == \"loop\") {\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tstd::string value = eventScalarValue(event);\n> > +\t\tif (value.empty())\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tloop_ = atoi(value.c_str());\n> > +\t\tif (!loop_) {\n> > +\t\t\tstd::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\t} else {\n> > +\t\tstd::cerr << \"Unsupported property: \" << prop << std::endl;\n\n\t\tstd::cerr << \"Unsupported property `\" << prop << \"'\" << std::endl;\n\nUse a colon in a message if you introduce a sentence that explains or\ncomplements the previous one, not when printing the value of a variable.\nCompare the following two messages, assuming that someone would use a\nYAML file that contains\n\n- properties:\n - \"file a bug report on bugs.libcamera.org\": 0\n\nFirst message;\n\nUnsupported property: file a bug report on bugs.libcamera.org\n\nSecond message:\n\nUnsuported property `file a bug report on bugs.libcamera.org'\n\nSame for the loop limit.\n\nReviewed-by: Laurent Pinchart \n\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tevent = nextEvent(YAML_MAPPING_END_EVENT);\n> > +\tif (!event)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int CaptureScript::parseProperties()\n> > +{\n> > +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > +\tif (!event)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\twhile (1) {\n> > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > +\t\t\treturn 0;\n> > +\n> > +\t\tint ret = parseProperty();\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tevent = nextEvent();\n> > +\t\tif (!event)\n> > +\t\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int CaptureScript::parseFrames()\n> > {\n> > \tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> > \t\treturn -EINVAL;\n> > \n> > \tunsigned int frameId = atoi(key.c_str());\n> > +\tif (loop_ && frameId >= loop_) {\n> > +\t\tstd::cerr\n> > +\t\t\t<< \"Frame id (\" << frameId << \") shall be smaller than\"\n> > +\t\t\t<< \"loop limit (\" << loop_ << \")\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > \n> > \tevent = nextEvent(YAML_MAPPING_START_EVENT);\n> > \tif (!event)\n> > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > index fffe67e5a3df..7a0ddebb00b5 100644\n> > --- a/src/cam/capture_script.h\n> > +++ b/src/cam/capture_script.h\n> > @@ -40,6 +40,7 @@ private:\n> > \tstd::map frameControls_;\n> > \tstd::shared_ptr camera_;\n> > \tyaml_parser_t parser_;\n> > +\tunsigned int loop_;\n> > \tbool valid_;\n> > \n> > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > @@ -49,6 +50,8 @@ private:\n> > \n> > \tint parseScript(FILE *script);\n> > \n> > +\tint parseProperties();\n> > +\tint parseProperty();\n> > \tint parseFrames();\n> > \tint parseFrame(EventPtr event);\n> > \tint parseControl(EventPtr event, libcamera::ControlList &controls);","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 646EFC0DA4\n\tfor ;\n\tSat, 3 Sep 2022 00:44:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1ECB6201F;\n\tSat, 3 Sep 2022 02:44:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 55EBE62003\n\tfor ;\n\tSat, 3 Sep 2022 02:44:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 895536DD;\n\tSat, 3 Sep 2022 02:44:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662165857;\n\tbh=wj1ZzynmC6U3uPYJdObj1yTWJbxWJfLXX2yEOzS1v0Q=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Cz+sJStMFIXZ3VxTgjASfMoxHNHgEGC2Bg4or9dkIUza6c8YwY/0hBGUBQKh0/OHE\n\tBLH5+mhn6H3dkrjVFzz5AlO2tZ3UqHWQbg8U4OJH+8qHnB9LAYWRCdcjcZ/7lIvWFU\n\tH2V2fx8tVmq5ijMlabSDEJYSfLjcMvefiXA/lICF4VLUU0VoLXYy8UY/aZpOmBTvIv\n\tCdHaIVVcM5Ht+y83ADQ9NMd+g1BQ3zKAh6zCFZXEKyn6tw/DAbOxoZTSYk0ageYi06\n\tbpJtLw7FvrQ9v7XeE4MZVo5DZecMAW49pue75StEVm9JMTk9V5CzANTXD3XZWNMG3K\n\tWNbMmOh5mctWA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662165854;\n\tbh=wj1ZzynmC6U3uPYJdObj1yTWJbxWJfLXX2yEOzS1v0Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=axm/SsS8XF/PNymmFL0+FEeA/ZvVEMPII0y6nCgxl6nW9nv6RnQ0o5QKKy1EVFXdK\n\tqYGoopFA6QOkQFSLZE7/ZY8SASpBymWFdhVyZCxiDIc6n+LNq27gOiEby2rUsXmdzP\n\tgo1rp6sWTTQCuAljew9346nQrhNBBB+wi+cl5Ehg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"axm/SsS8\"; dkim-atps=neutral","Date":"Sat, 3 Sep 2022 03:44:01 +0300","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20220902150031.31184-1-jacopo@jmondi.org>\n\t<20220903001901.GB5705@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220903001901.GB5705@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24909,"web_url":"https://patchwork.libcamera.org/comment/24909/","msgid":"<20220903010024.GD113531@pyrite.rasen.tech>","date":"2022-09-03T01:00:24","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:\n> On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:\n> > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > Add to the capture script support for properties that control the script\n> \n> \"Add support to the capture script for properties\" would be clearer.\n> \n> > > execution. Script properties are specified in a 'properties' section\n> > > before the actual list of controls specified in the 'frames' section.\n> > > \n> > > Define a first 'loop' property that allows to repeat the frame list\n> > \n> > s/to repeat/repeating/\n> > \n> > > periodically. All the frame ids in the 'frames' section shall be smaller\n> > > than the loop control.\n> > > \n> > > Modify the capture script example to show usage of the 'loop' property\n> > > and better document the frames list while at it.\n> > > \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/cam/capture-script.yaml | 50 +++++++++++------------\n> > > src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> > > src/cam/capture_script.h | 3 ++\n> > > 3 files changed, 102 insertions(+), 30 deletions(-)\n> > > \n> > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> > > index 6a749bc60cf7..dbea4a9f01a7 100644\n> > > --- a/src/cam/capture-script.yaml\n> > > +++ b/src/cam/capture-script.yaml\n> > > @@ -5,6 +5,20 @@\n> > > # A capture script allows to associate a list of controls and their values\n> > > # to frame numbers.\n> > > \n> > > +# The script allows to define a list of frames associated with controls and\n> > \n> > s/to define/defining/\n> > \n> > > +# an optional list of properties that controls the script behaviour\n> \n> s/controls/control/\n\nGrammar police here: \"controls\" is actually correct because it's a \"list\nthat controls\", and not \"properties that control\". \"List\" is the\nsubject, \"of properties\" is the modifier.\n\n> \n> > > +#\n> > > +# properties:\n> > > +# - loop: idx\n> > > +# Repeat the controls every 'idx' frames.\n> \n> This is confusing, it looks like the comment is part of the YAML file.\n> I'd write\n> \n> # properties:\n> # # Repeat the controls every 'idx' frames.\n> # - loop: idx\n> \n> > > +#\n> > > +# frames:\n> > > +# - frameid:\n> \n> I'd write \"frame-number\" instead of \"frameid\" to emphasize it's a frame\n> number, not just any identifier.\n> \n> > > +# Control1: value1\n> > > +# Control2: value2\n> > > +#\n> > > +# List of frame ids with associated a list of controls to be applied\n> \n> Same here.\n> \n> > > +#\n> > > # \\todo Formally define the capture script structure with a schema\n> > > \n> > > # Notes:\n> > > @@ -12,35 +26,17 @@\n> > > # libcamera::controls:: enumeration\n> > > # - Controls not supported by the camera currently operated are ignored\n> > > # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> > > +# strictly minor than the loop control\n> > \n> > s/minor/less than/\n> \n> Or \"smaller than\" ?\n\nDepends on if we want to sound mathematical. \"<\" is strongly associated\nwith the phrase \"less than\", which is what I parsed here because we're\ntalking about \"frame number < loop control\", but also I'm more inclined\nto use mathy language in the first place :p\n\n> \n> > Reviewed-by: Paul Elder \n> > \n> > > \n> > > -# Example:\n> > > -frames:\n> > > - - 1:\n> > > - Brightness: 0.0\n> > > +# Example: Turn brightness up and down every 50 frames\n> > > \n> > > - - 40:\n> > > - Brightness: 0.2\n> > > +properties:\n> > > + - loop: 50\n> > > \n> > > - - 80:\n> > > - Brightness: 0.4\n> > > -\n> > > - - 120:\n> > > - Brightness: 0.8\n> > > -\n> > > - - 160:\n> > > - Brightness: 0.4\n> > > -\n> > > - - 200:\n> > > - Brightness: 0.2\n> > > -\n> > > - - 240:\n> > > +frames:\n> > > + - 0:\n> \n> I like starting at 0 instead of 1.\n> \n> > > Brightness: 0.0\n> > > \n> > > - - 280:\n> > > - Brightness: -0.2\n> > > -\n> > > - - 300:\n> > > - Brightness: -0.4\n> > > -\n> > > - - 340:\n> > > - Brightness: -0.8\n> > > + - 25:\n> > > + Brightness: 0.8\n> \n> It's nice to have more than just two frames in the example script (I\n> regularly use it for testing :-)). Could you instead extend it to\n> continue with\n> \n> - 380:\n> Brightness: -0.4\n> - 380:\n> Brightness: -0.2\n> \n> and loop at 420 ?\n> \n> > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > > index 5e85b3ca604c..52bf19961c17 100644\n> > > --- a/src/cam/capture_script.cpp\n> > > +++ b/src/cam/capture_script.cpp\n> > > @@ -15,7 +15,7 @@ using namespace libcamera;\n> > > \n> > > CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > \t\t\t const std::string &fileName)\n> > > -\t: camera_(camera), valid_(false)\n> > > +\t: camera_(camera), loop_(0), valid_(false)\n> > > {\n> > > \tFILE *fh = fopen(fileName.c_str(), \"r\");\n> > > \tif (!fh) {\n> > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > const ControlList &CaptureScript::frameControls(unsigned int frame)\n> > > {\n> > > \tstatic ControlList controls{};\n> > > +\tunsigned int idx = frame;\n> > > \n> > > -\tauto it = frameControls_.find(frame);\n> > > +\t/* If we loop, repeat the controls every 'loop_' frames. */\n> > > +\tif (loop_)\n> > > +\t\tidx = frame % loop_;\n> > > +\n> > > +\tauto it = frameControls_.find(idx);\n> > > \tif (it == frameControls_.end())\n> > > \t\treturn controls;\n> > > \n> > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> > > \n> > > \t\tstd::string section = eventScalarValue(event);\n> > > \n> > > -\t\tif (section == \"frames\") {\n> > > +\t\tif (section == \"properties\") {\n> > > +\t\t\tret = parseProperties();\n> > > +\t\t\tif (ret)\n> > > +\t\t\t\treturn ret;\n> > > +\t\t} else if (section == \"frames\") {\n> > > \t\t\tret = parseFrames();\n> > > \t\t\tif (ret)\n> > > \t\t\t\treturn ret;\n> > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> > > \t}\n> > > }\n> > > \n> > > +int CaptureScript::parseProperty()\n> > > +{\n> > > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > +\tif (!event)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tstd::string prop = parseScalar();\n> > > +\tif (prop.empty())\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tif (prop == \"loop\") {\n> > > +\t\tevent = nextEvent();\n> > > +\t\tif (!event)\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\tstd::string value = eventScalarValue(event);\n> > > +\t\tif (value.empty())\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\tloop_ = atoi(value.c_str());\n> > > +\t\tif (!loop_) {\n> > > +\t\t\tstd::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\t} else {\n> > > +\t\tstd::cerr << \"Unsupported property: \" << prop << std::endl;\n> \n> \t\tstd::cerr << \"Unsupported property `\" << prop << \"'\" << std::endl;\n> \n> Use a colon in a message if you introduce a sentence that explains or\n> complements the previous one, not when printing the value of a variable.\n> Compare the following two messages, assuming that someone would use a\n> YAML file that contains\n> \n> - properties:\n> - \"file a bug report on bugs.libcamera.org\": 0\n> \n> First message;\n> \n> Unsupported property: file a bug report on bugs.libcamera.org\n\nlol\n\nBut it's an example like this that very clearly illustrates the point.\n\n\nPaul\n\n> \n> Second message:\n> \n> Unsuported property `file a bug report on bugs.libcamera.org'\n> \n> Same for the loop limit.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tevent = nextEvent(YAML_MAPPING_END_EVENT);\n> > > +\tif (!event)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int CaptureScript::parseProperties()\n> > > +{\n> > > +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > +\tif (!event)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\twhile (1) {\n> > > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > > +\t\t\treturn 0;\n> > > +\n> > > +\t\tint ret = parseProperty();\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\tevent = nextEvent();\n> > > +\t\tif (!event)\n> > > +\t\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > int CaptureScript::parseFrames()\n> > > {\n> > > \tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> > > \t\treturn -EINVAL;\n> > > \n> > > \tunsigned int frameId = atoi(key.c_str());\n> > > +\tif (loop_ && frameId >= loop_) {\n> > > +\t\tstd::cerr\n> > > +\t\t\t<< \"Frame id (\" << frameId << \") shall be smaller than\"\n> > > +\t\t\t<< \"loop limit (\" << loop_ << \")\" << std::endl;\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > \n> > > \tevent = nextEvent(YAML_MAPPING_START_EVENT);\n> > > \tif (!event)\n> > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > > index fffe67e5a3df..7a0ddebb00b5 100644\n> > > --- a/src/cam/capture_script.h\n> > > +++ b/src/cam/capture_script.h\n> > > @@ -40,6 +40,7 @@ private:\n> > > \tstd::map frameControls_;\n> > > \tstd::shared_ptr camera_;\n> > > \tyaml_parser_t parser_;\n> > > +\tunsigned int loop_;\n> > > \tbool valid_;\n> > > \n> > > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > @@ -49,6 +50,8 @@ private:\n> > > \n> > > \tint parseScript(FILE *script);\n> > > \n> > > +\tint parseProperties();\n> > > +\tint parseProperty();\n> > > \tint parseFrames();\n> > > \tint parseFrame(EventPtr event);\n> > > \tint parseControl(EventPtr event, libcamera::ControlList &controls);","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 5EE6DC3272\n\tfor ;\n\tSat, 3 Sep 2022 01:00:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DC6462022;\n\tSat, 3 Sep 2022 03:00:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49BAA62003\n\tfor ;\n\tSat, 3 Sep 2022 03:00:30 +0200 (CEST)","from pyrite.rasen.tech (unknown [50.228.9.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 30DD66DD;\n\tSat, 3 Sep 2022 03:00:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662166831;\n\tbh=2TDaf+oipLBhuunUfdT7VppGDgy16L7aYqOoMDWlYAA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YbJnmxoycr35xO+j5Sqs8RQwVOFnuYCjbMov572uMzL9xKCuXZD3f6Q3sQuieYxXD\n\thtpVRm+lXyCImjNkBToaoxWQ6vHM9iBxUuo8IhOWmPSQgpjrQ/zQzb7z2o9ct5+mLQ\n\trtjdOQKFUb1DS4GNSOc7oeSf3O7bx4jowvw3lg9RI+mfPsFh+mnnyqP9T8vuKLNSzC\n\tmCEXL/ZD3wr8Dn+Kyi8FmDJj812lYtlm74VlxwXKyRVnLIgnnvETP7RgTlUTrP6cAC\n\t+kvE9UZV+iJDbBAwDeJRYUmUA9ingdVhF3CsMjChsLDlp/186hsfqI/kIVWJlAQd7j\n\ty/y1bd/O2HDGA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662166829;\n\tbh=2TDaf+oipLBhuunUfdT7VppGDgy16L7aYqOoMDWlYAA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LcpjlTP5qSWIeO72L/apj2SpIm37kwapnkKzVv/n0rCelNHY1YJ/hKXMgkPL/XdE6\n\tjrkiIBIh+T/pkaCEzbjOojx4/okYrxBRR1K1sSdhydQEUwUmKAsFO0ofHtQ5Z0Tl+I\n\t4vxqNU9YLMmKolFA48BZmfNzJiDIv4cPJhD/wWWg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LcpjlTP5\"; dkim-atps=neutral","Date":"Fri, 2 Sep 2022 21:00:24 -0400","To":"Laurent Pinchart ","Message-ID":"<20220903010024.GD113531@pyrite.rasen.tech>","References":"<20220902150031.31184-1-jacopo@jmondi.org>\n\t<20220903001901.GB5705@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24910,"web_url":"https://patchwork.libcamera.org/comment/24910/","msgid":"","date":"2022-09-03T01:04:12","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:\n> On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:\n> > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:\n> > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > Add to the capture script support for properties that control the script\n> > \n> > \"Add support to the capture script for properties\" would be clearer.\n> > \n> > > > execution. Script properties are specified in a 'properties' section\n> > > > before the actual list of controls specified in the 'frames' section.\n> > > > \n> > > > Define a first 'loop' property that allows to repeat the frame list\n> > > \n> > > s/to repeat/repeating/\n> > > \n> > > > periodically. All the frame ids in the 'frames' section shall be smaller\n> > > > than the loop control.\n> > > > \n> > > > Modify the capture script example to show usage of the 'loop' property\n> > > > and better document the frames list while at it.\n> > > > \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/cam/capture-script.yaml | 50 +++++++++++------------\n> > > > src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> > > > src/cam/capture_script.h | 3 ++\n> > > > 3 files changed, 102 insertions(+), 30 deletions(-)\n> > > > \n> > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> > > > index 6a749bc60cf7..dbea4a9f01a7 100644\n> > > > --- a/src/cam/capture-script.yaml\n> > > > +++ b/src/cam/capture-script.yaml\n> > > > @@ -5,6 +5,20 @@\n> > > > # A capture script allows to associate a list of controls and their values\n> > > > # to frame numbers.\n> > > > \n> > > > +# The script allows to define a list of frames associated with controls and\n> > > \n> > > s/to define/defining/\n> > > \n> > > > +# an optional list of properties that controls the script behaviour\n> > \n> > s/controls/control/\n> \n> Grammar police here: \"controls\" is actually correct because it's a \"list\n> that controls\", and not \"properties that control\". \"List\" is the\n> subject, \"of properties\" is the modifier.\n\nI was thinking that it was the properties that controlled the behaviour,\nnot the list :-) I'm fine either way.\n\n> > > > +#\n> > > > +# properties:\n> > > > +# - loop: idx\n> > > > +# Repeat the controls every 'idx' frames.\n> > \n> > This is confusing, it looks like the comment is part of the YAML file.\n> > I'd write\n> > \n> > # properties:\n> > # # Repeat the controls every 'idx' frames.\n> > # - loop: idx\n> > \n> > > > +#\n> > > > +# frames:\n> > > > +# - frameid:\n> > \n> > I'd write \"frame-number\" instead of \"frameid\" to emphasize it's a frame\n> > number, not just any identifier.\n> > \n> > > > +# Control1: value1\n> > > > +# Control2: value2\n> > > > +#\n> > > > +# List of frame ids with associated a list of controls to be applied\n> > \n> > Same here.\n> > \n> > > > +#\n> > > > # \\todo Formally define the capture script structure with a schema\n> > > > \n> > > > # Notes:\n> > > > @@ -12,35 +26,17 @@\n> > > > # libcamera::controls:: enumeration\n> > > > # - Controls not supported by the camera currently operated are ignored\n> > > > # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> > > > +# strictly minor than the loop control\n> > > \n> > > s/minor/less than/\n> > \n> > Or \"smaller than\" ?\n> \n> Depends on if we want to sound mathematical. \"<\" is strongly associated\n> with the phrase \"less than\", which is what I parsed here because we're\n> talking about \"frame number < loop control\", but also I'm more inclined\n> to use mathy language in the first place :p\n> \n> > > Reviewed-by: Paul Elder \n> > > \n> > > > \n> > > > -# Example:\n> > > > -frames:\n> > > > - - 1:\n> > > > - Brightness: 0.0\n> > > > +# Example: Turn brightness up and down every 50 frames\n> > > > \n> > > > - - 40:\n> > > > - Brightness: 0.2\n> > > > +properties:\n> > > > + - loop: 50\n> > > > \n> > > > - - 80:\n> > > > - Brightness: 0.4\n> > > > -\n> > > > - - 120:\n> > > > - Brightness: 0.8\n> > > > -\n> > > > - - 160:\n> > > > - Brightness: 0.4\n> > > > -\n> > > > - - 200:\n> > > > - Brightness: 0.2\n> > > > -\n> > > > - - 240:\n> > > > +frames:\n> > > > + - 0:\n> > \n> > I like starting at 0 instead of 1.\n> > \n> > > > Brightness: 0.0\n> > > > \n> > > > - - 280:\n> > > > - Brightness: -0.2\n> > > > -\n> > > > - - 300:\n> > > > - Brightness: -0.4\n> > > > -\n> > > > - - 340:\n> > > > - Brightness: -0.8\n> > > > + - 25:\n> > > > + Brightness: 0.8\n> > \n> > It's nice to have more than just two frames in the example script (I\n> > regularly use it for testing :-)). Could you instead extend it to\n> > continue with\n> > \n> > - 380:\n> > Brightness: -0.4\n> > - 380:\n> > Brightness: -0.2\n> > \n> > and loop at 420 ?\n> > \n> > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > > > index 5e85b3ca604c..52bf19961c17 100644\n> > > > --- a/src/cam/capture_script.cpp\n> > > > +++ b/src/cam/capture_script.cpp\n> > > > @@ -15,7 +15,7 @@ using namespace libcamera;\n> > > > \n> > > > CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > \t\t\t const std::string &fileName)\n> > > > -\t: camera_(camera), valid_(false)\n> > > > +\t: camera_(camera), loop_(0), valid_(false)\n> > > > {\n> > > > \tFILE *fh = fopen(fileName.c_str(), \"r\");\n> > > > \tif (!fh) {\n> > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > const ControlList &CaptureScript::frameControls(unsigned int frame)\n> > > > {\n> > > > \tstatic ControlList controls{};\n> > > > +\tunsigned int idx = frame;\n> > > > \n> > > > -\tauto it = frameControls_.find(frame);\n> > > > +\t/* If we loop, repeat the controls every 'loop_' frames. */\n> > > > +\tif (loop_)\n> > > > +\t\tidx = frame % loop_;\n> > > > +\n> > > > +\tauto it = frameControls_.find(idx);\n> > > > \tif (it == frameControls_.end())\n> > > > \t\treturn controls;\n> > > > \n> > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> > > > \n> > > > \t\tstd::string section = eventScalarValue(event);\n> > > > \n> > > > -\t\tif (section == \"frames\") {\n> > > > +\t\tif (section == \"properties\") {\n> > > > +\t\t\tret = parseProperties();\n> > > > +\t\t\tif (ret)\n> > > > +\t\t\t\treturn ret;\n> > > > +\t\t} else if (section == \"frames\") {\n> > > > \t\t\tret = parseFrames();\n> > > > \t\t\tif (ret)\n> > > > \t\t\t\treturn ret;\n> > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> > > > \t}\n> > > > }\n> > > > \n> > > > +int CaptureScript::parseProperty()\n> > > > +{\n> > > > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > +\tif (!event)\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\tstd::string prop = parseScalar();\n> > > > +\tif (prop.empty())\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\tif (prop == \"loop\") {\n> > > > +\t\tevent = nextEvent();\n> > > > +\t\tif (!event)\n> > > > +\t\t\treturn -EINVAL;\n> > > > +\n> > > > +\t\tstd::string value = eventScalarValue(event);\n> > > > +\t\tif (value.empty())\n> > > > +\t\t\treturn -EINVAL;\n> > > > +\n> > > > +\t\tloop_ = atoi(value.c_str());\n> > > > +\t\tif (!loop_) {\n> > > > +\t\t\tstd::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> > > > +\t\t\treturn -EINVAL;\n> > > > +\t\t}\n> > > > +\t} else {\n> > > > +\t\tstd::cerr << \"Unsupported property: \" << prop << std::endl;\n> > \n> > \t\tstd::cerr << \"Unsupported property `\" << prop << \"'\" << std::endl;\n> > \n> > Use a colon in a message if you introduce a sentence that explains or\n> > complements the previous one, not when printing the value of a variable.\n> > Compare the following two messages, assuming that someone would use a\n> > YAML file that contains\n> > \n> > - properties:\n> > - \"file a bug report on bugs.libcamera.org\": 0\n> > \n> > First message;\n> > \n> > Unsupported property: file a bug report on bugs.libcamera.org\n> \n> lol\n> \n> But it's an example like this that very clearly illustrates the point.\n> \n> \n> Paul\n> \n> > Second message:\n> > \n> > Unsuported property `file a bug report on bugs.libcamera.org'\n> > \n> > Same for the loop limit.\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\tevent = nextEvent(YAML_MAPPING_END_EVENT);\n> > > > +\tif (!event)\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +int CaptureScript::parseProperties()\n> > > > +{\n> > > > +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > +\tif (!event)\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\twhile (1) {\n> > > > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > +\t\t\treturn 0;\n> > > > +\n> > > > +\t\tint ret = parseProperty();\n> > > > +\t\tif (ret)\n> > > > +\t\t\treturn ret;\n> > > > +\n> > > > +\t\tevent = nextEvent();\n> > > > +\t\tif (!event)\n> > > > +\t\t\treturn -EINVAL;\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > int CaptureScript::parseFrames()\n> > > > {\n> > > > \tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> > > > \t\treturn -EINVAL;\n> > > > \n> > > > \tunsigned int frameId = atoi(key.c_str());\n> > > > +\tif (loop_ && frameId >= loop_) {\n> > > > +\t\tstd::cerr\n> > > > +\t\t\t<< \"Frame id (\" << frameId << \") shall be smaller than\"\n> > > > +\t\t\t<< \"loop limit (\" << loop_ << \")\" << std::endl;\n> > > > +\t\treturn -EINVAL;\n> > > > +\t}\n> > > > \n> > > > \tevent = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > \tif (!event)\n> > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > > > index fffe67e5a3df..7a0ddebb00b5 100644\n> > > > --- a/src/cam/capture_script.h\n> > > > +++ b/src/cam/capture_script.h\n> > > > @@ -40,6 +40,7 @@ private:\n> > > > \tstd::map frameControls_;\n> > > > \tstd::shared_ptr camera_;\n> > > > \tyaml_parser_t parser_;\n> > > > +\tunsigned int loop_;\n> > > > \tbool valid_;\n> > > > \n> > > > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > @@ -49,6 +50,8 @@ private:\n> > > > \n> > > > \tint parseScript(FILE *script);\n> > > > \n> > > > +\tint parseProperties();\n> > > > +\tint parseProperty();\n> > > > \tint parseFrames();\n> > > > \tint parseFrame(EventPtr event);\n> > > > \tint parseControl(EventPtr event, libcamera::ControlList &controls);","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 5195EC0DA4\n\tfor ;\n\tSat, 3 Sep 2022 01:04:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9EB8162022;\n\tSat, 3 Sep 2022 03:04:27 +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 A015562003\n\tfor ;\n\tSat, 3 Sep 2022 03:04:25 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D19886DD;\n\tSat, 3 Sep 2022 03:04:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662167067;\n\tbh=6Q1Yo62hg8muxdYSu3laZe5mpMNkBk5bY4bgeZUaT0g=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=KrtDJzti8AI2Ie2R12H+sP6f/2W8S2gRxCjS83OsUhAo3YOOzBFtbN0a54uSaCegO\n\tzE+ExGGTs9QsB61t1dD5AR8DV6sH3UGJVwg6m8/bWeYtr/fc0CXjoLmx16lPifC9S1\n\trs73n1vturKwbPnBEhbqOYN9Qmvyxi4LZi1y+OvTW6oxptYRU2+y2yvZNCIrJL6MCQ\n\tUEODtSWW2GnudNQCnSPOLtfPtL9y18EUXZhCrJkY4fg6Xg3Ezb4No3FsQ/+YoGC8Ga\n\t8CD3YSu6CzYTYmQzcncjYvBkDfiRpJNA458tJGynuBNVgdSRiLtK5ZCfyj2GcQFcgk\n\t7SAysC5NBxLIA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662167065;\n\tbh=6Q1Yo62hg8muxdYSu3laZe5mpMNkBk5bY4bgeZUaT0g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VxN15ClNDTU/lx6scv5JnIOlFHg7kLtwJdXY2Ya5tnWHAzVoJvJo79i+24hPHd45q\n\tZ5EwuTDOkF0PZZAXzXJ4wpQxQpyzim65XRSidt7PhAWC+/kCff76shGeGQldI7kGWk\n\tQFxUYmj1BVsP96bcOIig9tmw2XTNf8p89pCSGxPc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VxN15ClN\"; dkim-atps=neutral","Date":"Sat, 3 Sep 2022 04:04:12 +0300","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20220902150031.31184-1-jacopo@jmondi.org>\n\t<20220903001901.GB5705@pyrite.rasen.tech>\n\t\n\t<20220903010024.GD113531@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220903010024.GD113531@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24912,"web_url":"https://patchwork.libcamera.org/comment/24912/","msgid":"<20220903011649.GF113531@pyrite.rasen.tech>","date":"2022-09-03T01:16:49","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Sat, Sep 03, 2022 at 04:04:12AM +0300, Laurent Pinchart wrote:\n> On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:\n> > On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:\n> > > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:\n> > > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > Add to the capture script support for properties that control the script\n> > > \n> > > \"Add support to the capture script for properties\" would be clearer.\n> > > \n> > > > > execution. Script properties are specified in a 'properties' section\n> > > > > before the actual list of controls specified in the 'frames' section.\n> > > > > \n> > > > > Define a first 'loop' property that allows to repeat the frame list\n> > > > \n> > > > s/to repeat/repeating/\n> > > > \n> > > > > periodically. All the frame ids in the 'frames' section shall be smaller\n> > > > > than the loop control.\n> > > > > \n> > > > > Modify the capture script example to show usage of the 'loop' property\n> > > > > and better document the frames list while at it.\n> > > > > \n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/cam/capture-script.yaml | 50 +++++++++++------------\n> > > > > src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> > > > > src/cam/capture_script.h | 3 ++\n> > > > > 3 files changed, 102 insertions(+), 30 deletions(-)\n> > > > > \n> > > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> > > > > index 6a749bc60cf7..dbea4a9f01a7 100644\n> > > > > --- a/src/cam/capture-script.yaml\n> > > > > +++ b/src/cam/capture-script.yaml\n> > > > > @@ -5,6 +5,20 @@\n> > > > > # A capture script allows to associate a list of controls and their values\n> > > > > # to frame numbers.\n> > > > > \n> > > > > +# The script allows to define a list of frames associated with controls and\n> > > > \n> > > > s/to define/defining/\n> > > > \n> > > > > +# an optional list of properties that controls the script behaviour\n> > > \n> > > s/controls/control/\n> > \n> > Grammar police here: \"controls\" is actually correct because it's a \"list\n> > that controls\", and not \"properties that control\". \"List\" is the\n> > subject, \"of properties\" is the modifier.\n> \n> I was thinking that it was the properties that controlled the behaviour,\n> not the list :-) I'm fine either way.\n\nHuh, you have a point. I guess that makes sense semantically then (as\nthe properties are the ones that do the controlling but they are\norganized in a list), but it's not correct syntactically (because the\nproperties are gramatically downgraded to a modifier).\n\nBut no matter how I imagine it, \"controls\" sounds more correct, since\n\"list\" is the subject :/\n\nEnglish is weird.\n\n\nPaul\n\n> \n> > > > > +#\n> > > > > +# properties:\n> > > > > +# - loop: idx\n> > > > > +# Repeat the controls every 'idx' frames.\n> > > \n> > > This is confusing, it looks like the comment is part of the YAML file.\n> > > I'd write\n> > > \n> > > # properties:\n> > > # # Repeat the controls every 'idx' frames.\n> > > # - loop: idx\n> > > \n> > > > > +#\n> > > > > +# frames:\n> > > > > +# - frameid:\n> > > \n> > > I'd write \"frame-number\" instead of \"frameid\" to emphasize it's a frame\n> > > number, not just any identifier.\n> > > \n> > > > > +# Control1: value1\n> > > > > +# Control2: value2\n> > > > > +#\n> > > > > +# List of frame ids with associated a list of controls to be applied\n> > > \n> > > Same here.\n> > > \n> > > > > +#\n> > > > > # \\todo Formally define the capture script structure with a schema\n> > > > > \n> > > > > # Notes:\n> > > > > @@ -12,35 +26,17 @@\n> > > > > # libcamera::controls:: enumeration\n> > > > > # - Controls not supported by the camera currently operated are ignored\n> > > > > # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> > > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> > > > > +# strictly minor than the loop control\n> > > > \n> > > > s/minor/less than/\n> > > \n> > > Or \"smaller than\" ?\n> > \n> > Depends on if we want to sound mathematical. \"<\" is strongly associated\n> > with the phrase \"less than\", which is what I parsed here because we're\n> > talking about \"frame number < loop control\", but also I'm more inclined\n> > to use mathy language in the first place :p\n> > \n> > > > Reviewed-by: Paul Elder \n> > > > \n> > > > > \n> > > > > -# Example:\n> > > > > -frames:\n> > > > > - - 1:\n> > > > > - Brightness: 0.0\n> > > > > +# Example: Turn brightness up and down every 50 frames\n> > > > > \n> > > > > - - 40:\n> > > > > - Brightness: 0.2\n> > > > > +properties:\n> > > > > + - loop: 50\n> > > > > \n> > > > > - - 80:\n> > > > > - Brightness: 0.4\n> > > > > -\n> > > > > - - 120:\n> > > > > - Brightness: 0.8\n> > > > > -\n> > > > > - - 160:\n> > > > > - Brightness: 0.4\n> > > > > -\n> > > > > - - 200:\n> > > > > - Brightness: 0.2\n> > > > > -\n> > > > > - - 240:\n> > > > > +frames:\n> > > > > + - 0:\n> > > \n> > > I like starting at 0 instead of 1.\n> > > \n> > > > > Brightness: 0.0\n> > > > > \n> > > > > - - 280:\n> > > > > - Brightness: -0.2\n> > > > > -\n> > > > > - - 300:\n> > > > > - Brightness: -0.4\n> > > > > -\n> > > > > - - 340:\n> > > > > - Brightness: -0.8\n> > > > > + - 25:\n> > > > > + Brightness: 0.8\n> > > \n> > > It's nice to have more than just two frames in the example script (I\n> > > regularly use it for testing :-)). Could you instead extend it to\n> > > continue with\n> > > \n> > > - 380:\n> > > Brightness: -0.4\n> > > - 380:\n> > > Brightness: -0.2\n> > > \n> > > and loop at 420 ?\n> > > \n> > > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > > > > index 5e85b3ca604c..52bf19961c17 100644\n> > > > > --- a/src/cam/capture_script.cpp\n> > > > > +++ b/src/cam/capture_script.cpp\n> > > > > @@ -15,7 +15,7 @@ using namespace libcamera;\n> > > > > \n> > > > > CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > > \t\t\t const std::string &fileName)\n> > > > > -\t: camera_(camera), valid_(false)\n> > > > > +\t: camera_(camera), loop_(0), valid_(false)\n> > > > > {\n> > > > > \tFILE *fh = fopen(fileName.c_str(), \"r\");\n> > > > > \tif (!fh) {\n> > > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > > const ControlList &CaptureScript::frameControls(unsigned int frame)\n> > > > > {\n> > > > > \tstatic ControlList controls{};\n> > > > > +\tunsigned int idx = frame;\n> > > > > \n> > > > > -\tauto it = frameControls_.find(frame);\n> > > > > +\t/* If we loop, repeat the controls every 'loop_' frames. */\n> > > > > +\tif (loop_)\n> > > > > +\t\tidx = frame % loop_;\n> > > > > +\n> > > > > +\tauto it = frameControls_.find(idx);\n> > > > > \tif (it == frameControls_.end())\n> > > > > \t\treturn controls;\n> > > > > \n> > > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > \n> > > > > \t\tstd::string section = eventScalarValue(event);\n> > > > > \n> > > > > -\t\tif (section == \"frames\") {\n> > > > > +\t\tif (section == \"properties\") {\n> > > > > +\t\t\tret = parseProperties();\n> > > > > +\t\t\tif (ret)\n> > > > > +\t\t\t\treturn ret;\n> > > > > +\t\t} else if (section == \"frames\") {\n> > > > > \t\t\tret = parseFrames();\n> > > > > \t\t\tif (ret)\n> > > > > \t\t\t\treturn ret;\n> > > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > \t}\n> > > > > }\n> > > > > \n> > > > > +int CaptureScript::parseProperty()\n> > > > > +{\n> > > > > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > +\tif (!event)\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\tstd::string prop = parseScalar();\n> > > > > +\tif (prop.empty())\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\tif (prop == \"loop\") {\n> > > > > +\t\tevent = nextEvent();\n> > > > > +\t\tif (!event)\n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\t\tstd::string value = eventScalarValue(event);\n> > > > > +\t\tif (value.empty())\n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\t\tloop_ = atoi(value.c_str());\n> > > > > +\t\tif (!loop_) {\n> > > > > +\t\t\tstd::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\t\t}\n> > > > > +\t} else {\n> > > > > +\t\tstd::cerr << \"Unsupported property: \" << prop << std::endl;\n> > > \n> > > \t\tstd::cerr << \"Unsupported property `\" << prop << \"'\" << std::endl;\n> > > \n> > > Use a colon in a message if you introduce a sentence that explains or\n> > > complements the previous one, not when printing the value of a variable.\n> > > Compare the following two messages, assuming that someone would use a\n> > > YAML file that contains\n> > > \n> > > - properties:\n> > > - \"file a bug report on bugs.libcamera.org\": 0\n> > > \n> > > First message;\n> > > \n> > > Unsupported property: file a bug report on bugs.libcamera.org\n> > \n> > lol\n> > \n> > But it's an example like this that very clearly illustrates the point.\n> > \n> > \n> > Paul\n> > \n> > > Second message:\n> > > \n> > > Unsuported property `file a bug report on bugs.libcamera.org'\n> > > \n> > > Same for the loop limit.\n> > > \n> > > Reviewed-by: Laurent Pinchart \n> > > \n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tevent = nextEvent(YAML_MAPPING_END_EVENT);\n> > > > > +\tif (!event)\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +int CaptureScript::parseProperties()\n> > > > > +{\n> > > > > +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > > +\tif (!event)\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\twhile (1) {\n> > > > > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > > +\t\t\treturn 0;\n> > > > > +\n> > > > > +\t\tint ret = parseProperty();\n> > > > > +\t\tif (ret)\n> > > > > +\t\t\treturn ret;\n> > > > > +\n> > > > > +\t\tevent = nextEvent();\n> > > > > +\t\tif (!event)\n> > > > > +\t\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > int CaptureScript::parseFrames()\n> > > > > {\n> > > > > \tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> > > > > \t\treturn -EINVAL;\n> > > > > \n> > > > > \tunsigned int frameId = atoi(key.c_str());\n> > > > > +\tif (loop_ && frameId >= loop_) {\n> > > > > +\t\tstd::cerr\n> > > > > +\t\t\t<< \"Frame id (\" << frameId << \") shall be smaller than\"\n> > > > > +\t\t\t<< \"loop limit (\" << loop_ << \")\" << std::endl;\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\t}\n> > > > > \n> > > > > \tevent = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > \tif (!event)\n> > > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > > > > index fffe67e5a3df..7a0ddebb00b5 100644\n> > > > > --- a/src/cam/capture_script.h\n> > > > > +++ b/src/cam/capture_script.h\n> > > > > @@ -40,6 +40,7 @@ private:\n> > > > > \tstd::map frameControls_;\n> > > > > \tstd::shared_ptr camera_;\n> > > > > \tyaml_parser_t parser_;\n> > > > > +\tunsigned int loop_;\n> > > > > \tbool valid_;\n> > > > > \n> > > > > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > > @@ -49,6 +50,8 @@ private:\n> > > > > \n> > > > > \tint parseScript(FILE *script);\n> > > > > \n> > > > > +\tint parseProperties();\n> > > > > +\tint parseProperty();\n> > > > > \tint parseFrames();\n> > > > > \tint parseFrame(EventPtr event);\n> > > > > \tint parseControl(EventPtr event, libcamera::ControlList &controls);","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 260CCC0DA4\n\tfor ;\n\tSat, 3 Sep 2022 01:17:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C11262027;\n\tSat, 3 Sep 2022 03:17:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 028CD62003\n\tfor ;\n\tSat, 3 Sep 2022 03:17:00 +0200 (CEST)","from pyrite.rasen.tech (unknown [50.228.9.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3B1B51E;\n\tSat, 3 Sep 2022 03:16:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662167821;\n\tbh=YOPmMs8+ZHw9mHQuosu8GsaQLOBV3fV/mn+erhDTA4Q=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OGVO2A7zXYnYq3sE7oP6oJ5CwXEO4XyxhlEPl10GquXnsJWhrbaclszjtzwcZvc4x\n\tN8wwqrfWVZhCS3KGf25hKwFQJzEj9VVcL7QdqHtkuBVG9xBub/Dj8UYj5wOwbeNL+1\n\tzdHQ0fRQ+ZYjL1OFb1purDnphpx+9zAMLbln/1s1YPl7PMZpJDHgoC3gFXn821rAkL\n\tv6ChG39htnY2A8ThOx5NFixAZcjcIBv/hkWsfXIYF39Sicg9UUkmbMOYkhDuObVJWj\n\tFAsAn8JUV1ODU/71SVxJ1kZdcbinKBwZDuHnNlRR3d55b73JSYHwxUEBl4akBuOpBN\n\tuoAMdqs5lEzFQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662167820;\n\tbh=YOPmMs8+ZHw9mHQuosu8GsaQLOBV3fV/mn+erhDTA4Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NY2yW9qL1+6CKdb/82Zik0v7aR5w0ZTNCSFvaMGWeKUK85pOAS8p2kcOc7aPc8NVx\n\tAbIJdwV2CILVOCqeRo5t9U78+BNMByuHzRKyjaX4k2yE/Aat/BkEstOiO+/69u5zmL\n\tGZElrQEvE79dXL8f450n+H2IMI+uTShdKrLIStTM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NY2yW9qL\"; dkim-atps=neutral","Date":"Fri, 2 Sep 2022 21:16:49 -0400","To":"Laurent Pinchart ","Message-ID":"<20220903011649.GF113531@pyrite.rasen.tech>","References":"<20220902150031.31184-1-jacopo@jmondi.org>\n\t<20220903001901.GB5705@pyrite.rasen.tech>\n\t\n\t<20220903010024.GD113531@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24913,"web_url":"https://patchwork.libcamera.org/comment/24913/","msgid":"<166220845957.956466.16329211463632494263@Monstersaurus>","date":"2022-09-03T12:34:19","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder via libcamera-devel (2022-09-03 02:16:49)\n> On Sat, Sep 03, 2022 at 04:04:12AM +0300, Laurent Pinchart wrote:\n> > On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:\n> > > On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:\n> > > > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:\n> > > > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > > Add to the capture script support for properties that control the script\n> > > > \n> > > > \"Add support to the capture script for properties\" would be clearer.\n> > > > \n> > > > > > execution. Script properties are specified in a 'properties' section\n> > > > > > before the actual list of controls specified in the 'frames' section.\n> > > > > > \n> > > > > > Define a first 'loop' property that allows to repeat the frame list\n> > > > > \n> > > > > s/to repeat/repeating/\n> > > > > \n> > > > > > periodically. All the frame ids in the 'frames' section shall be smaller\n> > > > > > than the loop control.\n> > > > > > \n> > > > > > Modify the capture script example to show usage of the 'loop' property\n> > > > > > and better document the frames list while at it.\n> > > > > > \n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/cam/capture-script.yaml | 50 +++++++++++------------\n> > > > > > src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> > > > > > src/cam/capture_script.h | 3 ++\n> > > > > > 3 files changed, 102 insertions(+), 30 deletions(-)\n> > > > > > \n> > > > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> > > > > > index 6a749bc60cf7..dbea4a9f01a7 100644\n> > > > > > --- a/src/cam/capture-script.yaml\n> > > > > > +++ b/src/cam/capture-script.yaml\n> > > > > > @@ -5,6 +5,20 @@\n> > > > > > # A capture script allows to associate a list of controls and their values\n> > > > > > # to frame numbers.\n> > > > > > \n> > > > > > +# The script allows to define a list of frames associated with controls and\n> > > > > \n> > > > > s/to define/defining/\n> > > > > \n> > > > > > +# an optional list of properties that controls the script behaviour\n> > > > \n> > > > s/controls/control/\n> > > \n> > > Grammar police here: \"controls\" is actually correct because it's a \"list\n> > > that controls\", and not \"properties that control\". \"List\" is the\n> > > subject, \"of properties\" is the modifier.\n> > \n> > I was thinking that it was the properties that controlled the behaviour,\n> > not the list :-) I'm fine either way.\n> \n> Huh, you have a point. I guess that makes sense semantically then (as\n> the properties are the ones that do the controlling but they are\n> organized in a list), but it's not correct syntactically (because the\n> properties are gramatically downgraded to a modifier).\n> \n> But no matter how I imagine it, \"controls\" sounds more correct, since\n> \"list\" is the subject :/\n> \n> English is weird.\n\nDefinitely, I'd fix this with:\n\n The script allows defining a list of frames associated with controls\n and an optional list of properties that can control the script\n behaviour.\n\nTo me it's either \"that controls\" or \"that can control\"\n--\nKieran\n\n\n> \n> \n> Paul\n> \n> > \n> > > > > > +#\n> > > > > > +# properties:\n> > > > > > +# - loop: idx\n> > > > > > +# Repeat the controls every 'idx' frames.\n> > > > \n> > > > This is confusing, it looks like the comment is part of the YAML file.\n> > > > I'd write\n> > > > \n> > > > # properties:\n> > > > # # Repeat the controls every 'idx' frames.\n> > > > # - loop: idx\n> > > > \n> > > > > > +#\n> > > > > > +# frames:\n> > > > > > +# - frameid:\n> > > > \n> > > > I'd write \"frame-number\" instead of \"frameid\" to emphasize it's a frame\n> > > > number, not just any identifier.\n> > > > \n> > > > > > +# Control1: value1\n> > > > > > +# Control2: value2\n> > > > > > +#\n> > > > > > +# List of frame ids with associated a list of controls to be applied\n> > > > \n> > > > Same here.\n> > > > \n> > > > > > +#\n> > > > > > # \\todo Formally define the capture script structure with a schema\n> > > > > > \n> > > > > > # Notes:\n> > > > > > @@ -12,35 +26,17 @@\n> > > > > > # libcamera::controls:: enumeration\n> > > > > > # - Controls not supported by the camera currently operated are ignored\n> > > > > > # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> > > > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> > > > > > +# strictly minor than the loop control\n> > > > > \n> > > > > s/minor/less than/\n> > > > \n> > > > Or \"smaller than\" ?\n> > > \n> > > Depends on if we want to sound mathematical. \"<\" is strongly associated\n> > > with the phrase \"less than\", which is what I parsed here because we're\n> > > talking about \"frame number < loop control\", but also I'm more inclined\n> > > to use mathy language in the first place :p\n> > > \n> > > > > Reviewed-by: Paul Elder \n> > > > > \n> > > > > > \n> > > > > > -# Example:\n> > > > > > -frames:\n> > > > > > - - 1:\n> > > > > > - Brightness: 0.0\n> > > > > > +# Example: Turn brightness up and down every 50 frames\n> > > > > > \n> > > > > > - - 40:\n> > > > > > - Brightness: 0.2\n> > > > > > +properties:\n> > > > > > + - loop: 50\n> > > > > > \n> > > > > > - - 80:\n> > > > > > - Brightness: 0.4\n> > > > > > -\n> > > > > > - - 120:\n> > > > > > - Brightness: 0.8\n> > > > > > -\n> > > > > > - - 160:\n> > > > > > - Brightness: 0.4\n> > > > > > -\n> > > > > > - - 200:\n> > > > > > - Brightness: 0.2\n> > > > > > -\n> > > > > > - - 240:\n> > > > > > +frames:\n> > > > > > + - 0:\n> > > > \n> > > > I like starting at 0 instead of 1.\n> > > > \n> > > > > > Brightness: 0.0\n> > > > > > \n> > > > > > - - 280:\n> > > > > > - Brightness: -0.2\n> > > > > > -\n> > > > > > - - 300:\n> > > > > > - Brightness: -0.4\n> > > > > > -\n> > > > > > - - 340:\n> > > > > > - Brightness: -0.8\n> > > > > > + - 25:\n> > > > > > + Brightness: 0.8\n> > > > \n> > > > It's nice to have more than just two frames in the example script (I\n> > > > regularly use it for testing :-)). Could you instead extend it to\n> > > > continue with\n> > > > \n> > > > - 380:\n> > > > Brightness: -0.4\n> > > > - 380:\n> > > > Brightness: -0.2\n> > > > \n> > > > and loop at 420 ?\n> > > > \n> > > > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > > > > > index 5e85b3ca604c..52bf19961c17 100644\n> > > > > > --- a/src/cam/capture_script.cpp\n> > > > > > +++ b/src/cam/capture_script.cpp\n> > > > > > @@ -15,7 +15,7 @@ using namespace libcamera;\n> > > > > > \n> > > > > > CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > > > const std::string &fileName)\n> > > > > > - : camera_(camera), valid_(false)\n> > > > > > + : camera_(camera), loop_(0), valid_(false)\n> > > > > > {\n> > > > > > FILE *fh = fopen(fileName.c_str(), \"r\");\n> > > > > > if (!fh) {\n> > > > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > > > const ControlList &CaptureScript::frameControls(unsigned int frame)\n> > > > > > {\n> > > > > > static ControlList controls{};\n> > > > > > + unsigned int idx = frame;\n> > > > > > \n> > > > > > - auto it = frameControls_.find(frame);\n> > > > > > + /* If we loop, repeat the controls every 'loop_' frames. */\n> > > > > > + if (loop_)\n> > > > > > + idx = frame % loop_;\n> > > > > > +\n> > > > > > + auto it = frameControls_.find(idx);\n> > > > > > if (it == frameControls_.end())\n> > > > > > return controls;\n> > > > > > \n> > > > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > > \n> > > > > > std::string section = eventScalarValue(event);\n> > > > > > \n> > > > > > - if (section == \"frames\") {\n> > > > > > + if (section == \"properties\") {\n> > > > > > + ret = parseProperties();\n> > > > > > + if (ret)\n> > > > > > + return ret;\n> > > > > > + } else if (section == \"frames\") {\n> > > > > > ret = parseFrames();\n> > > > > > if (ret)\n> > > > > > return ret;\n> > > > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > > }\n> > > > > > }\n> > > > > > \n> > > > > > +int CaptureScript::parseProperty()\n> > > > > > +{\n> > > > > > + EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + std::string prop = parseScalar();\n> > > > > > + if (prop.empty())\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + if (prop == \"loop\") {\n> > > > > > + event = nextEvent();\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + std::string value = eventScalarValue(event);\n> > > > > > + if (value.empty())\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + loop_ = atoi(value.c_str());\n> > > > > > + if (!loop_) {\n> > > > > > + std::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > > > > > + } else {\n> > > > > > + std::cerr << \"Unsupported property: \" << prop << std::endl;\n> > > > \n> > > > std::cerr << \"Unsupported property `\" << prop << \"'\" << std::endl;\n> > > > \n> > > > Use a colon in a message if you introduce a sentence that explains or\n> > > > complements the previous one, not when printing the value of a variable.\n> > > > Compare the following two messages, assuming that someone would use a\n> > > > YAML file that contains\n> > > > \n> > > > - properties:\n> > > > - \"file a bug report on bugs.libcamera.org\": 0\n> > > > \n> > > > First message;\n> > > > \n> > > > Unsupported property: file a bug report on bugs.libcamera.org\n> > > \n> > > lol\n> > > \n> > > But it's an example like this that very clearly illustrates the point.\n> > > \n> > > \n> > > Paul\n> > > \n> > > > Second message:\n> > > > \n> > > > Unsuported property `file a bug report on bugs.libcamera.org'\n> > > > \n> > > > Same for the loop limit.\n> > > > \n> > > > Reviewed-by: Laurent Pinchart \n> > > > \n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > > > > > +\n> > > > > > + event = nextEvent(YAML_MAPPING_END_EVENT);\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +int CaptureScript::parseProperties()\n> > > > > > +{\n> > > > > > + EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > > > + if (!event)\n> > > > > > + return -EINVAL;\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> > > > > > + return 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > int CaptureScript::parseFrames()\n> > > > > > {\n> > > > > > EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> > > > > > return -EINVAL;\n> > > > > > \n> > > > > > unsigned int frameId = atoi(key.c_str());\n> > > > > > + if (loop_ && frameId >= loop_) {\n> > > > > > + std::cerr\n> > > > > > + << \"Frame id (\" << frameId << \") shall be smaller than\"\n> > > > > > + << \"loop limit (\" << loop_ << \")\" << std::endl;\n> > > > > > + return -EINVAL;\n> > > > > > + }\n> > > > > > \n> > > > > > event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > > if (!event)\n> > > > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > > > > > index fffe67e5a3df..7a0ddebb00b5 100644\n> > > > > > --- a/src/cam/capture_script.h\n> > > > > > +++ b/src/cam/capture_script.h\n> > > > > > @@ -40,6 +40,7 @@ private:\n> > > > > > std::map frameControls_;\n> > > > > > std::shared_ptr camera_;\n> > > > > > yaml_parser_t parser_;\n> > > > > > + unsigned int loop_;\n> > > > > > bool valid_;\n> > > > > > \n> > > > > > EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > > > @@ -49,6 +50,8 @@ private:\n> > > > > > \n> > > > > > int parseScript(FILE *script);\n> > > > > > \n> > > > > > + int parseProperties();\n> > > > > > + int parseProperty();\n> > > > > > int parseFrames();\n> > > > > > int parseFrame(EventPtr event);\n> > > > > > int parseControl(EventPtr event, libcamera::ControlList &controls);","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 62EBFC0DA4\n\tfor ;\n\tSat, 3 Sep 2022 12:34:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFF416202E;\n\tSat, 3 Sep 2022 14:34:24 +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 C650961FBB\n\tfor ;\n\tSat, 3 Sep 2022 14:34:22 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 025C04A8;\n\tSat, 3 Sep 2022 14:34:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662208464;\n\tbh=/WPcnzio5K67mtk4t9CncTk3XKQ0lEyhCCkY+A2IeCg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ThsxO1WomdHz0uATrbmwYrIFfOdiRU9QVXAM+VeBBfyH19dvbCIz1VMZlkTymxH5u\n\teweBKcO/xPp6zzQPAo5ExGKlLvX/kBOaXNr9cIY7OYIGM9etQ+EmZhefRnaHCTX3Ug\n\tjXr3shQa1otOUIDTbZD1eO724P8ZRAkjCg2VRUBiC1IXwMWENKsYpmyvG5PQ17OyuC\n\tJgBLX8TGIyLMrDnXL0OX5W3/PtUS/x4A+a2RToeSRG7PR3xR/+yjJKGzEm4sMEsCoS\n\tDmrGhhR/xpgzWwRjKhs/IDs7Ou71vnY8FlOGhNc4QxUOHF4k8j4hKBPJ4XQwtKsBAN\n\tNOhjSVC+gdbfw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662208462;\n\tbh=/WPcnzio5K67mtk4t9CncTk3XKQ0lEyhCCkY+A2IeCg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=dfIlwm9GtB9bZxmMfjGBajdy4pDOY4DrLuJ02ESVyr3DK/ihdzRKQG/8SUktQx78b\n\t0PscL7WSPnths5R8E4hEHml2JvHvOOjF7TSzgm4Dm4vhk/k+5VsM+DjVD5Zok6ZDZ7\n\tpVBwFB0CK5WCAMMKI8obJmWgAsTux/eKz8mMEj/Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dfIlwm9G\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220903011649.GF113531@pyrite.rasen.tech>","References":"<20220902150031.31184-1-jacopo@jmondi.org>\n\t<20220903001901.GB5705@pyrite.rasen.tech>\n\t\n\t<20220903010024.GD113531@pyrite.rasen.tech>\n\t\n\t<20220903011649.GF113531@pyrite.rasen.tech>","To":"Laurent Pinchart ,\n\tPaul Elder via libcamera-devel , \n\tpaul.elder@ideasonboard.com","Date":"Sat, 03 Sep 2022 13:34:19 +0100","Message-ID":"<166220845957.956466.16329211463632494263@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24914,"web_url":"https://patchwork.libcamera.org/comment/24914/","msgid":"","date":"2022-09-03T13:19:43","subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Sep 02, 2022 at 09:16:49PM -0400, paul.elder@ideasonboard.com wrote:\n> On Sat, Sep 03, 2022 at 04:04:12AM +0300, Laurent Pinchart wrote:\n> > On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:\n> > > On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:\n> > > > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:\n> > > > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > > Add to the capture script support for properties that control the script\n> > > > \n> > > > \"Add support to the capture script for properties\" would be clearer.\n> > > > \n> > > > > > execution. Script properties are specified in a 'properties' section\n> > > > > > before the actual list of controls specified in the 'frames' section.\n> > > > > > \n> > > > > > Define a first 'loop' property that allows to repeat the frame list\n> > > > > \n> > > > > s/to repeat/repeating/\n> > > > > \n> > > > > > periodically. All the frame ids in the 'frames' section shall be smaller\n> > > > > > than the loop control.\n> > > > > > \n> > > > > > Modify the capture script example to show usage of the 'loop' property\n> > > > > > and better document the frames list while at it.\n> > > > > > \n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/cam/capture-script.yaml | 50 +++++++++++------------\n> > > > > > src/cam/capture_script.cpp | 79 +++++++++++++++++++++++++++++++++++--\n> > > > > > src/cam/capture_script.h | 3 ++\n> > > > > > 3 files changed, 102 insertions(+), 30 deletions(-)\n> > > > > > \n> > > > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml\n> > > > > > index 6a749bc60cf7..dbea4a9f01a7 100644\n> > > > > > --- a/src/cam/capture-script.yaml\n> > > > > > +++ b/src/cam/capture-script.yaml\n> > > > > > @@ -5,6 +5,20 @@\n> > > > > > # A capture script allows to associate a list of controls and their values\n> > > > > > # to frame numbers.\n> > > > > > \n> > > > > > +# The script allows to define a list of frames associated with controls and\n> > > > > \n> > > > > s/to define/defining/\n> > > > > \n> > > > > > +# an optional list of properties that controls the script behaviour\n> > > > \n> > > > s/controls/control/\n> > > \n> > > Grammar police here: \"controls\" is actually correct because it's a \"list\n> > > that controls\", and not \"properties that control\". \"List\" is the\n> > > subject, \"of properties\" is the modifier.\n> > \n> > I was thinking that it was the properties that controlled the behaviour,\n> > not the list :-) I'm fine either way.\n> \n> Huh, you have a point. I guess that makes sense semantically then (as\n> the properties are the ones that do the controlling but they are\n> organized in a list), but it's not correct syntactically (because the\n> properties are gramatically downgraded to a modifier).\n> \n> But no matter how I imagine it, \"controls\" sounds more correct, since\n> \"list\" is the subject :/\n> \n> English is weird.\n\nWe have the exact same thing in French ;-)\n\n> > > > > > +#\n> > > > > > +# properties:\n> > > > > > +# - loop: idx\n> > > > > > +# Repeat the controls every 'idx' frames.\n> > > > \n> > > > This is confusing, it looks like the comment is part of the YAML file.\n> > > > I'd write\n> > > > \n> > > > # properties:\n> > > > # # Repeat the controls every 'idx' frames.\n> > > > # - loop: idx\n> > > > \n> > > > > > +#\n> > > > > > +# frames:\n> > > > > > +# - frameid:\n> > > > \n> > > > I'd write \"frame-number\" instead of \"frameid\" to emphasize it's a frame\n> > > > number, not just any identifier.\n> > > > \n> > > > > > +# Control1: value1\n> > > > > > +# Control2: value2\n> > > > > > +#\n> > > > > > +# List of frame ids with associated a list of controls to be applied\n> > > > \n> > > > Same here.\n> > > > \n> > > > > > +#\n> > > > > > # \\todo Formally define the capture script structure with a schema\n> > > > > > \n> > > > > > # Notes:\n> > > > > > @@ -12,35 +26,17 @@\n> > > > > > # libcamera::controls:: enumeration\n> > > > > > # - Controls not supported by the camera currently operated are ignored\n> > > > > > # - Frame numbers shall be monotonically incrementing, gaps are allowed\n> > > > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be\n> > > > > > +# strictly minor than the loop control\n> > > > > \n> > > > > s/minor/less than/\n> > > > \n> > > > Or \"smaller than\" ?\n> > > \n> > > Depends on if we want to sound mathematical. \"<\" is strongly associated\n> > > with the phrase \"less than\", which is what I parsed here because we're\n> > > talking about \"frame number < loop control\", but also I'm more inclined\n> > > to use mathy language in the first place :p\n> > > \n> > > > > Reviewed-by: Paul Elder \n> > > > > \n> > > > > > \n> > > > > > -# Example:\n> > > > > > -frames:\n> > > > > > - - 1:\n> > > > > > - Brightness: 0.0\n> > > > > > +# Example: Turn brightness up and down every 50 frames\n> > > > > > \n> > > > > > - - 40:\n> > > > > > - Brightness: 0.2\n> > > > > > +properties:\n> > > > > > + - loop: 50\n> > > > > > \n> > > > > > - - 80:\n> > > > > > - Brightness: 0.4\n> > > > > > -\n> > > > > > - - 120:\n> > > > > > - Brightness: 0.8\n> > > > > > -\n> > > > > > - - 160:\n> > > > > > - Brightness: 0.4\n> > > > > > -\n> > > > > > - - 200:\n> > > > > > - Brightness: 0.2\n> > > > > > -\n> > > > > > - - 240:\n> > > > > > +frames:\n> > > > > > + - 0:\n> > > > \n> > > > I like starting at 0 instead of 1.\n> > > > \n> > > > > > Brightness: 0.0\n> > > > > > \n> > > > > > - - 280:\n> > > > > > - Brightness: -0.2\n> > > > > > -\n> > > > > > - - 300:\n> > > > > > - Brightness: -0.4\n> > > > > > -\n> > > > > > - - 340:\n> > > > > > - Brightness: -0.8\n> > > > > > + - 25:\n> > > > > > + Brightness: 0.8\n> > > > \n> > > > It's nice to have more than just two frames in the example script (I\n> > > > regularly use it for testing :-)). Could you instead extend it to\n> > > > continue with\n> > > > \n> > > > - 380:\n> > > > Brightness: -0.4\n> > > > - 380:\n> > > > Brightness: -0.2\n> > > > \n> > > > and loop at 420 ?\n> > > > \n> > > > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > > > > > index 5e85b3ca604c..52bf19961c17 100644\n> > > > > > --- a/src/cam/capture_script.cpp\n> > > > > > +++ b/src/cam/capture_script.cpp\n> > > > > > @@ -15,7 +15,7 @@ using namespace libcamera;\n> > > > > > \n> > > > > > CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > > > \t\t\t const std::string &fileName)\n> > > > > > -\t: camera_(camera), valid_(false)\n> > > > > > +\t: camera_(camera), loop_(0), valid_(false)\n> > > > > > {\n> > > > > > \tFILE *fh = fopen(fileName.c_str(), \"r\");\n> > > > > > \tif (!fh) {\n> > > > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr camera,\n> > > > > > const ControlList &CaptureScript::frameControls(unsigned int frame)\n> > > > > > {\n> > > > > > \tstatic ControlList controls{};\n> > > > > > +\tunsigned int idx = frame;\n> > > > > > \n> > > > > > -\tauto it = frameControls_.find(frame);\n> > > > > > +\t/* If we loop, repeat the controls every 'loop_' frames. */\n> > > > > > +\tif (loop_)\n> > > > > > +\t\tidx = frame % loop_;\n> > > > > > +\n> > > > > > +\tauto it = frameControls_.find(idx);\n> > > > > > \tif (it == frameControls_.end())\n> > > > > > \t\treturn controls;\n> > > > > > \n> > > > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > > \n> > > > > > \t\tstd::string section = eventScalarValue(event);\n> > > > > > \n> > > > > > -\t\tif (section == \"frames\") {\n> > > > > > +\t\tif (section == \"properties\") {\n> > > > > > +\t\t\tret = parseProperties();\n> > > > > > +\t\t\tif (ret)\n> > > > > > +\t\t\t\treturn ret;\n> > > > > > +\t\t} else if (section == \"frames\") {\n> > > > > > \t\t\tret = parseFrames();\n> > > > > > \t\t\tif (ret)\n> > > > > > \t\t\t\treturn ret;\n> > > > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)\n> > > > > > \t}\n> > > > > > }\n> > > > > > \n> > > > > > +int CaptureScript::parseProperty()\n> > > > > > +{\n> > > > > > +\tEventPtr event = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > > +\tif (!event)\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\n> > > > > > +\tstd::string prop = parseScalar();\n> > > > > > +\tif (prop.empty())\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\n> > > > > > +\tif (prop == \"loop\") {\n> > > > > > +\t\tevent = nextEvent();\n> > > > > > +\t\tif (!event)\n> > > > > > +\t\t\treturn -EINVAL;\n> > > > > > +\n> > > > > > +\t\tstd::string value = eventScalarValue(event);\n> > > > > > +\t\tif (value.empty())\n> > > > > > +\t\t\treturn -EINVAL;\n> > > > > > +\n> > > > > > +\t\tloop_ = atoi(value.c_str());\n> > > > > > +\t\tif (!loop_) {\n> > > > > > +\t\t\tstd::cerr << \"Invalid loop limit: \" << loop_ << std::endl;\n> > > > > > +\t\t\treturn -EINVAL;\n> > > > > > +\t\t}\n> > > > > > +\t} else {\n> > > > > > +\t\tstd::cerr << \"Unsupported property: \" << prop << std::endl;\n> > > > \n> > > > \t\tstd::cerr << \"Unsupported property `\" << prop << \"'\" << std::endl;\n> > > > \n> > > > Use a colon in a message if you introduce a sentence that explains or\n> > > > complements the previous one, not when printing the value of a variable.\n> > > > Compare the following two messages, assuming that someone would use a\n> > > > YAML file that contains\n> > > > \n> > > > - properties:\n> > > > - \"file a bug report on bugs.libcamera.org\": 0\n> > > > \n> > > > First message;\n> > > > \n> > > > Unsupported property: file a bug report on bugs.libcamera.org\n> > > \n> > > lol\n> > > \n> > > But it's an example like this that very clearly illustrates the point.\n> > > \n> > > \n> > > Paul\n> > > \n> > > > Second message:\n> > > > \n> > > > Unsuported property `file a bug report on bugs.libcamera.org'\n> > > > \n> > > > Same for the loop limit.\n> > > > \n> > > > Reviewed-by: Laurent Pinchart \n> > > > \n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tevent = nextEvent(YAML_MAPPING_END_EVENT);\n> > > > > > +\tif (!event)\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\n> > > > > > +\treturn 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +int CaptureScript::parseProperties()\n> > > > > > +{\n> > > > > > +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > > > +\tif (!event)\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\n> > > > > > +\twhile (1) {\n> > > > > > +\t\tif (event->type == YAML_SEQUENCE_END_EVENT)\n> > > > > > +\t\t\treturn 0;\n> > > > > > +\n> > > > > > +\t\tint ret = parseProperty();\n> > > > > > +\t\tif (ret)\n> > > > > > +\t\t\treturn ret;\n> > > > > > +\n> > > > > > +\t\tevent = nextEvent();\n> > > > > > +\t\tif (!event)\n> > > > > > +\t\t\treturn -EINVAL;\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\treturn 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > int CaptureScript::parseFrames()\n> > > > > > {\n> > > > > > \tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)\n> > > > > > \t\treturn -EINVAL;\n> > > > > > \n> > > > > > \tunsigned int frameId = atoi(key.c_str());\n> > > > > > +\tif (loop_ && frameId >= loop_) {\n> > > > > > +\t\tstd::cerr\n> > > > > > +\t\t\t<< \"Frame id (\" << frameId << \") shall be smaller than\"\n> > > > > > +\t\t\t<< \"loop limit (\" << loop_ << \")\" << std::endl;\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\t}\n> > > > > > \n> > > > > > \tevent = nextEvent(YAML_MAPPING_START_EVENT);\n> > > > > > \tif (!event)\n> > > > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > > > > > index fffe67e5a3df..7a0ddebb00b5 100644\n> > > > > > --- a/src/cam/capture_script.h\n> > > > > > +++ b/src/cam/capture_script.h\n> > > > > > @@ -40,6 +40,7 @@ private:\n> > > > > > \tstd::map frameControls_;\n> > > > > > \tstd::shared_ptr camera_;\n> > > > > > \tyaml_parser_t parser_;\n> > > > > > +\tunsigned int loop_;\n> > > > > > \tbool valid_;\n> > > > > > \n> > > > > > \tEventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);\n> > > > > > @@ -49,6 +50,8 @@ private:\n> > > > > > \n> > > > > > \tint parseScript(FILE *script);\n> > > > > > \n> > > > > > +\tint parseProperties();\n> > > > > > +\tint parseProperty();\n> > > > > > \tint parseFrames();\n> > > > > > \tint parseFrame(EventPtr event);\n> > > > > > \tint parseControl(EventPtr event, libcamera::ControlList &controls);","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 16770C3272\n\tfor ;\n\tSat, 3 Sep 2022 13:19:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89F5C61FBB;\n\tSat, 3 Sep 2022 15:19:58 +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 1EE7161FBB\n\tfor ;\n\tSat, 3 Sep 2022 15:19:57 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F42B4A8;\n\tSat, 3 Sep 2022 15:19:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662211198;\n\tbh=jJ0xyhi+VrQy+BKfR/qR7NM4k5jDdi1fgnbfr5qIkBA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zZVy/iUUkYg08IFAWHcCRWOgV98w27MPsvAJrofns7L9FWmUxVFyaqyCS1hS0EmMR\n\t4l1aOC3qulU1OO52+pDrUNSAoV5oqJt40fYvAU7H8CtVaexXezqtF+WLlknJKWWTob\n\tyCvJEXDtu03itGkYDGY9+qwSUWDlYeFKHeyfEb2pDqnonakcmLQ/EiKh6yyfil6JG7\n\tWc47w0I1m6J0z6+j05jp6T8vl/p2o85ZZOcSOhDAufVg4dnGsSZ/2aX0rHtJadoj4Z\n\tIgDjAvO10gKtnzveGRxu/Dby3aVwvPIuWt2UsJ2GhGuZ/FxpmWFXSx8CNBVLrtzjW5\n\tJjrYUKp5wTC4g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662211196;\n\tbh=jJ0xyhi+VrQy+BKfR/qR7NM4k5jDdi1fgnbfr5qIkBA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K1xVWUdO/X2bYrVsAXKRAK5uIqrU3KxPGxyFtaBJBUoA8pcOZIq4jibkIXh3WlMoY\n\tfFCk7dL6G6UsCZJibOGi0JLzNZnIetwgcCvO4q4Vw/r1KUCsmju7AoaQeTs4IA8F1B\n\tMFFnvj7vbx1jQKVXuVOIs/TyhPBSZY9SNo/dMv78="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K1xVWUdO\"; dkim-atps=neutral","Date":"Sat, 3 Sep 2022 16:19:43 +0300","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20220902150031.31184-1-jacopo@jmondi.org>\n\t<20220903001901.GB5705@pyrite.rasen.tech>\n\t\n\t<20220903010024.GD113531@pyrite.rasen.tech>\n\t\n\t<20220903011649.GF113531@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220903011649.GF113531@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop'\n\tproperty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]