[RFC,v3,3/4] libcamera: Implement YamlEmitter

21714 diff mbox series

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 Documentation/Doxyfile-internal.in        |   2 +
 include/libcamera/internal/meson.build    |   1 +
 include/libcamera/internal/yaml_emitter.h | 183 +++++++++++
 src/libcamera/meson.build                 |   1 +
 src/libcamera/yaml_emitter.cpp            | 360 ++++++++++++++++++++++
 5 files changed, 547 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.

On Mon, Oct 21, 2024 at 11:49:51AM +0200, Jacopo Mondi wrote:
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  Documentation/Doxyfile-internal.in        |   2 +
>  include/libcamera/internal/meson.build    |   1 +
>  include/libcamera/internal/yaml_emitter.h | 183 +++++++++++
>  src/libcamera/meson.build                 |   1 +
>  src/libcamera/yaml_emitter.cpp            | 360 ++++++++++++++++++++++
>  5 files changed, 547 insertions(+)
>  create mode 100644 include/libcamera/internal/yaml_emitter.h
>  create mode 100644 src/libcamera/yaml_emitter.cpp
> 
> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
> index cf9825537866..e0ba64da1bef 100644
> --- a/Documentation/Doxyfile-internal.in
> +++ b/Documentation/Doxyfile-internal.in
> @@ -21,9 +21,11 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
>                           @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
> +                         @TOP_SRCDIR@/include/libcamera/internal/yaml_emitter.h \
>                           @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>                           @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>                           @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> +                         @TOP_SRCDIR@/src/libcamera/yaml_emitter.cpp \
>                           @TOP_SRCDIR@/src/libcamera/pipeline/ \
>                           @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>                           @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> 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..bcbb574061cc
> --- /dev/null
> +++ b/include/libcamera/internal/yaml_emitter.h
> @@ -0,0 +1,183 @@
> +/* 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 <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

This class is not part of the public API (as far as I can tell). You can
just declare it here with

class YamlEmitter;

and define it in the .cpp file.

> +{
> +public:
> +	~YamlEmitter();
> +
> +	static YamlRoot root(std::string_view path);

To make this class more generic, how about passing a std::ostream
pointer ?

> +
> +	int emit();
> +	yaml_event_t *event() { return &event_; }
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> +
> +	class Emitter
> +	{
> +	public:
> +		~Emitter();
> +
> +		void init(File *file);
> +
> +		int emit(yaml_event_t *event);
> +
> +	private:
> +		void logError();
> +
> +		yaml_emitter_t emitter_;
> +	};

Why is this split in another class ? 

> +
> +	YamlEmitter() = default;
> +
> +	void init();
> +
> +	File file_;
> +	yaml_event_t event_;
> +	Emitter emitter_;
> +};
> +
> +class YamlOutput
> +{
> +public:
> +	virtual ~YamlOutput() {};

Extraneous trailing ;

But does the class need a virtual destructor ? Is there any case where
one would delete a pointer to YamlOutput ?

> +
> +	YamlOutput() = default;

The constructors go before the destructor. And it should be protected,
as nobody should create a YamlOutput instance.

> +
> +	YamlOutput(YamlOutput &&other)
> +	{
> +		emitter_ = other.emitter_;
> +		other.emitter_ = nullptr;
> +	}

Same here, this should be protected.

> +
> +	bool initialized() { return !!emitter_; }

I would name this function valid(), as it's not just whether or not the
object has been initialized, but it also becomes invalid when moved.

The function should also be const.

> +
> +	YamlScalar scalar();
> +	YamlDict dict();
> +	YamlList list();

I think those 3 functions should be protected, they're not meant to be
called by the user as far as I understand.

> +
> +protected:
> +	YamlOutput(YamlEmitter *emitter)
> +		: emitter_(emitter)
> +	{
> +	}
> +
> +	YamlOutput &operator=(YamlOutput &&other)
> +	{
> +		emitter_ = other.emitter_;
> +		other.emitter_ = nullptr;
> +
> +		return *this;
> +	}
> +
> +	int emitScalar(std::string_view scalar);
> +	int emitMappingStart();
> +	int emitMappingEnd();
> +	int emitSequenceStart();
> +	int emitSequenceEnd();
> +
> +	YamlEmitter *emitter_ = nullptr;
> +	yaml_event_t event_;

I think you should disable copying of this class with
LIBCAMERA_DISABLE_COPY().

> +};
> +
> +class YamlRoot : public YamlOutput
> +{
> +public:
> +	YamlRoot() = default;
> +	YamlRoot(YamlRoot &&other) = default;
> +	~YamlRoot();
> +
> +	YamlRoot &operator=(YamlRoot &&other) = default;
> +
> +	YamlList list();
> +	YamlDict dict();
> +	void scalar(std::string_view scalar);

You don't implement YamlRoot::scalar() in the .cpp file, which is a good
indication that the function is likely not needed :-) I would just drop
it.

> +
> +private:
> +	friend class YamlEmitter;
> +
> +	YamlRoot(std::unique_ptr<YamlEmitter> emitter)
> +		: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))
> +	{
> +	}
> +
> +	std::unique_ptr<YamlEmitter> emitterRoot_;
> +};
> +
> +class YamlScalar : public YamlOutput
> +{
> +public:
> +	~YamlScalar() = default;
> +
> +	void operator=(std::string_view scalar);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlScalar(YamlEmitter *emitter);
> +};
> +
> +class YamlList : public YamlOutput
> +{
> +public:
> +	YamlList() = default;
> +	YamlList(YamlList &&other) = default;
> +	~YamlList();
> +
> +	YamlList &operator=(YamlList &&other) = default;
> +
> +	YamlList list();
> +	YamlDict dict();
> +	void scalar(std::string_view scalar);
> +
> +private:
> +	friend class YamlOutput;
> +
> +	YamlList(YamlEmitter *emitter);
> +};
> +
> +class YamlDict : public YamlOutput
> +{
> +public:
> +	YamlDict() = default;
> +	YamlDict(YamlDict &&other) = default;
> +	~YamlDict();
> +
> +	YamlDict &operator=(YamlDict &&other) = default;
> +
> +	YamlList list(std::string_view key);
> +	YamlDict dict(std::string_view key);
> +
> +	YamlScalar operator[](std::string_view 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..9542ad2d9c18
> --- /dev/null
> +++ b/src/libcamera/yaml_emitter.cpp
> @@ -0,0 +1,360 @@
> +/* 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.

s/helpers/helper/

or

s/allows/allow/

> + *
> + * To emit YAML users of this classes should create a root node with

"this class" or "these classes"

s/should //

> + *
> + * \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.

That's an implementation detail, it's not relevant to the users of the
class.

> + *
> + * 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,

Not true anymore.

> + * 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.

Events are an implementation detail here too.

All these are opportunities to improve the doc for the first non-RFC
version :-)

> + *
> + * 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;

File::write() returns the error code, no need to get it from errno.

> +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \class YamlEmitter
> + *
> + * Yaml Emitter entry point. Allows to create a YamlRoot object that users

s/Yaml/YAML/

> + * can populate.
> + */
> +
> +YamlEmitter::~YamlEmitter()
> +{
> +	yaml_event_delete(&event_);
> +}
> +
> +/**
> + * \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

No unique pointer anymore. And we don't usually document the types in
the text, they're clearly visible in the generated documentation.

> + */
> +YamlRoot YamlEmitter::root(std::string_view path)
> +{
> +	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter() };
> +
> +	std::string filePath(path);

