[PATCH 2/2] apps: cam: Add support for loading configuration from capture script
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 3 01:26:50 CEST 2024
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.
> > > +
> > > + 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.
> > > +
> > > + 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.
> > > + } 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 ?
> > > + }
> > > +
> > > + 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.
> > > +
> > > + 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.
> > >
> > > libcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,
> > > const std::string repr);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list