[libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop' property
Kieran Bingham
kieran.bingham at ideasonboard.com
Sat Sep 3 14:34:19 CEST 2022
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 at 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 at 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 at 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 at 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);
More information about the libcamera-devel
mailing list