[RFC v3 3/4] libcamera: Implement YamlEmitter
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 6 00:50:00 CET 2024
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
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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list