[libcamera-devel] [PATCH v2 1/3] cam: Add a parser for capture scripts

[Laurent Pinchart]

Hi Jacopo,

On Sat, May 21, 2022 at 06:24:49PM +0200, Jacopo Mondi wrote:
> On Fri, May 20, 2022 at 07:52:32PM +0300, Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Add a parser class to the cam test application to control the capture
> > operations through a yaml script.
> >
> > The capture script currently allow to specify a list of controls and
> > their associated values to be applied per-frame.
> >
> > Also add a trivial capture script example to showcase the intended
> > script structure.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> You sob is missing here.
> You could almost change the authorship too :)

No no, you authored the original version and the structure is still the
same :-)

> > ---
> > Changes since v1:
> >
> > - Add SPDX license identifier to capture-script.yaml
> > - Drop gcc 7 support as libcamera requires gcc 8 or newer
> > - Make scriptPath_ a local variable in CaptureScript constructor
> > - Drop file check as we want to accept symlinks too
> > - Pass const reference to fileName to CaptureScript constructor
> > - Avoid manual deletes of tokens
> > - Simplify and improve error message handling
> > - Print line and column number in error message
> > - Simplify handling of key/value parsing
> > - Simplify parser deletion
> > - Don't consume last tokens as that's not needed.
> > - Simplify CaptureScript::unpackFailure()
> > - Switch to the simpler event API
> > - Typo fixes
> > ---
> >  src/cam/capture-script.yaml |  46 +++++
> >  src/cam/capture_script.cpp  | 334 ++++++++++++++++++++++++++++++++++++
> >  src/cam/capture_script.h    |  60 +++++++
> >  src/cam/meson.build         |   2 +
> >  4 files changed, 442 insertions(+)
> >  create mode 100644 src/cam/capture-script.yaml
> >  create mode 100644 src/cam/capture_script.cpp
> >  create mode 100644 src/cam/capture_script.h
> >
> > diff --git a/src/cam/capture-script.yaml b/src/cam/capture-script.yaml
> > new file mode 100644
> > index 000000000000..6a749bc60cf7
> > --- /dev/null
> > +++ b/src/cam/capture-script.yaml
> > @@ -0,0 +1,46 @@
> > +# SPDX-License-Identifier: CC0-1.0
> > +
> > +# Capture script example
> > +#
> > +# A capture script allows to associate a list of controls and their values
> > +# to frame numbers.
> > +
> > +# \todo Formally define the capture script structure with a schema
> > +
> > +# Notes:
> > +# - Controls have to be specified by name, as defined in the
> > +#   libcamera::controls:: enumeration
> > +# - Controls not supported by the camera currently operated are ignored
> > +# - Frame numbers shall be monotonically incrementing, gaps are allowed
> > +
> > +# Example:
> > +frames:
> > +  - 1:
> > +      Brightness: 0.0
> > +
> > +  - 40:
> > +      Brightness: 0.2
> > +
> > +  - 80:
> > +      Brightness: 0.4
> > +
> > +  - 120:
> > +      Brightness: 0.8
> > +
> > +  - 160:
> > +      Brightness: 0.4
> > +
> > +  - 200:
> > +      Brightness: 0.2
> > +
> > +  - 240:
> > +      Brightness: 0.0
> > +
> > +  - 280:
> > +      Brightness: -0.2
> > +
> > +  - 300:
> > +      Brightness: -0.4
> > +
> > +  - 340:
> > +      Brightness: -0.8
> > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp
> > new file mode 100644
> > index 000000000000..8e9120321a25
> > --- /dev/null
> > +++ b/src/cam/capture_script.cpp
> > @@ -0,0 +1,334 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * capture_script.cpp - Capture session configuration script
> 
> Missing copyright. Can you add IoB one ?

OK.

