[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