[libcamera-devel,3/9] libcamera: Introduce YamlParser as a helper to parse yaml files

15347 diff mbox series

Introduce YamlParser as a helper to convert contents of a yaml file to
a tree based structure for easier reading, and to avoid writing parser
with raw yaml tokens. The class is based on libyaml, and only support
reading but writing a yaml file.

The interface is inspired by Json::Value class from jsoncpp:
http://jsoncpp.sourceforge.net/class_json_1_1_value.html

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 README.rst                               |   2 +-
 include/libcamera/internal/meson.build   |   1 +
 include/libcamera/internal/yaml_parser.h |  82 +++
 src/libcamera/meson.build                |   3 +
 src/libcamera/yaml_parser.cpp            | 796 +++++++++++++++++++++++
 5 files changed, 883 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/internal/yaml_parser.h
 create mode 100644 src/libcamera/yaml_parser.cpp

Hi Han-Lin,

Thank you for the patch.

On Wed, Feb 09, 2022 at 03:19:11PM +0800, Han-Lin Chen wrote:
> Introduce YamlParser as a helper to convert contents of a yaml file to
> a tree based structure for easier reading, and to avoid writing parser
> with raw yaml tokens. The class is based on libyaml, and only support
> reading but writing a yaml file.
> 
> The interface is inspired by Json::Value class from jsoncpp:
> http://jsoncpp.sourceforge.net/class_json_1_1_value.html
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  README.rst                               |   2 +-
>  include/libcamera/internal/meson.build   |   1 +
>  include/libcamera/internal/yaml_parser.h |  82 +++
>  src/libcamera/meson.build                |   3 +
>  src/libcamera/yaml_parser.cpp            | 796 +++++++++++++++++++++++
>  5 files changed, 883 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/internal/yaml_parser.h
>  create mode 100644 src/libcamera/yaml_parser.cpp
> 
> diff --git a/README.rst b/README.rst
> index ca8a97cb..4a2a451a 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -60,7 +60,7 @@ Meson Build system: [required]
>              pip3 install --user --upgrade meson
>  
>  for the libcamera core: [required]
> -        python3-yaml python3-ply python3-jinja2
> +        libyaml-dev python3-yaml python3-ply python3-jinja2

You can then drop libyaml-dev from the android section further down.

>  
>  for IPA module signing: [required]
>          libgnutls28-dev openssl
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index c9e055d4..7a780d48 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -42,4 +42,5 @@ libcamera_internal_headers = files([
>      'v4l2_pixelformat.h',
>      'v4l2_subdevice.h',
>      'v4l2_videodevice.h',
> +    'yaml_parser.h',
>  ])
> diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> new file mode 100644
> index 00000000..7ba7c52c
> --- /dev/null
> +++ b/include/libcamera/internal/yaml_parser.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * yaml_parser.h - libcamera yaml parsing helper

Nit-picking, I think yaml should be written YAML in
comments/documentation as it's an acronym. "Yaml" in class names is
fine.

> + */
> +
> +#pragma once
> +
> +#include <map>
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +
> +#include <libcamera/geometry.h>

I really like how this fully encapsulates libyaml.