I've sent a patch to support std::string_view in the File class so this
becomes unnecessary.

> +	emitter->file_.setFileName(filePath);
> +	emitter->file_.open(File::OpenModeFlag::WriteOnly);
> +
> +	emitter->init();

I think there's room for simplification here.

	File file(path);
	file.open(File::OpenModeFlag::WriteOnly);

	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(std::move(file)) };

and drop YamlEmitter::init(); and make YamlEmitter::file_ private.

> +
> +	return YamlRoot(std::move(emitter));
> +}
> +
> +/**
> + * \brief Emit a yaml event
> + */
> +int YamlEmitter::emit()
> +{
> +	return emitter_.emit(&event_);
> +}
> +
> +void YamlEmitter::init()
> +{
> +	emitter_.init(&file_);
> +
> +	yaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);
> +	emitter_.emit(&event_);
> +
> +	yaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);
> +	emitter_.emit(&event_);
> +}
> +
> +/**
> + * \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(yaml_event_t *event)
> +{
> +	int ret = yaml_emitter_emit(&emitter_, 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.
> + */
> +
> +YamlScalar YamlOutput::scalar()
> +{
> +	return YamlScalar(emitter_);
> +}
> +
> +YamlDict YamlOutput::dict()
> +{
> +	return YamlDict(emitter_);
> +}
> +
> +YamlList YamlOutput::list()
> +{
> +	return YamlList(emitter_);
> +}
> +
> +int YamlOutput::emitScalar(std::string_view scalar)
> +{
> +	const unsigned char *value = reinterpret_cast<const unsigned char *>
> +				     (scalar.data());

I would use yaml_char_t instead of unsigned char here, as that's the
taype taken by the yaml_scalar_event_initialize() function.

> +	yaml_scalar_event_initialize(emitter_->event(), NULL, NULL, value,
> +				     scalar.length(), true, false,
> +				     YAML_PLAIN_SCALAR_STYLE);
> +	return emitter_->emit();
> +}
> +
> +int YamlOutput::emitMappingStart()
> +{
> +	yaml_mapping_start_event_initialize(emitter_->event(), NULL, NULL,
> +					    true, YAML_BLOCK_MAPPING_STYLE);
> +	return emitter_->emit();
> +}
> +
> +int YamlOutput::emitMappingEnd()
> +{
> +	yaml_mapping_end_event_initialize(emitter_->event());
> +	return emitter_->emit();
> +}
> +
> +int YamlOutput::emitSequenceStart()
> +{
> +	yaml_sequence_start_event_initialize(emitter_->event(), NULL, NULL, true,
> +					     YAML_BLOCK_SEQUENCE_STYLE);
> +	return emitter_->emit();
> +}
> +
> +int YamlOutput::emitSequenceEnd()
> +{
> +	yaml_sequence_end_event_initialize(emitter_->event());
> +	return emitter_->emit();
> +}
> +
> +/**
> + * \class YamlRoot
> + *
> + * Yaml root node. A root node can be populated with a scalar, a list or a dict.

s/Yaml/YAML/

> + */
> +
> +YamlRoot::~YamlRoot()
> +{
> +	if (!initialized())
> +		return;
> +
> +	yaml_document_end_event_initialize(emitter_->event(), 0);
> +	emitterRoot_->emit();
> +
> +	yaml_stream_end_event_initialize(emitter_->event());
> +	emitterRoot_->emit();
> +}
> +
> +YamlDict YamlRoot::dict()
> +{
> +	emitMappingStart();
> +
> +	return YamlOutput::dict();
> +}
> +
> +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=(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 (initialized())
> +		emitSequenceEnd();
> +}
> +
> +void YamlList::scalar(std::string_view scalar)
> +{
> +	emitScalar(scalar);
> +}
> +
> +YamlList YamlList::list()
> +{
> +	emitSequenceStart();
> +
> +	return YamlOutput::list();
> +}
> +
> +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 (initialized())
> +		emitMappingEnd();
> +}
> +
> +YamlList YamlDict::list(std::string_view key)
> +{
> +	emitScalar(key);
> +	emitSequenceStart();
> +
> +	return YamlOutput::list();
> +}
> +
> +YamlDict YamlDict::dict(std::string_view key)
> +{
> +	emitScalar(key);
> +	emitMappingStart();
> +
> +	return YamlOutput::dict();
> +}
> +
> +YamlScalar YamlDict::operator[](std::string_view key)
> +{
> +	emitScalar(key);
> +
> +	return YamlOutput::scalar();
> +}
> +
> +} /* namespace libcamera */