> > + */
> > +
> > +#include "capture_script.h"
> > +
> > +#include <iostream>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +
> > +using namespace libcamera;
> > +
> > +CaptureScript::CaptureScript(std::shared_ptr<Camera> camera,
> > +			     const std::string &fileName)
> > +	: camera_(camera), valid_(false)
> > +{
> > +	FILE *fh = fopen(fileName.c_str(), "r");
> > +	if (!fh) {
> > +		int ret = -errno;
> > +		std::cerr << "Failed to open capture script " << fileName
> > +			  << ": " << strerror(-ret) << std::endl;
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Map the camera's controls to their name so that they can be
> > +	 * easily identified when parsing the script file.
> > +	 */
> > +	for (const auto &[control, info] : camera_->controls())
> > +		controls_[control->name()] = control;
> > +
> > +	int ret = parseScript(fh);
> > +	fclose(fh);
> > +	if (ret)
> > +		return;
> > +
> > +	valid_ = true;
> > +}
> > +
> > +/* Retrieve the control list associated with a frame number. */
> > +const ControlList &CaptureScript::frameControls(unsigned int frame)
> > +{
> > +	static ControlList controls{};
> > +
> > +	auto it = frameControls_.find(frame);
> > +	if (it == frameControls_.end())
> > +		return controls;
> > +
> > +	return it->second;
> > +}
> > +
> > +CaptureScript::EventPtr CaptureScript::nextEvent(yaml_event_type_t expectedType)
> > +{
> > +	EventPtr event(new yaml_event_t);
> > +
> > +	if (!yaml_parser_parse(&parser_, event.get()))
> > +		return nullptr;
> > +
> > +	if (expectedType != YAML_NO_EVENT && !checkEvent(event, expectedType))
> 
> Could you move the expectedType != YAML_NO_EVENT check in checkEvent() ?
> 
> It's probably nicer to read, at the expense of an unconditional
> function call

I thought about it, but thought it would unnecessarily burden the other
callers of checkEvent() that never call the function with YAML_NO_EVENT.

> > +		return nullptr;
> > +
> > +	return event;
> > +}
> > +
> > +bool CaptureScript::checkEvent(const EventPtr &event, yaml_event_type_t expectedType) const
> > +{
> > +	if (event->type != expectedType) {
> > +		std::cerr << "Capture script error on line " << event->start_mark.line
> 
> Tried to insert a random error in the config file and I get line == 0,
> column == 0

That's weird, I've just tested the same, and I'm getting

Capture script error on line 23 column 10: Expected mapping-start event, got scalar

> Apart from these two minors, the automatic deletion and usage of
> event-based parsing is much nicer! Thanks
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +			  << " column " << event->start_mark.column << ": "
> > +			  << "Expected " << eventTypeName(expectedType)
> > +			  << " event, got " << eventTypeName(event->type)
> > +			  << std::endl;
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +std::string CaptureScript::eventScalarValue(const EventPtr &event)
> > +{
> > +	return std::string(reinterpret_cast<char *>(event->data.scalar.value),
> > +			   event->data.scalar.length);
> > +}
> > +
> > +std::string CaptureScript::eventTypeName(yaml_event_type_t type)
> > +{
> > +	static const std::map<yaml_event_type_t, std::string> typeNames = {
> > +		{ YAML_STREAM_START_EVENT, "stream-start" },
> > +		{ YAML_STREAM_END_EVENT, "stream-end" },
> > +		{ YAML_DOCUMENT_START_EVENT, "document-start" },
> > +		{ YAML_DOCUMENT_END_EVENT, "document-end" },
> > +		{ YAML_ALIAS_EVENT, "alias" },
> > +		{ YAML_SCALAR_EVENT, "scalar" },
> > +		{ YAML_SEQUENCE_START_EVENT, "sequence-start" },
> > +		{ YAML_SEQUENCE_END_EVENT, "sequence-end" },
> > +		{ YAML_MAPPING_START_EVENT, "mapping-start" },
> > +		{ YAML_MAPPING_END_EVENT, "mapping-end" },
> > +	};
> > +
> > +	auto it = typeNames.find(type);
> > +	if (it == typeNames.end())
> > +		return "[type " + std::to_string(type) + "]";
> > +
> > +	return it->second;
> > +}
> > +
> > +int CaptureScript::parseScript(FILE *script)
> > +{
> > +	int ret = yaml_parser_initialize(&parser_);
> > +	if (!ret) {
> > +		std::cerr << "Failed to initialize yaml parser" << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	/* Delete the parser upon function exit. */
> > +	struct ParserDeleter {
> > +		ParserDeleter(yaml_parser_t *parser) : parser_(parser) { }
> > +		~ParserDeleter() { yaml_parser_delete(parser_); }
> > +		yaml_parser_t *parser_;
> > +	} deleter(&parser_);
> > +
> > +	yaml_parser_set_input_file(&parser_, script);
> > +
> > +	EventPtr event = nextEvent(YAML_STREAM_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	event = nextEvent(YAML_DOCUMENT_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	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)
> > +			return 0;
> > +
> > +		if (!checkEvent(event, YAML_SCALAR_EVENT))
> > +			return -EINVAL;
> > +
> > +		std::string section = eventScalarValue(event);
> > +
> > +		if (section == "frames") {
> > +			parseFrames();
> > +		} else {
> > +			std::cerr << "Unsupported section '" << section << "'"
> > +				  << std::endl;
> > +			return -EINVAL;
> > +		}
> > +	}
> 3> +}
> > +
> > +int CaptureScript::parseFrames()
> > +{
> > +	EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	while (1) {
> > +		event = nextEvent();
> > +		if (!event)
> > +			return -EINVAL;
> > +
> > +		if (event->type == YAML_SEQUENCE_END_EVENT)
> > +			return 0;
> > +
> > +		int ret = parseFrame(std::move(event));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +}
> > +
> > +int CaptureScript::parseFrame(EventPtr event)
> > +{
> > +	if (!checkEvent(event, YAML_MAPPING_START_EVENT))
> > +		return -EINVAL;
> > +
> > +	std::string key = parseScalar();
> > +	if (key.empty())
> > +		return -EINVAL;
> > +
> > +	unsigned int frameId = atoi(key.c_str());
> > +
> > +	event = nextEvent(YAML_MAPPING_START_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	ControlList controls{};
> > +
> > +	while (1) {
> > +		event = nextEvent();
> > +		if (!event)
> > +			return -EINVAL;
> > +
> > +		if (event->type == YAML_MAPPING_END_EVENT)
> > +			break;
> > +
> > +		int ret = parseControl(std::move(event), controls);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	frameControls_[frameId] = std::move(controls);
> > +
> > +	event = nextEvent(YAML_MAPPING_END_EVENT);
> > +	if (!event)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +int CaptureScript::parseControl(EventPtr event, ControlList &controls)
> > +{
> > +	/* We expect a value after a key. */
> > +	std::string name = eventScalarValue(event);
> > +	if (name.empty())
> > +		return -EINVAL;
> > +
> > +	/* If the camera does not support the control just ignore it. */
> > +	auto it = controls_.find(name);
> > +	if (it == controls_.end()) {
> > +		std::cerr << "Unsupported control '" << name << "'" << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	std::string value = parseScalar();
> > +	if (value.empty())
> > +		return -EINVAL;
> > +
> > +	const ControlId *controlId = it->second;
> > +	ControlValue val = unpackControl(controlId, value);
> > +	controls.set(controlId->id(), val);
> > +
> > +	return 0;
> > +}
> > +
> > +std::string CaptureScript::parseScalar()
> > +{
> > +	EventPtr event = nextEvent(YAML_SCALAR_EVENT);
> > +	if (!event)
> > +		return "";
> > +
> > +	return eventScalarValue(event);
> > +}
> > +
> > +void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)
> > +{
> > +	static const std::map<unsigned int, const char *> typeNames = {
> > +		{ ControlTypeNone, "none" },
> > +		{ ControlTypeBool, "bool" },
> > +		{ ControlTypeByte, "byte" },
> > +		{ ControlTypeInteger32, "int32" },
> > +		{ ControlTypeInteger64, "int64" },
> > +		{ ControlTypeFloat, "float" },
> > +		{ ControlTypeString, "string" },
> > +		{ ControlTypeRectangle, "Rectangle" },
> > +		{ ControlTypeSize, "Size" },
> > +	};
> > +
> > +	const char *typeName;
> > +	auto it = typeNames.find(id->type());
> > +	if (it != typeNames.end())
> > +		typeName = it->second;
> > +	else
> > +		typeName = "unknown";
> > +
> > +	std::cerr << "Unsupported control '" << repr << "' for "
> > +		  << typeName << " control " << id->name() << std::endl;
> > +}
> > +
> > +ControlValue CaptureScript::unpackControl(const ControlId *id,
> > +					  const std::string &repr)
> > +{
> > +	ControlValue value{};
> > +
> > +	switch (id->type()) {
> > +	case ControlTypeNone:
> > +		break;
> > +	case ControlTypeBool: {
> > +		bool val;
> > +
> > +		if (repr == "true") {
> > +			val = true;
> > +		} else if (repr == "false") {
> > +			val = false;
> > +		} else {
> > +			unpackFailure(id, repr);
> > +			return value;
> > +		}
> > +
> > +		value.set<bool>(val);
> > +		break;
> > +	}
> > +	case ControlTypeByte: {
> > +		uint8_t val = strtol(repr.c_str(), NULL, 10);
> > +		value.set<uint8_t>(val);
> > +		break;
> > +	}
> > +	case ControlTypeInteger32: {
> > +		int32_t val = strtol(repr.c_str(), NULL, 10);
> > +		value.set<int32_t>(val);
> > +		break;
> > +	}
> > +	case ControlTypeInteger64: {
> > +		int64_t val = strtoll(repr.c_str(), NULL, 10);
> > +		value.set<int64_t>(val);
> > +		break;
> > +	}
> > +	case ControlTypeFloat: {
> > +		float val = strtof(repr.c_str(), NULL);
> > +		value.set<float>(val);
> > +		break;
> > +	}
> > +	case ControlTypeString: {
> > +		value.set<std::string>(repr);
> > +		break;
> > +	}
> > +	case ControlTypeRectangle:
> > +		/* \todo Parse rectangles. */
> > +		break;
> > +	case ControlTypeSize:
> > +		/* \todo Parse Sizes. */
> > +		break;
> > +	}
> > +
> > +	return value;
> > +}
> > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h
> > new file mode 100644
> > index 000000000000..44368f63f34c
> > --- /dev/null
> > +++ b/src/cam/capture_script.h
> > @@ -0,0 +1,60 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * capture_script.h - Capture session configuration script
> > + */
> > +
> > +#pragma once
> > +
> > +#include <map>
> > +#include <memory>
> > +#include <string>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/controls.h>
> > +
> > +#include <yaml.h>
> > +
> > +class CaptureScript
> > +{
> > +public:
> > +	CaptureScript(std::shared_ptr<libcamera::Camera> camera,
> > +		      const std::string &fileName);
> > +
> > +	bool valid() const { return valid_; }
> > +
> > +	const libcamera::ControlList &frameControls(unsigned int frame);
> > +
> > +private:
> > +	struct EventDeleter {
> > +		void operator()(yaml_event_t *event) const
> > +		{
> > +			yaml_event_delete(event);
> > +			delete event;
> > +		}
> > +	};
> > +	using EventPtr = std::unique_ptr<yaml_event_t, EventDeleter>;
> > +
> > +	std::map<std::string, const libcamera::ControlId *> controls_;
> > +	std::map<unsigned int, libcamera::ControlList> frameControls_;
> > +	std::shared_ptr<libcamera::Camera> camera_;
> > +	yaml_parser_t parser_;
> > +	bool valid_;
> > +
> > +	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);
> > +	static std::string eventTypeName(yaml_event_type_t type);
> > +
> > +	int parseScript(FILE *script);
> > +
> > +	int parseFrames();
> > +	int parseFrame(EventPtr event);
> > +	int parseControl(EventPtr event, libcamera::ControlList &controls);
> > +
> > +	std::string parseScalar();
> > +
> > +	void unpackFailure(const libcamera::ControlId *id,
> > +			   const std::string &repr);
> > +	libcamera::ControlValue unpackControl(const libcamera::ControlId *id,
> > +					      const std::string &repr);
> > +};
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index 5bab8c9e331b..c47a85592478 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -11,6 +11,7 @@ cam_enabled = true
> >
> >  cam_sources = files([
> >      'camera_session.cpp',
> > +    'capture_script.cpp',
> >      'event_loop.cpp',
> >      'file_sink.cpp',
> >      'frame_sink.cpp',
> > @@ -38,6 +39,7 @@ cam  = executable('cam', cam_sources,
> >                        libcamera_public,
> >                        libdrm,
> >                        libevent,
> > +                      libyaml,
> >                    ],
> >                    cpp_args : cam_cpp_args,
> >                    install : true)

-- 
Regards,

Laurent Pinchart