[{"id":32035,"web_url":"https://patchwork.libcamera.org/comment/32035/","msgid":"<20241105235000.GE17733@pendragon.ideasonboard.com>","date":"2024-11-05T23:50:00","subject":"Re: [RFC v3 3/4] libcamera: Implement YamlEmitter","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Oct 21, 2024 at 11:49:51AM +0200, Jacopo Mondi wrote:\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> ---\n>  Documentation/Doxyfile-internal.in        |   2 +\n>  include/libcamera/internal/meson.build    |   1 +\n>  include/libcamera/internal/yaml_emitter.h | 183 +++++++++++\n>  src/libcamera/meson.build                 |   1 +\n>  src/libcamera/yaml_emitter.cpp            | 360 ++++++++++++++++++++++\n>  5 files changed, 547 insertions(+)\n>  create mode 100644 include/libcamera/internal/yaml_emitter.h\n>  create mode 100644 src/libcamera/yaml_emitter.cpp\n> \n> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in\n> index cf9825537866..e0ba64da1bef 100644\n> --- a/Documentation/Doxyfile-internal.in\n> +++ b/Documentation/Doxyfile-internal.in\n> @@ -21,9 +21,11 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \\\n>                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \\\n>                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \\\n>                           @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \\\n> +                         @TOP_SRCDIR@/include/libcamera/internal/yaml_emitter.h \\\n>                           @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \\\n>                           @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \\\n>                           @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \\\n> +                         @TOP_SRCDIR@/src/libcamera/yaml_emitter.cpp \\\n>                           @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n>                           @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n>                           @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 1c5eef9cab80..7533b075fde2 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -41,6 +41,7 @@ libcamera_internal_headers = files([\n>      'v4l2_pixelformat.h',\n>      'v4l2_subdevice.h',\n>      'v4l2_videodevice.h',\n> +    'yaml_emitter.h',\n>      'yaml_parser.h',\n>  ])\n>  \n> diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h\n> new file mode 100644\n> index 000000000000..bcbb574061cc\n> --- /dev/null\n> +++ b/include/libcamera/internal/yaml_emitter.h\n> @@ -0,0 +1,183 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board Oy\n> + *\n> + * libcamera YAML emitter helper\n> + */\n> +\n> +#pragma once\n> +\n> +#include <memory>\n> +#include <string_view>\n> +\n> +#include <libcamera/base/class.h>\n> +#include <libcamera/base/file.h>\n> +#include <libcamera/orientation.h>\n> +\n> +#include <yaml.h>\n> +\n> +namespace libcamera {\n> +\n> +class YamlDict;\n> +class YamlEvent;\n> +class YamlList;\n> +class YamlRoot;\n> +class YamlScalar;\n> +\n> +class YamlEmitter final\n\nThis class is not part of the public API (as far as I can tell). You can\njust declare it here with\n\nclass YamlEmitter;\n\nand define it in the .cpp file.\n\n> +{\n> +public:\n> +\t~YamlEmitter();\n> +\n> +\tstatic YamlRoot root(std::string_view path);\n\nTo make this class more generic, how about passing a std::ostream\npointer ?\n\n> +\n> +\tint emit();\n> +\tyaml_event_t *event() { return &event_; }\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(YamlEmitter)\n> +\n> +\tclass Emitter\n> +\t{\n> +\tpublic:\n> +\t\t~Emitter();\n> +\n> +\t\tvoid init(File *file);\n> +\n> +\t\tint emit(yaml_event_t *event);\n> +\n> +\tprivate:\n> +\t\tvoid logError();\n> +\n> +\t\tyaml_emitter_t emitter_;\n> +\t};\n\nWhy is this split in another class ? \n\n> +\n> +\tYamlEmitter() = default;\n> +\n> +\tvoid init();\n> +\n> +\tFile file_;\n> +\tyaml_event_t event_;\n> +\tEmitter emitter_;\n> +};\n> +\n> +class YamlOutput\n> +{\n> +public:\n> +\tvirtual ~YamlOutput() {};\n\nExtraneous trailing ;\n\nBut does the class need a virtual destructor ? Is there any case where\none would delete a pointer to YamlOutput ?\n\n> +\n> +\tYamlOutput() = default;\n\nThe constructors go before the destructor. And it should be protected,\nas nobody should create a YamlOutput instance.\n\n> +\n> +\tYamlOutput(YamlOutput &&other)\n> +\t{\n> +\t\temitter_ = other.emitter_;\n> +\t\tother.emitter_ = nullptr;\n> +\t}\n\nSame here, this should be protected.\n\n> +\n> +\tbool initialized() { return !!emitter_; }\n\nI would name this function valid(), as it's not just whether or not the\nobject has been initialized, but it also becomes invalid when moved.\n\nThe function should also be const.\n\n> +\n> +\tYamlScalar scalar();\n> +\tYamlDict dict();\n> +\tYamlList list();\n\nI think those 3 functions should be protected, they're not meant to be\ncalled by the user as far as I understand.\n\n> +\n> +protected:\n> +\tYamlOutput(YamlEmitter *emitter)\n> +\t\t: emitter_(emitter)\n> +\t{\n> +\t}\n> +\n> +\tYamlOutput &operator=(YamlOutput &&other)\n> +\t{\n> +\t\temitter_ = other.emitter_;\n> +\t\tother.emitter_ = nullptr;\n> +\n> +\t\treturn *this;\n> +\t}\n> +\n> +\tint emitScalar(std::string_view scalar);\n> +\tint emitMappingStart();\n> +\tint emitMappingEnd();\n> +\tint emitSequenceStart();\n> +\tint emitSequenceEnd();\n> +\n> +\tYamlEmitter *emitter_ = nullptr;\n> +\tyaml_event_t event_;\n\nI think you should disable copying of this class with\nLIBCAMERA_DISABLE_COPY().\n\n> +};\n> +\n> +class YamlRoot : public YamlOutput\n> +{\n> +public:\n> +\tYamlRoot() = default;\n> +\tYamlRoot(YamlRoot &&other) = default;\n> +\t~YamlRoot();\n> +\n> +\tYamlRoot &operator=(YamlRoot &&other) = default;\n> +\n> +\tYamlList list();\n> +\tYamlDict dict();\n> +\tvoid scalar(std::string_view scalar);\n\nYou don't implement YamlRoot::scalar() in the .cpp file, which is a good\nindication that the function is likely not needed :-) I would just drop\nit.\n\n> +\n> +private:\n> +\tfriend class YamlEmitter;\n> +\n> +\tYamlRoot(std::unique_ptr<YamlEmitter> emitter)\n> +\t\t: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))\n> +\t{\n> +\t}\n> +\n> +\tstd::unique_ptr<YamlEmitter> emitterRoot_;\n> +};\n> +\n> +class YamlScalar : public YamlOutput\n> +{\n> +public:\n> +\t~YamlScalar() = default;\n> +\n> +\tvoid operator=(std::string_view scalar);\n> +\n> +private:\n> +\tfriend class YamlOutput;\n> +\n> +\tYamlScalar(YamlEmitter *emitter);\n> +};\n> +\n> +class YamlList : public YamlOutput\n> +{\n> +public:\n> +\tYamlList() = default;\n> +\tYamlList(YamlList &&other) = default;\n> +\t~YamlList();\n> +\n> +\tYamlList &operator=(YamlList &&other) = default;\n> +\n> +\tYamlList list();\n> +\tYamlDict dict();\n> +\tvoid scalar(std::string_view scalar);\n> +\n> +private:\n> +\tfriend class YamlOutput;\n> +\n> +\tYamlList(YamlEmitter *emitter);\n> +};\n> +\n> +class YamlDict : public YamlOutput\n> +{\n> +public:\n> +\tYamlDict() = default;\n> +\tYamlDict(YamlDict &&other) = default;\n> +\t~YamlDict();\n> +\n> +\tYamlDict &operator=(YamlDict &&other) = default;\n> +\n> +\tYamlList list(std::string_view key);\n> +\tYamlDict dict(std::string_view key);\n> +\n> +\tYamlScalar operator[](std::string_view key);\n> +\n> +private:\n> +\tfriend class YamlOutput;\n> +\n> +\tYamlDict(YamlEmitter *emitter);\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index aa9ab0291854..5b8af4103085 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -51,6 +51,7 @@ libcamera_internal_sources = files([\n>      'v4l2_pixelformat.cpp',\n>      'v4l2_subdevice.cpp',\n>      'v4l2_videodevice.cpp',\n> +    'yaml_emitter.cpp',\n>      'yaml_parser.cpp',\n>  ])\n>  \n> diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp\n> new file mode 100644\n> index 000000000000..9542ad2d9c18\n> --- /dev/null\n> +++ b/src/libcamera/yaml_emitter.cpp\n> @@ -0,0 +1,360 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board Oy\n> + *\n> + * libcamera YAML emitter helper\n> + */\n> +\n> +#include \"libcamera/internal/yaml_emitter.h\"\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +/**\n> + * \\file yaml_emitter.h\n> + * \\brief A YAML emitter helper\n> + *\n> + * The YAML Emitter helpers allows users to emit output in YAML format.\n\ns/helpers/helper/\n\nor\n\ns/allows/allow/\n\n> + *\n> + * To emit YAML users of this classes should create a root node with\n\n\"this class\" or \"these classes\"\n\ns/should //\n\n> + *\n> + * \\code\n> +\tstd::string filePath(\"...\");\n> +\tauto root = YamlEmitter::root(filePath);\n> +   \\endcode\n> + *\n> + * A YamlRoot implements RAII-style handling of YAML sequence and document\n> + * events handling. Creating a YamlRoot emits the STREAM_START and DOC_START\n> + * events. Once a YamlRoot gets deleted the DOC_SEND and STREAM_END events\n> + * gets automatically deleted.\n\nThat's an implementation detail, it's not relevant to the users of the\nclass.\n\n> + *\n> + * Once a root node has been created it is possible to populate it with\n> + * scalars, list or dictionaries.\n> + *\n> + * YamlList and YamlDict can only be created wrapped in unique_ptr<> instances,\n\nNot true anymore.\n\n> + * to implement a RAII-style handling of YAML of lists and dictionaries.\n> + * Creating a YamlList and a YamlDict emits the YAML sequence and mapping start\n> + * events. Once an instance gets deleted, the sequence and mapping gets\n> + * automatically emitted.\n\nEvents are an implementation detail here too.\n\nAll these are opportunities to improve the doc for the first non-RFC\nversion :-)\n\n> + *\n> + * A YamlScalar is a simpler object that can be assigned to different types\n> + * to emit them as strings.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(YamlEmitter)\n> +\n> +namespace {\n> +\n> +int yamlWrite(void *data, unsigned char *buffer, size_t size)\n> +{\n> +\tFile *file = static_cast<File *>(data);\n> +\n> +\tSpan<unsigned char> buf{ buffer, size };\n> +\tssize_t ret = file->write(buf);\n> +\tif (ret < 0) {\n> +\t\tret = errno;\n\nFile::write() returns the error code, no need to get it from errno.\n\n> +\t\tLOG(YamlEmitter, Error) << \"Write error : \" << strerror(ret);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn 1;\n> +}\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\class YamlEmitter\n> + *\n> + * Yaml Emitter entry point. Allows to create a YamlRoot object that users\n\ns/Yaml/YAML/\n\n> + * can populate.\n> + */\n> +\n> +YamlEmitter::~YamlEmitter()\n> +{\n> +\tyaml_event_delete(&event_);\n> +}\n> +\n> +/**\n> + * \\brief Create a YamlRoot that applications can start populating with YamlOutput\n> + * \\param[in] path The YAML output file path\n> + * \\return A unique pointer to a YamlRoot\n\nNo unique pointer anymore. And we don't usually document the types in\nthe text, they're clearly visible in the generated documentation.\n\n> + */\n> +YamlRoot YamlEmitter::root(std::string_view path)\n> +{\n> +\tstd::unique_ptr<YamlEmitter> emitter{ new YamlEmitter() };\n> +\n> +\tstd::string filePath(path);\n\nI've sent a patch to support std::string_view in the File class so this\nbecomes unnecessary.\n\n> +\temitter->file_.setFileName(filePath);\n> +\temitter->file_.open(File::OpenModeFlag::WriteOnly);\n> +\n> +\temitter->init();\n\nI think there's room for simplification here.\n\n\tFile file(path);\n\tfile.open(File::OpenModeFlag::WriteOnly);\n\n\tstd::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(std::move(file)) };\n\nand drop YamlEmitter::init(); and make YamlEmitter::file_ private.\n\n> +\n> +\treturn YamlRoot(std::move(emitter));\n> +}\n> +\n> +/**\n> + * \\brief Emit a yaml event\n> + */\n> +int YamlEmitter::emit()\n> +{\n> +\treturn emitter_.emit(&event_);\n> +}\n> +\n> +void YamlEmitter::init()\n> +{\n> +\temitter_.init(&file_);\n> +\n> +\tyaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);\n> +\temitter_.emit(&event_);\n> +\n> +\tyaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);\n> +\temitter_.emit(&event_);\n> +}\n> +\n> +/**\n> + * \\class YamlEmitter::Emitter\n> + *\n> + * yaml_emitter_t wrapper. Initialize the yaml_emitter_t on creation allows\n> + * YamlOutput classes to emit events.\n> + */\n> +\n> +YamlEmitter::Emitter::~Emitter()\n> +{\n> +\tyaml_emitter_delete(&emitter_);\n> +}\n> +\n> +void YamlEmitter::Emitter::init(File *file)\n> +{\n> +\tyaml_emitter_initialize(&emitter_);\n> +\tyaml_emitter_set_output(&emitter_, yamlWrite, file);\n> +}\n> +\n> +void YamlEmitter::Emitter::logError()\n> +{\n> +\tswitch (emitter_.error) {\n> +\tcase YAML_MEMORY_ERROR:\n> +\t\tLOG(YamlEmitter, Error)\n> +\t\t\t<< \"Memory error: Not enough memory for emitting\";\n> +\t\tbreak;\n> +\n> +\tcase YAML_WRITER_ERROR:\n> +\t\tLOG(YamlEmitter, Error)\n> +\t\t\t<< \"Writer error: \" << emitter_.problem;\n> +\t\tbreak;\n> +\n> +\tcase YAML_EMITTER_ERROR:\n> +\t\tLOG(YamlEmitter, Error)\n> +\t\t\t<< \"Emitter error: \" << emitter_.problem;\n> +\t\tbreak;\n> +\n> +\tdefault:\n> +\t\tLOG(YamlEmitter, Error) << \"Internal problem\";\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n> +int YamlEmitter::Emitter::emit(yaml_event_t *event)\n> +{\n> +\tint ret = yaml_emitter_emit(&emitter_, event);\n> +\tif (!ret) {\n> +\t\tlogError();\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\class YamlOutput\n> + *\n> + * The YamlOutput base class. From this class YamlScalar, YamlList and YamlDict\n> + * are derived.\n> + *\n> + * The YamlOutput class provides functions to create a scalar, a list or a\n> + * dictionary.\n> + *\n> + * The class cannot be instantiated directly by applications.\n> + */\n> +\n> +YamlScalar YamlOutput::scalar()\n> +{\n> +\treturn YamlScalar(emitter_);\n> +}\n> +\n> +YamlDict YamlOutput::dict()\n> +{\n> +\treturn YamlDict(emitter_);\n> +}\n> +\n> +YamlList YamlOutput::list()\n> +{\n> +\treturn YamlList(emitter_);\n> +}\n> +\n> +int YamlOutput::emitScalar(std::string_view scalar)\n> +{\n> +\tconst unsigned char *value = reinterpret_cast<const unsigned char *>\n> +\t\t\t\t     (scalar.data());\n\nI would use yaml_char_t instead of unsigned char here, as that's the\ntaype taken by the yaml_scalar_event_initialize() function.\n\n> +\tyaml_scalar_event_initialize(emitter_->event(), NULL, NULL, value,\n> +\t\t\t\t     scalar.length(), true, false,\n> +\t\t\t\t     YAML_PLAIN_SCALAR_STYLE);\n> +\treturn emitter_->emit();\n> +}\n> +\n> +int YamlOutput::emitMappingStart()\n> +{\n> +\tyaml_mapping_start_event_initialize(emitter_->event(), NULL, NULL,\n> +\t\t\t\t\t    true, YAML_BLOCK_MAPPING_STYLE);\n> +\treturn emitter_->emit();\n> +}\n> +\n> +int YamlOutput::emitMappingEnd()\n> +{\n> +\tyaml_mapping_end_event_initialize(emitter_->event());\n> +\treturn emitter_->emit();\n> +}\n> +\n> +int YamlOutput::emitSequenceStart()\n> +{\n> +\tyaml_sequence_start_event_initialize(emitter_->event(), NULL, NULL, true,\n> +\t\t\t\t\t     YAML_BLOCK_SEQUENCE_STYLE);\n> +\treturn emitter_->emit();\n> +}\n> +\n> +int YamlOutput::emitSequenceEnd()\n> +{\n> +\tyaml_sequence_end_event_initialize(emitter_->event());\n> +\treturn emitter_->emit();\n> +}\n> +\n> +/**\n> + * \\class YamlRoot\n> + *\n> + * Yaml root node. A root node can be populated with a scalar, a list or a dict.\n\ns/Yaml/YAML/\n\n> + */\n> +\n> +YamlRoot::~YamlRoot()\n> +{\n> +\tif (!initialized())\n> +\t\treturn;\n> +\n> +\tyaml_document_end_event_initialize(emitter_->event(), 0);\n> +\temitterRoot_->emit();\n> +\n> +\tyaml_stream_end_event_initialize(emitter_->event());\n> +\temitterRoot_->emit();\n> +}\n> +\n> +YamlDict YamlRoot::dict()\n> +{\n> +\temitMappingStart();\n> +\n> +\treturn YamlOutput::dict();\n> +}\n> +\n> +YamlList YamlRoot::list()\n> +{\n> +\temitSequenceStart();\n> +\n> +\treturn YamlOutput::list();\n> +}\n> +\n> +/**\n> + * \\class YamlScalar\n> + *\n> + * A YamlScalar can be assigned to an std::string_view and other libcamera\n> + * types to emit them as YAML scalars.\n> + */\n> +\n> +YamlScalar::YamlScalar(YamlEmitter *emitter)\n> +\t: YamlOutput(emitter)\n> +{\n> +}\n> +\n> +void YamlScalar::operator=(std::string_view scalar)\n> +{\n> +\temitScalar(scalar);\n> +}\n> +\n> +/**\n> + * \\class YamlList\n> + *\n> + * A YamlList can be populated with scalar and allows to create other lists\n> + * and dictionaries.\n> + */\n> +\n> +YamlList::YamlList(YamlEmitter *emitter)\n> +\t: YamlOutput(emitter)\n> +{\n> +}\n> +\n> +YamlList::~YamlList()\n> +{\n> +\tif (initialized())\n> +\t\temitSequenceEnd();\n> +}\n> +\n> +void YamlList::scalar(std::string_view scalar)\n> +{\n> +\temitScalar(scalar);\n> +}\n> +\n> +YamlList YamlList::list()\n> +{\n> +\temitSequenceStart();\n> +\n> +\treturn YamlOutput::list();\n> +}\n> +\n> +YamlDict YamlList::dict()\n> +{\n> +\temitMappingStart();\n> +\n> +\treturn YamlOutput::dict();\n> +}\n> +\n> +/**\n> + * \\class YamlDict\n> + *\n> + * A YamlDict derives an unordered_map that maps strings to YAML scalar.\n> + *\n> + * A YamlDict can be populated with scalars using operator[] and allows to\n> + * create other lists and dictionaries.\n> + */\n> +\n> +YamlDict::YamlDict(YamlEmitter *emitter)\n> +\t: YamlOutput(emitter)\n> +{\n> +}\n> +\n> +YamlDict::~YamlDict()\n> +{\n> +\tif (initialized())\n> +\t\temitMappingEnd();\n> +}\n> +\n> +YamlList YamlDict::list(std::string_view key)\n> +{\n> +\temitScalar(key);\n> +\temitSequenceStart();\n> +\n> +\treturn YamlOutput::list();\n> +}\n> +\n> +YamlDict YamlDict::dict(std::string_view key)\n> +{\n> +\temitScalar(key);\n> +\temitMappingStart();\n> +\n> +\treturn YamlOutput::dict();\n> +}\n> +\n> +YamlScalar YamlDict::operator[](std::string_view key)\n> +{\n> +\temitScalar(key);\n> +\n> +\treturn YamlOutput::scalar();\n> +}\n> +\n> +} /* namespace libcamera */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E6800BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  5 Nov 2024 23:50:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9D6365416;\n\tWed,  6 Nov 2024 00:50:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B046653DF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Nov 2024 00:50:09 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0FB56DC;\n\tWed,  6 Nov 2024 00:50:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vDjLZufB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730850601;\n\tbh=ZkeFRhZ7CgqkC/nIj72pZe5mKWsMNMrV2kLQMPMq13I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vDjLZufBfyjRo74EJSm29bQoJMNTvGwr017I4kJ3FP/FcvHs/XzMVpQOHtjb4NLOd\n\tPN0LzE7tUD4hWYcjvdn2eC+duxcrBFOkxn19sLVUIBaTdRpzx0uMSfiGbDegjT51ED\n\t3F1fm9lLVw5G608GqX73s3f3e2zdkeZdJwALWj10=","Date":"Wed, 6 Nov 2024 01:50:00 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC v3 3/4] libcamera: Implement YamlEmitter","Message-ID":"<20241105235000.GE17733@pendragon.ideasonboard.com>","References":"<20241021094955.26991-1-jacopo.mondi@ideasonboard.com>\n\t<20241021094955.26991-4-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241021094955.26991-4-jacopo.mondi@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32039,"web_url":"https://patchwork.libcamera.org/comment/32039/","msgid":"<eoz345e46k5auuhspze4htlgqkuf5zwnivi2rx63qgypoffiqf@v4gwb5c7eyqy>","date":"2024-11-06T09:45:37","subject":"Re: [RFC v3 3/4] libcamera: Implement YamlEmitter","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n   thanks for review\n\nOn Wed, Nov 06, 2024 at 01:50:00AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 21, 2024 at 11:49:51AM +0200, Jacopo Mondi wrote:\n> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > ---\n> >  Documentation/Doxyfile-internal.in        |   2 +\n> >  include/libcamera/internal/meson.build    |   1 +\n> >  include/libcamera/internal/yaml_emitter.h | 183 +++++++++++\n> >  src/libcamera/meson.build                 |   1 +\n> >  src/libcamera/yaml_emitter.cpp            | 360 ++++++++++++++++++++++\n> >  5 files changed, 547 insertions(+)\n> >  create mode 100644 include/libcamera/internal/yaml_emitter.h\n> >  create mode 100644 src/libcamera/yaml_emitter.cpp\n> >\n> > diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in\n> > index cf9825537866..e0ba64da1bef 100644\n> > --- a/Documentation/Doxyfile-internal.in\n> > +++ b/Documentation/Doxyfile-internal.in\n> > @@ -21,9 +21,11 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \\\n> >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \\\n> >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \\\n> >                           @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \\\n> > +                         @TOP_SRCDIR@/include/libcamera/internal/yaml_emitter.h \\\n> >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \\\n> >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \\\n> >                           @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \\\n> > +                         @TOP_SRCDIR@/src/libcamera/yaml_emitter.cpp \\\n> >                           @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> >                           @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> >                           @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 1c5eef9cab80..7533b075fde2 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([\n> >      'v4l2_pixelformat.h',\n> >      'v4l2_subdevice.h',\n> >      'v4l2_videodevice.h',\n> > +    'yaml_emitter.h',\n> >      'yaml_parser.h',\n> >  ])\n> >\n> > diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h\n> > new file mode 100644\n> > index 000000000000..bcbb574061cc\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/yaml_emitter.h\n> > @@ -0,0 +1,183 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board Oy\n> > + *\n> > + * libcamera YAML emitter helper\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include <memory>\n> > +#include <string_view>\n> > +\n> > +#include <libcamera/base/class.h>\n> > +#include <libcamera/base/file.h>\n> > +#include <libcamera/orientation.h>\n> > +\n> > +#include <yaml.h>\n> > +\n> > +namespace libcamera {\n> > +\n> > +class YamlDict;\n> > +class YamlEvent;\n> > +class YamlList;\n> > +class YamlRoot;\n> > +class YamlScalar;\n> > +\n> > +class YamlEmitter final\n>\n> This class is not part of the public API (as far as I can tell). You can\n> just declare it here with\n\nThis class is the only entry point from which users can create a\n\"root\" node. I think it should be part of the public API.\n\n>\n> class YamlEmitter;\n>\n> and define it in the .cpp file.\n> > +{\n> > +public:\n> > +\t~YamlEmitter();\n> > +\n> > +\tstatic YamlRoot root(std::string_view path);\n>\n> To make this class more generic, how about passing a std::ostream\n> pointer ?\n>\n> > +\n> > +\tint emit();\n> > +\tyaml_event_t *event() { return &event_; }\n> > +\n> > +private:\n> > +\tLIBCAMERA_DISABLE_COPY(YamlEmitter)\n> > +\n> > +\tclass Emitter\n> > +\t{\n> > +\tpublic:\n> > +\t\t~Emitter();\n> > +\n> > +\t\tvoid init(File *file);\n> > +\n> > +\t\tint emit(yaml_event_t *event);\n> > +\n> > +\tprivate:\n> > +\t\tvoid logError();\n> > +\n> > +\t\tyaml_emitter_t emitter_;\n> > +\t};\n>\n> Why is this split in another class ?\n>\n\nI probably thought it looked cleaner, but it's not. I'll drop it.\n\n> > +\n> > +\tYamlEmitter() = default;\n> > +\n> > +\tvoid init();\n> > +\n> > +\tFile file_;\n> > +\tyaml_event_t event_;\n> > +\tEmitter emitter_;\n> > +};\n> > +\n> > +class YamlOutput\n> > +{\n> > +public:\n> > +\tvirtual ~YamlOutput() {};\n>\n> Extraneous trailing ;\n>\n> But does the class need a virtual destructor ? Is there any case where\n> one would delete a pointer to YamlOutput ?\n>\n\nNot as a pointer to YamlOutput, no. I'll drop\n\n> > +\n> > +\tYamlOutput() = default;\n>\n> The constructors go before the destructor. And it should be protected,\n> as nobody should create a YamlOutput instance.\n>\n\nack\n\n> > +\n> > +\tYamlOutput(YamlOutput &&other)\n> > +\t{\n> > +\t\temitter_ = other.emitter_;\n> > +\t\tother.emitter_ = nullptr;\n> > +\t}\n>\n> Same here, this should be protected.\n>\n\nack\n\n> > +\n> > +\tbool initialized() { return !!emitter_; }\n>\n> I would name this function valid(), as it's not just whether or not the\n> object has been initialized, but it also becomes invalid when moved.\n>\n> The function should also be const.\n\nright\n\n>\n> > +\n> > +\tYamlScalar scalar();\n> > +\tYamlDict dict();\n> > +\tYamlList list();\n>\n> I think those 3 functions should be protected, they're not meant to be\n> called by the user as far as I understand.\n>\n\nYes, these can be protected\n\n> > +\n> > +protected:\n> > +\tYamlOutput(YamlEmitter *emitter)\n> > +\t\t: emitter_(emitter)\n> > +\t{\n> > +\t}\n> > +\n> > +\tYamlOutput &operator=(YamlOutput &&other)\n> > +\t{\n> > +\t\temitter_ = other.emitter_;\n> > +\t\tother.emitter_ = nullptr;\n> > +\n> > +\t\treturn *this;\n> > +\t}\n> > +\n> > +\tint emitScalar(std::string_view scalar);\n> > +\tint emitMappingStart();\n> > +\tint emitMappingEnd();\n> > +\tint emitSequenceStart();\n> > +\tint emitSequenceEnd();\n> > +\n> > +\tYamlEmitter *emitter_ = nullptr;\n> > +\tyaml_event_t event_;\n>\n> I think you should disable copying of this class with\n> LIBCAMERA_DISABLE_COPY().\n>\n\ndone, in the private: section\n\n> > +};\n> > +\n> > +class YamlRoot : public YamlOutput\n> > +{\n> > +public:\n> > +\tYamlRoot() = default;\n> > +\tYamlRoot(YamlRoot &&other) = default;\n> > +\t~YamlRoot();\n> > +\n> > +\tYamlRoot &operator=(YamlRoot &&other) = default;\n> > +\n> > +\tYamlList list();\n> > +\tYamlDict dict();\n> > +\tvoid scalar(std::string_view scalar);\n>\n> You don't implement YamlRoot::scalar() in the .cpp file, which is a good\n> indication that the function is likely not needed :-) I would just drop\n> it.\n>\n\nMaybe I'm not using it right now, but I think emitting a scalar in the\nroot node is still a valid use case. Maybe I should implement the\nfunction.\n\n\n> > +\n> > +private:\n> > +\tfriend class YamlEmitter;\n> > +\n> > +\tYamlRoot(std::unique_ptr<YamlEmitter> emitter)\n> > +\t\t: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))\n> > +\t{\n> > +\t}\n> > +\n> > +\tstd::unique_ptr<YamlEmitter> emitterRoot_;\n> > +};\n> > +\n> > +class YamlScalar : public YamlOutput\n> > +{\n> > +public:\n> > +\t~YamlScalar() = default;\n> > +\n> > +\tvoid operator=(std::string_view scalar);\n> > +\n> > +private:\n> > +\tfriend class YamlOutput;\n> > +\n> > +\tYamlScalar(YamlEmitter *emitter);\n> > +};\n> > +\n> > +class YamlList : public YamlOutput\n> > +{\n> > +public:\n> > +\tYamlList() = default;\n> > +\tYamlList(YamlList &&other) = default;\n> > +\t~YamlList();\n> > +\n> > +\tYamlList &operator=(YamlList &&other) = default;\n> > +\n> > +\tYamlList list();\n> > +\tYamlDict dict();\n> > +\tvoid scalar(std::string_view scalar);\n> > +\n> > +private:\n> > +\tfriend class YamlOutput;\n> > +\n> > +\tYamlList(YamlEmitter *emitter);\n> > +};\n> > +\n> > +class YamlDict : public YamlOutput\n> > +{\n> > +public:\n> > +\tYamlDict() = default;\n> > +\tYamlDict(YamlDict &&other) = default;\n> > +\t~YamlDict();\n> > +\n> > +\tYamlDict &operator=(YamlDict &&other) = default;\n> > +\n> > +\tYamlList list(std::string_view key);\n> > +\tYamlDict dict(std::string_view key);\n> > +\n> > +\tYamlScalar operator[](std::string_view key);\n> > +\n> > +private:\n> > +\tfriend class YamlOutput;\n> > +\n> > +\tYamlDict(YamlEmitter *emitter);\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index aa9ab0291854..5b8af4103085 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([\n> >      'v4l2_pixelformat.cpp',\n> >      'v4l2_subdevice.cpp',\n> >      'v4l2_videodevice.cpp',\n> > +    'yaml_emitter.cpp',\n> >      'yaml_parser.cpp',\n> >  ])\n> >\n> > diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp\n> > new file mode 100644\n> > index 000000000000..9542ad2d9c18\n> > --- /dev/null\n> > +++ b/src/libcamera/yaml_emitter.cpp\n> > @@ -0,0 +1,360 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board Oy\n> > + *\n> > + * libcamera YAML emitter helper\n> > + */\n> > +\n> > +#include \"libcamera/internal/yaml_emitter.h\"\n> > +\n> > +#include <libcamera/base/log.h>\n> > +\n> > +/**\n> > + * \\file yaml_emitter.h\n> > + * \\brief A YAML emitter helper\n> > + *\n> > + * The YAML Emitter helpers allows users to emit output in YAML format.\n>\n> s/helpers/helper/\n>\n> or\n>\n> s/allows/allow/\n>\n> > + *\n> > + * To emit YAML users of this classes should create a root node with\n>\n> \"this class\" or \"these classes\"\n>\n> s/should //\n>\n> > + *\n> > + * \\code\n> > +\tstd::string filePath(\"...\");\n> > +\tauto root = YamlEmitter::root(filePath);\n> > +   \\endcode\n> > + *\n> > + * A YamlRoot implements RAII-style handling of YAML sequence and document\n> > + * events handling. Creating a YamlRoot emits the STREAM_START and DOC_START\n> > + * events. Once a YamlRoot gets deleted the DOC_SEND and STREAM_END events\n> > + * gets automatically deleted.\n>\n> That's an implementation detail, it's not relevant to the users of the\n> class.\n>\n> > + *\n> > + * Once a root node has been created it is possible to populate it with\n> > + * scalars, list or dictionaries.\n> > + *\n> > + * YamlList and YamlDict can only be created wrapped in unique_ptr<> instances,\n>\n> Not true anymore.\n>\n> > + * to implement a RAII-style handling of YAML of lists and dictionaries.\n> > + * Creating a YamlList and a YamlDict emits the YAML sequence and mapping start\n> > + * events. Once an instance gets deleted, the sequence and mapping gets\n> > + * automatically emitted.\n>\n> Events are an implementation detail here too.\n>\n> All these are opportunities to improve the doc for the first non-RFC\n> version :-)\n>\n> > + *\n> > + * A YamlScalar is a simpler object that can be assigned to different types\n> > + * to emit them as strings.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(YamlEmitter)\n> > +\n> > +namespace {\n> > +\n> > +int yamlWrite(void *data, unsigned char *buffer, size_t size)\n> > +{\n> > +\tFile *file = static_cast<File *>(data);\n> > +\n> > +\tSpan<unsigned char> buf{ buffer, size };\n> > +\tssize_t ret = file->write(buf);\n> > +\tif (ret < 0) {\n> > +\t\tret = errno;\n>\n> File::write() returns the error code, no need to get it from errno.\n>\n> > +\t\tLOG(YamlEmitter, Error) << \"Write error : \" << strerror(ret);\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\treturn 1;\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > +/**\n> > + * \\class YamlEmitter\n> > + *\n> > + * Yaml Emitter entry point. Allows to create a YamlRoot object that users\n>\n> s/Yaml/YAML/\n>\n> > + * can populate.\n> > + */\n> > +\n> > +YamlEmitter::~YamlEmitter()\n> > +{\n> > +\tyaml_event_delete(&event_);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Create a YamlRoot that applications can start populating with YamlOutput\n> > + * \\param[in] path The YAML output file path\n> > + * \\return A unique pointer to a YamlRoot\n>\n> No unique pointer anymore. And we don't usually document the types in\n> the text, they're clearly visible in the generated documentation.\n>\n> > + */\n> > +YamlRoot YamlEmitter::root(std::string_view path)\n> > +{\n> > +\tstd::unique_ptr<YamlEmitter> emitter{ new YamlEmitter() };\n> > +\n> > +\tstd::string filePath(path);\n>\n> I've sent a patch to support std::string_view in the File class so this\n> becomes unnecessary.\n>\n\nWhere ? I missed it\n\n> > +\temitter->file_.setFileName(filePath);\n> > +\temitter->file_.open(File::OpenModeFlag::WriteOnly);\n> > +\n> > +\temitter->init();\n>\n> I think there's room for simplification here.\n>\n> \tFile file(path);\n> \tfile.open(File::OpenModeFlag::WriteOnly);\n>\n> \tstd::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(std::move(file)) };\n>\n> and drop YamlEmitter::init(); and make YamlEmitter::file_ private.\n>\n> > +\n> > +\treturn YamlRoot(std::move(emitter));\n> > +}\n> > +\n> > +/**\n> > + * \\brief Emit a yaml event\n> > + */\n> > +int YamlEmitter::emit()\n> > +{\n> > +\treturn emitter_.emit(&event_);\n> > +}\n> > +\n> > +void YamlEmitter::init()\n> > +{\n> > +\temitter_.init(&file_);\n> > +\n> > +\tyaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);\n> > +\temitter_.emit(&event_);\n> > +\n> > +\tyaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);\n> > +\temitter_.emit(&event_);\n> > +}\n> > +\n> > +/**\n> > + * \\class YamlEmitter::Emitter\n> > + *\n> > + * yaml_emitter_t wrapper. Initialize the yaml_emitter_t on creation allows\n> > + * YamlOutput classes to emit events.\n> > + */\n> > +\n> > +YamlEmitter::Emitter::~Emitter()\n> > +{\n> > +\tyaml_emitter_delete(&emitter_);\n> > +}\n> > +\n> > +void YamlEmitter::Emitter::init(File *file)\n> > +{\n> > +\tyaml_emitter_initialize(&emitter_);\n> > +\tyaml_emitter_set_output(&emitter_, yamlWrite, file);\n> > +}\n> > +\n> > +void YamlEmitter::Emitter::logError()\n> > +{\n> > +\tswitch (emitter_.error) {\n> > +\tcase YAML_MEMORY_ERROR:\n> > +\t\tLOG(YamlEmitter, Error)\n> > +\t\t\t<< \"Memory error: Not enough memory for emitting\";\n> > +\t\tbreak;\n> > +\n> > +\tcase YAML_WRITER_ERROR:\n> > +\t\tLOG(YamlEmitter, Error)\n> > +\t\t\t<< \"Writer error: \" << emitter_.problem;\n> > +\t\tbreak;\n> > +\n> > +\tcase YAML_EMITTER_ERROR:\n> > +\t\tLOG(YamlEmitter, Error)\n> > +\t\t\t<< \"Emitter error: \" << emitter_.problem;\n> > +\t\tbreak;\n> > +\n> > +\tdefault:\n> > +\t\tLOG(YamlEmitter, Error) << \"Internal problem\";\n> > +\t\tbreak;\n> > +\t}\n> > +}\n> > +\n> > +int YamlEmitter::Emitter::emit(yaml_event_t *event)\n> > +{\n> > +\tint ret = yaml_emitter_emit(&emitter_, event);\n> > +\tif (!ret) {\n> > +\t\tlogError();\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\class YamlOutput\n> > + *\n> > + * The YamlOutput base class. From this class YamlScalar, YamlList and YamlDict\n> > + * are derived.\n> > + *\n> > + * The YamlOutput class provides functions to create a scalar, a list or a\n> > + * dictionary.\n> > + *\n> > + * The class cannot be instantiated directly by applications.\n> > + */\n> > +\n> > +YamlScalar YamlOutput::scalar()\n> > +{\n> > +\treturn YamlScalar(emitter_);\n> > +}\n> > +\n> > +YamlDict YamlOutput::dict()\n> > +{\n> > +\treturn YamlDict(emitter_);\n> > +}\n> > +\n> > +YamlList YamlOutput::list()\n> > +{\n> > +\treturn YamlList(emitter_);\n> > +}\n> > +\n> > +int YamlOutput::emitScalar(std::string_view scalar)\n> > +{\n> > +\tconst unsigned char *value = reinterpret_cast<const unsigned char *>\n> > +\t\t\t\t     (scalar.data());\n>\n> I would use yaml_char_t instead of unsigned char here, as that's the\n> taype taken by the yaml_scalar_event_initialize() function.\n>\n\ndone\n\nThanks\n   j\n\n> > +\tyaml_scalar_event_initialize(emitter_->event(), NULL, NULL, value,\n> > +\t\t\t\t     scalar.length(), true, false,\n> > +\t\t\t\t     YAML_PLAIN_SCALAR_STYLE);\n> > +\treturn emitter_->emit();\n> > +}\n> > +\n> > +int YamlOutput::emitMappingStart()\n> > +{\n> > +\tyaml_mapping_start_event_initialize(emitter_->event(), NULL, NULL,\n> > +\t\t\t\t\t    true, YAML_BLOCK_MAPPING_STYLE);\n> > +\treturn emitter_->emit();\n> > +}\n> > +\n> > +int YamlOutput::emitMappingEnd()\n> > +{\n> > +\tyaml_mapping_end_event_initialize(emitter_->event());\n> > +\treturn emitter_->emit();\n> > +}\n> > +\n> > +int YamlOutput::emitSequenceStart()\n> > +{\n> > +\tyaml_sequence_start_event_initialize(emitter_->event(), NULL, NULL, true,\n> > +\t\t\t\t\t     YAML_BLOCK_SEQUENCE_STYLE);\n> > +\treturn emitter_->emit();\n> > +}\n> > +\n> > +int YamlOutput::emitSequenceEnd()\n> > +{\n> > +\tyaml_sequence_end_event_initialize(emitter_->event());\n> > +\treturn emitter_->emit();\n> > +}\n> > +\n> > +/**\n> > + * \\class YamlRoot\n> > + *\n> > + * Yaml root node. A root node can be populated with a scalar, a list or a dict.\n>\n> s/Yaml/YAML/\n>\n> > + */\n> > +\n> > +YamlRoot::~YamlRoot()\n> > +{\n> > +\tif (!initialized())\n> > +\t\treturn;\n> > +\n> > +\tyaml_document_end_event_initialize(emitter_->event(), 0);\n> > +\temitterRoot_->emit();\n> > +\n> > +\tyaml_stream_end_event_initialize(emitter_->event());\n> > +\temitterRoot_->emit();\n> > +}\n> > +\n> > +YamlDict YamlRoot::dict()\n> > +{\n> > +\temitMappingStart();\n> > +\n> > +\treturn YamlOutput::dict();\n> > +}\n> > +\n> > +YamlList YamlRoot::list()\n> > +{\n> > +\temitSequenceStart();\n> > +\n> > +\treturn YamlOutput::list();\n> > +}\n> > +\n> > +/**\n> > + * \\class YamlScalar\n> > + *\n> > + * A YamlScalar can be assigned to an std::string_view and other libcamera\n> > + * types to emit them as YAML scalars.\n> > + */\n> > +\n> > +YamlScalar::YamlScalar(YamlEmitter *emitter)\n> > +\t: YamlOutput(emitter)\n> > +{\n> > +}\n> > +\n> > +void YamlScalar::operator=(std::string_view scalar)\n> > +{\n> > +\temitScalar(scalar);\n> > +}\n> > +\n> > +/**\n> > + * \\class YamlList\n> > + *\n> > + * A YamlList can be populated with scalar and allows to create other lists\n> > + * and dictionaries.\n> > + */\n> > +\n> > +YamlList::YamlList(YamlEmitter *emitter)\n> > +\t: YamlOutput(emitter)\n> > +{\n> > +}\n> > +\n> > +YamlList::~YamlList()\n> > +{\n> > +\tif (initialized())\n> > +\t\temitSequenceEnd();\n> > +}\n> > +\n> > +void YamlList::scalar(std::string_view scalar)\n> > +{\n> > +\temitScalar(scalar);\n> > +}\n> > +\n> > +YamlList YamlList::list()\n> > +{\n> > +\temitSequenceStart();\n> > +\n> > +\treturn YamlOutput::list();\n> > +}\n> > +\n> > +YamlDict YamlList::dict()\n> > +{\n> > +\temitMappingStart();\n> > +\n> > +\treturn YamlOutput::dict();\n> > +}\n> > +\n> > +/**\n> > + * \\class YamlDict\n> > + *\n> > + * A YamlDict derives an unordered_map that maps strings to YAML scalar.\n> > + *\n> > + * A YamlDict can be populated with scalars using operator[] and allows to\n> > + * create other lists and dictionaries.\n> > + */\n> > +\n> > +YamlDict::YamlDict(YamlEmitter *emitter)\n> > +\t: YamlOutput(emitter)\n> > +{\n> > +}\n> > +\n> > +YamlDict::~YamlDict()\n> > +{\n> > +\tif (initialized())\n> > +\t\temitMappingEnd();\n> > +}\n> > +\n> > +YamlList YamlDict::list(std::string_view key)\n> > +{\n> > +\temitScalar(key);\n> > +\temitSequenceStart();\n> > +\n> > +\treturn YamlOutput::list();\n> > +}\n> > +\n> > +YamlDict YamlDict::dict(std::string_view key)\n> > +{\n> > +\temitScalar(key);\n> > +\temitMappingStart();\n> > +\n> > +\treturn YamlOutput::dict();\n> > +}\n> > +\n> > +YamlScalar YamlDict::operator[](std::string_view key)\n> > +{\n> > +\temitScalar(key);\n> > +\n> > +\treturn YamlOutput::scalar();\n> > +}\n> > +\n> > +} /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0CB50BDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  6 Nov 2024 09:45:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E643C6541D;\n\tWed,  6 Nov 2024 10:45:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B47B65417\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Nov 2024 10:45:40 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EEB8475;\n\tWed,  6 Nov 2024 10:45:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cuccbjvg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730886332;\n\tbh=nK3os8iN6c8cSt2yEUpCZovn0LDlZSxjPF7mfdEXhrk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cuccbjvgh1uvkjkPA5E7Rp28jf0y8+LejJWdQ4QDVCOnhNShxQY7QG2OBshWYEsE8\n\tIesopalMfEBCH4tR4tHLfHCQMY4yxBVWq+NG1J/IO0l45weJ9rdsyzKGecWEp8o7/v\n\tZa8Bqg1ZkZxhIvJEjazVa+RNTO7GDUK06pQuGS9o=","Date":"Wed, 6 Nov 2024 10:45:37 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC v3 3/4] libcamera: Implement YamlEmitter","Message-ID":"<eoz345e46k5auuhspze4htlgqkuf5zwnivi2rx63qgypoffiqf@v4gwb5c7eyqy>","References":"<20241021094955.26991-1-jacopo.mondi@ideasonboard.com>\n\t<20241021094955.26991-4-jacopo.mondi@ideasonboard.com>\n\t<20241105235000.GE17733@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241105235000.GE17733@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32044,"web_url":"https://patchwork.libcamera.org/comment/32044/","msgid":"<20241106113718.GB9369@pendragon.ideasonboard.com>","date":"2024-11-06T11:37:18","subject":"Re: [RFC v3 3/4] libcamera: Implement YamlEmitter","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Nov 06, 2024 at 10:45:37AM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 06, 2024 at 01:50:00AM +0200, Laurent Pinchart wrote:\n> > On Mon, Oct 21, 2024 at 11:49:51AM +0200, Jacopo Mondi wrote:\n> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > ---\n> > >  Documentation/Doxyfile-internal.in        |   2 +\n> > >  include/libcamera/internal/meson.build    |   1 +\n> > >  include/libcamera/internal/yaml_emitter.h | 183 +++++++++++\n> > >  src/libcamera/meson.build                 |   1 +\n> > >  src/libcamera/yaml_emitter.cpp            | 360 ++++++++++++++++++++++\n> > >  5 files changed, 547 insertions(+)\n> > >  create mode 100644 include/libcamera/internal/yaml_emitter.h\n> > >  create mode 100644 src/libcamera/yaml_emitter.cpp\n> > >\n> > > diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in\n> > > index cf9825537866..e0ba64da1bef 100644\n> > > --- a/Documentation/Doxyfile-internal.in\n> > > +++ b/Documentation/Doxyfile-internal.in\n> > > @@ -21,9 +21,11 @@ EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \\\n> > >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \\\n> > >                           @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \\\n> > >                           @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \\\n> > > +                         @TOP_SRCDIR@/include/libcamera/internal/yaml_emitter.h \\\n> > >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \\\n> > >                           @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \\\n> > >                           @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \\\n> > > +                         @TOP_SRCDIR@/src/libcamera/yaml_emitter.cpp \\\n> > >                           @TOP_SRCDIR@/src/libcamera/pipeline/ \\\n> > >                           @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \\\n> > >                           @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \\\n> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > index 1c5eef9cab80..7533b075fde2 100644\n> > > --- a/include/libcamera/internal/meson.build\n> > > +++ b/include/libcamera/internal/meson.build\n> > > @@ -41,6 +41,7 @@ libcamera_internal_headers = files([\n> > >      'v4l2_pixelformat.h',\n> > >      'v4l2_subdevice.h',\n> > >      'v4l2_videodevice.h',\n> > > +    'yaml_emitter.h',\n> > >      'yaml_parser.h',\n> > >  ])\n> > >\n> > > diff --git a/include/libcamera/internal/yaml_emitter.h b/include/libcamera/internal/yaml_emitter.h\n> > > new file mode 100644\n> > > index 000000000000..bcbb574061cc\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/yaml_emitter.h\n> > > @@ -0,0 +1,183 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board Oy\n> > > + *\n> > > + * libcamera YAML emitter helper\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include <memory>\n> > > +#include <string_view>\n> > > +\n> > > +#include <libcamera/base/class.h>\n> > > +#include <libcamera/base/file.h>\n> > > +#include <libcamera/orientation.h>\n> > > +\n> > > +#include <yaml.h>\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class YamlDict;\n> > > +class YamlEvent;\n> > > +class YamlList;\n> > > +class YamlRoot;\n> > > +class YamlScalar;\n> > > +\n> > > +class YamlEmitter final\n> >\n> > This class is not part of the public API (as far as I can tell). You can\n> > just declare it here with\n> \n> This class is the only entry point from which users can create a\n> \"root\" node. I think it should be part of the public API.\n\nI wonder how I missed that. I'll try to wake up.\n\n> > class YamlEmitter;\n> >\n> > and define it in the .cpp file.\n> > > +{\n> > > +public:\n> > > +\t~YamlEmitter();\n> > > +\n> > > +\tstatic YamlRoot root(std::string_view path);\n> >\n> > To make this class more generic, how about passing a std::ostream\n> > pointer ?\n> >\n> > > +\n> > > +\tint emit();\n> > > +\tyaml_event_t *event() { return &event_; }\n> > > +\n> > > +private:\n> > > +\tLIBCAMERA_DISABLE_COPY(YamlEmitter)\n> > > +\n> > > +\tclass Emitter\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\t~Emitter();\n> > > +\n> > > +\t\tvoid init(File *file);\n> > > +\n> > > +\t\tint emit(yaml_event_t *event);\n> > > +\n> > > +\tprivate:\n> > > +\t\tvoid logError();\n> > > +\n> > > +\t\tyaml_emitter_t emitter_;\n> > > +\t};\n> >\n> > Why is this split in another class ?\n> \n> I probably thought it looked cleaner, but it's not. I'll drop it.\n> \n> > > +\n> > > +\tYamlEmitter() = default;\n> > > +\n> > > +\tvoid init();\n> > > +\n> > > +\tFile file_;\n> > > +\tyaml_event_t event_;\n> > > +\tEmitter emitter_;\n> > > +};\n> > > +\n> > > +class YamlOutput\n> > > +{\n> > > +public:\n> > > +\tvirtual ~YamlOutput() {};\n> >\n> > Extraneous trailing ;\n> >\n> > But does the class need a virtual destructor ? Is there any case where\n> > one would delete a pointer to YamlOutput ?\n> \n> Not as a pointer to YamlOutput, no. I'll drop\n> \n> > > +\n> > > +\tYamlOutput() = default;\n> >\n> > The constructors go before the destructor. And it should be protected,\n> > as nobody should create a YamlOutput instance.\n> \n> ack\n> \n> > > +\n> > > +\tYamlOutput(YamlOutput &&other)\n> > > +\t{\n> > > +\t\temitter_ = other.emitter_;\n> > > +\t\tother.emitter_ = nullptr;\n> > > +\t}\n> >\n> > Same here, this should be protected.\n> \n> ack\n> \n> > > +\n> > > +\tbool initialized() { return !!emitter_; }\n> >\n> > I would name this function valid(), as it's not just whether or not the\n> > object has been initialized, but it also becomes invalid when moved.\n> >\n> > The function should also be const.\n> \n> right\n> \n> > > +\n> > > +\tYamlScalar scalar();\n> > > +\tYamlDict dict();\n> > > +\tYamlList list();\n> >\n> > I think those 3 functions should be protected, they're not meant to be\n> > called by the user as far as I understand.\n> \n> Yes, these can be protected\n> \n> > > +\n> > > +protected:\n> > > +\tYamlOutput(YamlEmitter *emitter)\n> > > +\t\t: emitter_(emitter)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tYamlOutput &operator=(YamlOutput &&other)\n> > > +\t{\n> > > +\t\temitter_ = other.emitter_;\n> > > +\t\tother.emitter_ = nullptr;\n> > > +\n> > > +\t\treturn *this;\n> > > +\t}\n> > > +\n> > > +\tint emitScalar(std::string_view scalar);\n> > > +\tint emitMappingStart();\n> > > +\tint emitMappingEnd();\n> > > +\tint emitSequenceStart();\n> > > +\tint emitSequenceEnd();\n> > > +\n> > > +\tYamlEmitter *emitter_ = nullptr;\n> > > +\tyaml_event_t event_;\n> >\n> > I think you should disable copying of this class with\n> > LIBCAMERA_DISABLE_COPY().\n> \n> done, in the private: section\n> \n> > > +};\n> > > +\n> > > +class YamlRoot : public YamlOutput\n> > > +{\n> > > +public:\n> > > +\tYamlRoot() = default;\n> > > +\tYamlRoot(YamlRoot &&other) = default;\n> > > +\t~YamlRoot();\n> > > +\n> > > +\tYamlRoot &operator=(YamlRoot &&other) = default;\n> > > +\n> > > +\tYamlList list();\n> > > +\tYamlDict dict();\n> > > +\tvoid scalar(std::string_view scalar);\n> >\n> > You don't implement YamlRoot::scalar() in the .cpp file, which is a good\n> > indication that the function is likely not needed :-) I would just drop\n> > it.\n> \n> Maybe I'm not using it right now, but I think emitting a scalar in the\n> root node is still a valid use case. Maybe I should implement the\n> function.\n\nThat means the YAML document would look like\n\n----------------------------------------\n%YAML 1.1\n---\n42\n----------------------------------------\n\nYes, in theory, that's well-formed YAML (as far as I can tell), but in\npractice, I don't think we would ever need this. I'm a bit reluctant to\ncarry dead code in libcamera, especially when we can easily add that\ncode later if needed without a big redesigning.\n\n> > > +\n> > > +private:\n> > > +\tfriend class YamlEmitter;\n> > > +\n> > > +\tYamlRoot(std::unique_ptr<YamlEmitter> emitter)\n> > > +\t\t: YamlOutput(emitter.get()), emitterRoot_(std::move(emitter))\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tstd::unique_ptr<YamlEmitter> emitterRoot_;\n> > > +};\n> > > +\n> > > +class YamlScalar : public YamlOutput\n> > > +{\n> > > +public:\n> > > +\t~YamlScalar() = default;\n> > > +\n> > > +\tvoid operator=(std::string_view scalar);\n> > > +\n> > > +private:\n> > > +\tfriend class YamlOutput;\n> > > +\n> > > +\tYamlScalar(YamlEmitter *emitter);\n> > > +};\n> > > +\n> > > +class YamlList : public YamlOutput\n> > > +{\n> > > +public:\n> > > +\tYamlList() = default;\n> > > +\tYamlList(YamlList &&other) = default;\n> > > +\t~YamlList();\n> > > +\n> > > +\tYamlList &operator=(YamlList &&other) = default;\n> > > +\n> > > +\tYamlList list();\n> > > +\tYamlDict dict();\n> > > +\tvoid scalar(std::string_view scalar);\n> > > +\n> > > +private:\n> > > +\tfriend class YamlOutput;\n> > > +\n> > > +\tYamlList(YamlEmitter *emitter);\n> > > +};\n> > > +\n> > > +class YamlDict : public YamlOutput\n> > > +{\n> > > +public:\n> > > +\tYamlDict() = default;\n> > > +\tYamlDict(YamlDict &&other) = default;\n> > > +\t~YamlDict();\n> > > +\n> > > +\tYamlDict &operator=(YamlDict &&other) = default;\n> > > +\n> > > +\tYamlList list(std::string_view key);\n> > > +\tYamlDict dict(std::string_view key);\n> > > +\n> > > +\tYamlScalar operator[](std::string_view key);\n> > > +\n> > > +private:\n> > > +\tfriend class YamlOutput;\n> > > +\n> > > +\tYamlDict(YamlEmitter *emitter);\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index aa9ab0291854..5b8af4103085 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -51,6 +51,7 @@ libcamera_internal_sources = files([\n> > >      'v4l2_pixelformat.cpp',\n> > >      'v4l2_subdevice.cpp',\n> > >      'v4l2_videodevice.cpp',\n> > > +    'yaml_emitter.cpp',\n> > >      'yaml_parser.cpp',\n> > >  ])\n> > >\n> > > diff --git a/src/libcamera/yaml_emitter.cpp b/src/libcamera/yaml_emitter.cpp\n> > > new file mode 100644\n> > > index 000000000000..9542ad2d9c18\n> > > --- /dev/null\n> > > +++ b/src/libcamera/yaml_emitter.cpp\n> > > @@ -0,0 +1,360 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board Oy\n> > > + *\n> > > + * libcamera YAML emitter helper\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/yaml_emitter.h\"\n> > > +\n> > > +#include <libcamera/base/log.h>\n> > > +\n> > > +/**\n> > > + * \\file yaml_emitter.h\n> > > + * \\brief A YAML emitter helper\n> > > + *\n> > > + * The YAML Emitter helpers allows users to emit output in YAML format.\n> >\n> > s/helpers/helper/\n> >\n> > or\n> >\n> > s/allows/allow/\n> >\n> > > + *\n> > > + * To emit YAML users of this classes should create a root node with\n> >\n> > \"this class\" or \"these classes\"\n> >\n> > s/should //\n> >\n> > > + *\n> > > + * \\code\n> > > +\tstd::string filePath(\"...\");\n> > > +\tauto root = YamlEmitter::root(filePath);\n> > > +   \\endcode\n> > > + *\n> > > + * A YamlRoot implements RAII-style handling of YAML sequence and document\n> > > + * events handling. Creating a YamlRoot emits the STREAM_START and DOC_START\n> > > + * events. Once a YamlRoot gets deleted the DOC_SEND and STREAM_END events\n> > > + * gets automatically deleted.\n> >\n> > That's an implementation detail, it's not relevant to the users of the\n> > class.\n> >\n> > > + *\n> > > + * Once a root node has been created it is possible to populate it with\n> > > + * scalars, list or dictionaries.\n> > > + *\n> > > + * YamlList and YamlDict can only be created wrapped in unique_ptr<> instances,\n> >\n> > Not true anymore.\n> >\n> > > + * to implement a RAII-style handling of YAML of lists and dictionaries.\n> > > + * Creating a YamlList and a YamlDict emits the YAML sequence and mapping start\n> > > + * events. Once an instance gets deleted, the sequence and mapping gets\n> > > + * automatically emitted.\n> >\n> > Events are an implementation detail here too.\n> >\n> > All these are opportunities to improve the doc for the first non-RFC\n> > version :-)\n> >\n> > > + *\n> > > + * A YamlScalar is a simpler object that can be assigned to different types\n> > > + * to emit them as strings.\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(YamlEmitter)\n> > > +\n> > > +namespace {\n> > > +\n> > > +int yamlWrite(void *data, unsigned char *buffer, size_t size)\n> > > +{\n> > > +\tFile *file = static_cast<File *>(data);\n> > > +\n> > > +\tSpan<unsigned char> buf{ buffer, size };\n> > > +\tssize_t ret = file->write(buf);\n> > > +\tif (ret < 0) {\n> > > +\t\tret = errno;\n> >\n> > File::write() returns the error code, no need to get it from errno.\n> >\n> > > +\t\tLOG(YamlEmitter, Error) << \"Write error : \" << strerror(ret);\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\treturn 1;\n> > > +}\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > +/**\n> > > + * \\class YamlEmitter\n> > > + *\n> > > + * Yaml Emitter entry point. Allows to create a YamlRoot object that users\n> >\n> > s/Yaml/YAML/\n> >\n> > > + * can populate.\n> > > + */\n> > > +\n> > > +YamlEmitter::~YamlEmitter()\n> > > +{\n> > > +\tyaml_event_delete(&event_);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Create a YamlRoot that applications can start populating with YamlOutput\n> > > + * \\param[in] path The YAML output file path\n> > > + * \\return A unique pointer to a YamlRoot\n> >\n> > No unique pointer anymore. And we don't usually document the types in\n> > the text, they're clearly visible in the generated documentation.\n> >\n> > > + */\n> > > +YamlRoot YamlEmitter::root(std::string_view path)\n> > > +{\n> > > +\tstd::unique_ptr<YamlEmitter> emitter{ new YamlEmitter() };\n> > > +\n> > > +\tstd::string filePath(path);\n> >\n> > I've sent a patch to support std::string_view in the File class so this\n> > becomes unnecessary.\n> \n> Where ? I missed it\n\nOops. I wrote that comment while working on string_view support in the\nFile class. The result was however not what I had expected, and I think\nwe would be better off with\n\n\tFile::setFileName(std::string name);\n\ninstead of the existing\n\n\tFile::setFileName(const std::string &name);\n\nor the planned\n\n\tFile::setFileName(std::string_view name);\n\nThe reason is that, internally, File::setFileName() stores the name in a\nstd::string member variable:\n\n\tname_ = name;\n\nWith the function argument being either 'const std::string &' or\n'std::string_view', this will create a copy. However, with the function\nargument being a 'std::string', we would change the implementation to\n\n\tname_ = std::move(name);\n\nIf the caller passes a 'std::string &' to the function, a copy will\nstill be create, by the caller this time. There will be no performance\nimpact, neither positive nor negative, compared to what we do today. The\nsame is true if the caller passes a C string literal, or a\nstd::stringview. However, if the caller already has a std::string\nvariable holding the name, *and* does not need the variable anymore\nafter the call, it could do\n\n\tstd::string fullName = path + name;\n\tfile.setFileName(std::move(fullName));\n\nor just\n\n\tfile.setFileName(path + name);\n\nand no copy would be made anywhere. This improves performance compared\nto the current situtation. This was an unexpected find for me, I aimed\nat replacing 'const std::string &' with 'std::string_view' and realized\nI should pass a 'std::string' by value instead.\n\nThere are still use cases where usage of std::string_view is best, but\nmaking clear and simple rules to decide what string type to use is\ndifficult. One annoying part is that the optimal string type depends on\nthe internal implementation of the callee, and designing APIs based on\ninternal implementations is really not a great idea in general. For\nanything private to a class that's fine, we can update the callers if a\nchange in a callee would benefit from switching to a different string\ntype. For APIs internal to libcamera that's more or less true as well,\nalthough the impact of a tree-wide change would be more annoying. For\npublic APIs, that's a no-go, and different rules are needed there, to\npick an optimal string type under the constraint of ABI stability.\n\nAll this to say that I wouldn't try to overuse std::string_view in\nlibcamera yet. Especially in this function, as you need to pass a\nstd::string reference to setFileName() anyway, you can as well use a\n'const std::string &' argument instead of a string view.\n\n> > > +\temitter->file_.setFileName(filePath);\n> > > +\temitter->file_.open(File::OpenModeFlag::WriteOnly);\n> > > +\n> > > +\temitter->init();\n> >\n> > I think there's room for simplification here.\n> >\n> > \tFile file(path);\n> > \tfile.open(File::OpenModeFlag::WriteOnly);\n> >\n> > \tstd::unique_ptr<YamlEmitter> emitter{ new YamlEmitter(std::move(file)) };\n> >\n> > and drop YamlEmitter::init(); and make YamlEmitter::file_ private.\n> >\n> > > +\n> > > +\treturn YamlRoot(std::move(emitter));\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Emit a yaml event\n> > > + */\n> > > +int YamlEmitter::emit()\n> > > +{\n> > > +\treturn emitter_.emit(&event_);\n> > > +}\n> > > +\n> > > +void YamlEmitter::init()\n> > > +{\n> > > +\temitter_.init(&file_);\n> > > +\n> > > +\tyaml_stream_start_event_initialize(&event_, YAML_UTF8_ENCODING);\n> > > +\temitter_.emit(&event_);\n> > > +\n> > > +\tyaml_document_start_event_initialize(&event_, NULL, NULL, NULL, 0);\n> > > +\temitter_.emit(&event_);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\class YamlEmitter::Emitter\n> > > + *\n> > > + * yaml_emitter_t wrapper. Initialize the yaml_emitter_t on creation allows\n> > > + * YamlOutput classes to emit events.\n> > > + */\n> > > +\n> > > +YamlEmitter::Emitter::~Emitter()\n> > > +{\n> > > +\tyaml_emitter_delete(&emitter_);\n> > > +}\n> > > +\n> > > +void YamlEmitter::Emitter::init(File *file)\n> > > +{\n> > > +\tyaml_emitter_initialize(&emitter_);\n> > > +\tyaml_emitter_set_output(&emitter_, yamlWrite, file);\n> > > +}\n> > > +\n> > > +void YamlEmitter::Emitter::logError()\n> > > +{\n> > > +\tswitch (emitter_.error) {\n> > > +\tcase YAML_MEMORY_ERROR:\n> > > +\t\tLOG(YamlEmitter, Error)\n> > > +\t\t\t<< \"Memory error: Not enough memory for emitting\";\n> > > +\t\tbreak;\n> > > +\n> > > +\tcase YAML_WRITER_ERROR:\n> > > +\t\tLOG(YamlEmitter, Error)\n> > > +\t\t\t<< \"Writer error: \" << emitter_.problem;\n> > > +\t\tbreak;\n> > > +\n> > > +\tcase YAML_EMITTER_ERROR:\n> > > +\t\tLOG(YamlEmitter, Error)\n> > > +\t\t\t<< \"Emitter error: \" << emitter_.problem;\n> > > +\t\tbreak;\n> > > +\n> > > +\tdefault:\n> > > +\t\tLOG(YamlEmitter, Error) << \"Internal problem\";\n> > > +\t\tbreak;\n> > > +\t}\n> > > +}\n> > > +\n> > > +int YamlEmitter::Emitter::emit(yaml_event_t *event)\n> > > +{\n> > > +\tint ret = yaml_emitter_emit(&emitter_, event);\n> > > +\tif (!ret) {\n> > > +\t\tlogError();\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\class YamlOutput\n> > > + *\n> > > + * The YamlOutput base class. From this class YamlScalar, YamlList and YamlDict\n> > > + * are derived.\n> > > + *\n> > > + * The YamlOutput class provides functions to create a scalar, a list or a\n> > > + * dictionary.\n> > > + *\n> > > + * The class cannot be instantiated directly by applications.\n> > > + */\n> > > +\n> > > +YamlScalar YamlOutput::scalar()\n> > > +{\n> > > +\treturn YamlScalar(emitter_);\n> > > +}\n> > > +\n> > > +YamlDict YamlOutput::dict()\n> > > +{\n> > > +\treturn YamlDict(emitter_);\n> > > +}\n> > > +\n> > > +YamlList YamlOutput::list()\n> > > +{\n> > > +\treturn YamlList(emitter_);\n> > > +}\n> > > +\n> > > +int YamlOutput::emitScalar(std::string_view scalar)\n> > > +{\n> > > +\tconst unsigned char *value = reinterpret_cast<const unsigned char *>\n> > > +\t\t\t\t     (scalar.data());\n> >\n> > I would use yaml_char_t instead of unsigned char here, as that's the\n> > taype taken by the yaml_scalar_event_initialize() function.\n> >\n> \n> done\n> \n> Thanks\n>    j\n> \n> > > +\tyaml_scalar_event_initialize(emitter_->event(), NULL, NULL, value,\n> > > +\t\t\t\t     scalar.length(), true, false,\n> > > +\t\t\t\t     YAML_PLAIN_SCALAR_STYLE);\n> > > +\treturn emitter_->emit();\n> > > +}\n> > > +\n> > > +int YamlOutput::emitMappingStart()\n> > > +{\n> > > +\tyaml_mapping_start_event_initialize(emitter_->event(), NULL, NULL,\n> > > +\t\t\t\t\t    true, YAML_BLOCK_MAPPING_STYLE);\n> > > +\treturn emitter_->emit();\n> > > +}\n> > > +\n> > > +int YamlOutput::emitMappingEnd()\n> > > +{\n> > > +\tyaml_mapping_end_event_initialize(emitter_->event());\n> > > +\treturn emitter_->emit();\n> > > +}\n> > > +\n> > > +int YamlOutput::emitSequenceStart()\n> > > +{\n> > > +\tyaml_sequence_start_event_initialize(emitter_->event(), NULL, NULL, true,\n> > > +\t\t\t\t\t     YAML_BLOCK_SEQUENCE_STYLE);\n> > > +\treturn emitter_->emit();\n> > > +}\n> > > +\n> > > +int YamlOutput::emitSequenceEnd()\n> > > +{\n> > > +\tyaml_sequence_end_event_initialize(emitter_->event());\n> > > +\treturn emitter_->emit();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\class YamlRoot\n> > > + *\n> > > + * Yaml root node. A root node can be populated with a scalar, a list or a dict.\n> >\n> > s/Yaml/YAML/\n> >\n> > > + */\n> > > +\n> > > +YamlRoot::~YamlRoot()\n> > > +{\n> > > +\tif (!initialized())\n> > > +\t\treturn;\n> > > +\n> > > +\tyaml_document_end_event_initialize(emitter_->event(), 0);\n> > > +\temitterRoot_->emit();\n> > > +\n> > > +\tyaml_stream_end_event_initialize(emitter_->event());\n> > > +\temitterRoot_->emit();\n> > > +}\n> > > +\n> > > +YamlDict YamlRoot::dict()\n> > > +{\n> > > +\temitMappingStart();\n> > > +\n> > > +\treturn YamlOutput::dict();\n> > > +}\n> > > +\n> > > +YamlList YamlRoot::list()\n> > > +{\n> > > +\temitSequenceStart();\n> > > +\n> > > +\treturn YamlOutput::list();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\class YamlScalar\n> > > + *\n> > > + * A YamlScalar can be assigned to an std::string_view and other libcamera\n> > > + * types to emit them as YAML scalars.\n> > > + */\n> > > +\n> > > +YamlScalar::YamlScalar(YamlEmitter *emitter)\n> > > +\t: YamlOutput(emitter)\n> > > +{\n> > > +}\n> > > +\n> > > +void YamlScalar::operator=(std::string_view scalar)\n> > > +{\n> > > +\temitScalar(scalar);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\class YamlList\n> > > + *\n> > > + * A YamlList can be populated with scalar and allows to create other lists\n> > > + * and dictionaries.\n> > > + */\n> > > +\n> > > +YamlList::YamlList(YamlEmitter *emitter)\n> > > +\t: YamlOutput(emitter)\n> > > +{\n> > > +}\n> > > +\n> > > +YamlList::~YamlList()\n> > > +{\n> > > +\tif (initialized())\n> > > +\t\temitSequenceEnd();\n> > > +}\n> > > +\n> > > +void YamlList::scalar(std::string_view scalar)\n> > > +{\n> > > +\temitScalar(scalar);\n> > > +}\n> > > +\n> > > +YamlList YamlList::list()\n> > > +{\n> > > +\temitSequenceStart();\n> > > +\n> > > +\treturn YamlOutput::list();\n> > > +}\n> > > +\n> > > +YamlDict YamlList::dict()\n> > > +{\n> > > +\temitMappingStart();\n> > > +\n> > > +\treturn YamlOutput::dict();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\class YamlDict\n> > > + *\n> > > + * A YamlDict derives an unordered_map that maps strings to YAML scalar.\n> > > + *\n> > > + * A YamlDict can be populated with scalars using operator[] and allows to\n> > > + * create other lists and dictionaries.\n> > > + */\n> > > +\n> > > +YamlDict::YamlDict(YamlEmitter *emitter)\n> > > +\t: YamlOutput(emitter)\n> > > +{\n> > > +}\n> > > +\n> > > +YamlDict::~YamlDict()\n> > > +{\n> > > +\tif (initialized())\n> > > +\t\temitMappingEnd();\n> > > +}\n> > > +\n> > > +YamlList YamlDict::list(std::string_view key)\n> > > +{\n> > > +\temitScalar(key);\n> > > +\temitSequenceStart();\n> > > +\n> > > +\treturn YamlOutput::list();\n> > > +}\n> > > +\n> > > +YamlDict YamlDict::dict(std::string_view key)\n> > > +{\n> > > +\temitScalar(key);\n> > > +\temitMappingStart();\n> > > +\n> > > +\treturn YamlOutput::dict();\n> > > +}\n> > > +\n> > > +YamlScalar YamlDict::operator[](std::string_view key)\n> > > +{\n> > > +\temitScalar(key);\n> > > +\n> > > +\treturn YamlOutput::scalar();\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E61F2BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  6 Nov 2024 11:37:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E35C16541D;\n\tWed,  6 Nov 2024 12:37:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD718653C5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Nov 2024 12:37:24 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 51C13475;\n\tWed,  6 Nov 2024 12:37:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pWrLsnRA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730893036;\n\tbh=D8Yx3nmjSPoerrzp1v6MAwKP48RNG3kWfVs3oT8x4Hw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pWrLsnRAv2ggkgpXfNejf6U/Z7KudsmB6P88heEdCTpMYRKX9iMBlF1RbDoQLVarq\n\t3GSPYWLnJDbVUY6lRkP6rK6vK/5gnOkbRDlumci7n1feu/TuQeqPIOKI/hMtTJZjPO\n\tiaarmIYYDJXOZt1xB5bdroTHIEslVoMS7hXWsxA8=","Date":"Wed, 6 Nov 2024 13:37:18 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC v3 3/4] libcamera: Implement YamlEmitter","Message-ID":"<20241106113718.GB9369@pendragon.ideasonboard.com>","References":"<20241021094955.26991-1-jacopo.mondi@ideasonboard.com>\n\t<20241021094955.26991-4-jacopo.mondi@ideasonboard.com>\n\t<20241105235000.GE17733@pendragon.ideasonboard.com>\n\t<eoz345e46k5auuhspze4htlgqkuf5zwnivi2rx63qgypoffiqf@v4gwb5c7eyqy>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<eoz345e46k5auuhspze4htlgqkuf5zwnivi2rx63qgypoffiqf@v4gwb5c7eyqy>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]