[libcamera-devel] cam: capture_script: Introduce 'loop' property

17281 diff mbox series

Add to the capture script support for properties that control the script
execution. Script properties are specified in a 'properties' section
before the actual list of controls specified in the 'frames' section.

Define a first 'loop' property that allows to repeat the frame list
periodically. All the frame ids in the 'frames' section shall be smaller
than the loop control.

Modify the capture script example to show usage of the 'loop' property
and better document the frames list while at it.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/cam/capture-script.yaml | 50 +++++++++++------------
 src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
 src/cam/capture_script.h    |  3 ++
 3 files changed, 102 insertions(+), 30 deletions(-)

On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Add to the capture script support for properties that control the script
> execution. Script properties are specified in a 'properties' section
> before the actual list of controls specified in the 'frames' section.
> 
> Define a first 'loop' property that allows to repeat the frame list

s/to repeat/repeating/

> periodically. All the frame ids in the 'frames' section shall be smaller
> than the loop control.
> 
> Modify the capture script example to show usage of the 'loop' property
> and better document the frames list while at it.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/cam/capture-script.yaml | 50 +++++++++++------------
>  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
>  src/cam/capture_script.h    |  3 ++
>  3 files changed, 102 insertions(+), 30 deletions(-)
> 
> diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> index 6a749bc60cf7..dbea4a9f01a7 100644
> --- a/src/cam/capture-script.yaml
> +++ b/src/cam/capture-script.yaml
> @@ -5,6 +5,20 @@
>  # A capture script allows to associate a list of controls and their values
>  # to frame numbers.
>  
> +# The script allows to define a list of frames associated with controls and

s/to define/defining/

> +# an optional list of properties that controls the script behaviour
> +#
> +# properties:
> +#   - loop: idx
> +#     Repeat the controls every 'idx' frames.
> +#
> +#  frames:
> +#    - frameid:
> +#        Control1: value1
> +#        Control2: value2
> +#
> +#    List of frame ids with associated a list of controls to be applied
> +#
>  # \todo Formally define the capture script structure with a schema
>  
>  # Notes:
> @@ -12,35 +26,17 @@
>  #   libcamera::controls:: enumeration
>  # - Controls not supported by the camera currently operated are ignored
>  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> +#   strictly minor than the loop control

s/minor/less than/


Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

