[RFC 3/4] libcamera: Implement YamlEmitter
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Oct 15 14:16:11 CEST 2024
Hi Laurent,
thanks for review
On Tue, Oct 15, 2024 at 01:21:33PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> I've kept the review high-level to start with.
>
> On Mon, Oct 14, 2024 at 11:59:35AM +0200, Jacopo Mondi wrote:
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> > include/libcamera/internal/meson.build | 1 +
> > include/libcamera/internal/yaml_emitter.h | 172 +++++++++
> > src/libcamera/meson.build | 1 +
> > src/libcamera/yaml_emitter.cpp | 427 ++++++++++++++++++++++
> > 4 files changed, 601 insertions(+)
> > create mode 100644 include/libcamera/internal/yaml_emitter.h
> > create mode 100644 src/libcamera/yaml_emitter.cpp
> >
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 1c5eef9cab80..7533b075fde2 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([
> > 'v4l2_pixelformat.h',
> > 'v4l2_subdevice.h',
> > 'v4l2_videodevice.h',
> > + 'yaml_emitter.h',
> > 'yaml_parser.h',
> > ])
> >
> > diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h
> > new file mode 100644
> > index 000000000000..3fe05f97de70
> > --- /dev/null
> > +++ b/include/libcamera/internal/yaml_emitter.h
> > @@ -0,0 +1,172 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board Oy
> > + *
> > + * libcamera YAML emitter helper
> > + */
> > +
> > +#pragma once
> > +
> > +#include <memory>
> > +#include <string_view>
> > +#include <unordered_map>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/file.h>
> > +#include <libcamera/orientation.h>
> > +
> > +#include <yaml.h>
> > +
> > +namespace libcamera {
> > +
> > +class YamlDict;
> > +class YamlEvent;
> > +class YamlList;
> > +class YamlRoot;
> > +class YamlScalar;
> > +
> > +class YamlEmitter final
> > +{
> > +public:
> > + ~YamlEmitter() = default;
> > +
> > + static std::unique_ptr<YamlRoot> root(std::string_view path);
> > +
> > + int emit(YamlEvent *event);
> > +
> > +private:
> > + LIBCAMERA_DISABLE_COPY(YamlEmitter)
> > +
> > + class Emitter
> > + {
> > + public:
> > + Emitter() = default;
> > + ~Emitter();
> > +
> > + void init(File *file);
> > +
> > + int emit(YamlEvent *event);
> > +
> > + private:
> > + void logError();
> > +
> > + yaml_emitter_t emitter_;
> > + };
> > +
> > + YamlEmitter() = default;
> > +
> > + void init();
> > +
> > + std::unique_ptr<File> file_;
> > + Emitter emitter_;
> > +};
> > +
> > +class YamlOutput
> > +{
> > +public:
>
> Shouldn't you disable copying of this class ?
>
yup
> > + virtual ~YamlOutput() = default;
> > +
> > + void close()
> > + {
> > + closed_ = true;
> > + }
>
> Can we avoid this and close on destruction only ? Having an explicit
> close is error-prone, what if someone calls one of the functions below
> after calling close() ? The API should make it very hard, if not
> impossible, to do the wrong thing.
>
Eh
my idea was ideed to rely on object's destruction to emit the correct
yaml events, then I thought it was worth to allow users to close a
sequence/mapping earlier if they didn't want to enforce this through
the object lifetime. I think I had a use case for this, but maybe it
has gone during the development, I'll try remove close() and solely
rely on object's lifetime for the correct events emission.
> > +
> > + std::unique_ptr<YamlScalar> scalar();
> > + std::unique_ptr<YamlDict> dict();
> > + std::unique_ptr<YamlList> list();
>
> In many cases the YamlScalar, YamlDict and YamlList classes will be
> local to the caller function. It would be best to avoid dynamic heap
> allocation. You could do so by returning values here. Same below. You
> will likely need to implement move constructors and operators.
>
good idea, I'll try
> > +
> > +protected:
> > + YamlOutput(YamlEmitter *emitter)
> > + : emitter_(emitter), closed_(false)
> > + {
> > + }
> > +
> > + int emitScalar(std::string_view scalar);
> > + int emitMappingStart();
> > + int emitMappingEnd();
> > + int emitSequenceStart();
> > + int emitSequenceEnd();
> > +
> > + YamlEmitter *emitter_;
> > +
> > + bool closed_;
> > +};
> > +
> > +class YamlRoot : public YamlOutput
> > +{
> > +public:
> > + ~YamlRoot();
> > + void close() {}
> > +
> > + std::unique_ptr<YamlList> list();
> > + std::unique_ptr<YamlDict> dict();
> > + void scalar(std::string_view scalar);
> > +
> > +private:
> > + friend class YamlEmitter;
> > +
> > + YamlRoot(YamlEmitter *emitter)
> > + : YamlOutput(emitter)
> > + {
> > + emitterRoot_ = std::unique_ptr<YamlEmitter>(emitter);
> > + }
> > +
> > + std::unique_ptr<YamlEmitter> emitterRoot_;
> > +};
> > +
> > +class YamlScalar : public YamlOutput
> > +{
> > +public:
> > + ~YamlScalar() = default;
> > +
> > + void close() {}
> > +
> > + void operator=(const Orientation &orientation);
>
> I don't think this should be a member of the YamlScalar class. You
> should convert the Orientation to a string in the caller.
>
This is something I wanted indeed feedbacks on. I see it two ways,
either we augment this interface with libcamera types as we need them
to ease for users of this class to emit them, or we require users to
convert to string for. I understand your preference is for the latter
option.
> > + void operator=(std::string_view scalar);
> > +
> > +private:
> > + friend class YamlOutput;
> > +
> > + YamlScalar(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlList : public YamlOutput
> > +{
> > +public:
> > + ~YamlList();
> > +
> > + void close();
> > +
> > + std::unique_ptr<YamlList> list();
> > + std::unique_ptr<YamlDict> dict();
> > + void scalar(std::string_view scalar);
> > +
> > +private:
> > + friend class YamlOutput;
> > +
> > + YamlList(YamlEmitter *emitter);
> > +};
> > +
> > +class YamlDict : public YamlOutput,
> > + private std::unordered_map<std::string_view,
> > + std::unique_ptr<YamlScalar>>
>
> Why do you need to store content ? That doesn't sound right,the
> emitter's job is to emit YAML, we shouldn't need to store anything.
>
because I wanted to override operator[] for YamlDict and it returns a
refernce, so I need to have a storage. I can try to remove it.
> > +{
> > +public:
> > + using Map = std::unordered_map<std::string_view, YamlScalar>;
> > +
> > + ~YamlDict();
> > +
> > + void close();
> > +
> > + std::unique_ptr<YamlList> list(std::string_view key);
> > + std::unique_ptr<YamlDict> dict(std::string_view key);
> > +
> > + YamlScalar &operator[](const Map::key_type &key);
> > +
> > +private:
> > + friend class YamlOutput;
> > +
> > + YamlDict(YamlEmitter *emitter);
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index aa9ab0291854..5b8af4103085 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([
> > 'v4l2_pixelformat.cpp',
> > 'v4l2_subdevice.cpp',
> > 'v4l2_videodevice.cpp',
> > + 'yaml_emitter.cpp',
> > 'yaml_parser.cpp',
> > ])
> >
> > diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp
> > new file mode 100644
> > index 000000000000..1f7651ffcb24
> > --- /dev/null
> > +++ b/src/libcamera/yaml_emitter.cpp
> > @@ -0,0 +1,427 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board Oy
> > + *
> > + * libcamera YAML emitter helper
> > + */
> > +
> > +#include "libcamera/internal/yaml_emitter.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +/**
> > + * \file yaml_emitter.h
> > + * \brief A YAML emitter helper
> > + *
> > + * The YAML Emitter helpers allows users to emit output in YAML format.
> > + *
> > + * To emit YAML users of this classes should create a root node with
> > + *
> > + * \code
> > + std::string filePath("...");
> > + auto root = YamlEmitter::root(filePath);
> > + \endcode
> > + *
> > + * A YamlRoot implements RAII-style handling of YAML sequence and document
> > + * events handling. Creating a YamlRoot emits the STREAM_START and DOC_START
> > + * events. Once a YamlRoot gets deleted the DOC_SEND and STREAM_END events
> > + * gets automatically deleted.
> > + *
> > + * Once a root node has been created it is possible to populate it with
> > + * scalars, list or dictionaries.
> > + *
> > + * YamlList and YamlDict can only be created wrapped in unique_ptr<> instances,
> > + * to implement a RAII-style handling of YAML of lists and dictionaries.
> > + * Creating a YamlList and a YamlDict emits the YAML sequence and mapping start
> > + * events. Once an instance gets deleted, the sequence and mapping gets
> > + * automatically emitted. If a list of dictionary needs to be closed before
> > + * it gets deleted, it can be explicitly closed with the close() function.
> > + *
> > + * A YamlScalar is a simpler object that can be assigned to different types
> > + * to emit them as strings.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(YamlEmitter)
> > +
> > +namespace {
> > +
> > +int yamlWrite(void *data, unsigned char *buffer, size_t size)
> > +{
> > + File *file = static_cast<File *>(data);
> > +
> > + Span<unsigned char> buf{ buffer, size };
> > + ssize_t ret = file->write(buf);
> > + if (ret < 0) {
> > + ret = errno;
> > + LOG(YamlEmitter, Error) << "Write error : " << strerror(ret);
> > + return 0;
> > + }
> > +
> > + return 1;
> > +}
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \class YamlEvent
> > + *
> > + * Class that represents a yaml_event that automatically cleans-up on
> > + * destruction.
> > + */
> > +class YamlEvent
> > +{
> > +public:
> > + static std::unique_ptr<YamlEvent> create();
> > +
> > + const yaml_event_t *event() const
> > + {
> > + return &event_;
> > + }
> > +
> > + yaml_event_t *event()
> > + {
> > + return &event_;
> > + }
> > +
> > +private:
> > + YamlEvent() = default;
> > +
> > + yaml_event_t event_;
> > +};
> > +
> > +std::unique_ptr<YamlEvent> YamlEvent::create()
> > +{
> > + struct Deleter : std::default_delete<YamlEvent> {
> > + void operator()(YamlEvent *yamlEvent)
> > + {
> > + yaml_event_delete(yamlEvent->event());
> > + }
> > + };
> > +
> > + return std::unique_ptr<YamlEvent>(new YamlEvent(), Deleter());
>
> Here too, we should avoid dynamic allocations on the heap. I don't think
> you need a custom deleter, you can simply call yaml_event_delete() in
> the destructor of the YamlEvent class, and you then don't need to use
> unique pointers.
>
> > +}
> > +
> > +/**
> > + * \class YamlEmitter
> > + *
> > + * Yaml Emitter entry point. Allows to create a YamlRoot object that users
> > + * can populate.
> > + */
> > +
> > +/**
> > + * \brief Create a YamlRoot that applications can start populating with YamlOutput
> > + * \param[in] path The YAML output file path
> > + * \return A unique pointer to a YamlRoot
> > + */
> > +std::unique_ptr<YamlRoot> YamlEmitter::root(std::string_view path)
> > +{
> > + YamlEmitter *emitter = new YamlEmitter();
> > +
> > + std::string filePath(path);
> > + emitter->file_ = std::make_unique<File>(filePath);
> > + emitter->file_->open(File::OpenModeFlag::WriteOnly);
> > +
> > + emitter->init();
> > +
> > + YamlRoot *root = new YamlRoot(emitter);
> > + return std::unique_ptr<YamlRoot>(root);
> > +}
> > +
> > +/**
> > + * \brief Emit a YamlEvent
> > + */
> > +int YamlEmitter::emit(YamlEvent *event)
> > +{
> > + return emitter_.emit(event);
> > +}
> > +
> > +void YamlEmitter::init()
> > +{
> > + emitter_.init(file_.get());
> > +
> > + std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +
> > + yaml_stream_start_event_initialize(event->event(), YAML_UTF8_ENCODING);
> > + emitter_.emit(event.get());
> > +
> > + yaml_document_start_event_initialize(event->event(), NULL, NULL,
> > + NULL, 0);
> > + emitter_.emit(event.get());
> > +}
> > +
> > +/**
> > + * \class YamlEmitter::Emitter
> > + *
> > + * yaml_emitter_t wrapper. Initialize the yaml_emitter_t on creation allows
> > + * YamlOutput classes to emit events.
> > + */
> > +
> > +YamlEmitter::Emitter::~Emitter()
> > +{
> > + yaml_emitter_delete(&emitter_);
> > +}
> > +
> > +void YamlEmitter::Emitter::init(File *file)
> > +{
> > + yaml_emitter_initialize(&emitter_);
> > + yaml_emitter_set_output(&emitter_, yamlWrite, file);
> > +}
> > +
> > +void YamlEmitter::Emitter::logError()
> > +{
> > + switch (emitter_.error) {
> > + case YAML_MEMORY_ERROR:
> > + LOG(YamlEmitter, Error)
> > + << "Memory error: Not enough memory for emitting";
> > + break;
> > +
> > + case YAML_WRITER_ERROR:
> > + LOG(YamlEmitter, Error)
> > + << "Writer error: " << emitter_.problem;
> > + break;
> > +
> > + case YAML_EMITTER_ERROR:
> > + LOG(YamlEmitter, Error)
> > + << "Emitter error: " << emitter_.problem;
> > + break;
> > +
> > + default:
> > + LOG(YamlEmitter, Error) << "Internal problem";
> > + break;
> > + }
> > +}
> > +
> > +int YamlEmitter::Emitter::emit(YamlEvent *event)
> > +{
> > + int ret = yaml_emitter_emit(&emitter_, event->event());
> > + if (!ret) {
> > + logError();
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \class YamlOutput
> > + *
> > + * The YamlOutput base class. From this class YamlScalar, YamlList and YamlDict
> > + * are derived.
> > + *
> > + * The YamlOutput class provides functions to create a scalar, a list or a
> > + * dictionary.
> > + *
> > + * The class cannot be instantiated directly by applications.
> > + */
> > +
> > +std::unique_ptr<YamlScalar> YamlOutput::scalar()
> > +{
> > + return std::unique_ptr<YamlScalar>(new YamlScalar(emitter_));
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlOutput::dict()
> > +{
> > + return std::unique_ptr<YamlDict>(new YamlDict(emitter_));
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlOutput::list()
> > +{
> > + return std::unique_ptr<YamlList>(new YamlList(emitter_));
> > +}
> > +
> > +int YamlOutput::emitScalar(std::string_view scalar)
> > +{
> > + std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +
> > + const unsigned char *value = reinterpret_cast<const unsigned char *>
> > + (scalar.data());
> > + yaml_scalar_event_initialize(event->event(), NULL, NULL, value,
> > + scalar.length(), true, false,
> > + YAML_PLAIN_SCALAR_STYLE);
> > + return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitMappingStart()
> > +{
> > + std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > + yaml_mapping_start_event_initialize(event->event(), NULL, NULL,
> > + true, YAML_BLOCK_MAPPING_STYLE);
> > + return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitMappingEnd()
> > +{
> > + std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > + yaml_mapping_end_event_initialize(event->event());
> > + return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitSequenceStart()
> > +{
> > + std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > + yaml_sequence_start_event_initialize(event->event(), NULL, NULL, true,
> > + YAML_BLOCK_SEQUENCE_STYLE);
> > + return emitter_->emit(event.get());
> > +}
> > +
> > +int YamlOutput::emitSequenceEnd()
> > +{
> > + std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > + yaml_sequence_end_event_initialize(event->event());
> > + return emitter_->emit(event.get());
> > +}
> > +
> > +/**
> > + * \class YamlRoot
> > + *
> > + * Yaml root node. A root node can be populated with a scalar, a list or a dict.
> > + */
> > +
> > +YamlRoot::~YamlRoot()
> > +{
> > + std::unique_ptr<YamlEvent> event = YamlEvent::create();
> > +
> > + yaml_document_end_event_initialize(event->event(), 0);
> > + emitterRoot_->emit(event.get());
> > +
> > + yaml_stream_end_event_initialize(event->event());
> > + emitterRoot_->emit(event.get());
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlRoot::dict()
> > +{
> > + emitMappingStart();
> > +
> > + return YamlOutput::dict();
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlRoot::list()
> > +{
> > + emitSequenceStart();
> > +
> > + return YamlOutput::list();
> > +}
> > +
> > +/**
> > + * \class YamlScalar
> > + *
> > + * A YamlScalar can be assigned to an std::string_view and other libcamera
> > + * types to emit them as YAML scalars.
> > + */
> > +
> > +YamlScalar::YamlScalar(YamlEmitter *emitter)
> > + : YamlOutput(emitter)
> > +{
> > +}
> > +
> > +void YamlScalar::operator=(const libcamera::Orientation &orientation)
> > +{
> > + std::stringstream o;
> > + o << orientation;
> > +
> > + emitScalar(o.str());
> > +}
> > +
> > +void YamlScalar::operator=(std::string_view scalar)
> > +{
> > + emitScalar(scalar);
> > +}
> > +
> > +/**
> > + * \class YamlList
> > + *
> > + * A YamlList can be populated with scalar and allows to create other lists
> > + * and dictionaries.
> > + */
> > +
> > +YamlList::YamlList(YamlEmitter *emitter)
> > + : YamlOutput(emitter)
> > +{
> > +}
> > +
> > +YamlList::~YamlList()
> > +{
> > + if (!closed_)
> > + close();
> > +}
> > +
> > +void YamlList::close()
> > +{
> > + emitSequenceEnd();
> > + YamlOutput::close();
> > +}
> > +
> > +void YamlList::scalar(std::string_view scalar)
> > +{
> > + emitScalar(scalar);
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlList::list()
> > +{
> > + emitSequenceStart();
> > +
> > + return YamlOutput::list();
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlList::dict()
> > +{
> > + emitMappingStart();
> > +
> > + return YamlOutput::dict();
> > +}
> > +
> > +/**
> > + * \class YamlDict
> > + *
> > + * A YamlDict derives an unordered_map that maps strings to YAML scalar.
> > + *
> > + * A YamlDict can be populated with scalars using operator[] and allows to
> > + * create other lists and dictionaries.
> > + */
> > +
> > +YamlDict::YamlDict(YamlEmitter *emitter)
> > + : YamlOutput(emitter)
> > +{
> > +}
> > +
> > +YamlDict::~YamlDict()
> > +{
> > + if (!closed_)
> > + close();
> > +
> > + clear();
> > +}
> > +
> > +void YamlDict::close()
> > +{
> > + emitMappingEnd();
> > + YamlOutput::close();
> > +}
> > +
> > +std::unique_ptr<YamlList> YamlDict::list(std::string_view key)
> > +{
> > + emitScalar(key);
> > + emitSequenceStart();
> > +
> > + return YamlOutput::list();
> > +}
> > +
> > +std::unique_ptr<YamlDict> YamlDict::dict(std::string_view key)
> > +{
> > + emitScalar(key);
> > + emitMappingStart();
> > +
> > + return YamlOutput::dict();
> > +}
> > +
> > +YamlScalar &YamlDict::operator[](const YamlDict::Map::key_type &key)
> > +{
> > + emplace(key, YamlOutput::scalar());
> > + emitScalar(key);
> > +
> > + return *at(key);
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list