Hi Laurent
   thanks for review

On Wed, Nov 06, 2024 at 01:50:00AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Oct 21, 2024 at 11:49:51AM +0200, Jacopo Mondi wrote:
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  Documentation/Doxyfile-internal.in        |   2 +
> >  include/libcamera/internal/meson.build    |   1 +
> >  include/libcamera/internal/yaml_emitter.h | 183 +++++++++++
> >  src/libcamera/meson.build                 |   1 +
> >  src/libcamera/yaml_emitter.cpp            | 360 ++++++++++++++++++++++
> >  5 files changed, 547 insertions(+)
> >  create mode 100644 include/libcamera/internal/yaml_emitter.h
> >  create mode 100644 src/libcamera/yaml_emitter.cpp
> >
> > diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
> > index cf9825537866..e0ba64da1bef 100644
> > --- a/Documentation/Doxyfile-internal.in
> > +++ b/Documentation/Doxyfile-internal.in
> > @@ -21,9 +21,11 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
> >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> >                           @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
> > +                         @TOP_SRCDIR@/include/libcamera/internal/yaml_emitter.h \
> >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> >                           @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> > +                         @TOP_SRCDIR@/src/libcamera/yaml_emitter.cpp \
> >                           @TOP_SRCDIR@/src/libcamera/pipeline/ \
> >                           @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> >                           @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> > 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..bcbb574061cc
> > --- /dev/null
> > +++ b/include/libcamera/internal/yaml_emitter.h
> > @@ -0,0 +1,183 @@
> > +/* 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 <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
>
> This class is not part of the public API (as far as I can tell). You can
> just declare it here with

This class is the only entry point from which users can create a
"root" node. I think it should be part of the public API.

>
> class YamlEmitter;
>
> and define it in the .cpp file.
> > +{
> > +public:
> > +	~YamlEmitter();
> > +
> > +	static YamlRoot root(std::string_view path);
>
> To make this class more generic, how about passing a std::ostream
> pointer ?
>
> > +
> > +	int emit();
> > +	yaml_event_t *event() { return &event_; }
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> > +
> > +	class Emitter
> > +	{
> > +	public:
> > +		~Emitter();
> > +
> > +		void init(File *file);
> > +
> > +		int emit(yaml_event_t *event);
> > +
> > +	private:
> > +		void logError();
> > +
> > +		yaml_emitter_t emitter_;
> > +	};
>
> Why is this split in another class ?
>

I probably thought it looked cleaner, but it's not. I'll drop it.

> > +
> > +	YamlEmitter() = default;
> > +
> > +	void init();
> > +
> > +	File file_;
> > +	yaml_event_t event_;
> > +	Emitter emitter_;
> > +};
> > +
> > +class YamlOutput
> > +{
> > +public:
> > +	virtual ~YamlOutput() {};
>
> Extraneous trailing ;
>
> But does the class need a virtual destructor ? Is there any case where
> one would delete a pointer to YamlOutput ?
>

Not as a pointer to YamlOutput, no. I'll drop

> > +
> > +	YamlOutput() = default;
>
> The constructors go before the destructor. And it should be protected,
> as nobody should create a YamlOutput instance.
>

ack

> > +
> > +	YamlOutput(YamlOutput &&other)
> > +	{
> > +		emitter_ = other.emitter_;
> > +		other.emitter_ = nullptr;
> > +	}
>
> Same here, this should be protected.
>

ack

> > +
> > +	bool initialized() { return !!emitter_; }
>
> I would name this function valid(), as it's not just whether or not the
> object has been initialized, but it also becomes invalid when moved.
>
> The function should also be const.

right

>
> > +
> > +	YamlScalar scalar();
> > +	YamlDict dict();
> > +	YamlList list();
>
> I think those 3 functions should be protected, they're not meant to be
> called by the user as far as I understand.
>

Yes, these can be protected

> > +
> > +protected:
> > +	YamlOutput(YamlEmitter *emitter)
> > +		: emitter_(emitter)
> > +	{
> > +	}
> > +
> > +	YamlOutput &operator=(YamlOutput &&other)
> > +	{
> > +		emitter_ = other.emitter_;
> > +		other.emitter_ = nullptr;
> > +
> > +		return *this;
> > +	}
> > +
> > +	int emitScalar(std::string_view scalar);
> > +	int emitMappingStart();
> > +	int emitMappingEnd();
> > +	int emitSequenceStart();
> > +	int emitSequenceEnd();
> > +
> > +	YamlEmitter *emitter_ = nullptr;
> > +	yaml_event_t event_;
>
> I think you should disable copying of this class with
> LIBCAMERA_DISABLE_COPY().
>

done, in the private: section

> > +};
> > +
> > +class YamlRoot : public YamlOutput
> > +{
> > +public:
> > +	YamlRoot() = default;
> > +	YamlRoot(YamlRoot &&other) = default;
> > +	~YamlRoot();
> > +
> > +	YamlRoot &operator=(YamlRoot &&other) = default;
> > +
> > +	YamlList list();
> > +	YamlDict dict();
> > +	void scalar(std::string_view scalar);
>
> You don't implement YamlRoot::scalar() in the .cpp file, which is a good
> indication that the function is likely not needed :-) I would just drop
> it.
>

Maybe I'm not using it right now, but I think emitting a scalar in the
root node is still a valid use case. Maybe I should implement the
function.


> > +
> > +private:
> > +	friend class YamlEmitter;
> > +
> > +	YamlRoot(std::unique_ptr<YamlEmitter> emitter)
> > +		: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))
> > +	{
> > +	}
> > +
> > +	std::unique_ptr<YamlEmitter> emitterRoot_;
> > +};
> > +
> > +class YamlScalar : public YamlOutput
> > +{
> > +public:
> > +	~YamlScalar() = default;
> > +
> > +	void operator=(std::string_view scalar);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlScalar(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlList : public YamlOutput
> > +{
> > +public:
> > +	YamlList() = default;
> > +	YamlList(YamlList &&other) = default;
> > +	~YamlList();
> > +
> > +	YamlList &operator=(YamlList &&other) = default;
> > +
> > +	YamlList list();
> > +	YamlDict dict();
> > +	void scalar(std::string_view scalar);
> > +
> > +private:
> > +	friend class YamlOutput;
> > +
> > +	YamlList(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlDict : public YamlOutput
> > +{
> > +public:
> > +	YamlDict() = default;
> > +	YamlDict(YamlDict &&other) = default;
> > +	~YamlDict();
> > +
> > +	YamlDict &operator=(YamlDict &&other) = default;
> > +
> > +	YamlList list(std::string_view key);
> > +	YamlDict dict(std::string_view key);
> > +
> > +	YamlScalar operator[](std::string_view 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..9542ad2d9c18
> > --- /dev/null
> > +++ b/src/libcamera/yaml_emitter.cpp
> > @@ -0,0 +1,360 @@
> > +/* 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.
>
> s/helpers/helper/
>
> or
>
> s/allows/allow/
>
> > + *
> > + * To emit YAML users of this classes should create a root node with
>
> "this class" or "these classes"
>
> s/should //
>
> > + *
> > + * \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.
>
> That's an implementation detail, it's not relevant to the users of the
> class.
>
> > + *
> > + * 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,
>
> Not true anymore.
>
> > + * 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.
>
> Events are an implementation detail here too.
>
> All these are opportunities to improve the doc for the first non-RFC
> version :-)
>
> > + *
> > + * 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;
>
> File::write() returns the error code, no need to get it from errno.
>
> > +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \class YamlEmitter
> > + *
> > + * Yaml Emitter entry point. Allows to create a YamlRoot object that users
>
> s/Yaml/YAML/
>
> > + * can populate.
> > + */
> > +
> > +YamlEmitter::~YamlEmitter()
> > +{
> > +	yaml_event_delete(&event_);
> > +}
> > +
> > +/**
> > + * \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
>
> No unique pointer anymore. And we don't usually document the types in
> the text, they're clearly visible in the generated documentation.
>
> > + */
> > +YamlRoot YamlEmitter::root(std::string_view path)
> > +{
> > +	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter() };
> > +
> > +	std::string filePath(path);
>
> I've sent a patch to support std::string_view in the File class so this
> becomes unnecessary.
>