>  
> -# Example:
> -frames:
> -  - 1:
> -      Brightness: 0.0
> +# Example: Turn brightness up and down every 50 frames
>  
> -  - 40:
> -      Brightness: 0.2
> +properties:
> +  - loop: 50
>  
> -  - 80:
> -      Brightness: 0.4
> -
> -  - 120:
> -      Brightness: 0.8
> -
> -  - 160:
> -      Brightness: 0.4
> -
> -  - 200:
> -      Brightness: 0.2
> -
> -  - 240:
> +frames:
> +  - 0:
>        Brightness: 0.0
>  
> -  - 280:
> -      Brightness: -0.2
> -
> -  - 300:
> -      Brightness: -0.4
> -
> -  - 340:
> -      Brightness: -0.8
> +  - 25:
> +      Brightness: 0.8
> diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> index 5e85b3ca604c..52bf19961c17 100644
> --- a/src/cam/capture_script.cpp
> +++ b/src/cam/capture_script.cpp
> @@ -15,7 +15,7 @@ using namespace libcamera;
>  
>  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
>  			     const std::string &fileName)
> -	: camera_(camera), valid_(false)
> +	: camera_(camera), loop_(0), valid_(false)
>  {
>  	FILE *fh = fopen(fileName.c_str(), "r");
>  	if (!fh) {
> @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
>  const ControlList &CaptureScript::frameControls(unsigned int frame)
>  {
>  	static ControlList controls{};
> +	unsigned int idx = frame;
>  
> -	auto it = frameControls_.find(frame);
> +	/* If we loop, repeat the controls every 'loop_' frames. */
> +	if (loop_)
> +		idx = frame % loop_;
> +
> +	auto it = frameControls_.find(idx);
>  	if (it == frameControls_.end())
>  		return controls;
>  
> @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
>  
>  		std::string section = eventScalarValue(event);
>  
> -		if (section == "frames") {
> +		if (section == "properties") {
> +			ret = parseProperties();
> +			if (ret)
> +				return ret;
> +		} else if (section == "frames") {
>  			ret = parseFrames();
>  			if (ret)
>  				return ret;
> @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
>  	}
>  }
>  
> +int CaptureScript::parseProperty()
> +{
> +	EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> +	if (!event)
> +		return -EINVAL;
> +
> +	std::string prop = parseScalar();
> +	if (prop.empty())
> +		return -EINVAL;
> +
> +	if (prop == "loop") {
> +		event = nextEvent();
> +		if (!event)
> +			return -EINVAL;
> +
> +		std::string value = eventScalarValue(event);
> +		if (value.empty())
> +			return -EINVAL;
> +
> +		loop_ = atoi(value.c_str());
> +		if (!loop_) {
> +			std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> +			return -EINVAL;
> +		}
> +	} else {
> +		std::cerr << "Unsupported property: " << prop << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	event = nextEvent(YAML_MAPPING_END_EVENT);
> +	if (!event)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +int CaptureScript::parseProperties()
> +{
> +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> +	if (!event)
> +		return -EINVAL;
> +
> +	while (1) {
> +		if (event->type == YAML_SEQUENCE_END_EVENT)
> +			return 0;
> +
> +		int ret = parseProperty();
> +		if (ret)
> +			return ret;
> +
> +		event = nextEvent();
> +		if (!event)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  int CaptureScript::parseFrames()
>  {
>  	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
>  		return -EINVAL;
>  
>  	unsigned int frameId = atoi(key.c_str());
> +	if (loop_ && frameId >= loop_) {
> +		std::cerr
> +			<< "Frame id (" << frameId << ") shall be smaller than"
> +			<< "loop limit (" << loop_ << ")" << std::endl;
> +		return -EINVAL;
> +	}
>  
>  	event = nextEvent(YAML_MAPPING_START_EVENT);
>  	if (!event)
> diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> index fffe67e5a3df..7a0ddebb00b5 100644
> --- a/src/cam/capture_script.h
> +++ b/src/cam/capture_script.h
> @@ -40,6 +40,7 @@ private:
>  	std::map<unsigned int, libcamera::ControlList> frameControls_;
>  	std::shared_ptr<libcamera::Camera> camera_;
>  	yaml_parser_t parser_;
> +	unsigned int loop_;
>  	bool valid_;
>  
>  	EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> @@ -49,6 +50,8 @@ private:
>  
>  	int parseScript(FILE *script);
>  
> +	int parseProperties();
> +	int parseProperty();
>  	int parseFrames();
>  	int parseFrame(EventPtr event);
>  	int parseControl(EventPtr event, libcamera::ControlList &controls);
> -- 
> 2.37.2
>

On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:
> On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > Add to the capture script support for properties that control the script
> > execution. Script properties are specified in a 'properties' section
> > before the actual list of controls specified in the 'frames' section.
> > 
> > Define a first 'loop' property that allows to repeat the frame list
> 
> s/to repeat/repeating/
> 
> > periodically. All the frame ids in the 'frames' section shall be smaller
> > than the loop control.
> > 
> > Modify the capture script example to show usage of the 'loop' property
> > and better document the frames list while at it.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/cam/capture-script.yaml | 50 +++++++++++------------
> >  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
> >  src/cam/capture_script.h    |  3 ++
> >  3 files changed, 102 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > index 6a749bc60cf7..dbea4a9f01a7 100644
> > --- a/src/cam/capture-script.yaml
> > +++ b/src/cam/capture-script.yaml
> > @@ -5,6 +5,20 @@
> >  # A capture script allows to associate a list of controls and their values
> >  # to frame numbers.
> >  
> > +# The script allows to define a list of frames associated with controls and
> 
> s/to define/defining/
> 
> > +# an optional list of properties that controls the script behaviour
> > +#
> > +# properties:
> > +#   - loop: idx
> > +#     Repeat the controls every 'idx' frames.
> > +#
> > +#  frames:
> > +#    - frameid:
> > +#        Control1: value1
> > +#        Control2: value2
> > +#
> > +#    List of frame ids with associated a list of controls to be applied
> > +#
> >  # \todo Formally define the capture script structure with a schema
> >  
> >  # Notes:
> > @@ -12,35 +26,17 @@
> >  #   libcamera::controls:: enumeration
> >  # - Controls not supported by the camera currently operated are ignored
> >  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> > +#   strictly minor than the loop control
> 
> s/minor/less than/

Oops:

s/less than/less/


Paul

> 
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> >  
> > -# Example:
> > -frames:
> > -  - 1:
> > -      Brightness: 0.0
> > +# Example: Turn brightness up and down every 50 frames
> >  
> > -  - 40:
> > -      Brightness: 0.2
> > +properties:
> > +  - loop: 50
> >  
> > -  - 80:
> > -      Brightness: 0.4
> > -
> > -  - 120:
> > -      Brightness: 0.8
> > -
> > -  - 160:
> > -      Brightness: 0.4
> > -
> > -  - 200:
> > -      Brightness: 0.2
> > -
> > -  - 240:
> > +frames:
> > +  - 0:
> >        Brightness: 0.0
> >  
> > -  - 280:
> > -      Brightness: -0.2
> > -
> > -  - 300:
> > -      Brightness: -0.4
> > -
> > -  - 340:
> > -      Brightness: -0.8
> > +  - 25:
> > +      Brightness: 0.8
> > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > index 5e85b3ca604c..52bf19961c17 100644
> > --- a/src/cam/capture_script.cpp
> > +++ b/src/cam/capture_script.cpp
> > @@ -15,7 +15,7 @@ using namespace libcamera;
> >  
> >  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> >  			     const std::string &fileName)
> > -	: camera_(camera), valid_(false)
> > +	: camera_(camera), loop_(0), valid_(false)
> >  {
> >  	FILE *fh = fopen(fileName.c_str(), "r");
> >  	if (!fh) {
> > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> >  const ControlList &CaptureScript::frameControls(unsigned int frame)
> >  {
> >  	static ControlList controls{};
> > +	unsigned int idx = frame;
> >  
> > -	auto it = frameControls_.find(frame);
> > +	/* If we loop, repeat the controls every 'loop_' frames. */
> > +	if (loop_)
> > +		idx = frame % loop_;
> > +
> > +	auto it = frameControls_.find(idx);
> >  	if (it == frameControls_.end())
> >  		return controls;
> >  
> > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
> >  
> >  		std::string section = eventScalarValue(event);
> >  
> > -		if (section == "frames") {
> > +		if (section == "properties") {
> > +			ret = parseProperties();
> > +			if (ret)
> > +				return ret;
> > +		} else if (section == "frames") {
> >  			ret = parseFrames();
> >  			if (ret)
> >  				return ret;
> > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
> >  	}
> >  }
> >  
> > +int CaptureScript::parseProperty()
> > +{
> > +	EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	std::string prop = parseScalar();
> > +	if (prop.empty())
> > +		return -EINVAL;
> > +
> > +	if (prop == "loop") {
> > +		event = nextEvent();
> > +		if (!event)
> > +			return -EINVAL;
> > +
> > +		std::string value = eventScalarValue(event);
> > +		if (value.empty())
> > +			return -EINVAL;
> > +
> > +		loop_ = atoi(value.c_str());
> > +		if (!loop_) {
> > +			std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		std::cerr << "Unsupported property: " << prop << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	event = nextEvent(YAML_MAPPING_END_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +int CaptureScript::parseProperties()
> > +{
> > +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	while (1) {
> > +		if (event->type == YAML_SEQUENCE_END_EVENT)
> > +			return 0;
> > +
> > +		int ret = parseProperty();
> > +		if (ret)
> > +			return ret;
> > +
> > +		event = nextEvent();
> > +		if (!event)
> > +			return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int CaptureScript::parseFrames()
> >  {
> >  	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
> >  		return -EINVAL;
> >  
> >  	unsigned int frameId = atoi(key.c_str());
> > +	if (loop_ && frameId >= loop_) {
> > +		std::cerr
> > +			<< "Frame id (" << frameId << ") shall be smaller than"
> > +			<< "loop limit (" << loop_ << ")" << std::endl;
> > +		return -EINVAL;
> > +	}
> >  
> >  	event = nextEvent(YAML_MAPPING_START_EVENT);
> >  	if (!event)
> > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > index fffe67e5a3df..7a0ddebb00b5 100644
> > --- a/src/cam/capture_script.h
> > +++ b/src/cam/capture_script.h
> > @@ -40,6 +40,7 @@ private:
> >  	std::map<unsigned int, libcamera::ControlList> frameControls_;
> >  	std::shared_ptr<libcamera::Camera> camera_;
> >  	yaml_parser_t parser_;
> > +	unsigned int loop_;
> >  	bool valid_;
> >  
> >  	EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > @@ -49,6 +50,8 @@ private:
> >  
> >  	int parseScript(FILE *script);
> >  
> > +	int parseProperties();
> > +	int parseProperty();
> >  	int parseFrames();
> >  	int parseFrame(EventPtr event);
> >  	int parseControl(EventPtr event, libcamera::ControlList &controls);
> > -- 
> > 2.37.2
> >

On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:
> On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > Add to the capture script support for properties that control the script

"Add support to the capture script for properties" would be clearer.

> > execution. Script properties are specified in a 'properties' section
> > before the actual list of controls specified in the 'frames' section.
> > 
> > Define a first 'loop' property that allows to repeat the frame list
> 
> s/to repeat/repeating/
> 
> > periodically. All the frame ids in the 'frames' section shall be smaller
> > than the loop control.
> > 
> > Modify the capture script example to show usage of the 'loop' property
> > and better document the frames list while at it.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/cam/capture-script.yaml | 50 +++++++++++------------
> >  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
> >  src/cam/capture_script.h    |  3 ++
> >  3 files changed, 102 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > index 6a749bc60cf7..dbea4a9f01a7 100644
> > --- a/src/cam/capture-script.yaml
> > +++ b/src/cam/capture-script.yaml
> > @@ -5,6 +5,20 @@
> >  # A capture script allows to associate a list of controls and their values
> >  # to frame numbers.
> >  
> > +# The script allows to define a list of frames associated with controls and
> 
> s/to define/defining/
> 
> > +# an optional list of properties that controls the script behaviour

s/controls/control/

> > +#
> > +# properties:
> > +#   - loop: idx
> > +#     Repeat the controls every 'idx' frames.

This is confusing, it looks like the comment is part of the YAML file.
I'd write

# properties:
#   # Repeat the controls every 'idx' frames.
#   - loop: idx

> > +#
> > +#  frames:
> > +#    - frameid:

I'd write "frame-number" instead of "frameid" to emphasize it's a frame
number, not just any identifier.

> > +#        Control1: value1
> > +#        Control2: value2
> > +#
> > +#    List of frame ids with associated a list of controls to be applied

Same here.

> > +#
> >  # \todo Formally define the capture script structure with a schema
> >  
> >  # Notes:
> > @@ -12,35 +26,17 @@
> >  #   libcamera::controls:: enumeration
> >  # - Controls not supported by the camera currently operated are ignored
> >  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> > +#   strictly minor than the loop control
> 
> s/minor/less than/

Or "smaller than" ?

> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> >  
> > -# Example:
> > -frames:
> > -  - 1:
> > -      Brightness: 0.0
> > +# Example: Turn brightness up and down every 50 frames
> >  
> > -  - 40:
> > -      Brightness: 0.2
> > +properties:
> > +  - loop: 50
> >  
> > -  - 80:
> > -      Brightness: 0.4
> > -
> > -  - 120:
> > -      Brightness: 0.8
> > -
> > -  - 160:
> > -      Brightness: 0.4
> > -
> > -  - 200:
> > -      Brightness: 0.2
> > -
> > -  - 240:
> > +frames:
> > +  - 0:

I like starting at 0 instead of 1.

> >        Brightness: 0.0
> >  
> > -  - 280:
> > -      Brightness: -0.2
> > -
> > -  - 300:
> > -      Brightness: -0.4
> > -
> > -  - 340:
> > -      Brightness: -0.8
> > +  - 25:
> > +      Brightness: 0.8

It's nice to have more than just two frames in the example script (I
regularly use it for testing :-)). Could you instead extend it to
continue with

- 380:
    Brightness: -0.4
- 380:
    Brightness: -0.2

and loop at 420 ?

> > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > index 5e85b3ca604c..52bf19961c17 100644
> > --- a/src/cam/capture_script.cpp
> > +++ b/src/cam/capture_script.cpp
> > @@ -15,7 +15,7 @@ using namespace libcamera;
> >  
> >  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> >  			     const std::string &fileName)
> > -	: camera_(camera), valid_(false)
> > +	: camera_(camera), loop_(0), valid_(false)
> >  {
> >  	FILE *fh = fopen(fileName.c_str(), "r");
> >  	if (!fh) {
> > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> >  const ControlList &CaptureScript::frameControls(unsigned int frame)
> >  {
> >  	static ControlList controls{};
> > +	unsigned int idx = frame;
> >  
> > -	auto it = frameControls_.find(frame);
> > +	/* If we loop, repeat the controls every 'loop_' frames. */
> > +	if (loop_)
> > +		idx = frame % loop_;
> > +
> > +	auto it = frameControls_.find(idx);
> >  	if (it == frameControls_.end())
> >  		return controls;
> >  
> > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
> >  
> >  		std::string section = eventScalarValue(event);
> >  
> > -		if (section == "frames") {
> > +		if (section == "properties") {
> > +			ret = parseProperties();
> > +			if (ret)
> > +				return ret;
> > +		} else if (section == "frames") {
> >  			ret = parseFrames();
> >  			if (ret)
> >  				return ret;
> > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
> >  	}
> >  }
> >  
> > +int CaptureScript::parseProperty()
> > +{
> > +	EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	std::string prop = parseScalar();
> > +	if (prop.empty())
> > +		return -EINVAL;
> > +
> > +	if (prop == "loop") {
> > +		event = nextEvent();
> > +		if (!event)
> > +			return -EINVAL;
> > +
> > +		std::string value = eventScalarValue(event);
> > +		if (value.empty())
> > +			return -EINVAL;
> > +
> > +		loop_ = atoi(value.c_str());
> > +		if (!loop_) {
> > +			std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		std::cerr << "Unsupported property: " << prop << std::endl;

		std::cerr << "Unsupported property `" << prop << "'" << std::endl;

Use a colon in a message if you introduce a sentence that explains or
complements the previous one, not when printing the value of a variable.
Compare the following two messages, assuming that someone would use a
YAML file that contains

- properties:
  - "file a bug report on bugs.libcamera.org": 0

First message;

Unsupported property: file a bug report on bugs.libcamera.org

Second message:

Unsuported property `file a bug report on bugs.libcamera.org'

Same for the loop limit.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +		return -EINVAL;
> > +	}
> > +
> > +	event = nextEvent(YAML_MAPPING_END_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +int CaptureScript::parseProperties()
> > +{
> > +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	while (1) {
> > +		if (event->type == YAML_SEQUENCE_END_EVENT)
> > +			return 0;
> > +
> > +		int ret = parseProperty();
> > +		if (ret)
> > +			return ret;
> > +
> > +		event = nextEvent();
> > +		if (!event)
> > +			return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int CaptureScript::parseFrames()
> >  {
> >  	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
> >  		return -EINVAL;
> >  
> >  	unsigned int frameId = atoi(key.c_str());
> > +	if (loop_ && frameId >= loop_) {
> > +		std::cerr
> > +			<< "Frame id (" << frameId << ") shall be smaller than"
> > +			<< "loop limit (" << loop_ << ")" << std::endl;
> > +		return -EINVAL;
> > +	}
> >  
> >  	event = nextEvent(YAML_MAPPING_START_EVENT);
> >  	if (!event)
> > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > index fffe67e5a3df..7a0ddebb00b5 100644
> > --- a/src/cam/capture_script.h
> > +++ b/src/cam/capture_script.h
> > @@ -40,6 +40,7 @@ private:
> >  	std::map<unsigned int, libcamera::ControlList> frameControls_;
> >  	std::shared_ptr<libcamera::Camera> camera_;
> >  	yaml_parser_t parser_;
> > +	unsigned int loop_;
> >  	bool valid_;
> >  
> >  	EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > @@ -49,6 +50,8 @@ private:
> >  
> >  	int parseScript(FILE *script);
> >  
> > +	int parseProperties();
> > +	int parseProperty();
> >  	int parseFrames();
> >  	int parseFrame(EventPtr event);
> >  	int parseControl(EventPtr event, libcamera::ControlList &controls);

On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:
> On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:
> > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > Add to the capture script support for properties that control the script
> 
> "Add support to the capture script for properties" would be clearer.
> 
> > > execution. Script properties are specified in a 'properties' section
> > > before the actual list of controls specified in the 'frames' section.
> > > 
> > > Define a first 'loop' property that allows to repeat the frame list
> > 
> > s/to repeat/repeating/
> > 
> > > periodically. All the frame ids in the 'frames' section shall be smaller
> > > than the loop control.
> > > 
> > > Modify the capture script example to show usage of the 'loop' property
> > > and better document the frames list while at it.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/cam/capture-script.yaml | 50 +++++++++++------------
> > >  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
> > >  src/cam/capture_script.h    |  3 ++
> > >  3 files changed, 102 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > > index 6a749bc60cf7..dbea4a9f01a7 100644
> > > --- a/src/cam/capture-script.yaml
> > > +++ b/src/cam/capture-script.yaml
> > > @@ -5,6 +5,20 @@
> > >  # A capture script allows to associate a list of controls and their values
> > >  # to frame numbers.
> > >  
> > > +# The script allows to define a list of frames associated with controls and
> > 
> > s/to define/defining/
> > 
> > > +# an optional list of properties that controls the script behaviour
> 
> s/controls/control/

Grammar police here: "controls" is actually correct because it's a "list
that controls", and not "properties that control". "List" is the
subject, "of properties" is the modifier.

> 
> > > +#
> > > +# properties:
> > > +#   - loop: idx
> > > +#     Repeat the controls every 'idx' frames.
> 
> This is confusing, it looks like the comment is part of the YAML file.
> I'd write
> 
> # properties:
> #   # Repeat the controls every 'idx' frames.
> #   - loop: idx
> 
> > > +#
> > > +#  frames:
> > > +#    - frameid:
> 
> I'd write "frame-number" instead of "frameid" to emphasize it's a frame
> number, not just any identifier.
> 
> > > +#        Control1: value1
> > > +#        Control2: value2
> > > +#
> > > +#    List of frame ids with associated a list of controls to be applied
> 
> Same here.
> 
> > > +#
> > >  # \todo Formally define the capture script structure with a schema
> > >  
> > >  # Notes:
> > > @@ -12,35 +26,17 @@
> > >  #   libcamera::controls:: enumeration
> > >  # - Controls not supported by the camera currently operated are ignored
> > >  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> > > +#   strictly minor than the loop control
> > 
> > s/minor/less than/
> 
> Or "smaller than" ?

Depends on if we want to sound mathematical. "<" is strongly associated
with the phrase "less than", which is what I parsed here because we're
talking about "frame number < loop control", but also I'm more inclined
to use mathy language in the first place :p

> 
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > >  
> > > -# Example:
> > > -frames:
> > > -  - 1:
> > > -      Brightness: 0.0
> > > +# Example: Turn brightness up and down every 50 frames
> > >  
> > > -  - 40:
> > > -      Brightness: 0.2
> > > +properties:
> > > +  - loop: 50
> > >  
> > > -  - 80:
> > > -      Brightness: 0.4
> > > -
> > > -  - 120:
> > > -      Brightness: 0.8
> > > -
> > > -  - 160:
> > > -      Brightness: 0.4
> > > -
> > > -  - 200:
> > > -      Brightness: 0.2
> > > -
> > > -  - 240:
> > > +frames:
> > > +  - 0:
> 
> I like starting at 0 instead of 1.
> 
> > >        Brightness: 0.0
> > >  
> > > -  - 280:
> > > -      Brightness: -0.2
> > > -
> > > -  - 300:
> > > -      Brightness: -0.4
> > > -
> > > -  - 340:
> > > -      Brightness: -0.8
> > > +  - 25:
> > > +      Brightness: 0.8
> 
> It's nice to have more than just two frames in the example script (I
> regularly use it for testing :-)). Could you instead extend it to
> continue with
> 
> - 380:
>     Brightness: -0.4
> - 380:
>     Brightness: -0.2
> 
> and loop at 420 ?
> 
> > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > > index 5e85b3ca604c..52bf19961c17 100644
> > > --- a/src/cam/capture_script.cpp
> > > +++ b/src/cam/capture_script.cpp
> > > @@ -15,7 +15,7 @@ using namespace libcamera;
> > >  
> > >  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > >  			     const std::string &fileName)
> > > -	: camera_(camera), valid_(false)
> > > +	: camera_(camera), loop_(0), valid_(false)
> > >  {
> > >  	FILE *fh = fopen(fileName.c_str(), "r");
> > >  	if (!fh) {
> > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > >  const ControlList &CaptureScript::frameControls(unsigned int frame)
> > >  {
> > >  	static ControlList controls{};
> > > +	unsigned int idx = frame;
> > >  
> > > -	auto it = frameControls_.find(frame);
> > > +	/* If we loop, repeat the controls every 'loop_' frames. */
> > > +	if (loop_)
> > > +		idx = frame % loop_;
> > > +
> > > +	auto it = frameControls_.find(idx);
> > >  	if (it == frameControls_.end())
> > >  		return controls;
> > >  
> > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
> > >  
> > >  		std::string section = eventScalarValue(event);
> > >  
> > > -		if (section == "frames") {
> > > +		if (section == "properties") {
> > > +			ret = parseProperties();
> > > +			if (ret)
> > > +				return ret;
> > > +		} else if (section == "frames") {
> > >  			ret = parseFrames();
> > >  			if (ret)
> > >  				return ret;
> > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
> > >  	}
> > >  }
> > >  
> > > +int CaptureScript::parseProperty()
> > > +{
> > > +	EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > > +	if (!event)
> > > +		return -EINVAL;
> > > +
> > > +	std::string prop = parseScalar();
> > > +	if (prop.empty())
> > > +		return -EINVAL;
> > > +
> > > +	if (prop == "loop") {
> > > +		event = nextEvent();
> > > +		if (!event)
> > > +			return -EINVAL;
> > > +
> > > +		std::string value = eventScalarValue(event);
> > > +		if (value.empty())
> > > +			return -EINVAL;
> > > +
> > > +		loop_ = atoi(value.c_str());
> > > +		if (!loop_) {
> > > +			std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> > > +			return -EINVAL;
> > > +		}
> > > +	} else {
> > > +		std::cerr << "Unsupported property: " << prop << std::endl;
> 
> 		std::cerr << "Unsupported property `" << prop << "'" << std::endl;
> 
> Use a colon in a message if you introduce a sentence that explains or
> complements the previous one, not when printing the value of a variable.
> Compare the following two messages, assuming that someone would use a
> YAML file that contains
> 
> - properties:
>   - "file a bug report on bugs.libcamera.org": 0
> 
> First message;
> 
> Unsupported property: file a bug report on bugs.libcamera.org

lol

But it's an example like this that very clearly illustrates the point.


Paul

> 
> Second message:
> 
> Unsuported property `file a bug report on bugs.libcamera.org'
> 
> Same for the loop limit.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	event = nextEvent(YAML_MAPPING_END_EVENT);
> > > +	if (!event)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int CaptureScript::parseProperties()
> > > +{
> > > +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > +	if (!event)
> > > +		return -EINVAL;
> > > +
> > > +	while (1) {
> > > +		if (event->type == YAML_SEQUENCE_END_EVENT)
> > > +			return 0;
> > > +
> > > +		int ret = parseProperty();
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		event = nextEvent();
> > > +		if (!event)
> > > +			return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int CaptureScript::parseFrames()
> > >  {
> > >  	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
> > >  		return -EINVAL;
> > >  
> > >  	unsigned int frameId = atoi(key.c_str());
> > > +	if (loop_ && frameId >= loop_) {
> > > +		std::cerr
> > > +			<< "Frame id (" << frameId << ") shall be smaller than"
> > > +			<< "loop limit (" << loop_ << ")" << std::endl;
> > > +		return -EINVAL;
> > > +	}
> > >  
> > >  	event = nextEvent(YAML_MAPPING_START_EVENT);
> > >  	if (!event)
> > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > > index fffe67e5a3df..7a0ddebb00b5 100644
> > > --- a/src/cam/capture_script.h
> > > +++ b/src/cam/capture_script.h
> > > @@ -40,6 +40,7 @@ private:
> > >  	std::map<unsigned int, libcamera::ControlList> frameControls_;
> > >  	std::shared_ptr<libcamera::Camera> camera_;
> > >  	yaml_parser_t parser_;
> > > +	unsigned int loop_;
> > >  	bool valid_;
> > >  
> > >  	EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > > @@ -49,6 +50,8 @@ private:
> > >  
> > >  	int parseScript(FILE *script);
> > >  
> > > +	int parseProperties();
> > > +	int parseProperty();
> > >  	int parseFrames();
> > >  	int parseFrame(EventPtr event);
> > >  	int parseControl(EventPtr event, libcamera::ControlList &controls);

On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:
> On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:
> > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > Add to the capture script support for properties that control the script
> > 
> > "Add support to the capture script for properties" would be clearer.
> > 
> > > > execution. Script properties are specified in a 'properties' section
> > > > before the actual list of controls specified in the 'frames' section.
> > > > 
> > > > Define a first 'loop' property that allows to repeat the frame list
> > > 
> > > s/to repeat/repeating/
> > > 
> > > > periodically. All the frame ids in the 'frames' section shall be smaller
> > > > than the loop control.
> > > > 
> > > > Modify the capture script example to show usage of the 'loop' property
> > > > and better document the frames list while at it.
> > > > 
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  src/cam/capture-script.yaml | 50 +++++++++++------------
> > > >  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
> > > >  src/cam/capture_script.h    |  3 ++
> > > >  3 files changed, 102 insertions(+), 30 deletions(-)
> > > > 
> > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > > > index 6a749bc60cf7..dbea4a9f01a7 100644
> > > > --- a/src/cam/capture-script.yaml
> > > > +++ b/src/cam/capture-script.yaml
> > > > @@ -5,6 +5,20 @@
> > > >  # A capture script allows to associate a list of controls and their values
> > > >  # to frame numbers.
> > > >  
> > > > +# The script allows to define a list of frames associated with controls and
> > > 
> > > s/to define/defining/
> > > 
> > > > +# an optional list of properties that controls the script behaviour
> > 
> > s/controls/control/
> 
> Grammar police here: "controls" is actually correct because it's a "list
> that controls", and not "properties that control". "List" is the
> subject, "of properties" is the modifier.

I was thinking that it was the properties that controlled the behaviour,
not the list :-) I'm fine either way.

> > > > +#
> > > > +# properties:
> > > > +#   - loop: idx
> > > > +#     Repeat the controls every 'idx' frames.
> > 
> > This is confusing, it looks like the comment is part of the YAML file.
> > I'd write
> > 
> > # properties:
> > #   # Repeat the controls every 'idx' frames.
> > #   - loop: idx
> > 
> > > > +#
> > > > +#  frames:
> > > > +#    - frameid:
> > 
> > I'd write "frame-number" instead of "frameid" to emphasize it's a frame
> > number, not just any identifier.
> > 
> > > > +#        Control1: value1
> > > > +#        Control2: value2
> > > > +#
> > > > +#    List of frame ids with associated a list of controls to be applied
> > 
> > Same here.
> > 
> > > > +#
> > > >  # \todo Formally define the capture script structure with a schema
> > > >  
> > > >  # Notes:
> > > > @@ -12,35 +26,17 @@
> > > >  #   libcamera::controls:: enumeration
> > > >  # - Controls not supported by the camera currently operated are ignored
> > > >  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> > > > +#   strictly minor than the loop control
> > > 
> > > s/minor/less than/
> > 
> > Or "smaller than" ?
> 
> Depends on if we want to sound mathematical. "<" is strongly associated
> with the phrase "less than", which is what I parsed here because we're
> talking about "frame number < loop control", but also I'm more inclined
> to use mathy language in the first place :p
> 
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > >  
> > > > -# Example:
> > > > -frames:
> > > > -  - 1:
> > > > -      Brightness: 0.0
> > > > +# Example: Turn brightness up and down every 50 frames
> > > >  
> > > > -  - 40:
> > > > -      Brightness: 0.2
> > > > +properties:
> > > > +  - loop: 50
> > > >  
> > > > -  - 80:
> > > > -      Brightness: 0.4
> > > > -
> > > > -  - 120:
> > > > -      Brightness: 0.8
> > > > -
> > > > -  - 160:
> > > > -      Brightness: 0.4
> > > > -
> > > > -  - 200:
> > > > -      Brightness: 0.2
> > > > -
> > > > -  - 240:
> > > > +frames:
> > > > +  - 0:
> > 
> > I like starting at 0 instead of 1.
> > 
> > > >        Brightness: 0.0
> > > >  
> > > > -  - 280:
> > > > -      Brightness: -0.2
> > > > -
> > > > -  - 300:
> > > > -      Brightness: -0.4
> > > > -
> > > > -  - 340:
> > > > -      Brightness: -0.8
> > > > +  - 25:
> > > > +      Brightness: 0.8
> > 
> > It's nice to have more than just two frames in the example script (I
> > regularly use it for testing :-)). Could you instead extend it to
> > continue with
> > 
> > - 380:
> >     Brightness: -0.4
> > - 380:
> >     Brightness: -0.2
> > 
> > and loop at 420 ?
> > 
> > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > > > index 5e85b3ca604c..52bf19961c17 100644
> > > > --- a/src/cam/capture_script.cpp
> > > > +++ b/src/cam/capture_script.cpp
> > > > @@ -15,7 +15,7 @@ using namespace libcamera;
> > > >  
> > > >  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > >  			     const std::string &fileName)
> > > > -	: camera_(camera), valid_(false)
> > > > +	: camera_(camera), loop_(0), valid_(false)
> > > >  {
> > > >  	FILE *fh = fopen(fileName.c_str(), "r");
> > > >  	if (!fh) {
> > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > >  const ControlList &CaptureScript::frameControls(unsigned int frame)
> > > >  {
> > > >  	static ControlList controls{};
> > > > +	unsigned int idx = frame;
> > > >  
> > > > -	auto it = frameControls_.find(frame);
> > > > +	/* If we loop, repeat the controls every 'loop_' frames. */
> > > > +	if (loop_)
> > > > +		idx = frame % loop_;
> > > > +
> > > > +	auto it = frameControls_.find(idx);
> > > >  	if (it == frameControls_.end())
> > > >  		return controls;
> > > >  
> > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
> > > >  
> > > >  		std::string section = eventScalarValue(event);
> > > >  
> > > > -		if (section == "frames") {
> > > > +		if (section == "properties") {
> > > > +			ret = parseProperties();
> > > > +			if (ret)
> > > > +				return ret;
> > > > +		} else if (section == "frames") {
> > > >  			ret = parseFrames();
> > > >  			if (ret)
> > > >  				return ret;
> > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
> > > >  	}
> > > >  }
> > > >  
> > > > +int CaptureScript::parseProperty()
> > > > +{
> > > > +	EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > +	if (!event)
> > > > +		return -EINVAL;
> > > > +
> > > > +	std::string prop = parseScalar();
> > > > +	if (prop.empty())
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (prop == "loop") {
> > > > +		event = nextEvent();
> > > > +		if (!event)
> > > > +			return -EINVAL;
> > > > +
> > > > +		std::string value = eventScalarValue(event);
> > > > +		if (value.empty())
> > > > +			return -EINVAL;
> > > > +
> > > > +		loop_ = atoi(value.c_str());
> > > > +		if (!loop_) {
> > > > +			std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> > > > +			return -EINVAL;
> > > > +		}
> > > > +	} else {
> > > > +		std::cerr << "Unsupported property: " << prop << std::endl;
> > 
> > 		std::cerr << "Unsupported property `" << prop << "'" << std::endl;
> > 
> > Use a colon in a message if you introduce a sentence that explains or
> > complements the previous one, not when printing the value of a variable.
> > Compare the following two messages, assuming that someone would use a
> > YAML file that contains
> > 
> > - properties:
> >   - "file a bug report on bugs.libcamera.org": 0
> > 
> > First message;
> > 
> > Unsupported property: file a bug report on bugs.libcamera.org
> 
> lol
> 
> But it's an example like this that very clearly illustrates the point.
> 
> 
> Paul
> 
> > Second message:
> > 
> > Unsuported property `file a bug report on bugs.libcamera.org'
> > 
> > Same for the loop limit.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	event = nextEvent(YAML_MAPPING_END_EVENT);
> > > > +	if (!event)
> > > > +		return -EINVAL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int CaptureScript::parseProperties()
> > > > +{
> > > > +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > +	if (!event)
> > > > +		return -EINVAL;
> > > > +
> > > > +	while (1) {
> > > > +		if (event->type == YAML_SEQUENCE_END_EVENT)
> > > > +			return 0;
> > > > +
> > > > +		int ret = parseProperty();
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		event = nextEvent();
> > > > +		if (!event)
> > > > +			return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  int CaptureScript::parseFrames()
> > > >  {
> > > >  	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
> > > >  		return -EINVAL;
> > > >  
> > > >  	unsigned int frameId = atoi(key.c_str());
> > > > +	if (loop_ && frameId >= loop_) {
> > > > +		std::cerr
> > > > +			<< "Frame id (" << frameId << ") shall be smaller than"
> > > > +			<< "loop limit (" << loop_ << ")" << std::endl;
> > > > +		return -EINVAL;
> > > > +	}
> > > >  
> > > >  	event = nextEvent(YAML_MAPPING_START_EVENT);
> > > >  	if (!event)
> > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > > > index fffe67e5a3df..7a0ddebb00b5 100644
> > > > --- a/src/cam/capture_script.h
> > > > +++ b/src/cam/capture_script.h
> > > > @@ -40,6 +40,7 @@ private:
> > > >  	std::map<unsigned int, libcamera::ControlList> frameControls_;
> > > >  	std::shared_ptr<libcamera::Camera> camera_;
> > > >  	yaml_parser_t parser_;
> > > > +	unsigned int loop_;
> > > >  	bool valid_;
> > > >  
> > > >  	EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > > > @@ -49,6 +50,8 @@ private:
> > > >  
> > > >  	int parseScript(FILE *script);
> > > >  
> > > > +	int parseProperties();
> > > > +	int parseProperty();
> > > >  	int parseFrames();
> > > >  	int parseFrame(EventPtr event);
> > > >  	int parseControl(EventPtr event, libcamera::ControlList &controls);

On Sat, Sep 03, 2022 at 04:04:12AM +0300, Laurent Pinchart wrote:
> On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:
> > On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:
> > > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:
> > > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > > Add to the capture script support for properties that control the script
> > > 
> > > "Add support to the capture script for properties" would be clearer.
> > > 
> > > > > execution. Script properties are specified in a 'properties' section
> > > > > before the actual list of controls specified in the 'frames' section.
> > > > > 
> > > > > Define a first 'loop' property that allows to repeat the frame list
> > > > 
> > > > s/to repeat/repeating/
> > > > 
> > > > > periodically. All the frame ids in the 'frames' section shall be smaller
> > > > > than the loop control.
> > > > > 
> > > > > Modify the capture script example to show usage of the 'loop' property
> > > > > and better document the frames list while at it.
> > > > > 
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > >  src/cam/capture-script.yaml | 50 +++++++++++------------
> > > > >  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
> > > > >  src/cam/capture_script.h    |  3 ++
> > > > >  3 files changed, 102 insertions(+), 30 deletions(-)
> > > > > 
> > > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > > > > index 6a749bc60cf7..dbea4a9f01a7 100644
> > > > > --- a/src/cam/capture-script.yaml
> > > > > +++ b/src/cam/capture-script.yaml
> > > > > @@ -5,6 +5,20 @@
> > > > >  # A capture script allows to associate a list of controls and their values
> > > > >  # to frame numbers.
> > > > >  
> > > > > +# The script allows to define a list of frames associated with controls and
> > > > 
> > > > s/to define/defining/
> > > > 
> > > > > +# an optional list of properties that controls the script behaviour
> > > 
> > > s/controls/control/
> > 
> > Grammar police here: "controls" is actually correct because it's a "list
> > that controls", and not "properties that control". "List" is the
> > subject, "of properties" is the modifier.
> 
> I was thinking that it was the properties that controlled the behaviour,
> not the list :-) I'm fine either way.

Huh, you have a point. I guess that makes sense semantically then (as
the properties are the ones that do the controlling but they are
organized in a list), but it's not correct syntactically (because the
properties are gramatically downgraded to a modifier).

But no matter how I imagine it, "controls" sounds more correct, since
"list" is the subject :/

English is weird.


Paul

> 
> > > > > +#
> > > > > +# properties:
> > > > > +#   - loop: idx
> > > > > +#     Repeat the controls every 'idx' frames.
> > > 
> > > This is confusing, it looks like the comment is part of the YAML file.
> > > I'd write
> > > 
> > > # properties:
> > > #   # Repeat the controls every 'idx' frames.
> > > #   - loop: idx
> > > 
> > > > > +#
> > > > > +#  frames:
> > > > > +#    - frameid:
> > > 
> > > I'd write "frame-number" instead of "frameid" to emphasize it's a frame
> > > number, not just any identifier.
> > > 
> > > > > +#        Control1: value1
> > > > > +#        Control2: value2
> > > > > +#
> > > > > +#    List of frame ids with associated a list of controls to be applied
> > > 
> > > Same here.
> > > 
> > > > > +#
> > > > >  # \todo Formally define the capture script structure with a schema
> > > > >  
> > > > >  # Notes:
> > > > > @@ -12,35 +26,17 @@
> > > > >  #   libcamera::controls:: enumeration
> > > > >  # - Controls not supported by the camera currently operated are ignored
> > > > >  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> > > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> > > > > +#   strictly minor than the loop control
> > > > 
> > > > s/minor/less than/
> > > 
> > > Or "smaller than" ?
> > 
> > Depends on if we want to sound mathematical. "<" is strongly associated
> > with the phrase "less than", which is what I parsed here because we're
> > talking about "frame number < loop control", but also I'm more inclined
> > to use mathy language in the first place :p
> > 
> > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > 
> > > > >  
> > > > > -# Example:
> > > > > -frames:
> > > > > -  - 1:
> > > > > -      Brightness: 0.0
> > > > > +# Example: Turn brightness up and down every 50 frames
> > > > >  
> > > > > -  - 40:
> > > > > -      Brightness: 0.2
> > > > > +properties:
> > > > > +  - loop: 50
> > > > >  
> > > > > -  - 80:
> > > > > -      Brightness: 0.4
> > > > > -
> > > > > -  - 120:
> > > > > -      Brightness: 0.8
> > > > > -
> > > > > -  - 160:
> > > > > -      Brightness: 0.4
> > > > > -
> > > > > -  - 200:
> > > > > -      Brightness: 0.2
> > > > > -
> > > > > -  - 240:
> > > > > +frames:
> > > > > +  - 0:
> > > 
> > > I like starting at 0 instead of 1.
> > > 
> > > > >        Brightness: 0.0
> > > > >  
> > > > > -  - 280:
> > > > > -      Brightness: -0.2
> > > > > -
> > > > > -  - 300:
> > > > > -      Brightness: -0.4
> > > > > -
> > > > > -  - 340:
> > > > > -      Brightness: -0.8
> > > > > +  - 25:
> > > > > +      Brightness: 0.8
> > > 
> > > It's nice to have more than just two frames in the example script (I
> > > regularly use it for testing :-)). Could you instead extend it to
> > > continue with
> > > 
> > > - 380:
> > >     Brightness: -0.4
> > > - 380:
> > >     Brightness: -0.2
> > > 
> > > and loop at 420 ?
> > > 
> > > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > > > > index 5e85b3ca604c..52bf19961c17 100644
> > > > > --- a/src/cam/capture_script.cpp
> > > > > +++ b/src/cam/capture_script.cpp
> > > > > @@ -15,7 +15,7 @@ using namespace libcamera;
> > > > >  
> > > > >  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > > >  			     const std::string &fileName)
> > > > > -	: camera_(camera), valid_(false)
> > > > > +	: camera_(camera), loop_(0), valid_(false)
> > > > >  {
> > > > >  	FILE *fh = fopen(fileName.c_str(), "r");
> > > > >  	if (!fh) {
> > > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > > >  const ControlList &CaptureScript::frameControls(unsigned int frame)
> > > > >  {
> > > > >  	static ControlList controls{};
> > > > > +	unsigned int idx = frame;
> > > > >  
> > > > > -	auto it = frameControls_.find(frame);
> > > > > +	/* If we loop, repeat the controls every 'loop_' frames. */
> > > > > +	if (loop_)
> > > > > +		idx = frame % loop_;
> > > > > +
> > > > > +	auto it = frameControls_.find(idx);
> > > > >  	if (it == frameControls_.end())
> > > > >  		return controls;
> > > > >  
> > > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
> > > > >  
> > > > >  		std::string section = eventScalarValue(event);
> > > > >  
> > > > > -		if (section == "frames") {
> > > > > +		if (section == "properties") {
> > > > > +			ret = parseProperties();
> > > > > +			if (ret)
> > > > > +				return ret;
> > > > > +		} else if (section == "frames") {
> > > > >  			ret = parseFrames();
> > > > >  			if (ret)
> > > > >  				return ret;
> > > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +int CaptureScript::parseProperty()
> > > > > +{
> > > > > +	EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > > +	if (!event)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	std::string prop = parseScalar();
> > > > > +	if (prop.empty())
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (prop == "loop") {
> > > > > +		event = nextEvent();
> > > > > +		if (!event)
> > > > > +			return -EINVAL;
> > > > > +
> > > > > +		std::string value = eventScalarValue(event);
> > > > > +		if (value.empty())
> > > > > +			return -EINVAL;
> > > > > +
> > > > > +		loop_ = atoi(value.c_str());
> > > > > +		if (!loop_) {
> > > > > +			std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +	} else {
> > > > > +		std::cerr << "Unsupported property: " << prop << std::endl;
> > > 
> > > 		std::cerr << "Unsupported property `" << prop << "'" << std::endl;
> > > 
> > > Use a colon in a message if you introduce a sentence that explains or
> > > complements the previous one, not when printing the value of a variable.
> > > Compare the following two messages, assuming that someone would use a
> > > YAML file that contains
> > > 
> > > - properties:
> > >   - "file a bug report on bugs.libcamera.org": 0
> > > 
> > > First message;
> > > 
> > > Unsupported property: file a bug report on bugs.libcamera.org
> > 
> > lol
> > 
> > But it's an example like this that very clearly illustrates the point.
> > 
> > 
> > Paul
> > 
> > > Second message:
> > > 
> > > Unsuported property `file a bug report on bugs.libcamera.org'
> > > 
> > > Same for the loop limit.
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	event = nextEvent(YAML_MAPPING_END_EVENT);
> > > > > +	if (!event)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +int CaptureScript::parseProperties()
> > > > > +{
> > > > > +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > > +	if (!event)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	while (1) {
> > > > > +		if (event->type == YAML_SEQUENCE_END_EVENT)
> > > > > +			return 0;
> > > > > +
> > > > > +		int ret = parseProperty();
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +
> > > > > +		event = nextEvent();
> > > > > +		if (!event)
> > > > > +			return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  int CaptureScript::parseFrames()
> > > > >  {
> > > > >  	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
> > > > >  		return -EINVAL;
> > > > >  
> > > > >  	unsigned int frameId = atoi(key.c_str());
> > > > > +	if (loop_ && frameId >= loop_) {
> > > > > +		std::cerr
> > > > > +			<< "Frame id (" << frameId << ") shall be smaller than"
> > > > > +			<< "loop limit (" << loop_ << ")" << std::endl;
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > >  
> > > > >  	event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > >  	if (!event)
> > > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > > > > index fffe67e5a3df..7a0ddebb00b5 100644
> > > > > --- a/src/cam/capture_script.h
> > > > > +++ b/src/cam/capture_script.h
> > > > > @@ -40,6 +40,7 @@ private:
> > > > >  	std::map<unsigned int, libcamera::ControlList> frameControls_;
> > > > >  	std::shared_ptr<libcamera::Camera> camera_;
> > > > >  	yaml_parser_t parser_;
> > > > > +	unsigned int loop_;
> > > > >  	bool valid_;
> > > > >  
> > > > >  	EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > > > > @@ -49,6 +50,8 @@ private:
> > > > >  
> > > > >  	int parseScript(FILE *script);
> > > > >  
> > > > > +	int parseProperties();
> > > > > +	int parseProperty();
> > > > >  	int parseFrames();
> > > > >  	int parseFrame(EventPtr event);
> > > > >  	int parseControl(EventPtr event, libcamera::ControlList &controls);

Quoting Paul Elder via libcamera-devel (2022-09-03 02:16:49)
> On Sat, Sep 03, 2022 at 04:04:12AM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:
> > > On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:
> > > > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > > > Add to the capture script support for properties that control the script
> > > > 
> > > > "Add support to the capture script for properties" would be clearer.
> > > > 
> > > > > > execution. Script properties are specified in a 'properties' section
> > > > > > before the actual list of controls specified in the 'frames' section.
> > > > > > 
> > > > > > Define a first 'loop' property that allows to repeat the frame list
> > > > > 
> > > > > s/to repeat/repeating/
> > > > > 
> > > > > > periodically. All the frame ids in the 'frames' section shall be smaller
> > > > > > than the loop control.
> > > > > > 
> > > > > > Modify the capture script example to show usage of the 'loop' property
> > > > > > and better document the frames list while at it.
> > > > > > 
> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > ---
> > > > > >  src/cam/capture-script.yaml | 50 +++++++++++------------
> > > > > >  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
> > > > > >  src/cam/capture_script.h    |  3 ++
> > > > > >  3 files changed, 102 insertions(+), 30 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > > > > > index 6a749bc60cf7..dbea4a9f01a7 100644
> > > > > > --- a/src/cam/capture-script.yaml
> > > > > > +++ b/src/cam/capture-script.yaml
> > > > > > @@ -5,6 +5,20 @@
> > > > > >  # A capture script allows to associate a list of controls and their values
> > > > > >  # to frame numbers.
> > > > > >  
> > > > > > +# The script allows to define a list of frames associated with controls and
> > > > > 
> > > > > s/to define/defining/
> > > > > 
> > > > > > +# an optional list of properties that controls the script behaviour
> > > > 
> > > > s/controls/control/
> > > 
> > > Grammar police here: "controls" is actually correct because it's a "list
> > > that controls", and not "properties that control". "List" is the
> > > subject, "of properties" is the modifier.
> > 
> > I was thinking that it was the properties that controlled the behaviour,
> > not the list :-) I'm fine either way.
> 
> Huh, you have a point. I guess that makes sense semantically then (as
> the properties are the ones that do the controlling but they are
> organized in a list), but it's not correct syntactically (because the
> properties are gramatically downgraded to a modifier).
> 
> But no matter how I imagine it, "controls" sounds more correct, since
> "list" is the subject :/
> 
> English is weird.

Definitely, I'd fix this with:

  The script allows defining a list of frames associated with controls
  and an optional list of properties that can control the script
  behaviour.

To me it's either "that controls" or "that can control"
--
Kieran


> 
> 
> Paul
> 
> > 
> > > > > > +#
> > > > > > +# properties:
> > > > > > +#   - loop: idx
> > > > > > +#     Repeat the controls every 'idx' frames.
> > > > 
> > > > This is confusing, it looks like the comment is part of the YAML file.
> > > > I'd write
> > > > 
> > > > # properties:
> > > > #   # Repeat the controls every 'idx' frames.
> > > > #   - loop: idx
> > > > 
> > > > > > +#
> > > > > > +#  frames:
> > > > > > +#    - frameid:
> > > > 
> > > > I'd write "frame-number" instead of "frameid" to emphasize it's a frame
> > > > number, not just any identifier.
> > > > 
> > > > > > +#        Control1: value1
> > > > > > +#        Control2: value2
> > > > > > +#
> > > > > > +#    List of frame ids with associated a list of controls to be applied
> > > > 
> > > > Same here.
> > > > 
> > > > > > +#
> > > > > >  # \todo Formally define the capture script structure with a schema
> > > > > >  
> > > > > >  # Notes:
> > > > > > @@ -12,35 +26,17 @@
> > > > > >  #   libcamera::controls:: enumeration
> > > > > >  # - Controls not supported by the camera currently operated are ignored
> > > > > >  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> > > > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> > > > > > +#   strictly minor than the loop control
> > > > > 
> > > > > s/minor/less than/
> > > > 
> > > > Or "smaller than" ?
> > > 
> > > Depends on if we want to sound mathematical. "<" is strongly associated
> > > with the phrase "less than", which is what I parsed here because we're
> > > talking about "frame number < loop control", but also I'm more inclined
> > > to use mathy language in the first place :p
> > > 
> > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > 
> > > > > >  
> > > > > > -# Example:
> > > > > > -frames:
> > > > > > -  - 1:
> > > > > > -      Brightness: 0.0
> > > > > > +# Example: Turn brightness up and down every 50 frames
> > > > > >  
> > > > > > -  - 40:
> > > > > > -      Brightness: 0.2
> > > > > > +properties:
> > > > > > +  - loop: 50
> > > > > >  
> > > > > > -  - 80:
> > > > > > -      Brightness: 0.4
> > > > > > -
> > > > > > -  - 120:
> > > > > > -      Brightness: 0.8
> > > > > > -
> > > > > > -  - 160:
> > > > > > -      Brightness: 0.4
> > > > > > -
> > > > > > -  - 200:
> > > > > > -      Brightness: 0.2
> > > > > > -
> > > > > > -  - 240:
> > > > > > +frames:
> > > > > > +  - 0:
> > > > 
> > > > I like starting at 0 instead of 1.
> > > > 
> > > > > >        Brightness: 0.0
> > > > > >  
> > > > > > -  - 280:
> > > > > > -      Brightness: -0.2
> > > > > > -
> > > > > > -  - 300:
> > > > > > -      Brightness: -0.4
> > > > > > -
> > > > > > -  - 340:
> > > > > > -      Brightness: -0.8
> > > > > > +  - 25:
> > > > > > +      Brightness: 0.8
> > > > 
> > > > It's nice to have more than just two frames in the example script (I
> > > > regularly use it for testing :-)). Could you instead extend it to
> > > > continue with
> > > > 
> > > > - 380:
> > > >     Brightness: -0.4
> > > > - 380:
> > > >     Brightness: -0.2
> > > > 
> > > > and loop at 420 ?
> > > > 
> > > > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > > > > > index 5e85b3ca604c..52bf19961c17 100644
> > > > > > --- a/src/cam/capture_script.cpp
> > > > > > +++ b/src/cam/capture_script.cpp
> > > > > > @@ -15,7 +15,7 @@ using namespace libcamera;
> > > > > >  
> > > > > >  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > > > >                            const std::string &fileName)
> > > > > > -     : camera_(camera), valid_(false)
> > > > > > +     : camera_(camera), loop_(0), valid_(false)
> > > > > >  {
> > > > > >       FILE *fh = fopen(fileName.c_str(), "r");
> > > > > >       if (!fh) {
> > > > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > > > >  const ControlList &CaptureScript::frameControls(unsigned int frame)
> > > > > >  {
> > > > > >       static ControlList controls{};
> > > > > > +     unsigned int idx = frame;
> > > > > >  
> > > > > > -     auto it = frameControls_.find(frame);
> > > > > > +     /* If we loop, repeat the controls every 'loop_' frames. */
> > > > > > +     if (loop_)
> > > > > > +             idx = frame % loop_;
> > > > > > +
> > > > > > +     auto it = frameControls_.find(idx);
> > > > > >       if (it == frameControls_.end())
> > > > > >               return controls;
> > > > > >  
> > > > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
> > > > > >  
> > > > > >               std::string section = eventScalarValue(event);
> > > > > >  
> > > > > > -             if (section == "frames") {
> > > > > > +             if (section == "properties") {
> > > > > > +                     ret = parseProperties();
> > > > > > +                     if (ret)
> > > > > > +                             return ret;
> > > > > > +             } else if (section == "frames") {
> > > > > >                       ret = parseFrames();
> > > > > >                       if (ret)
> > > > > >                               return ret;
> > > > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
> > > > > >       }
> > > > > >  }
> > > > > >  
> > > > > > +int CaptureScript::parseProperty()
> > > > > > +{
> > > > > > +     EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > > > +     if (!event)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     std::string prop = parseScalar();
> > > > > > +     if (prop.empty())
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     if (prop == "loop") {
> > > > > > +             event = nextEvent();
> > > > > > +             if (!event)
> > > > > > +                     return -EINVAL;
> > > > > > +
> > > > > > +             std::string value = eventScalarValue(event);
> > > > > > +             if (value.empty())
> > > > > > +                     return -EINVAL;
> > > > > > +
> > > > > > +             loop_ = atoi(value.c_str());
> > > > > > +             if (!loop_) {
> > > > > > +                     std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> > > > > > +                     return -EINVAL;
> > > > > > +             }
> > > > > > +     } else {
> > > > > > +             std::cerr << "Unsupported property: " << prop << std::endl;
> > > > 
> > > >           std::cerr << "Unsupported property `" << prop << "'" << std::endl;
> > > > 
> > > > Use a colon in a message if you introduce a sentence that explains or
> > > > complements the previous one, not when printing the value of a variable.
> > > > Compare the following two messages, assuming that someone would use a
> > > > YAML file that contains
> > > > 
> > > > - properties:
> > > >   - "file a bug report on bugs.libcamera.org": 0
> > > > 
> > > > First message;
> > > > 
> > > > Unsupported property: file a bug report on bugs.libcamera.org
> > > 
> > > lol
> > > 
> > > But it's an example like this that very clearly illustrates the point.
> > > 
> > > 
> > > Paul
> > > 
> > > > Second message:
> > > > 
> > > > Unsuported property `file a bug report on bugs.libcamera.org'
> > > > 
> > > > Same for the loop limit.
> > > > 
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > > > +             return -EINVAL;
> > > > > > +     }
> > > > > > +
> > > > > > +     event = nextEvent(YAML_MAPPING_END_EVENT);
> > > > > > +     if (!event)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +int CaptureScript::parseProperties()
> > > > > > +{
> > > > > > +     EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > > > +     if (!event)
> > > > > > +             return -EINVAL;
> > > > > > +
> > > > > > +     while (1) {
> > > > > > +             if (event->type == YAML_SEQUENCE_END_EVENT)
> > > > > > +                     return 0;
> > > > > > +
> > > > > > +             int ret = parseProperty();
> > > > > > +             if (ret)
> > > > > > +                     return ret;
> > > > > > +
> > > > > > +             event = nextEvent();
> > > > > > +             if (!event)
> > > > > > +                     return -EINVAL;
> > > > > > +     }
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > >  int CaptureScript::parseFrames()
> > > > > >  {
> > > > > >       EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
> > > > > >               return -EINVAL;
> > > > > >  
> > > > > >       unsigned int frameId = atoi(key.c_str());
> > > > > > +     if (loop_ && frameId >= loop_) {
> > > > > > +             std::cerr
> > > > > > +                     << "Frame id (" << frameId << ") shall be smaller than"
> > > > > > +                     << "loop limit (" << loop_ << ")" << std::endl;
> > > > > > +             return -EINVAL;
> > > > > > +     }
> > > > > >  
> > > > > >       event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > > >       if (!event)
> > > > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > > > > > index fffe67e5a3df..7a0ddebb00b5 100644
> > > > > > --- a/src/cam/capture_script.h
> > > > > > +++ b/src/cam/capture_script.h
> > > > > > @@ -40,6 +40,7 @@ private:
> > > > > >       std::map<unsigned int, libcamera::ControlList> frameControls_;
> > > > > >       std::shared_ptr<libcamera::Camera> camera_;
> > > > > >       yaml_parser_t parser_;
> > > > > > +     unsigned int loop_;
> > > > > >       bool valid_;
> > > > > >  
> > > > > >       EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > > > > > @@ -49,6 +50,8 @@ private:
> > > > > >  
> > > > > >       int parseScript(FILE *script);
> > > > > >  
> > > > > > +     int parseProperties();
> > > > > > +     int parseProperty();
> > > > > >       int parseFrames();
> > > > > >       int parseFrame(EventPtr event);
> > > > > >       int parseControl(EventPtr event, libcamera::ControlList &controls);

On Fri, Sep 02, 2022 at 09:16:49PM -0400, paul.elder@ideasonboard.com wrote:
> On Sat, Sep 03, 2022 at 04:04:12AM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 02, 2022 at 09:00:24PM -0400, paul.elder@ideasonboard.com wrote:
> > > On Sat, Sep 03, 2022 at 03:44:01AM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 02, 2022 at 08:19:01PM -0400, Paul Elder via libcamera-devel wrote:
> > > > > On Fri, Sep 02, 2022 at 05:00:31PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > > > Add to the capture script support for properties that control the script
> > > > 
> > > > "Add support to the capture script for properties" would be clearer.
> > > > 
> > > > > > execution. Script properties are specified in a 'properties' section
> > > > > > before the actual list of controls specified in the 'frames' section.
> > > > > > 
> > > > > > Define a first 'loop' property that allows to repeat the frame list
> > > > > 
> > > > > s/to repeat/repeating/
> > > > > 
> > > > > > periodically. All the frame ids in the 'frames' section shall be smaller
> > > > > > than the loop control.
> > > > > > 
> > > > > > Modify the capture script example to show usage of the 'loop' property
> > > > > > and better document the frames list while at it.
> > > > > > 
> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > ---
> > > > > >  src/cam/capture-script.yaml | 50 +++++++++++------------
> > > > > >  src/cam/capture_script.cpp  | 79 +++++++++++++++++++++++++++++++++++--
> > > > > >  src/cam/capture_script.h    |  3 ++
> > > > > >  3 files changed, 102 insertions(+), 30 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > > > > > index 6a749bc60cf7..dbea4a9f01a7 100644
> > > > > > --- a/src/cam/capture-script.yaml
> > > > > > +++ b/src/cam/capture-script.yaml
> > > > > > @@ -5,6 +5,20 @@
> > > > > >  # A capture script allows to associate a list of controls and their values
> > > > > >  # to frame numbers.
> > > > > >  
> > > > > > +# The script allows to define a list of frames associated with controls and
> > > > > 
> > > > > s/to define/defining/
> > > > > 
> > > > > > +# an optional list of properties that controls the script behaviour
> > > > 
> > > > s/controls/control/
> > > 
> > > Grammar police here: "controls" is actually correct because it's a "list
> > > that controls", and not "properties that control". "List" is the
> > > subject, "of properties" is the modifier.
> > 
> > I was thinking that it was the properties that controlled the behaviour,
> > not the list :-) I'm fine either way.
> 
> Huh, you have a point. I guess that makes sense semantically then (as
> the properties are the ones that do the controlling but they are
> organized in a list), but it's not correct syntactically (because the
> properties are gramatically downgraded to a modifier).
> 
> But no matter how I imagine it, "controls" sounds more correct, since
> "list" is the subject :/
> 
> English is weird.

We have the exact same thing in French ;-)

> > > > > > +#
> > > > > > +# properties:
> > > > > > +#   - loop: idx
> > > > > > +#     Repeat the controls every 'idx' frames.
> > > > 
> > > > This is confusing, it looks like the comment is part of the YAML file.
> > > > I'd write
> > > > 
> > > > # properties:
> > > > #   # Repeat the controls every 'idx' frames.
> > > > #   - loop: idx
> > > > 
> > > > > > +#
> > > > > > +#  frames:
> > > > > > +#    - frameid:
> > > > 
> > > > I'd write "frame-number" instead of "frameid" to emphasize it's a frame
> > > > number, not just any identifier.
> > > > 
> > > > > > +#        Control1: value1
> > > > > > +#        Control2: value2
> > > > > > +#
> > > > > > +#    List of frame ids with associated a list of controls to be applied
> > > > 
> > > > Same here.
> > > > 
> > > > > > +#
> > > > > >  # \todo Formally define the capture script structure with a schema
> > > > > >  
> > > > > >  # Notes:
> > > > > > @@ -12,35 +26,17 @@
> > > > > >  #   libcamera::controls:: enumeration
> > > > > >  # - Controls not supported by the camera currently operated are ignored
> > > > > >  # - Frame numbers shall be monotonically incrementing, gaps are allowed
> > > > > > +# - If a loop limit is specified, frame numbers in the 'frames' list shall be
> > > > > > +#   strictly minor than the loop control
> > > > > 
> > > > > s/minor/less than/
> > > > 
> > > > Or "smaller than" ?
> > > 
> > > Depends on if we want to sound mathematical. "<" is strongly associated
> > > with the phrase "less than", which is what I parsed here because we're
> > > talking about "frame number < loop control", but also I'm more inclined
> > > to use mathy language in the first place :p
> > > 
> > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > 
> > > > > >  
> > > > > > -# Example:
> > > > > > -frames:
> > > > > > -  - 1:
> > > > > > -      Brightness: 0.0
> > > > > > +# Example: Turn brightness up and down every 50 frames
> > > > > >  
> > > > > > -  - 40:
> > > > > > -      Brightness: 0.2
> > > > > > +properties:
> > > > > > +  - loop: 50
> > > > > >  
> > > > > > -  - 80:
> > > > > > -      Brightness: 0.4
> > > > > > -
> > > > > > -  - 120:
> > > > > > -      Brightness: 0.8
> > > > > > -
> > > > > > -  - 160:
> > > > > > -      Brightness: 0.4
> > > > > > -
> > > > > > -  - 200:
> > > > > > -      Brightness: 0.2
> > > > > > -
> > > > > > -  - 240:
> > > > > > +frames:
> > > > > > +  - 0:
> > > > 
> > > > I like starting at 0 instead of 1.
> > > > 
> > > > > >        Brightness: 0.0
> > > > > >  
> > > > > > -  - 280:
> > > > > > -      Brightness: -0.2
> > > > > > -
> > > > > > -  - 300:
> > > > > > -      Brightness: -0.4
> > > > > > -
> > > > > > -  - 340:
> > > > > > -      Brightness: -0.8
> > > > > > +  - 25:
> > > > > > +      Brightness: 0.8
> > > > 
> > > > It's nice to have more than just two frames in the example script (I
> > > > regularly use it for testing :-)). Could you instead extend it to
> > > > continue with
> > > > 
> > > > - 380:
> > > >     Brightness: -0.4
> > > > - 380:
> > > >     Brightness: -0.2
> > > > 
> > > > and loop at 420 ?
> > > > 
> > > > > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > > > > > index 5e85b3ca604c..52bf19961c17 100644
> > > > > > --- a/src/cam/capture_script.cpp
> > > > > > +++ b/src/cam/capture_script.cpp
> > > > > > @@ -15,7 +15,7 @@ using namespace libcamera;
> > > > > >  
> > > > > >  CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > > > >  			     const std::string &fileName)
> > > > > > -	: camera_(camera), valid_(false)
> > > > > > +	: camera_(camera), loop_(0), valid_(false)
> > > > > >  {
> > > > > >  	FILE *fh = fopen(fileName.c_str(), "r");
> > > > > >  	if (!fh) {
> > > > > > @@ -44,8 +44,13 @@ CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > > > > >  const ControlList &CaptureScript::frameControls(unsigned int frame)
> > > > > >  {
> > > > > >  	static ControlList controls{};
> > > > > > +	unsigned int idx = frame;
> > > > > >  
> > > > > > -	auto it = frameControls_.find(frame);
> > > > > > +	/* If we loop, repeat the controls every 'loop_' frames. */
> > > > > > +	if (loop_)
> > > > > > +		idx = frame % loop_;
> > > > > > +
> > > > > > +	auto it = frameControls_.find(idx);
> > > > > >  	if (it == frameControls_.end())
> > > > > >  		return controls;
> > > > > >  
> > > > > > @@ -149,7 +154,11 @@ int CaptureScript::parseScript(FILE *script)
> > > > > >  
> > > > > >  		std::string section = eventScalarValue(event);
> > > > > >  
> > > > > > -		if (section == "frames") {
> > > > > > +		if (section == "properties") {
> > > > > > +			ret = parseProperties();
> > > > > > +			if (ret)
> > > > > > +				return ret;
> > > > > > +		} else if (section == "frames") {
> > > > > >  			ret = parseFrames();
> > > > > >  			if (ret)
> > > > > >  				return ret;
> > > > > > @@ -161,6 +170,64 @@ int CaptureScript::parseScript(FILE *script)
> > > > > >  	}
> > > > > >  }
> > > > > >  
> > > > > > +int CaptureScript::parseProperty()
> > > > > > +{
> > > > > > +	EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > > > +	if (!event)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	std::string prop = parseScalar();
> > > > > > +	if (prop.empty())
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (prop == "loop") {
> > > > > > +		event = nextEvent();
> > > > > > +		if (!event)
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		std::string value = eventScalarValue(event);
> > > > > > +		if (value.empty())
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		loop_ = atoi(value.c_str());
> > > > > > +		if (!loop_) {
> > > > > > +			std::cerr << "Invalid loop limit: " << loop_ << std::endl;
> > > > > > +			return -EINVAL;
> > > > > > +		}
> > > > > > +	} else {
> > > > > > +		std::cerr << "Unsupported property: " << prop << std::endl;
> > > > 
> > > > 		std::cerr << "Unsupported property `" << prop << "'" << std::endl;
> > > > 
> > > > Use a colon in a message if you introduce a sentence that explains or
> > > > complements the previous one, not when printing the value of a variable.
> > > > Compare the following two messages, assuming that someone would use a
> > > > YAML file that contains
> > > > 
> > > > - properties:
> > > >   - "file a bug report on bugs.libcamera.org": 0
> > > > 
> > > > First message;
> > > > 
> > > > Unsupported property: file a bug report on bugs.libcamera.org
> > > 
> > > lol
> > > 
> > > But it's an example like this that very clearly illustrates the point.
> > > 
> > > 
> > > Paul
> > > 
> > > > Second message:
> > > > 
> > > > Unsuported property `file a bug report on bugs.libcamera.org'
> > > > 
> > > > Same for the loop limit.
> > > > 
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	event = nextEvent(YAML_MAPPING_END_EVENT);
> > > > > > +	if (!event)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +int CaptureScript::parseProperties()
> > > > > > +{
> > > > > > +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > > > +	if (!event)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	while (1) {
> > > > > > +		if (event->type == YAML_SEQUENCE_END_EVENT)
> > > > > > +			return 0;
> > > > > > +
> > > > > > +		int ret = parseProperty();
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +
> > > > > > +		event = nextEvent();
> > > > > > +		if (!event)
> > > > > > +			return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  int CaptureScript::parseFrames()
> > > > > >  {
> > > > > >  	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > > > > > @@ -191,6 +258,12 @@ int CaptureScript::parseFrame(EventPtr event)
> > > > > >  		return -EINVAL;
> > > > > >  
> > > > > >  	unsigned int frameId = atoi(key.c_str());
> > > > > > +	if (loop_ && frameId >= loop_) {
> > > > > > +		std::cerr
> > > > > > +			<< "Frame id (" << frameId << ") shall be smaller than"
> > > > > > +			<< "loop limit (" << loop_ << ")" << std::endl;
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > >  
> > > > > >  	event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > > >  	if (!event)
> > > > > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > > > > > index fffe67e5a3df..7a0ddebb00b5 100644
> > > > > > --- a/src/cam/capture_script.h
> > > > > > +++ b/src/cam/capture_script.h
> > > > > > @@ -40,6 +40,7 @@ private:
> > > > > >  	std::map<unsigned int, libcamera::ControlList> frameControls_;
> > > > > >  	std::shared_ptr<libcamera::Camera> camera_;
> > > > > >  	yaml_parser_t parser_;
> > > > > > +	unsigned int loop_;
> > > > > >  	bool valid_;
> > > > > >  
> > > > > >  	EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > > > > > @@ -49,6 +50,8 @@ private:
> > > > > >  
> > > > > >  	int parseScript(FILE *script);
> > > > > >  
> > > > > > +	int parseProperties();
> > > > > > +	int parseProperty();
> > > > > >  	int parseFrames();
> > > > > >  	int parseFrame(EventPtr event);
> > > > > >  	int parseControl(EventPtr event, libcamera::ControlList &controls);