[PATCH 2/2] apps: cam: Add support for loading configuration from capture script
Paul Elder
paul.elder at ideasonboard.com
Fri Oct 4 14:05:29 CEST 2024
On Thu, Oct 03, 2024 at 02:26:50AM +0300, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Wed, Oct 02, 2024 at 07:35:21PM +0900, Paul Elder wrote:
> > On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:
> > > On Tue, Oct 01, 2024 at 02:09:07AM GMT, Paul Elder wrote:
> > > > Add support to the cam application for loading the camera configuration
> > > > from a capture script. These are not expected to be written by hand, but
> > >
> > > Wouldn't it be useful to write camera configurations by hand ? It
> > > doesn't seem to require any special handling compared to what you have
> > > already done here, right ?
> >
> > I didn't expect it to happen but you certainly can. I suppose I can
> > remove this to remove confusion?
>
> You could then replace the last sentence with "These can be written
> manually, or dumped from a capture session via the ...".
>
> > > > rather dumped via the LIBCAMERA_DUMP_CAPTURE_SCRIPT environment
> > > > variable.
> > > >
> > > > If any configuration options are specified by command line parameters,
> > > > those will take precedence.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > ---
> > > > src/apps/cam/camera_session.cpp | 22 +++--
> > > > src/apps/cam/capture_script.cpp | 164 ++++++++++++++++++++++++++++++++
> > > > src/apps/cam/capture_script.h | 8 ++
> > > > 3 files changed, 184 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > > > index edc49b875450..6f1d8b58870f 100644
> > > > --- a/src/apps/cam/camera_session.cpp
> > > > +++ b/src/apps/cam/camera_session.cpp
> > > > @@ -70,6 +70,18 @@ CameraSession::CameraSession(CameraManager *cm,
> > > > return;
> > > > }
> > > >
> > > > + if (options_.isSet(OptCaptureScript)) {
> > > > + std::string scriptName = options_[OptCaptureScript].toString();
> > > > + script_ = std::make_unique<CaptureScript>(camera_, scriptName);
> > > > + if (!script_->valid()) {
> > > > + std::cerr << "Invalid capture script '" << scriptName
> > > > + << "'" << std::endl;
> > > > + return;
> > > > + }
> > > > +
> > >
> > > What about adding a comment here to record that options specified on
> > > the command line take precendece over options coming from the
> > > configuration file ?
> >
> > Good idea.
>
> It would also explain why you're moving this block of code.
>
> /*
> * Parse the capture script first to populate the configuration, and let
> * command line arguments take precendence.
> */
>
> This should ideally also be mentioned in the help text.
>
> > > > + script_->populateConfiguration(config.get());
> > > > + }
> > > > +
> > > > if (options_.isSet(OptOrientation)) {
> > > > std::string orientOpt = options_[OptOrientation].toString();
> > > > static const std::map<std::string, libcamera::Orientation> orientations{
> > > > @@ -119,16 +131,6 @@ CameraSession::CameraSession(CameraManager *cm,
> > > > }
> > > > #endif
> > > >
> > > > - if (options_.isSet(OptCaptureScript)) {
> > > > - std::string scriptName = options_[OptCaptureScript].toString();
> > > > - script_ = std::make_unique<CaptureScript>(camera_, scriptName);
> > > > - if (!script_->valid()) {
> > > > - std::cerr << "Invalid capture script '" << scriptName
> > > > - << "'" << std::endl;
> > > > - return;
> > > > - }
> > > > - }
> > > > -
> > > > switch (config->validate()) {
> > > > case CameraConfiguration::Valid:
> > > > break;
> > > > diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp
> > > > index fc1dfa75f2d4..7f166f45657e 100644
> > > > --- a/src/apps/cam/capture_script.cpp
> > > > +++ b/src/apps/cam/capture_script.cpp
> > > > @@ -7,6 +7,7 @@
> > > >
> > > > #include "capture_script.h"
> > > >
> > > > +#include <algorithm>
> > > > #include <iostream>
> > > > #include <stdio.h>
> > > > #include <stdlib.h>
> > > > @@ -162,6 +163,10 @@ int CaptureScript::parseScript(FILE *script)
> > > > ret = parseFrames();
> > > > if (ret)
> > > > return ret;
> > > > + } else if (section == "configuration") {
> > > > + ret = parseConfiguration();
> > > > + if (ret)
> > > > + return ret;
>
> I'd move this before "frames" to reflect the expected order in the
> capture script.
>
> > > > } else {
> > > > std::cerr << "Unsupported section '" << section << "'"
> > > > << std::endl;
> > > > @@ -322,6 +327,165 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)
> > > > return 0;
> > > > }
> > > >
> > > > +int CaptureScript::parseOrientation(EventPtr event)
> > > > +{
> > > > + static const std::map<std::string, libcamera::Orientation> orientations{
> > > > + { "Rotate0", libcamera::Orientation::Rotate0 },
> > > > + { "Rotate0Mirror", libcamera::Orientation::Rotate0Mirror },
> > > > + { "Rotate180", libcamera::Orientation::Rotate180 },
> > > > + { "Rotate180Mirror", libcamera::Orientation::Rotate180Mirror },
> > > > + { "Rotate90Mirror", libcamera::Orientation::Rotate90Mirror },
> > > > + { "Rotate270", libcamera::Orientation::Rotate270 },
> > > > + { "Rotate270Mirror", libcamera::Orientation::Rotate270Mirror },
> > > > + { "Rotate90", libcamera::Orientation::Rotate90 },
> > > > + };
> > > > +
> > > > + std::string orientation = eventScalarValue(event);
>
> If you move this line to the caller, this function could be a shared
> helper, there's similar code in CameraSession::CameraSession(). The
> helper could be a global function in a new utils.cpp file in the same
> directory.
No, because the code in CameraSession is for parsing the command line
parameters, which don't match the enum names. Unless you think we should
change the command line parameter orientation names to match the enums,
but that wouldn't be very nice to type.
>
> > > > +
> > > > + auto it = orientations.find(orientation);
> > > > + if (it == orientations.end()) {
> > > > + std::cerr << "Invalid orientation '" << orientation
> > > > + << "' in capture script" << std::endl;
>
> The error message would also need to move to the caller, or drop the "in
> capture script" part.
>
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + orientation_ = it->second;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int CaptureScript::parseStream(EventPtr event, unsigned int index)
> > > > +{
> > > > + if (!checkEvent(event, YAML_MAPPING_START_EVENT))
> > > > + return -EINVAL;
> > > > +
> > > > + StreamConfiguration config;
> > > > + while (1) {
> > > > + event = nextEvent();
> > > > + if (!event)
> > > > + return -EINVAL;
>
> Add a blank line here for consistency with existing code.
>
> > > > + if (event->type == YAML_MAPPING_END_EVENT)
> > > > + break;
> > > > +
> > > > + std::string key = eventScalarValue(event);
> > > > +
> > > > + event = nextEvent();
> > > > + if (!event)
> > > > + return -EINVAL;
> > > > + if (event->type == YAML_MAPPING_END_EVENT)
> > > > + break;
> > >
> > > isn't this an error condition ? If you get a MAPPING_END after a key
> > > isn't the scrip malformed ?
> >
> > I'm not sure how it shows up in the yaml parser, but afaiu it's possible
> > have a key with no value. We do that in tuning files. (Not that this is
> > a tuning file, but speaking from a yaml-correctness perspective).
> >
> > > > +
> > > > + std::string value = eventScalarValue(event);
>
> I think you can replace this with
>
> std::string key = eventScalarValue(event);
>
> std::string value = parseScalar();
> if (value.empty())
> return -EINVAL;
>
> as the value must be present.
ack
>
> > > > +
> > > > + if (key == "pixelFormat") {
> > > > + config.pixelFormat = libcamera::PixelFormat::fromString(value);
> > >
> > > Should this be validated ?
> >
> > It should be fine not to. Camera::configure() will adjust it if it's not
> > valid.
> >
> > > > + } else if (key == "size") {
> > > > + unsigned int split = value.find("x");
> > > > + if (split == std::string::npos) {
> > > > + std::cerr << "Invalid size '" << value
> > > > + << "' in stream configuration "
> > > > + << index << std::endl;
> > > > + }
> > > > +
> > > > + std::string width = value.substr(0, split);
> > > > + std::string height = value.substr(split + 1);
> > > > + config.size = Size(std::stoi(width), std::stoi(height));
>
> We specify width and height independently on the command line, would it
> make sense to do the same in yaml ? It would simplify the code here.
No, I think it's fine to keep it symmetric to what is dumped, since it's
a single field of type Size, not two fields each of int.
>
> > > > + } else if (key == "stride") {
> > > > + config.stride = std::stoi(value);
> > > > + } else if (key == "frameSize") {
> > > > + config.frameSize = std::stoi(value);
> > >
> > > Do we allow to specify the frame size ? What's the purpose ? Also, we
> > > already have stride so it can be calculated from there ?
> >
> > The purpose was to support all fields from the dump. If the
> > configuration comes from a dump, you'll have all fields. If the
> > configuration is handwritten (which I didn't actually expect in the
> > first place but is technically possible), then you can just skip the
> > field, or if its wrong it'll just get adjusted by Camera::configure().
> > So either way it should be fine.
> >
> > > > + } else if (key == "bufferCount") {
> > > > + config.bufferCount = std::stoi(value);
> > > > + } else if (key == "colorSpace") {
> > > > + config.colorSpace = libcamera::ColorSpace::fromString(value);
> > > > + } else {
> > > > + std::cerr << "Unknown key-value pair '"
> > > > + << key << "': '" << value
> > > > + << "' in stream configuration "
> > > > + << index << std::endl;
> > > > + return -EINVAL;
> > > > + }
> > > > + }
> > >
> > > Are there mandatory fields to be specified ? Should we check that all
> > > the above have been populated in the script ? I'm concerned that a
> > > script might specify only a partial set of configurations. Ofc if you
> > > consider this to be only generated by a dump, that's not an issue ;)
> >
> > See above.
> >
> > > > +
> > > > + streamConfigs_.push_back(config);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int CaptureScript::parseStreams(EventPtr event)
> > > > +{
> > > > + if (!checkEvent(event, YAML_SEQUENCE_START_EVENT))
> > > > + return -EINVAL;
> > > > +
> > > > + unsigned int index = 0;
>
> Add a blank line.
>
> > > > + while (1) {
> > > > + event = nextEvent();
> > > > + if (!event)
> > > > + return -EINVAL;
>
> Add a blank line here too.
>
> > > > + if (event->type == YAML_SEQUENCE_END_EVENT)
> > > > + return 0;
> > > > +
> > > > + if (event->type == YAML_MAPPING_START_EVENT) {
> > > > + parseStream(std::move(event), index++);
>
> Missing error handling.
>
> > > > + continue;
>
> You can drop the continue.
>
> > > > + } else {
> > > > + std::cerr << "UNKNOWN TYPE" << std::endl;
> > >
> > > no need to scream :)
> >
> > Oops :p Leftover debugging...
> >
> > > > + return -EINVAL;
> > > > + }
>
> parseProperties() implements something similar with
>
> while (1) {
> if (event->type == YAML_SEQUENCE_END_EVENT)
> return 0;
>
> int ret = parseProperty();
> if (ret)
> return ret;
>
> event = nextEvent();
> if (!event)
> return -EINVAL;
> }
>
> and parseProperty() starts with
>
> EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> if (!event)
> return -EINVAL;
>
> should you do the same here for consistency ?
>
parseProperties() is a "top-level" parsing function, at the same level
as parseConfiguration(). Anything at levels below that takes an
EventPtr, which is why I have it like this (also I don't really like the
statefulness of not passing the EventPtr but anyway).
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int CaptureScript::parseConfiguration()
> > > > +{
> > > > + int ret;
> > > > +
> > > > + EventPtr event = nextEvent(YAML_MAPPING_START_EVENT);
> > > > + if (!event)
> > > > + return -EINVAL;
> > > > +
> > > > + while (1) {
> > > > + event = nextEvent();
> > > > + if (!event)
> > > > + return -EINVAL;
>
> Add a blank line here too.
>
> > > > + if (event->type == YAML_MAPPING_END_EVENT)
> > > > + break;
> > > > +
> > > > + std::string key = eventScalarValue(event);
> > > > +
> > > > + event = nextEvent();
> > > > + if (!event)
> > > > + return -EINVAL;
> > > > + if (event->type == YAML_MAPPING_END_EVENT)
> > > > + break;
> > >
> > > Same question, a mapping_end event after a key is an error ?
> > >
> > > > +
> > > > + /* TODO Load sensor configuration */
> > > > + if (key == "orientation") {
> > > > + ret = parseOrientation(std::move(event));
> > > > + if (ret)
> > > > + return ret;
> > > > + } else if (key == "streams") {
> > > > + ret = parseStreams(std::move(event));
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void CaptureScript::populateConfiguration(CameraConfiguration *configuration) const
> > > > +{
> > > > + if (!configuration)
> > > > + return;
>
> Is this a valid use case ? If not you can turn the argument into a
> reference, and drop this check.
>
It is indeed not. I'll do that.
> > > > +
> > > > + configuration->orientation = orientation_;
> > > > +
> > > > + for (unsigned int i = 0; i < streamConfigs_.size(); i++)
> > > > + (*configuration)[i] = streamConfigs_[i];
> > > > +}
> > > > +
> > > > std::string CaptureScript::parseScalar()
> > > > {
> > > > EventPtr event = nextEvent(YAML_SCALAR_EVENT);
> > > > diff --git a/src/apps/cam/capture_script.h b/src/apps/cam/capture_script.h
> > > > index 294b920363ba..4ba862d742cf 100644
> > > > --- a/src/apps/cam/capture_script.h
> > > > +++ b/src/apps/cam/capture_script.h
> > > > @@ -26,6 +26,7 @@ public:
> > > >
> > > > const libcamera::ControlList &frameControls(unsigned int frame);
> > > >
> > > > + void populateConfiguration(libcamera::CameraConfiguration *configuration) const;
>
> Missing blank line.
>
> > > > private:
> > > > struct EventDeleter {
> > > > void operator()(yaml_event_t *event) const
> > > > @@ -43,6 +44,9 @@ private:
> > > > unsigned int loop_;
> > > > bool valid_;
> > > >
> > > > + libcamera::Orientation orientation_;
> > > > + std::vector<libcamera::StreamConfiguration> streamConfigs_;
> > > > +
> > > > EventPtr nextEvent(yaml_event_type_t expectedType = YAML_NO_EVENT);
> > > > bool checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const;
> > > > static std::string eventScalarValue(const EventPtr &event);
> > > > @@ -55,6 +59,10 @@ private:
> > > > int parseFrames();
> > > > int parseFrame(EventPtr event);
> > > > int parseControl(EventPtr event, libcamera::ControlList &controls);
> > > > + int parseConfiguration();
> > > > + int parseOrientation(EventPtr event);
> > > > + int parseStreams(EventPtr event);
> > > > + int parseStream(EventPtr event, unsigned int index);
>
> Maybe also order the functions (here and in the .cpp file) in the order
> in which the elements are expected to appear in the yaml file.
ack
Thanks,
Paul
>
> > > >
> > > > libcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,
> > > > const std::string repr);
More information about the libcamera-devel
mailing list