[RFC v3 3/4] libcamera: Implement YamlEmitter

Jacopo Mondi jacopo.mondi at ideasonboard.com
Wed Nov 6 10:45:37 CET 2024


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


More information about the libcamera-devel mailing list