[RFC,3/4] libcamera: Implement YamlEmitter

21615 diff mbox series

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/internal/meson.build    |   1 +
 include/libcamera/internal/yaml_emitter.h | 172 +++++++++
 src/libcamera/meson.build                 |   1 +
 src/libcamera/yaml_emitter.cpp            | 427 ++++++++++++++++++++++
 4 files changed, 601 insertions(+)
 create mode 100644 include/libcamera/internal/yaml_emitter.h
 create mode 100644 src/libcamera/yaml_emitter.cpp

Hi Jacopo,

Thank you for the patch.

I've kept the review high-level to start with.

On Mon, Oct 14, 2024 at 11:59:35AM +0200, Jacopo Mondi wrote:
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/internal/meson.build    |   1 +
>  include/libcamera/internal/yaml_emitter.h | 172 +++++++++
>  src/libcamera/meson.build                 |   1 +
>  src/libcamera/yaml_emitter.cpp            | 427 ++++++++++++++++++++++
>  4 files changed, 601 insertions(+)
>  create mode 100644 include/libcamera/internal/yaml_emitter.h
>  create mode 100644 src/libcamera/yaml_emitter.cpp
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1c5eef9cab80..7533b075fde2 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -41,6 +41,7 @@ libcamera_internal_headers = files([
>      'v4l2_pixelformat.h',
>      'v4l2_subdevice.h',
>      'v4l2_videodevice.h',
> +    'yaml_emitter.h',
>      'yaml_parser.h',
>  ])
>  
> diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h
> new file mode 100644
> index 000000000000..3fe05f97de70
> --- /dev/null
> +++ b/include/libcamera/internal/yaml_emitter.h
> @@ -0,0 +1,172 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board Oy
> + *
> + * libcamera YAML emitter helper
> + */
> +
> +#pragma once
> +
> +#include <memory>
> +#include <string_view>
> +#include <unordered_map>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/file.h>
> +#include <libcamera/orientation.h>
> +
> +#include <yaml.h>
> +
> +namespace libcamera {
> +
> +class YamlDict;
> +class YamlEvent;
> +class YamlList;
> +class YamlRoot;
> +class YamlScalar;
> +
> +class YamlEmitter final
> +{
> +public:
> +	~YamlEmitter() = default;
> +
> +	static std::unique_ptr<YamlRoot> root(std::string_view path);
> +
> +	int emit(YamlEvent *event);
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> +
> +	class Emitter
> +	{
> +	public:
> +		Emitter() = default;
> +		~Emitter();
> +
> +		void init(File *file);
> +
> +		int emit(YamlEvent *event);
> +
> +	private:
> +		void logError();
> +
> +		yaml_emitter_t emitter_;
> +	};
> +
> +	YamlEmitter() = default;
> +
> +	void init();
> +
> +	std::unique_ptr<File> file_;
> +	Emitter emitter_;
> +};
> +
> +class YamlOutput
> +{
> +public:

Shouldn't you disable copying of this class ?

> +	virtual ~YamlOutput() = default;
> +
> +	void close()
> +	{
> +		closed_ = true;
> +	}

Can we avoid this and close on destruction only ? Having an explicit
close is error-prone, what if someone calls one of the functions below
after calling close() ? The API should make it very hard, if not
impossible, to do the wrong thing.

> +
> +	std::unique_ptr<YamlScalar> scalar();
> +	std::unique_ptr<YamlDict> dict();
> +	std::unique_ptr<YamlList> list();

In many cases the YamlScalar, YamlDict and YamlList classes will be
local to the caller function. It would be best to avoid dynamic heap
allocation. You could do so by returning values here. Same below. You
will likely need to implement move constructors and operators.

> +
> +protected:
> +	YamlOutput(YamlEmitter *emitter)
> +		: emitter_(emitter), closed_(false)
> +	{
> +	}
> +
> +	int emitScalar(std::string_view scalar);
> +	int emitMappingStart();
> +	int emitMappingEnd();
> +	int emitSequenceStart();
> +	int emitSequenceEnd();
> +
> +	YamlEmitter *emitter_;
> +
> +	bool closed_;
> +};
> +
> +class YamlRoot : public YamlOutput
> +{
> +public:
> +	~YamlRoot();
> +	void close() {}
> +
> +	std::unique_ptr<YamlList> list();
> +	std::unique_ptr<YamlDict> dict();
> +	void scalar(std::string_view scalar);
> +
> +private:
> +	friend class YamlEmitter;
> +
> +	YamlRoot(YamlEmitter *emitter)
> +		: YamlOutput(emitter)
> +	{
> +		emitterRoot_ = std::unique_ptr<YamlEmitter>(emitter);
> +	}
> +
> +	std::unique_ptr<YamlEmitter> emitterRoot_;
> +};
> +
> +class YamlScalar : public YamlOutput
> +{
> +public:
> +	~YamlScalar() = default;
> +
> +	void close() {}
> +
> +	void operator=(const Orientation &orientation);

I don't think this should be a member of the YamlScalar class. You
should convert the Orientation to a string in the caller.

> +	void operator=(std::string_view scalar);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlScalar(YamlEmitter *emitter);
> +};
> +
> +class YamlList : public YamlOutput
> +{
> +public:
> +	~YamlList();
> +
> +	void close();
> +
> +	std::unique_ptr<YamlList> list();
> +	std::unique_ptr<YamlDict> dict();
> +	void scalar(std::string_view scalar);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlList(YamlEmitter *emitter);
> +};
> +
> +class YamlDict : public YamlOutput,
> +		 private std::unordered_map<std::string_view,
> +					    std::unique_ptr<YamlScalar>>

Why do you need to store content ? That doesn't sound right,the
emitter's job is to emit YAML, we shouldn't need to store anything.

> +{
> +public:
> +	using Map = std::unordered_map<std::string_view, YamlScalar>;
> +
> +	~YamlDict();
> +
> +	void close();
> +
> +	std::unique_ptr<YamlList> list(std::string_view key);
> +	std::unique_ptr<YamlDict> dict(std::string_view key);
> +
> +	YamlScalar &operator[](const Map::key_type &key);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlDict(YamlEmitter *emitter);
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index aa9ab0291854..5b8af4103085 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -51,6 +51,7 @@ libcamera_internal_sources = files([
>      'v4l2_pixelformat.cpp',
>      'v4l2_subdevice.cpp',
>      'v4l2_videodevice.cpp',
> +    'yaml_emitter.cpp',
>      'yaml_parser.cpp',
>  ])
>  
> diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp
> new file mode 100644
> index 000000000000..1f7651ffcb24
> --- /dev/null
> +++ b/src/libcamera/yaml_emitter.cpp
> @@ -0,0 +1,427 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board Oy
> + *
> + * libcamera YAML emitter helper
> + */
> +
> +#include "libcamera/internal/yaml_emitter.h"
> +
> +#include <libcamera/base/log.h>
> +
> +/**
> + * \file yaml_emitter.h
> + * \brief A YAML emitter helper
> + *
> + * The YAML Emitter helpers allows users to emit output in YAML format.
> + *
> + * To emit YAML users of this classes should create a root node with
> + *
> + * \code
> +	std::string filePath("...");
> +	auto root = YamlEmitter::root(filePath);
> +   \endcode
> + *
> + * A YamlRoot implements RAII-style handling of YAML sequence and document
> + * events handling. Creating a YamlRoot emits the STREAM_START and DOC_START
> + * events. Once a YamlRoot gets deleted the DOC_SEND and STREAM_END events
> + * gets automatically deleted.
> + *
> + * Once a root node has been created it is possible to populate it with
> + * scalars, list or dictionaries.
> + *
> + * YamlList and YamlDict can only be created wrapped in unique_ptr<> instances,
> + * to implement a RAII-style handling of YAML of lists and dictionaries.
> + * Creating a YamlList and a YamlDict emits the YAML sequence and mapping start
> + * events. Once an instance gets deleted, the sequence and mapping gets
> + * automatically emitted. If a list of dictionary needs to be closed before
> + * it gets deleted, it can be explicitly closed with the close() function.
> + *
> + * A YamlScalar is a simpler object that can be assigned to different types
> + * to emit them as strings.
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(YamlEmitter)
> +
> +namespace {
> +
> +int yamlWrite(void *data, unsigned char *buffer, size_t size)
> +{
> +	File *file = static_cast<File *>(data);
> +
> +	Span<unsigned char> buf{ buffer, size };
> +	ssize_t ret = file->write(buf);
> +	if (ret < 0) {
> +		ret = errno;
> +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \class YamlEvent
> + *
> + * Class that represents a yaml_event that automatically cleans-up on
> + * destruction.
> + */
> +class YamlEvent
> +{
> +public:
> +	static std::unique_ptr<YamlEvent> create();
> +
> +	const yaml_event_t *event() const
> +	{
> +		return &event_;
> +	}
> +
> +	yaml_event_t *event()
> +	{
> +		return &event_;
> +	}
> +
> +private:
> +	YamlEvent() = default;
> +
> +	yaml_event_t event_;
> +};
> +
> +std::unique_ptr<YamlEvent> YamlEvent::create()
> +{
> +	struct Deleter : std::default_delete<YamlEvent> {
> +		void operator()(YamlEvent *yamlEvent)
> +		{
> +			yaml_event_delete(yamlEvent->event());
> +		}
> +	};
> +
> +	return std::unique_ptr<YamlEvent>(new YamlEvent(), Deleter());

Here too, we should avoid dynamic allocations on the heap. I don't think
you need a custom deleter, you can simply call yaml_event_delete() in
the destructor of the YamlEvent class, and you then don't need to use
unique pointers.

> +}
> +
> +/**
> + * \class YamlEmitter
> + *
> + * Yaml Emitter entry point. Allows to create a YamlRoot object that users
> + * can populate.
> + */
> +
> +/**
> + * \brief Create a YamlRoot that applications can start populating with YamlOutput
> + * \param[in] path The YAML output file path
> + * \return A unique pointer to a YamlRoot
> + */
> +std::unique_ptr<YamlRoot> YamlEmitter::root(std::string_view path)
> +{
> +	YamlEmitter *emitter = new YamlEmitter();
> +
> +	std::string filePath(path);
> +	emitter->file_ = std::make_unique<File>(filePath);
> +	emitter->file_->open(File::OpenModeFlag::WriteOnly);
> +
> +	emitter->init();
> +
> +	YamlRoot *root = new YamlRoot(emitter);
> +	return std::unique_ptr<YamlRoot>(root);
> +}
> +
> +/**
> + * \brief Emit a YamlEvent
> + */
> +int YamlEmitter::emit(YamlEvent *event)
> +{
> +	return emitter_.emit(event);
> +}
> +
> +void YamlEmitter::init()
> +{
> +	emitter_.init(file_.get());
> +
> +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> +
> +	yaml_stream_start_event_initialize(event->event(), YAML_UTF8_ENCODING);
> +	emitter_.emit(event.get());
> +
> +	yaml_document_start_event_initialize(event->event(), NULL, NULL,
> +					     NULL, 0);
> +	emitter_.emit(event.get());
> +}
> +
> +/**
> + * \class YamlEmitter::Emitter
> + *
> + * yaml_emitter_t wrapper. Initialize the yaml_emitter_t on creation allows
> + * YamlOutput classes to emit events.
> + */
> +
> +YamlEmitter::Emitter::~Emitter()
> +{
> +	yaml_emitter_delete(&emitter_);
> +}
> +
> +void YamlEmitter::Emitter::init(File *file)
> +{
> +	yaml_emitter_initialize(&emitter_);
> +	yaml_emitter_set_output(&emitter_, yamlWrite, file);
> +}
> +
> +void YamlEmitter::Emitter::logError()
> +{
> +	switch (emitter_.error) {
> +	case YAML_MEMORY_ERROR:
> +		LOG(YamlEmitter, Error)
> +			<< "Memory error: Not enough memory for emitting";
> +		break;
> +
> +	case YAML_WRITER_ERROR:
> +		LOG(YamlEmitter, Error)
> +			<< "Writer error: " << emitter_.problem;
> +		break;
> +
> +	case YAML_EMITTER_ERROR:
> +		LOG(YamlEmitter, Error)
> +			<< "Emitter error: " << emitter_.problem;
> +		break;
> +
> +	default:
> +		LOG(YamlEmitter, Error) << "Internal problem";
> +		break;
> +	}
> +}
> +
> +int YamlEmitter::Emitter::emit(YamlEvent *event)
> +{
> +	int ret = yaml_emitter_emit(&emitter_, event->event());
> +	if (!ret) {
> +		logError();
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \class YamlOutput
> + *
> + * The YamlOutput base class. From this class YamlScalar, YamlList and YamlDict
> + * are derived.
> + *
> + * The YamlOutput class provides functions to create a scalar, a list or a
> + * dictionary.
> + *
> + * The class cannot be instantiated directly by applications.
> + */
> +
> +std::unique_ptr<YamlScalar> YamlOutput::scalar()
> +{
> +	return std::unique_ptr<YamlScalar>(new YamlScalar(emitter_));
> +}
> +
> +std::unique_ptr<YamlDict> YamlOutput::dict()
> +{
> +	return std::unique_ptr<YamlDict>(new YamlDict(emitter_));
> +}
> +
> +std::unique_ptr<YamlList> YamlOutput::list()
> +{
> +	return std::unique_ptr<YamlList>(new YamlList(emitter_));
> +}
> +
> +int YamlOutput::emitScalar(std::string_view scalar)
> +{
> +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> +
> +	const unsigned char *value = reinterpret_cast<const unsigned char *>
> +				     (scalar.data());
> +	yaml_scalar_event_initialize(event->event(), NULL, NULL, value,
> +				     scalar.length(), true, false,
> +				     YAML_PLAIN_SCALAR_STYLE);
> +	return emitter_->emit(event.get());
> +}
> +
> +int YamlOutput::emitMappingStart()
> +{
> +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> +	yaml_mapping_start_event_initialize(event->event(), NULL, NULL,
> +					    true, YAML_BLOCK_MAPPING_STYLE);
> +	return emitter_->emit(event.get());
> +}
> +
> +int YamlOutput::emitMappingEnd()
> +{
> +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> +	yaml_mapping_end_event_initialize(event->event());
> +	return emitter_->emit(event.get());
> +}
> +
> +int YamlOutput::emitSequenceStart()
> +{
> +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> +	yaml_sequence_start_event_initialize(event->event(), NULL, NULL, true,
> +					     YAML_BLOCK_SEQUENCE_STYLE);
> +	return emitter_->emit(event.get());
> +}
> +
> +int YamlOutput::emitSequenceEnd()
> +{
> +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> +	yaml_sequence_end_event_initialize(event->event());
> +	return emitter_->emit(event.get());
> +}
> +
> +/**
> + * \class YamlRoot
> + *
> + * Yaml root node. A root node can be populated with a scalar, a list or a dict.
> + */
> +
> +YamlRoot::~YamlRoot()
> +{
> +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> +
> +	yaml_document_end_event_initialize(event->event(), 0);
> +	emitterRoot_->emit(event.get());
> +
> +	yaml_stream_end_event_initialize(event->event());
> +	emitterRoot_->emit(event.get());
> +}
> +
> +std::unique_ptr<YamlDict> YamlRoot::dict()
> +{
> +	emitMappingStart();
> +
> +	return YamlOutput::dict();
> +}
> +
> +std::unique_ptr<YamlList> YamlRoot::list()
> +{
> +	emitSequenceStart();
> +
> +	return YamlOutput::list();
> +}
> +
> +/**
> + * \class YamlScalar
> + *
> + * A YamlScalar can be assigned to an std::string_view and other libcamera
> + * types to emit them as YAML scalars.
> + */
> +
> +YamlScalar::YamlScalar(YamlEmitter *emitter)
> +	: YamlOutput(emitter)
> +{
> +}
> +
> +void YamlScalar::operator=(const libcamera::Orientation &orientation)
> +{
> +	std::stringstream o;
> +	o << orientation;
> +
> +	emitScalar(o.str());
> +}
> +
> +void YamlScalar::operator=(std::string_view scalar)
> +{
> +	emitScalar(scalar);
> +}
> +
> +/**
> + * \class YamlList
> + *
> + * A YamlList can be populated with scalar and allows to create other lists
> + * and dictionaries.
> + */
> +
> +YamlList::YamlList(YamlEmitter *emitter)
> +	: YamlOutput(emitter)
> +{
> +}
> +
> +YamlList::~YamlList()
> +{
> +	if (!closed_)
> +		close();
> +}
> +
> +void YamlList::close()
> +{
> +	emitSequenceEnd();
> +	YamlOutput::close();
> +}
> +
> +void YamlList::scalar(std::string_view scalar)
> +{
> +	emitScalar(scalar);
> +}
> +
> +std::unique_ptr<YamlList> YamlList::list()
> +{
> +	emitSequenceStart();
> +
> +	return YamlOutput::list();
> +}
> +
> +std::unique_ptr<YamlDict> YamlList::dict()
> +{
> +	emitMappingStart();
> +
> +	return YamlOutput::dict();
> +}
> +
> +/**
> + * \class YamlDict
> + *
> + * A YamlDict derives an unordered_map that maps strings to YAML scalar.
> + *
> + * A YamlDict can be populated with scalars using operator[] and allows to
> + * create other lists and dictionaries.
> + */
> +
> +YamlDict::YamlDict(YamlEmitter *emitter)
> +	: YamlOutput(emitter)
> +{
> +}
> +
> +YamlDict::~YamlDict()
> +{
> +	if (!closed_)
> +		close();
> +
> +	clear();
> +}
> +
> +void YamlDict::close()
> +{
> +	emitMappingEnd();
> +	YamlOutput::close();
> +}
> +
> +std::unique_ptr<YamlList> YamlDict::list(std::string_view key)
> +{
> +	emitScalar(key);
> +	emitSequenceStart();
> +
> +	return YamlOutput::list();
> +}
> +
> +std::unique_ptr<YamlDict> YamlDict::dict(std::string_view key)
> +{
> +	emitScalar(key);
> +	emitMappingStart();
> +
> +	return YamlOutput::dict();
> +}
> +
> +YamlScalar &YamlDict::operator[](const YamlDict::Map::key_type &key)
> +{
> +	emplace(key, YamlOutput::scalar());
> +	emitScalar(key);
> +
> +	return *at(key);
> +}
> +
> +} /* namespace libcamera */

Hi Laurent,
    thanks for review

On Tue, Oct 15, 2024 at 01:21:33PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> I've kept the review high-level to start with.
>
> On Mon, Oct 14, 2024 at 11:59:35AM +0200, Jacopo Mondi wrote:
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  include/libcamera/internal/meson.build    |   1 +
> >  include/libcamera/internal/yaml_emitter.h | 172 +++++++++
> >  src/libcamera/meson.build                 |   1 +
> >  src/libcamera/yaml_emitter.cpp            | 427 ++++++++++++++++++++++
> >  4 files changed, 601 insertions(+)
> >  create mode 100644 include/libcamera/internal/yaml_emitter.h
> >  create mode 100644 src/libcamera/yaml_emitter.cpp
> >
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 1c5eef9cab80..7533b075fde2 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([
> >      'v4l2_pixelformat.h',
> >      'v4l2_subdevice.h',
> >      'v4l2_videodevice.h',
> > +    'yaml_emitter.h',
> >      'yaml_parser.h',
> >  ])
> >
> > diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h
> > new file mode 100644
> > index 000000000000..3fe05f97de70
> > --- /dev/null
> > +++ b/include/libcamera/internal/yaml_emitter.h
> > @@ -0,0 +1,172 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board Oy
> > + *
> > + * libcamera YAML emitter helper
> > + */
> > +
> > +#pragma once
> > +
> > +#include <memory>
> > +#include <string_view>
> > +#include <unordered_map>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/file.h>
> > +#include <libcamera/orientation.h>
> > +
> > +#include <yaml.h>
> > +
> > +namespace libcamera {
> > +
> > +class YamlDict;
> > +class YamlEvent;
> > +class YamlList;
> > +class YamlRoot;
> > +class YamlScalar;
> > +
> > +class YamlEmitter final
> > +{
> > +public:
> > +	~YamlEmitter() = default;
> > +
> > +	static std::unique_ptr<YamlRoot> root(std::string_view path);
> > +
> > +	int emit(YamlEvent *event);
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> > +
> > +	class Emitter
> > +	{
> > +	public:
> > +		Emitter() = default;
> > +		~Emitter();
> > +
> > +		void init(File *file);
> > +
> > +		int emit(YamlEvent *event);
> > +
> > +	private:
> > +		void logError();
> > +
> > +		yaml_emitter_t emitter_;
> > +	};
> > +
> > +	YamlEmitter() = default;
> > +
> > +	void init();
> > +
> > +	std::unique_ptr<File> file_;
> > +	Emitter emitter_;
> > +};
> > +
> > +class YamlOutput
> > +{
> > +public:
>
> Shouldn't you disable copying of this class ?
>

yup

> > +	virtual ~YamlOutput() = default;
> > +
> > +	void close()
> > +	{
> > +		closed_ = true;
> > +	}
>
> Can we avoid this and close on destruction only ? Having an explicit
> close is error-prone, what if someone calls one of the functions below
> after calling close() ? The API should make it very hard, if not
> impossible, to do the wrong thing.
>

Eh

my idea was ideed to rely on object's destruction to emit the correct
yaml events, then I thought it was worth to allow users to close a
sequence/mapping earlier if they didn't want to enforce this through
the object lifetime. I think I had a use case for this, but maybe it
has gone during the development, I'll try remove close() and solely
rely on object's lifetime for the correct events emission.

> > +
> > +	std::unique_ptr<YamlScalar> scalar();
> > +	std::unique_ptr<YamlDict> dict();
> > +	std::unique_ptr<YamlList> list();
>
> In many cases the YamlScalar, YamlDict and YamlList classes will be
> local to the caller function. It would be best to avoid dynamic heap
> allocation. You could do so by returning values here. Same below. You
> will likely need to implement move constructors and operators.
>


good idea, I'll try

> > +
> > +protected:
> > +	YamlOutput(YamlEmitter *emitter)
> > +		: emitter_(emitter), closed_(false)
> > +	{
> > +	}
> > +
> > +	int emitScalar(std::string_view scalar);
> > +	int emitMappingStart();
> > +	int emitMappingEnd();
> > +	int emitSequenceStart();
> > +	int emitSequenceEnd();
> > +
> > +	YamlEmitter *emitter_;
> > +
> > +	bool closed_;
> > +};
> > +
> > +class YamlRoot : public YamlOutput
> > +{
> > +public:
> > +	~YamlRoot();
> > +	void close() {}
> > +
> > +	std::unique_ptr<YamlList> list();
> > +	std::unique_ptr<YamlDict> dict();
> > +	void scalar(std::string_view scalar);
> > +
> > +private:
> > +	friend class YamlEmitter;
> > +
> > +	YamlRoot(YamlEmitter *emitter)
> > +		: YamlOutput(emitter)
> > +	{
> > +		emitterRoot_ = std::unique_ptr<YamlEmitter>(emitter);
> > +	}
> > +
> > +	std::unique_ptr<YamlEmitter> emitterRoot_;
> > +};
> > +
> > +class YamlScalar : public YamlOutput
> > +{
> > +public:
> > +	~YamlScalar() = default;
> > +
> > +	void close() {}
> > +
> > +	void operator=(const Orientation &orientation);
>
> I don't think this should be a member of the YamlScalar class. You
> should convert the Orientation to a string in the caller.
>

This is something I wanted indeed feedbacks on. I see it two ways,
either we augment this interface with libcamera types as we need them
to ease for users of this class to emit them, or we require users to
convert to string for. I understand your preference is for the latter
option.

> > +	void operator=(std::string_view scalar);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlScalar(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlList : public YamlOutput
> > +{
> > +public:
> > +	~YamlList();
> > +
> > +	void close();
> > +
> > +	std::unique_ptr<YamlList> list();
> > +	std::unique_ptr<YamlDict> dict();
> > +	void scalar(std::string_view scalar);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlList(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlDict : public YamlOutput,
> > +		 private std::unordered_map<std::string_view,
> > +					    std::unique_ptr<YamlScalar>>
>
> Why do you need to store content ? That doesn't sound right,the
> emitter's job is to emit YAML, we shouldn't need to store anything.
>

because I wanted to override operator[] for YamlDict and it returns a
refernce, so I need to have a storage. I can try to remove it.

> > +{
> > +public:
> > +	using Map = std::unordered_map<std::string_view, YamlScalar>;
> > +
> > +	~YamlDict();
> > +
> > +	void close();
> > +
> > +	std::unique_ptr<YamlList> list(std::string_view key);
> > +	std::unique_ptr<YamlDict> dict(std::string_view key);
> > +
> > +	YamlScalar &operator[](const Map::key_type &key);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlDict(YamlEmitter *emitter);
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index aa9ab0291854..5b8af4103085 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([
> >      'v4l2_pixelformat.cpp',
> >      'v4l2_subdevice.cpp',
> >      'v4l2_videodevice.cpp',
> > +    'yaml_emitter.cpp',
> >      'yaml_parser.cpp',
> >  ])
> >
> > diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp
> > new file mode 100644
> > index 000000000000..1f7651ffcb24
> > --- /dev/null
> > +++ b/src/libcamera/yaml_emitter.cpp
> > @@ -0,0 +1,427 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board Oy
> > + *
> > + * libcamera YAML emitter helper
> > + */
> > +
> > +#include "libcamera/internal/yaml_emitter.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +/**
> > + * \file yaml_emitter.h
> > + * \brief A YAML emitter helper
> > + *
> > + * The YAML Emitter helpers allows users to emit output in YAML format.
> > + *
> > + * To emit YAML users of this classes should create a root node with
> > + *
> > + * \code
> > +	std::string filePath("...");
> > +	auto root = YamlEmitter::root(filePath);
> > +   \endcode
> > + *
> > + * A YamlRoot implements RAII-style handling of YAML sequence and document
> > + * events handling. Creating a YamlRoot emits the STREAM_START and DOC_START
> > + * events. Once a YamlRoot gets deleted the DOC_SEND and STREAM_END events
> > + * gets automatically deleted.
> > + *
> > + * Once a root node has been created it is possible to populate it with
> > + * scalars, list or dictionaries.
> > + *
> > + * YamlList and YamlDict can only be created wrapped in unique_ptr<> instances,
> > + * to implement a RAII-style handling of YAML of lists and dictionaries.
> > + * Creating a YamlList and a YamlDict emits the YAML sequence and mapping start
> > + * events. Once an instance gets deleted, the sequence and mapping gets
> > + * automatically emitted. If a list of dictionary needs to be closed before
> > + * it gets deleted, it can be explicitly closed with the close() function.
> > + *
> > + * A YamlScalar is a simpler object that can be assigned to different types
> > + * to emit them as strings.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(YamlEmitter)
> > +
> > +namespace {
> > +
> > +int yamlWrite(void *data, unsigned char *buffer, size_t size)
> > +{
> > +	File *file = static_cast<File *>(data);
> > +
> > +	Span<unsigned char> buf{ buffer, size };
> > +	ssize_t ret = file->write(buf);
> > +	if (ret < 0) {
> > +		ret = errno;
> > +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \class YamlEvent
> > + *
> > + * Class that represents a yaml_event that automatically cleans-up on
> > + * destruction.
> > + */
> > +class YamlEvent
> > +{
> > +public:
> > +	static std::unique_ptr<YamlEvent> create();
> > +
> > +	const yaml_event_t *event() const
> > +	{
> > +		return &event_;
> > +	}
> > +
> > +	yaml_event_t *event()
> > +	{
> > +		return &event_;
> > +	}
> > +
> > +private:
> > +	YamlEvent() = default;
> > +
> > +	yaml_event_t event_;
> > +};
> > +
> > +std::unique_ptr<YamlEvent> YamlEvent::create()
> > +{
> > +	struct Deleter : std::default_delete<YamlEvent> {
> > +		void operator()(YamlEvent *yamlEvent)
> > +		{
> > +			yaml_event_delete(yamlEvent->event());
> > +		}
> > +	};
> > +
> > +	return std::unique_ptr<YamlEvent>(new YamlEvent(), Deleter());
>
> Here too, we should avoid dynamic allocations on the heap. I don't think
> you need a custom deleter, you can simply call yaml_event_delete() in
> the destructor of the YamlEvent class, and you then don't need to use
> unique pointers.
>
> > +}
> > +
> > +/**
> > + * \class YamlEmitter
> > + *
> > + * Yaml Emitter entry point. Allows to create a YamlRoot object that users
> > + * can populate.
> > + */
> > +
> > +/**
> > + * \brief Create a YamlRoot that applications can start populating with YamlOutput
> > + * \param[in] path The YAML output file path
> > + * \return A unique pointer to a YamlRoot
> > + */
> > +std::unique_ptr<YamlRoot> YamlEmitter::root(std::string_view path)
> > +{
> > +	YamlEmitter *emitter = new YamlEmitter();
> > +
> > +	std::string filePath(path);
> > +	emitter->file_ = std::make_unique<File>(filePath);
> > +	emitter->file_->open(File::OpenModeFlag::WriteOnly);
> > +
> > +	emitter->init();
> > +
> > +	YamlRoot *root = new YamlRoot(emitter);
> > +	return std::unique_ptr<YamlRoot>(root);
> > +}
> > +
> > +/**
> > + * \brief Emit a YamlEvent
> > + */
> > +int YamlEmitter::emit(YamlEvent *event)
> > +{
> > +	return emitter_.emit(event);
> > +}
> > +
> > +void YamlEmitter::init()
> > +{
> > +	emitter_.init(file_.get());
> > +
> > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +
> > +	yaml_stream_start_event_initialize(event->event(), YAML_UTF8_ENCODING);
> > +	emitter_.emit(event.get());
> > +
> > +	yaml_document_start_event_initialize(event->event(), NULL, NULL,
> > +					     NULL, 0);
> > +	emitter_.emit(event.get());
> > +}
> > +
> > +/**
> > + * \class YamlEmitter::Emitter
> > + *
> > + * yaml_emitter_t wrapper. Initialize the yaml_emitter_t on creation allows
> > + * YamlOutput classes to emit events.
> > + */
> > +
> > +YamlEmitter::Emitter::~Emitter()
> > +{
> > +	yaml_emitter_delete(&emitter_);
> > +}
> > +
> > +void YamlEmitter::Emitter::init(File *file)
> > +{
> > +	yaml_emitter_initialize(&emitter_);
> > +	yaml_emitter_set_output(&emitter_, yamlWrite, file);
> > +}
> > +
> > +void YamlEmitter::Emitter::logError()
> > +{
> > +	switch (emitter_.error) {
> > +	case YAML_MEMORY_ERROR:
> > +		LOG(YamlEmitter, Error)
> > +			<< "Memory error: Not enough memory for emitting";
> > +		break;
> > +
> > +	case YAML_WRITER_ERROR:
> > +		LOG(YamlEmitter, Error)
> > +			<< "Writer error: " << emitter_.problem;
> > +		break;
> > +
> > +	case YAML_EMITTER_ERROR:
> > +		LOG(YamlEmitter, Error)
> > +			<< "Emitter error: " << emitter_.problem;
> > +		break;
> > +
> > +	default:
> > +		LOG(YamlEmitter, Error) << "Internal problem";
> > +		break;
> > +	}
> > +}
> > +
> > +int YamlEmitter::Emitter::emit(YamlEvent *event)
> > +{
> > +	int ret = yaml_emitter_emit(&emitter_, event->event());
> > +	if (!ret) {
> > +		logError();
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \class YamlOutput
> > + *
> > + * The YamlOutput base class. From this class YamlScalar, YamlList and YamlDict
> > + * are derived.
> > + *
> > + * The YamlOutput class provides functions to create a scalar, a list or a
> > + * dictionary.
> > + *
> > + * The class cannot be instantiated directly by applications.
> > + */
> > +
> > +std::unique_ptr<YamlScalar> YamlOutput::scalar()
> > +{
> > +	return std::unique_ptr<YamlScalar>(new YamlScalar(emitter_));
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlOutput::dict()
> > +{
> > +	return std::unique_ptr<YamlDict>(new YamlDict(emitter_));
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlOutput::list()
> > +{
> > +	return std::unique_ptr<YamlList>(new YamlList(emitter_));
> > +}
> > +
> > +int YamlOutput::emitScalar(std::string_view scalar)
> > +{
> > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +
> > +	const unsigned char *value = reinterpret_cast<const unsigned char *>
> > +				     (scalar.data());
> > +	yaml_scalar_event_initialize(event->event(), NULL, NULL, value,
> > +				     scalar.length(), true, false,
> > +				     YAML_PLAIN_SCALAR_STYLE);
> > +	return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitMappingStart()
> > +{
> > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +	yaml_mapping_start_event_initialize(event->event(), NULL, NULL,
> > +					    true, YAML_BLOCK_MAPPING_STYLE);
> > +	return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitMappingEnd()
> > +{
> > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +	yaml_mapping_end_event_initialize(event->event());
> > +	return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitSequenceStart()
> > +{
> > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +	yaml_sequence_start_event_initialize(event->event(), NULL, NULL, true,
> > +					     YAML_BLOCK_SEQUENCE_STYLE);
> > +	return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitSequenceEnd()
> > +{
> > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +	yaml_sequence_end_event_initialize(event->event());
> > +	return emitter_->emit(event.get());
> > +}
> > +
> > +/**
> > + * \class YamlRoot
> > + *
> > + * Yaml root node. A root node can be populated with a scalar, a list or a dict.
> > + */
> > +
> > +YamlRoot::~YamlRoot()
> > +{
> > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +
> > +	yaml_document_end_event_initialize(event->event(), 0);
> > +	emitterRoot_->emit(event.get());
> > +
> > +	yaml_stream_end_event_initialize(event->event());
> > +	emitterRoot_->emit(event.get());
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlRoot::dict()
> > +{
> > +	emitMappingStart();
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlRoot::list()
> > +{
> > +	emitSequenceStart();
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +/**
> > + * \class YamlScalar
> > + *
> > + * A YamlScalar can be assigned to an std::string_view and other libcamera
> > + * types to emit them as YAML scalars.
> > + */
> > +
> > +YamlScalar::YamlScalar(YamlEmitter *emitter)
> > +	: YamlOutput(emitter)
> > +{
> > +}
> > +
> > +void YamlScalar::operator=(const libcamera::Orientation &orientation)
> > +{
> > +	std::stringstream o;
> > +	o << orientation;
> > +
> > +	emitScalar(o.str());
> > +}
> > +
> > +void YamlScalar::operator=(std::string_view scalar)
> > +{
> > +	emitScalar(scalar);
> > +}
> > +
> > +/**
> > + * \class YamlList
> > + *
> > + * A YamlList can be populated with scalar and allows to create other lists
> > + * and dictionaries.
> > + */
> > +
> > +YamlList::YamlList(YamlEmitter *emitter)
> > +	: YamlOutput(emitter)
> > +{
> > +}
> > +
> > +YamlList::~YamlList()
> > +{
> > +	if (!closed_)
> > +		close();
> > +}
> > +
> > +void YamlList::close()
> > +{
> > +	emitSequenceEnd();
> > +	YamlOutput::close();
> > +}
> > +
> > +void YamlList::scalar(std::string_view scalar)
> > +{
> > +	emitScalar(scalar);
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlList::list()
> > +{
> > +	emitSequenceStart();
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlList::dict()
> > +{
> > +	emitMappingStart();
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +/**
> > + * \class YamlDict
> > + *
> > + * A YamlDict derives an unordered_map that maps strings to YAML scalar.
> > + *
> > + * A YamlDict can be populated with scalars using operator[] and allows to
> > + * create other lists and dictionaries.
> > + */
> > +
> > +YamlDict::YamlDict(YamlEmitter *emitter)
> > +	: YamlOutput(emitter)
> > +{
> > +}
> > +
> > +YamlDict::~YamlDict()
> > +{
> > +	if (!closed_)
> > +		close();
> > +
> > +	clear();
> > +}
> > +
> > +void YamlDict::close()
> > +{
> > +	emitMappingEnd();
> > +	YamlOutput::close();
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlDict::list(std::string_view key)
> > +{
> > +	emitScalar(key);
> > +	emitSequenceStart();
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlDict::dict(std::string_view key)
> > +{
> > +	emitScalar(key);
> > +	emitMappingStart();
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +YamlScalar &YamlDict::operator[](const YamlDict::Map::key_type &key)
> > +{
> > +	emplace(key, YamlOutput::scalar());
> > +	emitScalar(key);
> > +
> > +	return *at(key);
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Tue, Oct 15, 2024 at 02:16:11PM +0200, Jacopo Mondi wrote:
> On Tue, Oct 15, 2024 at 01:21:33PM GMT, Laurent Pinchart wrote:
> > On Mon, Oct 14, 2024 at 11:59:35AM +0200, Jacopo Mondi wrote:
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/meson.build    |   1 +
> > >  include/libcamera/internal/yaml_emitter.h | 172 +++++++++
> > >  src/libcamera/meson.build                 |   1 +
> > >  src/libcamera/yaml_emitter.cpp            | 427 ++++++++++++++++++++++
> > >  4 files changed, 601 insertions(+)
> > >  create mode 100644 include/libcamera/internal/yaml_emitter.h
> > >  create mode 100644 src/libcamera/yaml_emitter.cpp
> > >
> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > index 1c5eef9cab80..7533b075fde2 100644
> > > --- a/include/libcamera/internal/meson.build
> > > +++ b/include/libcamera/internal/meson.build
> > > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([
> > >      'v4l2_pixelformat.h',
> > >      'v4l2_subdevice.h',
> > >      'v4l2_videodevice.h',
> > > +    'yaml_emitter.h',
> > >      'yaml_parser.h',
> > >  ])
> > >
> > > diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h
> > > new file mode 100644
> > > index 000000000000..3fe05f97de70
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/yaml_emitter.h
> > > @@ -0,0 +1,172 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board Oy
> > > + *
> > > + * libcamera YAML emitter helper
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <memory>
> > > +#include <string_view>
> > > +#include <unordered_map>
> > > +
> > > +#include <libcamera/base/class.h>
> > > +#include <libcamera/base/file.h>
> > > +#include <libcamera/orientation.h>
> > > +
> > > +#include <yaml.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class YamlDict;
> > > +class YamlEvent;
> > > +class YamlList;
> > > +class YamlRoot;
> > > +class YamlScalar;
> > > +
> > > +class YamlEmitter final
> > > +{
> > > +public:
> > > +	~YamlEmitter() = default;
> > > +
> > > +	static std::unique_ptr<YamlRoot> root(std::string_view path);
> > > +
> > > +	int emit(YamlEvent *event);
> > > +
> > > +private:
> > > +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> > > +
> > > +	class Emitter
> > > +	{
> > > +	public:
> > > +		Emitter() = default;
> > > +		~Emitter();
> > > +
> > > +		void init(File *file);
> > > +
> > > +		int emit(YamlEvent *event);
> > > +
> > > +	private:
> > > +		void logError();
> > > +
> > > +		yaml_emitter_t emitter_;
> > > +	};
> > > +
> > > +	YamlEmitter() = default;
> > > +
> > > +	void init();
> > > +
> > > +	std::unique_ptr<File> file_;
> > > +	Emitter emitter_;
> > > +};
> > > +
> > > +class YamlOutput
> > > +{
> > > +public:
> >
> > Shouldn't you disable copying of this class ?
> 
> yup
> 
> > > +	virtual ~YamlOutput() = default;
> > > +
> > > +	void close()
> > > +	{
> > > +		closed_ = true;
> > > +	}
> >
> > Can we avoid this and close on destruction only ? Having an explicit
> > close is error-prone, what if someone calls one of the functions below
> > after calling close() ? The API should make it very hard, if not
> > impossible, to do the wrong thing.
> 
> Eh
> 
> my idea was ideed to rely on object's destruction to emit the correct
> yaml events, then I thought it was worth to allow users to close a
> sequence/mapping earlier if they didn't want to enforce this through
> the object lifetime. I think I had a use case for this, but maybe it
> has gone during the development, I'll try remove close() and solely
> rely on object's lifetime for the correct events emission.

I'd like to at least hear about shortcomings of this option, if there
are any.

One thing I'm concerned about is how this kind of pattern is invalid:

	YamlList &parent = ...;

	YamlList child = parent.list();

	parent.scalar("value");

	child.scalar("value");

It would be nice if we could design an API that would prevent this at
compile time. I don't know if that's possible (with reasonable
implementation and runtime complexity).

> > > +
> > > +	std::unique_ptr<YamlScalar> scalar();
> > > +	std::unique_ptr<YamlDict> dict();
> > > +	std::unique_ptr<YamlList> list();
> >
> > In many cases the YamlScalar, YamlDict and YamlList classes will be
> > local to the caller function. It would be best to avoid dynamic heap
> > allocation. You could do so by returning values here. Same below. You
> > will likely need to implement move constructors and operators.
> 
> good idea, I'll try
> 
> > > +
> > > +protected:
> > > +	YamlOutput(YamlEmitter *emitter)
> > > +		: emitter_(emitter), closed_(false)
> > > +	{
> > > +	}
> > > +
> > > +	int emitScalar(std::string_view scalar);
> > > +	int emitMappingStart();
> > > +	int emitMappingEnd();
> > > +	int emitSequenceStart();
> > > +	int emitSequenceEnd();
> > > +
> > > +	YamlEmitter *emitter_;
> > > +
> > > +	bool closed_;
> > > +};
> > > +
> > > +class YamlRoot : public YamlOutput
> > > +{
> > > +public:
> > > +	~YamlRoot();
> > > +	void close() {}
> > > +
> > > +	std::unique_ptr<YamlList> list();
> > > +	std::unique_ptr<YamlDict> dict();
> > > +	void scalar(std::string_view scalar);
> > > +
> > > +private:
> > > +	friend class YamlEmitter;
> > > +
> > > +	YamlRoot(YamlEmitter *emitter)
> > > +		: YamlOutput(emitter)
> > > +	{
> > > +		emitterRoot_ = std::unique_ptr<YamlEmitter>(emitter);
> > > +	}
> > > +
> > > +	std::unique_ptr<YamlEmitter> emitterRoot_;
> > > +};
> > > +
> > > +class YamlScalar : public YamlOutput
> > > +{
> > > +public:
> > > +	~YamlScalar() = default;
> > > +
> > > +	void close() {}
> > > +
> > > +	void operator=(const Orientation &orientation);
> >
> > I don't think this should be a member of the YamlScalar class. You
> > should convert the Orientation to a string in the caller.
> 
> This is something I wanted indeed feedbacks on. I see it two ways,
> either we augment this interface with libcamera types as we need them
> to ease for users of this class to emit them, or we require users to
> convert to string for. I understand your preference is for the latter
> option.

One option would be to add an operator<<() implementation to output a
libcamera type to a YamlScaler. That way the YamlScalar implementation
would not be dependent on specific types, and each type could provide
its own implementation. Let's see how the callers look like, it may be
overkill if we only use the YamlEmitter in a single place.

> > > +	void operator=(std::string_view scalar);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlScalar(YamlEmitter *emitter);
> > > +};
> > > +
> > > +class YamlList : public YamlOutput
> > > +{
> > > +public:
> > > +	~YamlList();
> > > +
> > > +	void close();
> > > +
> > > +	std::unique_ptr<YamlList> list();
> > > +	std::unique_ptr<YamlDict> dict();
> > > +	void scalar(std::string_view scalar);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlList(YamlEmitter *emitter);
> > > +};
> > > +
> > > +class YamlDict : public YamlOutput,
> > > +		 private std::unordered_map<std::string_view,
> > > +					    std::unique_ptr<YamlScalar>>
> >
> > Why do you need to store content ? That doesn't sound right,the
> > emitter's job is to emit YAML, we shouldn't need to store anything.
> 
> because I wanted to override operator[] for YamlDict and it returns a
> refernce, so I need to have a storage. I can try to remove it.

It would be nice to remove it. Let's discuss the drawbacks if you run
into issues.

> > > +{
> > > +public:
> > > +	using Map = std::unordered_map<std::string_view, YamlScalar>;
> > > +
> > > +	~YamlDict();
> > > +
> > > +	void close();
> > > +
> > > +	std::unique_ptr<YamlList> list(std::string_view key);
> > > +	std::unique_ptr<YamlDict> dict(std::string_view key);
> > > +
> > > +	YamlScalar &operator[](const Map::key_type &key);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlDict(YamlEmitter *emitter);
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index aa9ab0291854..5b8af4103085 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([
> > >      'v4l2_pixelformat.cpp',
> > >      'v4l2_subdevice.cpp',
> > >      'v4l2_videodevice.cpp',
> > > +    'yaml_emitter.cpp',
> > >      'yaml_parser.cpp',
> > >  ])
> > >
> > > diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp
> > > new file mode 100644
> > > index 000000000000..1f7651ffcb24
> > > --- /dev/null
> > > +++ b/src/libcamera/yaml_emitter.cpp
> > > @@ -0,0 +1,427 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas On Board Oy
> > > + *
> > > + * libcamera YAML emitter helper
> > > + */
> > > +
> > > +#include "libcamera/internal/yaml_emitter.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +/**
> > > + * \file yaml_emitter.h
> > > + * \brief A YAML emitter helper
> > > + *
> > > + * The YAML Emitter helpers allows users to emit output in YAML format.
> > > + *
> > > + * To emit YAML users of this classes should create a root node with
> > > + *
> > > + * \code
> > > +	std::string filePath("...");
> > > +	auto root = YamlEmitter::root(filePath);
> > > +   \endcode
> > > + *
> > > + * A YamlRoot implements RAII-style handling of YAML sequence and document
> > > + * events handling. Creating a YamlRoot emits the STREAM_START and DOC_START
> > > + * events. Once a YamlRoot gets deleted the DOC_SEND and STREAM_END events
> > > + * gets automatically deleted.
> > > + *
> > > + * Once a root node has been created it is possible to populate it with
> > > + * scalars, list or dictionaries.
> > > + *
> > > + * YamlList and YamlDict can only be created wrapped in unique_ptr<> instances,
> > > + * to implement a RAII-style handling of YAML of lists and dictionaries.
> > > + * Creating a YamlList and a YamlDict emits the YAML sequence and mapping start
> > > + * events. Once an instance gets deleted, the sequence and mapping gets
> > > + * automatically emitted. If a list of dictionary needs to be closed before
> > > + * it gets deleted, it can be explicitly closed with the close() function.
> > > + *
> > > + * A YamlScalar is a simpler object that can be assigned to different types
> > > + * to emit them as strings.
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(YamlEmitter)
> > > +
> > > +namespace {
> > > +
> > > +int yamlWrite(void *data, unsigned char *buffer, size_t size)
> > > +{
> > > +	File *file = static_cast<File *>(data);
> > > +
> > > +	Span<unsigned char> buf{ buffer, size };
> > > +	ssize_t ret = file->write(buf);
> > > +	if (ret < 0) {
> > > +		ret = errno;
> > > +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> > > +		return 0;
> > > +	}
> > > +
> > > +	return 1;
> > > +}
> > > +
> > > +} /* namespace */
> > > +
> > > +/**
> > > + * \class YamlEvent
> > > + *
> > > + * Class that represents a yaml_event that automatically cleans-up on
> > > + * destruction.
> > > + */
> > > +class YamlEvent
> > > +{
> > > +public:
> > > +	static std::unique_ptr<YamlEvent> create();
> > > +
> > > +	const yaml_event_t *event() const
> > > +	{
> > > +		return &event_;
> > > +	}
> > > +
> > > +	yaml_event_t *event()
> > > +	{
> > > +		return &event_;
> > > +	}
> > > +
> > > +private:
> > > +	YamlEvent() = default;
> > > +
> > > +	yaml_event_t event_;
> > > +};
> > > +
> > > +std::unique_ptr<YamlEvent> YamlEvent::create()
> > > +{
> > > +	struct Deleter : std::default_delete<YamlEvent> {
> > > +		void operator()(YamlEvent *yamlEvent)
> > > +		{
> > > +			yaml_event_delete(yamlEvent->event());
> > > +		}
> > > +	};
> > > +
> > > +	return std::unique_ptr<YamlEvent>(new YamlEvent(), Deleter());
> >
> > Here too, we should avoid dynamic allocations on the heap. I don't think
> > you need a custom deleter, you can simply call yaml_event_delete() in
> > the destructor of the YamlEvent class, and you then don't need to use
> > unique pointers.
> >
> > > +}
> > > +
> > > +/**
> > > + * \class YamlEmitter
> > > + *
> > > + * Yaml Emitter entry point. Allows to create a YamlRoot object that users
> > > + * can populate.
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a YamlRoot that applications can start populating with YamlOutput
> > > + * \param[in] path The YAML output file path
> > > + * \return A unique pointer to a YamlRoot
> > > + */
> > > +std::unique_ptr<YamlRoot> YamlEmitter::root(std::string_view path)
> > > +{
> > > +	YamlEmitter *emitter = new YamlEmitter();
> > > +
> > > +	std::string filePath(path);
> > > +	emitter->file_ = std::make_unique<File>(filePath);
> > > +	emitter->file_->open(File::OpenModeFlag::WriteOnly);
> > > +
> > > +	emitter->init();
> > > +
> > > +	YamlRoot *root = new YamlRoot(emitter);
> > > +	return std::unique_ptr<YamlRoot>(root);
> > > +}
> > > +
> > > +/**
> > > + * \brief Emit a YamlEvent
> > > + */
> > > +int YamlEmitter::emit(YamlEvent *event)
> > > +{
> > > +	return emitter_.emit(event);
> > > +}
> > > +
> > > +void YamlEmitter::init()
> > > +{
> > > +	emitter_.init(file_.get());
> > > +
> > > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > > +
> > > +	yaml_stream_start_event_initialize(event->event(), YAML_UTF8_ENCODING);
> > > +	emitter_.emit(event.get());
> > > +
> > > +	yaml_document_start_event_initialize(event->event(), NULL, NULL,
> > > +					     NULL, 0);
> > > +	emitter_.emit(event.get());
> > > +}
> > > +
> > > +/**
> > > + * \class YamlEmitter::Emitter
> > > + *
> > > + * yaml_emitter_t wrapper. Initialize the yaml_emitter_t on creation allows
> > > + * YamlOutput classes to emit events.
> > > + */
> > > +
> > > +YamlEmitter::Emitter::~Emitter()
> > > +{
> > > +	yaml_emitter_delete(&emitter_);
> > > +}
> > > +
> > > +void YamlEmitter::Emitter::init(File *file)
> > > +{
> > > +	yaml_emitter_initialize(&emitter_);
> > > +	yaml_emitter_set_output(&emitter_, yamlWrite, file);
> > > +}
> > > +
> > > +void YamlEmitter::Emitter::logError()
> > > +{
> > > +	switch (emitter_.error) {
> > > +	case YAML_MEMORY_ERROR:
> > > +		LOG(YamlEmitter, Error)
> > > +			<< "Memory error: Not enough memory for emitting";
> > > +		break;
> > > +
> > > +	case YAML_WRITER_ERROR:
> > > +		LOG(YamlEmitter, Error)
> > > +			<< "Writer error: " << emitter_.problem;
> > > +		break;
> > > +
> > > +	case YAML_EMITTER_ERROR:
> > > +		LOG(YamlEmitter, Error)
> > > +			<< "Emitter error: " << emitter_.problem;
> > > +		break;
> > > +
> > > +	default:
> > > +		LOG(YamlEmitter, Error) << "Internal problem";
> > > +		break;
> > > +	}
> > > +}
> > > +
> > > +int YamlEmitter::Emitter::emit(YamlEvent *event)
> > > +{
> > > +	int ret = yaml_emitter_emit(&emitter_, event->event());
> > > +	if (!ret) {
> > > +		logError();
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \class YamlOutput
> > > + *
> > > + * The YamlOutput base class. From this class YamlScalar, YamlList and YamlDict
> > > + * are derived.
> > > + *
> > > + * The YamlOutput class provides functions to create a scalar, a list or a
> > > + * dictionary.
> > > + *
> > > + * The class cannot be instantiated directly by applications.
> > > + */
> > > +
> > > +std::unique_ptr<YamlScalar> YamlOutput::scalar()
> > > +{
> > > +	return std::unique_ptr<YamlScalar>(new YamlScalar(emitter_));
> > > +}
> > > +
> > > +std::unique_ptr<YamlDict> YamlOutput::dict()
> > > +{
> > > +	return std::unique_ptr<YamlDict>(new YamlDict(emitter_));
> > > +}
> > > +
> > > +std::unique_ptr<YamlList> YamlOutput::list()
> > > +{
> > > +	return std::unique_ptr<YamlList>(new YamlList(emitter_));
> > > +}
> > > +
> > > +int YamlOutput::emitScalar(std::string_view scalar)
> > > +{
> > > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > > +
> > > +	const unsigned char *value = reinterpret_cast<const unsigned char *>
> > > +				     (scalar.data());
> > > +	yaml_scalar_event_initialize(event->event(), NULL, NULL, value,
> > > +				     scalar.length(), true, false,
> > > +				     YAML_PLAIN_SCALAR_STYLE);
> > > +	return emitter_->emit(event.get());
> > > +}
> > > +
> > > +int YamlOutput::emitMappingStart()
> > > +{
> > > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > > +	yaml_mapping_start_event_initialize(event->event(), NULL, NULL,
> > > +					    true, YAML_BLOCK_MAPPING_STYLE);
> > > +	return emitter_->emit(event.get());
> > > +}
> > > +
> > > +int YamlOutput::emitMappingEnd()
> > > +{
> > > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > > +	yaml_mapping_end_event_initialize(event->event());
> > > +	return emitter_->emit(event.get());
> > > +}
> > > +
> > > +int YamlOutput::emitSequenceStart()
> > > +{
> > > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > > +	yaml_sequence_start_event_initialize(event->event(), NULL, NULL, true,
> > > +					     YAML_BLOCK_SEQUENCE_STYLE);
> > > +	return emitter_->emit(event.get());
> > > +}
> > > +
> > > +int YamlOutput::emitSequenceEnd()
> > > +{
> > > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > > +	yaml_sequence_end_event_initialize(event->event());
> > > +	return emitter_->emit(event.get());
> > > +}
> > > +
> > > +/**
> > > + * \class YamlRoot
> > > + *
> > > + * Yaml root node. A root node can be populated with a scalar, a list or a dict.
> > > + */
> > > +
> > > +YamlRoot::~YamlRoot()
> > > +{
> > > +	std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > > +
> > > +	yaml_document_end_event_initialize(event->event(), 0);
> > > +	emitterRoot_->emit(event.get());
> > > +
> > > +	yaml_stream_end_event_initialize(event->event());
> > > +	emitterRoot_->emit(event.get());
> > > +}
> > > +
> > > +std::unique_ptr<YamlDict> YamlRoot::dict()
> > > +{
> > > +	emitMappingStart();
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +std::unique_ptr<YamlList> YamlRoot::list()
> > > +{
> > > +	emitSequenceStart();
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +/**
> > > + * \class YamlScalar
> > > + *
> > > + * A YamlScalar can be assigned to an std::string_view and other libcamera
> > > + * types to emit them as YAML scalars.
> > > + */
> > > +
> > > +YamlScalar::YamlScalar(YamlEmitter *emitter)
> > > +	: YamlOutput(emitter)
> > > +{
> > > +}
> > > +
> > > +void YamlScalar::operator=(const libcamera::Orientation &orientation)
> > > +{
> > > +	std::stringstream o;
> > > +	o << orientation;
> > > +
> > > +	emitScalar(o.str());
> > > +}
> > > +
> > > +void YamlScalar::operator=(std::string_view scalar)
> > > +{
> > > +	emitScalar(scalar);
> > > +}
> > > +
> > > +/**
> > > + * \class YamlList
> > > + *
> > > + * A YamlList can be populated with scalar and allows to create other lists
> > > + * and dictionaries.
> > > + */
> > > +
> > > +YamlList::YamlList(YamlEmitter *emitter)
> > > +	: YamlOutput(emitter)
> > > +{
> > > +}
> > > +
> > > +YamlList::~YamlList()
> > > +{
> > > +	if (!closed_)
> > > +		close();
> > > +}
> > > +
> > > +void YamlList::close()
> > > +{
> > > +	emitSequenceEnd();
> > > +	YamlOutput::close();
> > > +}
> > > +
> > > +void YamlList::scalar(std::string_view scalar)
> > > +{
> > > +	emitScalar(scalar);
> > > +}
> > > +
> > > +std::unique_ptr<YamlList> YamlList::list()
> > > +{
> > > +	emitSequenceStart();
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +std::unique_ptr<YamlDict> YamlList::dict()
> > > +{
> > > +	emitMappingStart();
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +/**
> > > + * \class YamlDict
> > > + *
> > > + * A YamlDict derives an unordered_map that maps strings to YAML scalar.
> > > + *
> > > + * A YamlDict can be populated with scalars using operator[] and allows to
> > > + * create other lists and dictionaries.
> > > + */
> > > +
> > > +YamlDict::YamlDict(YamlEmitter *emitter)
> > > +	: YamlOutput(emitter)
> > > +{
> > > +}
> > > +
> > > +YamlDict::~YamlDict()
> > > +{
> > > +	if (!closed_)
> > > +		close();
> > > +
> > > +	clear();
> > > +}
> > > +
> > > +void YamlDict::close()
> > > +{
> > > +	emitMappingEnd();
> > > +	YamlOutput::close();
> > > +}
> > > +
> > > +std::unique_ptr<YamlList> YamlDict::list(std::string_view key)
> > > +{
> > > +	emitScalar(key);
> > > +	emitSequenceStart();
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +std::unique_ptr<YamlDict> YamlDict::dict(std::string_view key)
> > > +{
> > > +	emitScalar(key);
> > > +	emitMappingStart();
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +YamlScalar &YamlDict::operator[](const YamlDict::Map::key_type &key)
> > > +{
> > > +	emplace(key, YamlOutput::scalar());
> > > +	emitScalar(key);
> > > +
> > > +	return *at(key);
> > > +}
> > > +
> > > +} /* namespace libcamera */