> +namespace libcamera {
> +
> +class YamlObject
> +{
> +public:
> +	YamlObject() = default;
> +	~YamlObject() = default;

You may want to move the = default to the .cpp file, to avoid inlining
constructors and destructors.

> +
> +	YamlObject(const YamlObject &) = delete;
> +	YamlObject &operator=(const YamlObject &) = delete;

You can use the LIBCAMERA_DISABLE_COPY() macro instead (there are a few
examples through libcamera).

> +
> +	bool isValue() const
> +	{
> +		return type_ == VALUE;
> +	}
> +	bool isList() const
> +	{
> +		return type_ == LIST;
> +	}
> +	bool isDictionary() const
> +	{
> +		return type_ == DICTIONARY;
> +	}
> +
> +	bool asBool(bool defaultValue = false, bool *ok = nullptr) const;
> +	double asDouble(double defaultValue = 0.0, bool *ok = nullptr) const;
> +	int32_t asInt32(int32_t defaultValue = 0, bool *ok = nullptr) const;
> +	uint32_t asUint32(uint32_t defaultValue = 0, bool *ok = nullptr) const;
> +	std::string asString(std::string defaultValue = "", bool *ok = nullptr) const;
> +	Size asSize(Size defaultValue = Size(), bool *ok = nullptr) const;

Would a template function we simpler ?

	template<typename T>
	const T &get(const T &defaultValue, bool *ok = nullptr) const;

It could then be called as

	yaml.get<int32_t>(0);

That may prevent default values for the defaultValue argument though
(unless = {} works), but maybe that's not a bad thing, specifying the
default value explictly forces the caller to think about it.

> +
> +	int length() const;

To make this more std::-like, how about renaming length to size ?

The function should return an unsigned int, as the size can't be
negative. We could make it std::size_t too.

> +	const YamlObject &operator[](int index) const;

Same here, the index should be unsigned, possible std::size_t (and the
same as the return type of the size() function).

> +
> +	bool isMember(const std::string &key) const;

s/isMember/hasMember/

But to make it more std::-like, I would rename this to contains(). The
get() function could then be an overload of operator[] with a
std::string argument.

> +	const YamlObject &get(const std::string &key) const;
> +	std::vector<std::string> getMemberNames() const;

As this is typically used to iterator over the dictionary entries, it
would be nice to implement begin() and end() instead, with an iterator.
It could be as simple as

	using const_iterator = std::map<const std::string, std::unique_ptr<YamlObject>>::iterator;
	const_iterator begin() const { return dictionary_.cbegin(); }
	const_iterator end() const { return dictionary_.cbegin(); }

but that would expose the unique_ptr and the non-const YamlObject. An
iterator that wraps the map iterator and returns a const
std::pair<std::string, YamlObject &> could be better.

This however only takes care of iteration over the dictionary, not the
list. I'll try to see if we could combine both, you don't have to
address it in the next version.

> +
> +private:
> +	friend class YamlParser;
> +
> +	enum PropertyType {

Maybe just Type ? You could also make it an enum class, it's more
type-safe.

> +		DICTIONARY,
> +		LIST,
> +		VALUE,

These should use CamelCase.

> +	} type_;

Could you split the type and variable declaration ? We don't usually
combine them.

> +
> +	std::string value_;
> +	std::vector<std::unique_ptr<YamlObject>> list_;
> +	std::map<const std::string, std::unique_ptr<YamlObject>> dictionary_;
> +};
> +
> +class YamlParser final : public Extensible
> +{
> +	LIBCAMERA_DECLARE_PRIVATE()
> +
> +public:
> +	YamlParser();
> +	int ParseAsYamlObject(FILE *fh, YamlObject &yamlObject);

Function names should start with a lower case letter.

> +
> +private:
> +	int ParseNextYamlObject(YamlObject &yamlObject);

Same here.

Is there a reason this function is here and not in the Private class ?
You can make YamlParser::Private a friend of YamlObject if needed.

> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 26912ca1..f8e18e03 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -46,6 +46,7 @@ libcamera_sources = files([
>      'v4l2_pixelformat.cpp',
>      'v4l2_subdevice.cpp',
>      'v4l2_videodevice.cpp',
> +    'yaml_parser.cpp',
>  ])
>  
>  libcamera_sources += libcamera_public_headers
> @@ -66,6 +67,7 @@ subdir('proxy')
>  libdl = cc.find_library('dl')
>  libgnutls = cc.find_library('gnutls', required : true)
>  libudev = dependency('libudev', required : false)
> +libyaml = dependency('yaml-0.1', required : true)
>  
>  if libgnutls.found()
>      config_h.set('HAVE_GNUTLS', 1)
> @@ -126,6 +128,7 @@ libcamera_deps = [
>      libgnutls,
>      liblttng,
>      libudev,
> +    libyaml,
>  ]
>  
>  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> new file mode 100644
> index 00000000..203d9cf4
> --- /dev/null
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -0,0 +1,796 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * yaml_parser.cpp - libcamera yaml parsing helper
> + */
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +#include <assert.h>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <yaml.h>
> +
> +/**
> + * \file libcamera/internal/yaml_parser.h
> + * \brief A yaml parser helper
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(YamlParser)
> +
> +namespace {
> +
> +void setOk(bool *ok, bool result)
> +{
> +	if (ok)
> +		*ok = result;
> +}
> +
> +} /* namespace */
> +
> +/**
> + * \class YamlObject
> + * \brief A class representing the tree structure of the yaml content
> + *
> + * The YamlObject class represents the tree structure of a yaml content. A

s/a yaml content/yaml content/

> + * YamlObject can be a dictionary or list of YamlObjects or a value if a tree
> + * leaf.
> + */
> +
> +/**
> + * \fn YamlObject::isValue()
> + * \brief Return whether the YamlObject is a value
> + *
> + * \return Whether the YamlObject is a value

 * \return True if the YammObject is a value, false otherwise

to match the usual wording in the documentation. Same below.

> + */
> +
> +/**
> + * \fn YamlObject::isList()
> + * \brief Return whether the YamlObject is a list
> + *
> + * \return Whether the YamlObject is a list
> + */
> +
> +/**
> + * \fn YamlObject::isDictionary()
> + * \brief Return whether the YamlObject is a dictionary
> + *
> + * \return Whether the YamlObject is a dictionary
> + */
> +
> +/**
> + * \fn YamlObject::asBool()
> + * \brief Helper function to parse the YamlObject as a bool value

s/Helper function to parse/Parse/

You can drop "helper function" from other locations below.

> + * \param[in] defaultValue The default value when fail to parse

s/when fail to parse/when failing to parse/ (or "when parsing fails")

Same below.

> + * \param[in] ok The result of whether the parse success

s/param[in]/param[out]/
s/success/succeeded/

> + *
> + * Helper function to parse the YamlObject as a bool value.

There's no need to duplicate the \brief, but extending it is useful:

 * This function parses the value of the YamlObject as a boolean, and returns
 * the value. If parsing fails (usually because the YamlObject doesn't store a
 * boolean value), the \a defaultValue is returned, and \a ok is set to false.
 * Otherwise, the YamlObject boolean value is returned, and \a ok is set to
 * true.
 *
 * The \a ok pointer is optional and can be a nullptr if the caller doesn't
 * need to know if parsing succeeded.

Same below.

> + *
> + * \return Value as a bool type
> + */
> +bool YamlObject::asBool(bool defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != VALUE) {
> +		return defaultValue;
> +	}

No need for curly braces.

> +
> +	if (value_ == "true") {
> +		setOk(ok, true);
> +		return true;
> +	} else if (value_ == "false") {
> +		setOk(ok, true);
> +		return false;
> +	}
> +
> +	return defaultValue;
> +}
> +
> +/**
> + * \fn YamlObject::asInt32()
> + * \brief Helper function to parse the YamlObject as a int32_t value
> + * \param[in] defaultValue The default value when fail to parse
> + * \param[in] ok The result of whether the parse success
> + *
> + * Helper function to parse the YamlObject as a int32_t value.
> + *
> + * \return Value as a int32_t type
> + */
> +int32_t YamlObject::asInt32(int32_t defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != VALUE)
> +		return defaultValue;
> +
> +	int32_t value = std::strtol(value_.c_str(), nullptr, 10);
> +	if (errno == ERANGE)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return value;
> +}
> +
> +/**
> + * \fn YamlObject::asUint32()
> + * \brief Helper function to parse the YamlObject as a uint32_t value
> + * \param[in] defaultValue The default value when fail to parse
> + * \param[in] ok The result of whether the parse success
> + *
> + * Helper function to parse the YamlObject as a uint32_t value.
> + *
> + * \return Value as a uint32_t type
> + */
> +uint32_t YamlObject::asUint32(uint32_t defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != VALUE) {
> +		return defaultValue;
> +	}