Where ? I missed it

> > +	emitter->file_.setFileName(filePath);
> > +	emitter->file_.open(File::OpenModeFlag::WriteOnly);
> > +
> > +	emitter->init();
>
> I think there's room for simplification here.
>
> 	File file(path);
> 	file.open(File::OpenModeFlag::WriteOnly);
>
> 	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(std::move(file)) };
>
> and drop YamlEmitter::init(); and make YamlEmitter::file_ private.
>
> > +
> > +	return YamlRoot(std::move(emitter));
> > +}
> > +
> > +/**
> > + * \brief Emit a yaml event
> > + */
> > +int YamlEmitter::emit()
> > +{
> > +	return emitter_.emit(&event_);
> > +}
> > +
> > +void YamlEmitter::init()
> > +{
> > +	emitter_.init(&file_);
> > +
> > +	yaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);
> > +	emitter_.emit(&event_);
> > +
> > +	yaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);
> > +	emitter_.emit(&event_);
> > +}
> > +
> > +/**
> > + * \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(yaml_event_t *event)
> > +{
> > +	int ret = yaml_emitter_emit(&emitter_, 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.
> > + */
> > +
> > +YamlScalar YamlOutput::scalar()
> > +{
> > +	return YamlScalar(emitter_);
> > +}
> > +
> > +YamlDict YamlOutput::dict()
> > +{
> > +	return YamlDict(emitter_);
> > +}
> > +
> > +YamlList YamlOutput::list()
> > +{
> > +	return YamlList(emitter_);
> > +}
> > +
> > +int YamlOutput::emitScalar(std::string_view scalar)
> > +{
> > +	const unsigned char *value = reinterpret_cast<const unsigned char *>
> > +				     (scalar.data());
>
> I would use yaml_char_t instead of unsigned char here, as that's the
> taype taken by the yaml_scalar_event_initialize() function.
>

done

Thanks
   j

> > +	yaml_scalar_event_initialize(emitter_->event(), NULL, NULL, value,
> > +				     scalar.length(), true, false,
> > +				     YAML_PLAIN_SCALAR_STYLE);
> > +	return emitter_->emit();
> > +}
> > +
> > +int YamlOutput::emitMappingStart()
> > +{
> > +	yaml_mapping_start_event_initialize(emitter_->event(), NULL, NULL,
> > +					    true, YAML_BLOCK_MAPPING_STYLE);
> > +	return emitter_->emit();
> > +}
> > +
> > +int YamlOutput::emitMappingEnd()
> > +{
> > +	yaml_mapping_end_event_initialize(emitter_->event());
> > +	return emitter_->emit();
> > +}
> > +
> > +int YamlOutput::emitSequenceStart()
> > +{
> > +	yaml_sequence_start_event_initialize(emitter_->event(), NULL, NULL, true,
> > +					     YAML_BLOCK_SEQUENCE_STYLE);
> > +	return emitter_->emit();
> > +}
> > +
> > +int YamlOutput::emitSequenceEnd()
> > +{
> > +	yaml_sequence_end_event_initialize(emitter_->event());
> > +	return emitter_->emit();
> > +}
> > +
> > +/**
> > + * \class YamlRoot
> > + *
> > + * Yaml root node. A root node can be populated with a scalar, a list or a dict.
>
> s/Yaml/YAML/
>
> > + */
> > +
> > +YamlRoot::~YamlRoot()
> > +{
> > +	if (!initialized())
> > +		return;
> > +
> > +	yaml_document_end_event_initialize(emitter_->event(), 0);
> > +	emitterRoot_->emit();
> > +
> > +	yaml_stream_end_event_initialize(emitter_->event());
> > +	emitterRoot_->emit();
> > +}
> > +
> > +YamlDict YamlRoot::dict()
> > +{
> > +	emitMappingStart();
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +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=(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 (initialized())
> > +		emitSequenceEnd();
> > +}
> > +
> > +void YamlList::scalar(std::string_view scalar)
> > +{
> > +	emitScalar(scalar);
> > +}
> > +
> > +YamlList YamlList::list()
> > +{
> > +	emitSequenceStart();
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +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 (initialized())
> > +		emitMappingEnd();
> > +}
> > +
> > +YamlList YamlDict::list(std::string_view key)
> > +{
> > +	emitScalar(key);
> > +	emitSequenceStart();
> > +
> > +	return YamlOutput::list();
> > +}
> > +
> > +YamlDict YamlDict::dict(std::string_view key)
> > +{
> > +	emitScalar(key);
> > +	emitMappingStart();
> > +
> > +	return YamlOutput::dict();
> > +}
> > +
> > +YamlScalar YamlDict::operator[](std::string_view key)
> > +{
> > +	emitScalar(key);
> > +
> > +	return YamlOutput::scalar();
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Wed, Nov 06, 2024 at 10:45:37AM +0100, Jacopo Mondi wrote:
> On Wed, Nov 06, 2024 at 01:50:00AM +0200, Laurent Pinchart wrote:
> > On Mon, Oct 21, 2024 at 11:49:51AM +0200, Jacopo Mondi wrote:
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  Documentation/Doxyfile-internal.in        |   2 +
> > >  include/libcamera/internal/meson.build    |   1 +
> > >  include/libcamera/internal/yaml_emitter.h | 183 +++++++++++
> > >  src/libcamera/meson.build                 |   1 +
> > >  src/libcamera/yaml_emitter.cpp            | 360 ++++++++++++++++++++++
> > >  5 files changed, 547 insertions(+)
> > >  create mode 100644 include/libcamera/internal/yaml_emitter.h
> > >  create mode 100644 src/libcamera/yaml_emitter.cpp
> > >
> > > diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
> > > index cf9825537866..e0ba64da1bef 100644
> > > --- a/Documentation/Doxyfile-internal.in
> > > +++ b/Documentation/Doxyfile-internal.in
> > > @@ -21,9 +21,11 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
> > >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> > >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> > >                           @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
> > > +                         @TOP_SRCDIR@/include/libcamera/internal/yaml_emitter.h \
> > >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> > >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> > >                           @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> > > +                         @TOP_SRCDIR@/src/libcamera/yaml_emitter.cpp \
> > >                           @TOP_SRCDIR@/src/libcamera/pipeline/ \
> > >                           @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> > >                           @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> > > 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..bcbb574061cc
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/yaml_emitter.h
> > > @@ -0,0 +1,183 @@
> > > +/* 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 <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
> >
> > This class is not part of the public API (as far as I can tell). You can
> > just declare it here with
> 
> This class is the only entry point from which users can create a
> "root" node. I think it should be part of the public API.

I wonder how I missed that. I'll try to wake up.

> > class YamlEmitter;
> >
> > and define it in the .cpp file.
> > > +{
> > > +public:
> > > +	~YamlEmitter();
> > > +
> > > +	static YamlRoot root(std::string_view path);
> >
> > To make this class more generic, how about passing a std::ostream
> > pointer ?
> >
> > > +
> > > +	int emit();
> > > +	yaml_event_t *event() { return &event_; }
> > > +
> > > +private:
> > > +	LIBCAMERA_DISABLE_COPY(YamlEmitter)
> > > +
> > > +	class Emitter
> > > +	{
> > > +	public:
> > > +		~Emitter();
> > > +
> > > +		void init(File *file);
> > > +
> > > +		int emit(yaml_event_t *event);
> > > +
> > > +	private:
> > > +		void logError();
> > > +
> > > +		yaml_emitter_t emitter_;
> > > +	};
> >
> > Why is this split in another class ?
> 
> I probably thought it looked cleaner, but it's not. I'll drop it.
> 
> > > +
> > > +	YamlEmitter() = default;
> > > +
> > > +	void init();
> > > +
> > > +	File file_;
> > > +	yaml_event_t event_;
> > > +	Emitter emitter_;
> > > +};
> > > +
> > > +class YamlOutput
> > > +{
> > > +public:
> > > +	virtual ~YamlOutput() {};
> >
> > Extraneous trailing ;
> >
> > But does the class need a virtual destructor ? Is there any case where
> > one would delete a pointer to YamlOutput ?
> 
> Not as a pointer to YamlOutput, no. I'll drop
> 
> > > +
> > > +	YamlOutput() = default;
> >
> > The constructors go before the destructor. And it should be protected,
> > as nobody should create a YamlOutput instance.
> 
> ack
> 
> > > +
> > > +	YamlOutput(YamlOutput &&other)
> > > +	{
> > > +		emitter_ = other.emitter_;
> > > +		other.emitter_ = nullptr;
> > > +	}
> >
> > Same here, this should be protected.
> 
> ack
> 
> > > +
> > > +	bool initialized() { return !!emitter_; }
> >
> > I would name this function valid(), as it's not just whether or not the
> > object has been initialized, but it also becomes invalid when moved.
> >
> > The function should also be const.
> 
> right
> 
> > > +
> > > +	YamlScalar scalar();
> > > +	YamlDict dict();
> > > +	YamlList list();
> >
> > I think those 3 functions should be protected, they're not meant to be
> > called by the user as far as I understand.
> 
> Yes, these can be protected
> 
> > > +
> > > +protected:
> > > +	YamlOutput(YamlEmitter *emitter)
> > > +		: emitter_(emitter)
> > > +	{
> > > +	}
> > > +
> > > +	YamlOutput &operator=(YamlOutput &&other)
> > > +	{
> > > +		emitter_ = other.emitter_;
> > > +		other.emitter_ = nullptr;
> > > +
> > > +		return *this;
> > > +	}
> > > +
> > > +	int emitScalar(std::string_view scalar);
> > > +	int emitMappingStart();
> > > +	int emitMappingEnd();
> > > +	int emitSequenceStart();
> > > +	int emitSequenceEnd();
> > > +
> > > +	YamlEmitter *emitter_ = nullptr;
> > > +	yaml_event_t event_;
> >
> > I think you should disable copying of this class with
> > LIBCAMERA_DISABLE_COPY().
> 
> done, in the private: section
> 
> > > +};
> > > +
> > > +class YamlRoot : public YamlOutput
> > > +{
> > > +public:
> > > +	YamlRoot() = default;
> > > +	YamlRoot(YamlRoot &&other) = default;
> > > +	~YamlRoot();
> > > +
> > > +	YamlRoot &operator=(YamlRoot &&other) = default;
> > > +
> > > +	YamlList list();
> > > +	YamlDict dict();
> > > +	void scalar(std::string_view scalar);
> >
> > You don't implement YamlRoot::scalar() in the .cpp file, which is a good
> > indication that the function is likely not needed :-) I would just drop
> > it.
> 
> Maybe I'm not using it right now, but I think emitting a scalar in the
> root node is still a valid use case. Maybe I should implement the
> function.

That means the YAML document would look like

----------------------------------------
%YAML 1.1
---
42
----------------------------------------

Yes, in theory, that's well-formed YAML (as far as I can tell), but in
practice, I don't think we would ever need this. I'm a bit reluctant to
carry dead code in libcamera, especially when we can easily add that
code later if needed without a big redesigning.

> > > +
> > > +private:
> > > +	friend class YamlEmitter;
> > > +
> > > +	YamlRoot(std::unique_ptr<YamlEmitter> emitter)
> > > +		: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))
> > > +	{
> > > +	}
> > > +
> > > +	std::unique_ptr<YamlEmitter> emitterRoot_;
> > > +};
> > > +
> > > +class YamlScalar : public YamlOutput
> > > +{
> > > +public:
> > > +	~YamlScalar() = default;
> > > +
> > > +	void operator=(std::string_view scalar);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlScalar(YamlEmitter *emitter);
> > > +};
> > > +
> > > +class YamlList : public YamlOutput
> > > +{
> > > +public:
> > > +	YamlList() = default;
> > > +	YamlList(YamlList &&other) = default;
> > > +	~YamlList();
> > > +
> > > +	YamlList &operator=(YamlList &&other) = default;
> > > +
> > > +	YamlList list();
> > > +	YamlDict dict();
> > > +	void scalar(std::string_view scalar);
> > > +
> > > +private:
> > > +	friend class YamlOutput;
> > > +
> > > +	YamlList(YamlEmitter *emitter);
> > > +};
> > > +
> > > +class YamlDict : public YamlOutput
> > > +{
> > > +public:
> > > +	YamlDict() = default;
> > > +	YamlDict(YamlDict &&other) = default;
> > > +	~YamlDict();
> > > +
> > > +	YamlDict &operator=(YamlDict &&other) = default;
> > > +
> > > +	YamlList list(std::string_view key);
> > > +	YamlDict dict(std::string_view key);
> > > +
> > > +	YamlScalar operator[](std::string_view 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..9542ad2d9c18
> > > --- /dev/null
> > > +++ b/src/libcamera/yaml_emitter.cpp
> > > @@ -0,0 +1,360 @@
> > > +/* 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.
> >
> > s/helpers/helper/
> >
> > or
> >
> > s/allows/allow/
> >
> > > + *
> > > + * To emit YAML users of this classes should create a root node with
> >
> > "this class" or "these classes"
> >
> > s/should //
> >
> > > + *
> > > + * \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.
> >
> > That's an implementation detail, it's not relevant to the users of the
> > class.
> >
> > > + *
> > > + * 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,
> >
> > Not true anymore.
> >
> > > + * 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.
> >
> > Events are an implementation detail here too.
> >
> > All these are opportunities to improve the doc for the first non-RFC
> > version :-)
> >
> > > + *
> > > + * 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;
> >
> > File::write() returns the error code, no need to get it from errno.
> >
> > > +		LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> > > +		return 0;
> > > +	}
> > > +
> > > +	return 1;
> > > +}
> > > +
> > > +} /* namespace */
> > > +
> > > +/**
> > > + * \class YamlEmitter
> > > + *
> > > + * Yaml Emitter entry point. Allows to create a YamlRoot object that users
> >
> > s/Yaml/YAML/
> >
> > > + * can populate.
> > > + */
> > > +
> > > +YamlEmitter::~YamlEmitter()
> > > +{
> > > +	yaml_event_delete(&event_);
> > > +}
> > > +
> > > +/**
> > > + * \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
> >
> > No unique pointer anymore. And we don't usually document the types in
> > the text, they're clearly visible in the generated documentation.
> >
> > > + */
> > > +YamlRoot YamlEmitter::root(std::string_view path)
> > > +{
> > > +	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter() };
> > > +
> > > +	std::string filePath(path);
> >
> > I've sent a patch to support std::string_view in the File class so this
> > becomes unnecessary.
> 
> Where ? I missed it

Oops. I wrote that comment while working on string_view support in the
File class. The result was however not what I had expected, and I think
we would be better off with

	File::setFileName(std::string name);

instead of the existing

	File::setFileName(const std::string &name);

or the planned

	File::setFileName(std::string_view name);

The reason is that, internally, File::setFileName() stores the name in a
std::string member variable:

	name_ = name;

With the function argument being either 'const std::string &' or
'std::string_view', this will create a copy. However, with the function
argument being a 'std::string', we would change the implementation to

	name_ = std::move(name);

If the caller passes a 'std::string &' to the function, a copy will
still be create, by the caller this time. There will be no performance
impact, neither positive nor negative, compared to what we do today. The
same is true if the caller passes a C string literal, or a
std::stringview. However, if the caller already has a std::string
variable holding the name, *and* does not need the variable anymore
after the call, it could do

	std::string fullName = path + name;
	file.setFileName(std::move(fullName));

or just

	file.setFileName(path + name);

and no copy would be made anywhere. This improves performance compared
to the current situtation. This was an unexpected find for me, I aimed
at replacing 'const std::string &' with 'std::string_view' and realized
I should pass a 'std::string' by value instead.

There are still use cases where usage of std::string_view is best, but
making clear and simple rules to decide what string type to use is
difficult. One annoying part is that the optimal string type depends on
the internal implementation of the callee, and designing APIs based on
internal implementations is really not a great idea in general. For
anything private to a class that's fine, we can update the callers if a
change in a callee would benefit from switching to a different string
type. For APIs internal to libcamera that's more or less true as well,
although the impact of a tree-wide change would be more annoying. For
public APIs, that's a no-go, and different rules are needed there, to
pick an optimal string type under the constraint of ABI stability.

All this to say that I wouldn't try to overuse std::string_view in
libcamera yet. Especially in this function, as you need to pass a
std::string reference to setFileName() anyway, you can as well use a
'const std::string &' argument instead of a string view.

> > > +	emitter->file_.setFileName(filePath);
> > > +	emitter->file_.open(File::OpenModeFlag::WriteOnly);
> > > +
> > > +	emitter->init();
> >
> > I think there's room for simplification here.
> >
> > 	File file(path);
> > 	file.open(File::OpenModeFlag::WriteOnly);
> >
> > 	std::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(std::move(file)) };
> >
> > and drop YamlEmitter::init(); and make YamlEmitter::file_ private.
> >
> > > +
> > > +	return YamlRoot(std::move(emitter));
> > > +}
> > > +
> > > +/**
> > > + * \brief Emit a yaml event
> > > + */
> > > +int YamlEmitter::emit()
> > > +{
> > > +	return emitter_.emit(&event_);
> > > +}
> > > +
> > > +void YamlEmitter::init()
> > > +{
> > > +	emitter_.init(&file_);
> > > +
> > > +	yaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);
> > > +	emitter_.emit(&event_);
> > > +
> > > +	yaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);
> > > +	emitter_.emit(&event_);
> > > +}
> > > +
> > > +/**
> > > + * \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(yaml_event_t *event)
> > > +{
> > > +	int ret = yaml_emitter_emit(&emitter_, 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.
> > > + */
> > > +
> > > +YamlScalar YamlOutput::scalar()
> > > +{
> > > +	return YamlScalar(emitter_);
> > > +}
> > > +
> > > +YamlDict YamlOutput::dict()
> > > +{
> > > +	return YamlDict(emitter_);
> > > +}
> > > +
> > > +YamlList YamlOutput::list()
> > > +{
> > > +	return YamlList(emitter_);
> > > +}
> > > +
> > > +int YamlOutput::emitScalar(std::string_view scalar)
> > > +{
> > > +	const unsigned char *value = reinterpret_cast<const unsigned char *>
> > > +				     (scalar.data());
> >
> > I would use yaml_char_t instead of unsigned char here, as that's the
> > taype taken by the yaml_scalar_event_initialize() function.
> >
> 
> done
> 
> Thanks
>    j
> 
> > > +	yaml_scalar_event_initialize(emitter_->event(), NULL, NULL, value,
> > > +				     scalar.length(), true, false,
> > > +				     YAML_PLAIN_SCALAR_STYLE);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +int YamlOutput::emitMappingStart()
> > > +{
> > > +	yaml_mapping_start_event_initialize(emitter_->event(), NULL, NULL,
> > > +					    true, YAML_BLOCK_MAPPING_STYLE);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +int YamlOutput::emitMappingEnd()
> > > +{
> > > +	yaml_mapping_end_event_initialize(emitter_->event());
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +int YamlOutput::emitSequenceStart()
> > > +{
> > > +	yaml_sequence_start_event_initialize(emitter_->event(), NULL, NULL, true,
> > > +					     YAML_BLOCK_SEQUENCE_STYLE);
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +int YamlOutput::emitSequenceEnd()
> > > +{
> > > +	yaml_sequence_end_event_initialize(emitter_->event());
> > > +	return emitter_->emit();
> > > +}
> > > +
> > > +/**
> > > + * \class YamlRoot
> > > + *
> > > + * Yaml root node. A root node can be populated with a scalar, a list or a dict.
> >
> > s/Yaml/YAML/
> >
> > > + */
> > > +
> > > +YamlRoot::~YamlRoot()
> > > +{
> > > +	if (!initialized())
> > > +		return;
> > > +
> > > +	yaml_document_end_event_initialize(emitter_->event(), 0);
> > > +	emitterRoot_->emit();
> > > +
> > > +	yaml_stream_end_event_initialize(emitter_->event());
> > > +	emitterRoot_->emit();
> > > +}
> > > +
> > > +YamlDict YamlRoot::dict()
> > > +{
> > > +	emitMappingStart();
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +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=(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 (initialized())
> > > +		emitSequenceEnd();
> > > +}
> > > +
> > > +void YamlList::scalar(std::string_view scalar)
> > > +{
> > > +	emitScalar(scalar);
> > > +}
> > > +
> > > +YamlList YamlList::list()
> > > +{
> > > +	emitSequenceStart();
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +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 (initialized())
> > > +		emitMappingEnd();
> > > +}
> > > +
> > > +YamlList YamlDict::list(std::string_view key)
> > > +{
> > > +	emitScalar(key);
> > > +	emitSequenceStart();
> > > +
> > > +	return YamlOutput::list();
> > > +}
> > > +
> > > +YamlDict YamlDict::dict(std::string_view key)
> > > +{
> > > +	emitScalar(key);
> > > +	emitMappingStart();
> > > +
> > > +	return YamlOutput::dict();
> > > +}
> > > +
> > > +YamlScalar YamlDict::operator[](std::string_view key)
> > > +{
> > > +	emitScalar(key);
> > > +
> > > +	return YamlOutput::scalar();
> > > +}
> > > +
> > > +} /* namespace libcamera */