[{"id":31839,"web_url":"https://patchwork.libcamera.org/comment/31839/","msgid":"<20241020210917.GJ7770@pendragon.ideasonboard.com>","date":"2024-10-20T21:09:17","subject":"Re: [RFC v2 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 Thu, Oct 17, 2024 at 02:52:18PM +0200, Jacopo Mondi wrote:\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> ---\n>  include/libcamera/internal/meson.build    |   1 +\n>  include/libcamera/internal/yaml_emitter.h | 162 ++++++++++\n>  src/libcamera/meson.build                 |   1 +\n>  src/libcamera/yaml_emitter.cpp            | 362 ++++++++++++++++++++++\n>  4 files changed, 526 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/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..e4a0e3b440a5\n> --- /dev/null\n> +++ b/include/libcamera/internal/yaml_emitter.h\n> @@ -0,0 +1,162 @@\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> +public:\n> +\t~YamlEmitter();\n> +\n> +\tstatic std::unique_ptr<YamlRoot> root(std::string_view path);\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> +\tYamlEmitter() = default;\n> +\n> +\tvoid init();\n> +\n> +\tstd::unique_ptr<File> file_;\n\nNo need for a dynamic allocation.\n\n\tFile file_;\n\n> +\tyaml_event_t event_;\n> +\tEmitter emitter_;\n> +};\n> +\n> +class YamlOutput\n> +{\n> +public:\n> +\tvirtual ~YamlOutput() {};\n> +\n> +\tYamlOutput(YamlOutput &&other)\n> +\t{\n> +\t\temitter_ = other.emitter_;\n> +\t\tother.emitter_ = nullptr;\n> +\t}\n> +\n> +\tYamlScalar scalar();\n> +\tstd::unique_ptr<YamlDict> dict();\n> +\tstd::unique_ptr<YamlList> list();\n\nYou still allocate everything on the heap. Perhaps with the exception of\nthe YamlEmitter argument to the YamlRoot constructor, there should be no\nusage of std::unique_ptr<> anywhere in this header. Heap allocations are\ncostly, and I think we should be able to use stack allocations only.\n\nYou will need to define default constructors, as well as move assignment\noperators. Be careful about the YamlOutput::event_ member, it's a C\nstructure so it won't have a move constructor or assignment operator.\nMoving it trivially by copying the contents may cause issues if anything\nholds a pointer to the yaml_event_t within libyaml.\n\n> +\n> +protected:\n> +\tYamlOutput(YamlEmitter *emitter)\n> +\t\t: emitter_(emitter)\n> +\t{\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_;\n> +\tyaml_event_t event_;\n> +};\n> +\n> +class YamlRoot : public YamlOutput\n> +{\n> +public:\n> +\t~YamlRoot();\n> +\n> +\tstd::unique_ptr<YamlList> list();\n> +\tstd::unique_ptr<YamlDict> dict();\n> +\tvoid scalar(std::string_view scalar);\n> +\n> +private:\n> +\tfriend class YamlEmitter;\n> +\n> +\tYamlRoot(YamlEmitter *emitter)\n> +\t\t: YamlOutput(emitter)\n> +\t{\n> +\t\temitterRoot_ = std::unique_ptr<YamlEmitter>(emitter);\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(YamlList &&other) = default;\n> +\t~YamlList();\n> +\n> +\tstd::unique_ptr<YamlList> list();\n> +\tstd::unique_ptr<YamlDict> 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(YamlDict &&other) = default;\n> +\t~YamlDict();\n> +\n> +\tstd::unique_ptr<YamlList> list(std::string_view key);\n> +\tstd::unique_ptr<YamlDict> 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..f2aaa1c1c1a6\n> --- /dev/null\n> +++ b/src/libcamera/yaml_emitter.cpp\n> @@ -0,0 +1,362 @@\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> + * To emit YAML users of this classes should create a root node with\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> + * 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> + * 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> + * 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> +\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> + * 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> +std::unique_ptr<YamlRoot> YamlEmitter::root(std::string_view path)\n> +{\n> +\tYamlEmitter *emitter = new YamlEmitter();\n> +\n> +\tstd::string filePath(path);\n> +\temitter->file_ = std::make_unique<File>(filePath);\n> +\temitter->file_->open(File::OpenModeFlag::WriteOnly);\n> +\n> +\temitter->init();\n> +\n> +\tYamlRoot *root = new YamlRoot(emitter);\n> +\treturn std::unique_ptr<YamlRoot>(root);\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_.get());\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> +std::unique_ptr<YamlDict> YamlOutput::dict()\n> +{\n> +\tYamlDict dict(emitter_);\n> +\n> +\treturn std::make_unique<YamlDict>(std::move(dict));\n> +}\n> +\n> +std::unique_ptr<YamlList> YamlOutput::list()\n> +{\n> +\tYamlList list(emitter_);\n> +\n> +\treturn std::make_unique<YamlList>(std::move(list));\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> +\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> +\n> +YamlRoot::~YamlRoot()\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> +std::unique_ptr<YamlDict> YamlRoot::dict()\n> +{\n> +\temitMappingStart();\n> +\n> +\treturn YamlOutput::dict();\n> +}\n> +\n> +std::unique_ptr<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 (emitter_)\n> +\t\temitSequenceEnd();\n> +}\n> +\n> +void YamlList::scalar(std::string_view scalar)\n> +{\n> +\temitScalar(scalar);\n> +}\n> +\n> +std::unique_ptr<YamlList> YamlList::list()\n> +{\n> +\temitSequenceStart();\n> +\n> +\treturn YamlOutput::list();\n> +}\n> +\n> +std::unique_ptr<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 (emitter_)\n> +\t\temitMappingEnd();\n> +}\n> +\n> +std::unique_ptr<YamlList> YamlDict::list(std::string_view key)\n> +{\n> +\temitScalar(key);\n> +\temitSequenceStart();\n> +\n> +\treturn YamlOutput::list();\n> +}\n> +\n> +std::unique_ptr<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 385FFC32A3\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 20 Oct 2024 21:09:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D8D965391;\n\tSun, 20 Oct 2024 23:09:25 +0200 (CEST)","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 524EB65379\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 20 Oct 2024 23:09:23 +0200 (CEST)","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 811C9502;\n\tSun, 20 Oct 2024 23:07:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pAxWxnSB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729458457;\n\tbh=wbRRbFjYGBPpv4mdSeXRO2RW6IHBxOCckIVp1Dl4h9E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pAxWxnSBH2kCqcRrat43zKNFXq0TvkLN0aHPrdycuCUYyxmZRfseVMlC+owHxq5wK\n\tzbrDPrn2YzmM6mqV7JgpIbgAz9b6fI1KnnFNejs3UOU4UiKx/QUTCFJBNXXkaCsiSt\n\tt854N469lPOdmSPh5XWIuhxnJQgQS+jcryiU/K80=","Date":"Mon, 21 Oct 2024 00:09:17 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC v2 3/4] libcamera: Implement YamlEmitter","Message-ID":"<20241020210917.GJ7770@pendragon.ideasonboard.com>","References":"<20241017125220.60567-1-jacopo.mondi@ideasonboard.com>\n\t<20241017125220.60567-4-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241017125220.60567-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>"}}]