[2/2] apps: cam: Add support for loading configuration from capture script
diff mbox series

Message ID 20240930170907.1404844-3-paul.elder@ideasonboard.com
State New
Headers show
Series
  • libcamera: Add support for dumping capture script
Related show

Commit Message

Paul Elder Sept. 30, 2024, 5:09 p.m. UTC
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
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@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(-)

Comments

Jacopo Mondi Oct. 2, 2024, 7:14 a.m. UTC | #1
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 ?

> 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@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 ?

> +		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 ?

> +
> +		std::string value = eventScalarValue(event);
> +
> +		if (key == "pixelFormat") {
> +			config.pixelFormat = libcamera::PixelFormat::fromString(value);

Should this be validated ?

> +		} 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 ?

> +		} 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 ;)

> +
> +	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 :)

> +			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
>
Paul Elder Oct. 2, 2024, 10:35 a.m. UTC | #2
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@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
> >
Laurent Pinchart Oct. 2, 2024, 11:26 p.m. UTC | #3
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@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);
Paul Elder Oct. 4, 2024, 12:05 p.m. UTC | #4
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@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);
Laurent Pinchart Oct. 7, 2024, 6:45 p.m. UTC | #5
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@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);
Harvey Yang Oct. 8, 2024, 7:38 a.m. UTC | #6
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@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@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
Kieran Bingham Oct. 10, 2024, 2:42 p.m. UTC | #7
Quoting Cheng-Hao Yang (2024-10-08 08:38:03)
> 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 think the capture happens in core doesn't it ? It's only the replay
that happens from cam... Are you looking for a way to replay capture
scripts through the android layer?

Perhaps that's something that would need to be in an android
application? or maybe some specific capturing from the android layer?

We shouldn't 'inject' processing into the android layer...

But you could use this implementation to capture the requests and
configuration from an android applciation, and likely replay it back (on
the same hardware) through cam ?


> 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@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@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

Patch
diff mbox series

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;
+		}
+
+		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;
+
+		std::string value = eventScalarValue(event);
+
+		if (key == "pixelFormat") {
+			config.pixelFormat = libcamera::PixelFormat::fromString(value);
+		} 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);
+		} 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;
+		}
+	}
+
+	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;
+			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;
+
+		/* 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);