[PATCH 2/2] apps: cam: Add support for loading configuration from capture script
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Oct 8 09:38:03 CEST 2024
Hi Paul and reviewers,
Maybe it's a bit late, but I have a question:
This patch adds the load of captures on libcamera app: cam,
while the Android adapter hasn't got the support.
I understand that it's difficult to add such a support in the
Android adapter, as it's just a translation layer, and that the
capture file contains StreamConfiguration settings besides
frames' controls, but have we considered loading the controls
in the core libraries (e.g. libcamera::PipelineHandler)?
We might need to check if the active streams are the same or
similar though.
BR,
Harvey
On Tue, Oct 8, 2024 at 2:45 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Paul,
>
> On Fri, Oct 04, 2024 at 09:05:29PM +0900, Paul Elder wrote:
> > On Thu, Oct 03, 2024 at 02:26:50AM +0300, Laurent Pinchart wrote:
> > > 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.
>
> I was thinking of doing it the other way around, but thinking about it
> again, we should keep the strings in the capture script identical to the
> enumerator names.
>
> I'm in two minds about the command line arguments. Short strings are
> nicer to type, but consistency is good too.
>
> > > > > > +
> > > > > > + 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;
>
> You're missing a return here.
>
> > > > > > + }
> > > > > > +
> > > > > > + 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.
>
> Fair point.
>
> > > > > > + } 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).
>
> I see it more as parseProperties() parsing a collection and
> parseProperty() parsing one element. parseStreams() and parseStream()
> are similar and I think consistency would be good.
>
> > > > > > + }
> > > > > > +
> > > > > > + 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
> >
> > > > > >
> > > > > > libcamera::ControlValue parseScalarControl(const libcamera::ControlId *id,
> > > > > > const std::string repr);
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list