[libcamera-devel] [PATCH] cam: capture_script: Introduce 'loop' property
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Sat Sep 3 03:00:24 CEST 2022
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.
>
> > > +#
> > > +# 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