No need for curly braces.

> +
> +	uint32_t value = std::strtoul(value_.c_str(), nullptr, 10);
> +	if (errno == ERANGE)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return value;
> +}
> +
> +/**
> + * \fn YamlObject::asDouble()
> + * \brief Helper function to parse the YamlObject as a double value
> + * \param[in] defaultValue The default value when fail to parse
> + * \param[in] ok The result of whether the parse success
> + *
> + * Helper function to parse the YamlObject as a double value.
> + *
> + * \return Value as a double type
> + */
> +double YamlObject::asDouble(double defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);

Please add a blank line here, to match the other functions.

> +	if (type_ != VALUE)
> +		return defaultValue;
> +
> +	double value = std::strtod(value_.c_str(), nullptr);
> +	if (errno == ERANGE)
> +		return defaultValue;
> +
> +	setOk(ok, true);
> +	return value;
> +}
> +
> +/**
> + * \fn YamlObject::asString()
> + * \brief Helper function to parse the YamlObject as a string value
> + * \param[in] defaultValue The default value when fail to parse
> + * \param[in] ok The result of whether the parse success
> + *
> + * Helper function to parse the YamlObject as a string value.
> + *
> + * \return Value as a string type
> + */
> +std::string YamlObject::asString(std::string defaultValue, bool *ok) const
> +{
µ> +	if (type_ != VALUE) {
> +		setOk(ok, false);
> +		return defaultValue;
> +	}
> +

*ok will not be set to true here. I'd use the same style as in the above
functions.

> +	return value_;
> +}
> +
> +/**
> + * \fn YamlObject::asSize()
> + * \brief Helper function to parse the YamlObject as a Size value
> + * \param[in] defaultValue The default value when fail to parse
> + * \param[in] ok The result of whether the parse success
> + *
> + * Helper function to parse the YamlObject as a Size value. The Size structure
> + * is represented as a list of two non-negative numbers.
> + *
> + * \return Value as a Size type
> + */
> +Size YamlObject::asSize(Size defaultValue, bool *ok) const
> +{
> +	setOk(ok, false);
> +
> +	if (type_ != LIST)
> +		return defaultValue;
> +
> +	if (list_.size() != 2)
> +		return defaultValue;
> +
> +	bool isInt = false;
> +	int width = list_[0]->asUint32(0, &isInt);

s/int/unsigned int/

You could pass ok to the function instead of &isInt, and drop the isInt
variable.

> +	if (!isInt)
> +		return Size(0, 0);
> +
> +	int height = list_[1]->asUint32(0, &isInt);

Here too.

> +	if (!isInt)
> +		return Size(0, 0);
> +
> +	setOk(ok, true);
> +	return Size(width, height);
> +}
> +
> +/**
> + * \fn YamlObject::length()
> + * \brief Helper function to return length of the list
> + *
> + * Helper function to parse the YamlObject as a List and return the length of
> + * the List.
> + *
> + * \return The length as a list

(Assuming the function gets renamed to size() as discussed above)

 * \brief Retrieve the number of elements in a list YamlObject
 *
 * This function retrieves the size of the YamlObject, defined as the number of
 * child elements it contains. Only YamlObject instances of list type have a
 * size, calling this function on other types of instances is invalid and
 * results in undefined behaviour.
 *
 * \return The size of the YamlObject

Similarly below for the undefined behaviour description.

> + */
> +int YamlObject::length() const
> +{
> +	assert(type_ == LIST);
> +
> +	return list_.size();
> +}
> +
> +/**
> + * \fn YamlObject::operator[](int index)
> + * \brief Helper function to return an element of a list YamlObject by index
> + *
> + * Helper function to return an element of a list YamlObject by index.
> + *
> + * \return The YamlObject as an element of the list
> + */
> +const YamlObject &YamlObject::operator[](int index) const
> +{
> +	assert(type_ == LIST);
> +
> +	return *list_[index];
> +}
> +
> +/**
> + * \fn YamlObject::isMember()
> + * \brief Helper function to check if an element of a dictionary exists
> + *
> + * Helper function to return an element of a list YamlObject by index.
> + *
> + * \return Whether an element exists
> + */
> +bool YamlObject::isMember(const std::string &key) const
> +{
> +	assert(type_ == DICTIONARY);
> +
> +	if (dictionary_.find(key) == dictionary_.end())
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * \fn YamlObject::getMemberNames()
> + * \brief Helper function to get all member names of the dictionary
> + *
> + * Helper function to get all member names of the dictionary.
> + *
> + * \return A vector of string as the member names
> + */
> +std::vector<std::string> YamlObject::getMemberNames() const
> +{
> +	assert(type_ == DICTIONARY);
> +
> +	std::vector<std::string> memberNames;
> +	for (auto &[key, _] : dictionary_)
> +		memberNames.push_back(key);
> +
> +	return memberNames;
> +}
> +
> +/**
> + * \fn YamlObject::get()
> + * \brief Helper function to get a member by name from the dictionary
> + *
> + * Helper function to get a member by name from the dictionary
> + *
> + * \return A YamlObject as a member
> + */
> +const YamlObject &YamlObject::get(const std::string &key) const
> +{
> +	assert(type_ == DICTIONARY);
> +	assert(isMember(key));
> +
> +	auto iter = dictionary_.find(key);
> +	return *iter->second;
> +}
> +
> +class YamlParser::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(YamlParser)
> +
> +public:
> +	Private();
> +	~Private();
> +
> +	using ItemParser = const std::function<int()> &;
> +	using RecordParser = const std::function<int(const std::string &)> &;
> +
> +	int initParser(FILE *fh);
> +	void release();
> +
> +	int consumeDocumentStart();
> +	int consumeDocumentEnd();
> +
> +	int parseString(std::string &value);
> +	int parseList(ItemParser parseItem);
> +	int parseDictionary(RecordParser parseRecord);
> +
> +private:
> +	int nextEvent(yaml_event_t &event);
> +	int consume(yaml_event_type_t);
> +
> +	int parseDictionaryOrList(bool isDictionary,
> +				  const std::function<int()> &parseItem);
> +
> +	bool nextEventLoaded_;
> +	yaml_event_t nextEvent_;
> +
> +	bool parserValid_;
> +	yaml_parser_t parser_;
> +};
> +
> +/**
> + * \class YamlParser::Private
> + * \brief A Private class helps control event based parsing for yaml files.

Maybe

 * \brief Base class for YamlParser private data

to match the documentation of other private classes ?

> + *
> + * The YamlParser::Private class stores the internal yaml_parser_t and provides
> + * helper functions to do event based parsing for yaml files.

s/event base/event-based/
s/yaml/YAML/

> + */
> +YamlParser::Private::Private()
> +	: nextEventLoaded_(false), parserValid_(false)
> +{
> +}
> +
> +/**
> + * \class YamlParser::Private
> + * \brief Destructor of YamlParser::Private.
> + */
> +YamlParser::Private::~Private()
> +{
> +	release();
> +}
> +
> +/**
> + * \fn YamlParser::Private::initParser()
> + * \brief Initialize a parser with an opened file for parsing
> + * \param[in] fh The yaml file to parse
> + *
> + * Prior to parsing the yaml content, the YamlParser must be initialized with
> + * an opened FILE to create an internal parser. The FILE need to stay valid
> + * during the process. The release() should be called after use.

  The release() function must be called after parsing completes.

> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to initialize

s/is failed/failed/ (of "has failed")

> + */
> +int YamlParser::Private::initParser(FILE *fh)
> +{
> +	/* yaml_parser_initialize returns 1 when succeeded */

s/when succeeded/on success/ (or "when it succeeds")

> +	if (!yaml_parser_initialize(&parser_)) {
> +		LOG(YamlParser, Error) << "Failed to initialize yaml parser";
> +		return -EINVAL;
> +	}
> +	parserValid_ = true;
> +	yaml_parser_set_input_file(&parser_, fh);
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParser::Private::release()
> + * \brief Release the internal parser
> + *
> + * After parsing the yaml content, The YamlParser::release() should be called
> + * to release the internal parser.
> + */
> +void YamlParser::Private::release()
> +{
> +	if (nextEventLoaded_) {
> +		yaml_event_delete(&nextEvent_);
> +		nextEventLoaded_ = false;
> +	}
> +
> +	if (parserValid_) {
> +		parserValid_ = false;
> +		yaml_parser_delete(&parser_);
> +	}
> +}
> +
> +/**
> + * \fn YamlParser::Private::consume()
> + * \brief Cosume the next event with an expected event type

s/Cosume/Consume/

> + *
> + * \param[in] type The expected event type to consume
> + *
> + * Consume next event and check whether next event has an expected type.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to initialize
> + */
> +int YamlParser::Private::consume(yaml_event_type_t type)
> +{
> +	yaml_event_t event;
> +	int ret = nextEvent(event);
> +	if (ret)
> +		return ret;
> +
> +	if (event.type != type) {
> +		LOG(YamlParser, Error)
> +			<< "Expect event: " << type
> +			<< " but get " << event.type
> +			<< " at line: " << event.start_mark.line
> +			<< " column: " << event.start_mark.column;
> +		return -EINVAL;
> +	}
> +
> +	yaml_event_delete(&event);
> +	nextEventLoaded_ = false;
> +
> +	return 0;
> +}
> +
> +/**
> + * \typedef YamlParser::Private::ItemParser
> + * \brief The functor to handle an item in a yaml list
> + */
> +
> +/**
> + * \typedef YamlParser::Private::RecordParser
> + * \brief The functor to handle an item in a yaml dictionary
> + */
> +
> +/**
> + * \fn YamlParser::Private::consumeDocumentStart()
> + * \brief Consume start of a yaml document
> + *
> + * Check yaml start of a yaml document. The function should be called and
> + * checked before parsing any content of the yaml file.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to validate start of a yaml file
> + */
> +int YamlParser::Private::consumeDocumentStart()
> +{
> +	if (consume(YAML_STREAM_START_EVENT))
> +		return -EINVAL;
> +
> +	if (consume(YAML_DOCUMENT_START_EVENT))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParser::Private::consumeDocumentEnd()
> + * \brief Consume end of a yaml document
> + *
> + * Check yaml end of a yaml document. The function should be called and
> + * checked after parsing any content of the yaml file.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to validate end of a yaml file
> + */
> +int YamlParser::Private::consumeDocumentEnd()
> +{
> +	if (consume(YAML_DOCUMENT_END_EVENT))
> +		return -EINVAL;
> +
> +	if (consume(YAML_STREAM_END_EVENT))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParser::Private::parseString()
> + * \brief Parse scalar and read its content as a string
> + * \param[in] value The string reference to fill value
> + *
> + * A helper function to peek the next event, check whether it's scalar type
> + * and read its content as a string.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to initialize
> + */
> +int YamlParser::Private::parseString(std::string &value)
> +{
> +	yaml_event_t event;
> +	int ret = nextEvent(event);
> +	if (ret)
> +		return ret;
> +
> +	if (event.type != YAML_SCALAR_EVENT) {
> +		LOG(YamlParser, Error) << "Expect scalar at line: "
> +				       << event.start_mark.line
> +				       << " column: "
> +				       << event.start_mark.column;
> +		return -EINVAL;
> +	}
> +
> +	value.assign(reinterpret_cast<char *>(event.data.scalar.value),
> +		     event.data.scalar.length);
> +
> +	consume(YAML_SCALAR_EVENT);
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParser::Private::parseList()
> + * \brief Parse a list with an callback function to parse each item in the list
> + * \param[in] parseItem The functor to parse a single item in the list
> + *
> + * Start to parse a list from the current position and call back to parseItem
> + * for parsing each item in the list. The parseItem should return 0 on success
> + * or a negative error code otherwise.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to parse the list
> + */
> +int YamlParser::Private::parseList(ItemParser parseItem)
> +{
> +	return parseDictionaryOrList(false, parseItem);
> +}
> +
> +/**
> + * \fn YamlParser::Private::parseDictionary()
> + * \brief Parse a dictionary with an callback function to parse each item
> + * \param[in] parseRecord The functor to parse a single item in the dictionary
> + *
> + * Start to parse a dictionary from the current position and call back to
> + * parseRecord to parse value by a string argument as the key to the value.
> + * The parseRecord should return 0 on success or a negative error code
> + * otherwise.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to parse the dictionary
> + */
> +int YamlParser::Private::parseDictionary(RecordParser parseRecord)
> +{
> +	auto parseItem = [this, &parseRecord]() {
> +		std::string key;
> +		int ret = parseString(key);
> +		if (ret)
> +			return -EINVAL;
> +
> +		return parseRecord(key);
> +	};
> +
> +	return parseDictionaryOrList(true, parseItem);
> +}
> +
> +/**
> + * \fn YamlParser::Private::parseDictionaryOrList()
> + * \brief A helper function to abstract common part of parsing dictionary or list
> + *
> + * \param[in] isDictionary True for parsing a dictionary, and false for a list
> + * \param[in] parseItem The callback to handle an item
> + *
> + * A helper function to abstract parsing a item from a dictionary or a list.
> + * The differences of them in a yaml event stream are:
> + *
> + * 1. The start and end event type are different
> + * 2. There is a leading scalar string as key in the items of a dictionary
> + *
> + * The caller should handle the leading key string in its callback parseItem
> + * when it's a dictionary.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to initialize
> + */
> +int YamlParser::Private::parseDictionaryOrList(bool isDictionary,
> +					       const std::function<int()> &parseItem)
> +{
> +	yaml_event_type_t startEventType = YAML_SEQUENCE_START_EVENT;
> +	yaml_event_type_t endEventType = YAML_SEQUENCE_END_EVENT;
> +
> +	if (isDictionary) {
> +		startEventType = YAML_MAPPING_START_EVENT;
> +		endEventType = YAML_MAPPING_END_EVENT;
> +	}
> +
> +	if (consume(startEventType))
> +		return -EINVAL;
> +
> +	/*
> +	 * Add a safety counter to make sure we don't loop indefinitely in case
> +	 * the configuration file is malformed.
> +	 */
> +	unsigned int sentinel = 1000;
> +	yaml_event_t event;
> +	do {
> +		int ret = nextEvent(event);
> +		if (ret)
> +			return ret;
> +
> +		if (event.type == endEventType)
> +			return consume(endEventType);
> +
> +		ret = parseItem();
> +		if (ret)
> +			return ret;
> +
> +		--sentinel;
> +	} while (sentinel);
> +
> +	if (!sentinel)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn YamlParser::Private::nextEvent()
> + * \brief Peek the next event
> + *
> + * \param[in] event The event reference to fill information
> + *
> + * Peek the next event in the current yaml event stream, and return -EINVAL when
> + * there is no more event.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The parser is failed to initialize
> + */
> +int YamlParser::Private::nextEvent(yaml_event_t &event)
> +{
> +	if (nextEventLoaded_) {
> +		event = nextEvent_;
> +		return 0;
> +	}
> +
> +	/* yaml_parser_parse returns 1 when succeeded */
> +	if (1 != yaml_parser_parse(&parser_, &nextEvent_)) {
> +		return -EINVAL;
> +	}
> +
> +	nextEventLoaded_ = true;
> +	event = nextEvent_;
> +
> +	return 0;
> +}

I'll review the internals of the parser in details on the next
iteration, as there are enough review comments already :-)

> +
> +/**
> + * \class YamlParser
> + * \brief A helper class for parsing yaml files.

s/yaml files./YAML files/

> + *
> + * The YamlParser class eases handling of parsing a yaml file by providing
> + * helper function to extract content yaml files into a tree base yaml object.

 * The YamlParser class provides an easy interface to parse the contents of a
 * YAML file into a tree of YamlObject instances.

> + *
> + * Example usage 1:
> + * The following code illustrates how to parse the following yaml file:
> + *
> + * name:
> + * 	"John"
> + * numbers:
> + * 	- 1
> + * 	- 2
> + *
> + * @code

That should be \code to match the style of the other doxygen tags. Same
for endcode.

> + *
> + *	YamlObject root;
> + *	YamlParser yamlParser;
> + *	if (yamlParser.ParseAsYamlObject(fh, root));

Extra ; at the end of the line.

> + *		return;
> + *
> + *	if (!root.isDictionary())
> + *		return;
> + *
> + *	std::string name = root.get("name");
> + *	cout << name.asString();

 *	std::cout << name.asString() << std::endl;

> + *
> + *	const YamlObject &numbers = root.get("numbers");
> + *	if (!numbers.isList())
> + *		return;
> + *
> + *	for (int i = 0; i < numbers.size; i++)
> + *		cout << numbers[i].asInt();

 *		std::cout << numbers[i].asInt() << std::endl;

It would be nice to be able to write

 *	for (const YamlObject &number : numbers)
 *		std::cout << number.asInt() << std::endl;

> + *
> + * @endcode
> + *
> + * Function ParseAsYamlObject(FILE *, YamlObject &) accept an opened FILE and
> + * initialize an internal parser. The FILE need to stay valid during function
> + * call.

 * The ParseAsYamlObject() function accepts an open FILE and initializes an
 * internal parse.

The second sentence can be dropped, as there's no way for the caller to
close the file in the middle of the call :-)

> + */
> +
> +/**
> + * \brief Construct a YamlParser
> + */
> +YamlParser::YamlParser()
> +	: Extensible(std::make_unique<Private>())
> +{
> +}
> +
> +/**
> + * \fn YamlParser::ParseAsYamlObject()

As we can't parse it as anything else than a YamlObject, I think I'd
call this function just parse().

> + * \brief Parse a yaml file as a YamlObject
> + * \param[in] fh The yaml file to parse
> + * \param[in] yamlObject The result fo YamlObject

Wouldn't it be simpler for the caller if this function returned the
YamlObject instance ? The only error code we return is -EINVAL so
there's no need to differentiate between errors, 

> + *
> + * Prior to parsing the yaml content, the function accepts an opened FILE to
> + * create an internal parser. The FILE need to stay valid during the function
> + * call. When fails, the contect of the YamlObject is undefined.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL Fail to parse the yaml file.
> + */
> +int YamlParser::ParseAsYamlObject(FILE *fh, YamlObject &yamlObject)
> +{
> +	if (_d()->initParser(fh))
> +		return -EINVAL;
> +
> +	if (_d()->consumeDocumentStart())
> +		goto error;
> +
> +	if (ParseNextYamlObject(yamlObject))
> +		goto error;
> +
> +	if (_d()->consumeDocumentEnd())
> +		goto error;
> +
> +	_d()->release();
> +	return 0;
> +
> +error:
> +	_d()->release();
> +	return -EINVAL;
> +}

I'm tempted to make this function static, drop inheritance from
Extensible, and rename YamlParser::Private to YamlParserContext.

> +
> +/**
> + * \fn YamlParser::ParseNextEvent()
> + * \brief Helper function to parse next yaml event and read it as a YamlObject
> + * \param[in] yamlObject The result of YamlObject
> + *
> + * A helper function to parse next yaml event by peeking next event and parse
> + * them separately as a value, list or dictionary.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL Fail to parse the yaml file.
> + */
> +int YamlParser::ParseNextYamlObject(YamlObject &yamlObject)
> +{
> +	yaml_event_t event;
> +
> +	if (_d()->nextEvent(event))
> +		return -EINVAL;
> +
> +	if (event.type == YAML_SCALAR_EVENT) {
> +		yamlObject.type_ = YamlObject::VALUE;
> +		_d()->parseString(yamlObject.value_);
> +		return 0;
> +	}
> +
> +	if (event.type == YAML_SEQUENCE_START_EVENT) {
> +		yamlObject.type_ = YamlObject::LIST;
> +		auto &list = yamlObject.list_;
> +		auto handler = [this, &list]() {
> +			list.emplace_back(new YamlObject());
> +			return ParseNextYamlObject(*list.back());
> +		};
> +		return _d()->parseList(handler);
> +	}
> +
> +	if (event.type == YAML_MAPPING_START_EVENT) {
> +		yamlObject.type_ = YamlObject::DICTIONARY;
> +		auto &dictionary = yamlObject.dictionary_;
> +		auto handler = [this, &dictionary](const std::string &key) {
> +			dictionary[key].reset(new YamlObject());
> +			return ParseNextYamlObject(*dictionary[key]);
> +		};
> +		return _d()->parseDictionary(handler);
> +	}
> +
> +	LOG(YamlParser, Error) << "Invalid yaml file";
> +	return -EINVAL;
> +}
> +
> +} /* namespace libcamera */