[PATCH 2/2] apps: cam: Add support for loading configuration from capture script
Paul Elder
paul.elder at ideasonboard.com
Wed Oct 2 12:35:21 CEST 2024
On Wed, Oct 02, 2024 at 09:14:38AM +0200, Jacopo Mondi wrote:
> Hi Paul
>
> 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?
>
> > 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.
>
> > + 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;
> > } 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);
> > +
> > + auto it = orientations.find(orientation);
> > + if (it == orientations.end()) {
> > + std::cerr << "Invalid orientation '" << orientation
> > + << "' in capture script" << std::endl;
> > + 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;
> > + 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);
> > +
> > + 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));
> > + } 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;
> > + while (1) {
> > + event = nextEvent();
> > + if (!event)
> > + return -EINVAL;
> > + if (event->type == YAML_SEQUENCE_END_EVENT)
> > + return 0;
> > +
> > + if (event->type == YAML_MAPPING_START_EVENT) {
> > + parseStream(std::move(event), index++);
> > + continue;
> > + } else {
> > + std::cerr << "UNKNOWN TYPE" << std::endl;
>
> no need to scream :)
Oops :p Leftover debugging...
Thanks,
Paul
>
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + 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;
> > + 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 ?
>
> Thanks
> j
>
> > +
> > + /* 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;
> > +
> > + 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;
> > 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);
> >
> > libcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,
> > const std::string repr);
> > --
> > 2.39.2
> >
More information about the libcamera-devel
mailing list