[libcamera-devel] [RFC PATCH 6/8] libcamera: Add configuration interface

[Kieran Bingham]

Hi Jacopo,

On 25/11/2020 08:52, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Nov 23, 2020 at 04:43:17PM +0000, Kieran Bingham wrote:
>> Provide an interface to support reading configuration files.
>>
>> Initial support is included for JSON formatted files, but extending this
>> with other configuration file formats is not excluded.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  README.rst                                 |  2 +-
>>  include/libcamera/internal/configuration.h | 37 +++++++++
>>  src/libcamera/configuration.cpp            | 91 ++++++++++++++++++++++
>>  src/libcamera/meson.build                  |  1 +
>>  4 files changed, 130 insertions(+), 1 deletion(-)
>>  create mode 100644 include/libcamera/internal/configuration.h
>>  create mode 100644 src/libcamera/configuration.cpp
>>
>> diff --git a/README.rst b/README.rst
>> index 251291b77c62..c09e262fcc43 100644
>> --- a/README.rst
>> +++ b/README.rst
>> @@ -58,7 +58,7 @@ Meson Build system: [required]
>>              pip3 install --user --upgrade meson
>>
>>  for the libcamera core: [required]
>> -        python3-yaml python3-ply python3-jinja2
>> +        python3-yaml python3-ply python3-jinja2 nlohmann-json3-dev
> 
> First question I have is about the dependency on this package.
> 
> I have no real reasons to doubt about the availability of a packaged
> version in most linux distro, nor about their stability and update
> rate. It might be that projects named after a single person always
> sound fragile to me, might be the rather low bus factor :)


Yes, I find the 'named after me' packaging a little ... awkward. But
there's really two choices for json libraries if we don't want boost.
nlohmann-json and json-cpp.

For bus factor, nlohmann-json has 178 contributors ... so I don't think
it's just 'one guy'.

I had planned to try both in earnest, and subclass Configuration so that
they could be swapped at will, but the nlohmann was seemingly easier to
get started with in the end, and subclassing to swap would mean wrapping
the nlohmann generic container types in a generic container type and
creating my own tree structures.

We can do that of course, but that will take longer, and I honestly
couldn't see the point of wrapping it for the internal usages (yet).

If this was public API then it would be different.

And maybe we'll update/change later - but I wanted to get things
actually going so we can start figuring out what and where we store data
in the first place.

Who knows, maybe Configuration() will sometime need to parse yaml files
so it will build a ConfigurationJSON or a ConfigurationYAML depending on
the data - but I don't want to over engineer just to be able to read
some key-values at this stage.

> Joking aside this seems to be packaged in most distros
> https://repology.org/project/nlohmann-json/versions
> with rather different versions (3.9.1 on Arch, 2.1.1 on Ubuntu 18.04
> which is an LTS if I'm not mistaken).

It did seem to be quite well packaged, but I would probably lean towards
bringing the headers in under third_party/ or such as it's header only
to prevent any potential issues.


> There are notable exception though, in example Linux Mint.
> 
> In ChromiumOS R87 I don't see it packaged, my equery skills are very
> low so I might have missed it.

Also, while available in the Raspbian OS distribution - it's an older
version which didn't have a feature I was using "j.contains()".

I thought that was actually a nicer way to implement the code rather
than using iterators and j.find(), even if the .find() is perhaps more
efficient as it will only do one look up - I think the code just doesn't
flow quite as nicely. (Having to suddenly dereference iterators to a new
type etc...)


> The real question is:
> - Are we happy to have this as externally packaged dependency ?
> 
> If I look at the Arch package content:
> https://www.archlinux.org/packages/community/any/nlohmann-json/files/
> 
> This seems to be an header-only library. I wonder if we should not
> rope it in during the build. There seems to be built-in support in
> meson for that:
> https://github.com/nlohmann/json#package-managers
> https://wrapdb.mesonbuild.com/nlohmann_json

It is in wrapdb - but it's out of date.

We can either include the headers in libcamera, or we can update the
wrapdb ;-)

I'd probably lean towards updating the wrapdb as that's more beneficial
everywhere.

The meson wraps seem to work quite well - and will seemlessly download
the package if it's missing and build/use that.

Part of me is then weary of requiring an internet connection for that -
but I really don't' think that's an issue anymore, as if you've cloned
the repo - we can expect that there is a connection.



> Or include ourselves a version of the library in our source tree,
> which opens a different question: how do we feel about distributing MIT
> licensed code ? It should be no issue, but IANAL so it's better to
> give this a thought.

That's also what makes me lean towards updating and using the wrapdb ;-)

Alternatively of course we can rewrite all this with json-cpp
(https://github.com/open-source-parsers/jsoncpp) - but then we'll have
exactly the same discussion on that library/package too ;-)

Kieran


> Thanks
>   j
> 
>>
>>  for IPA module signing: [required]
>>          libgnutls28-dev openssl
>> diff --git a/include/libcamera/internal/configuration.h b/include/libcamera/internal/configuration.h
>> new file mode 100644
>> index 000000000000..a89732f0210f
>> --- /dev/null
>> +++ b/include/libcamera/internal/configuration.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * configuration.h - Parsing configuration files
>> + */
>> +#ifndef __LIBCAMERA_INTERNAL_CONFIGURATION_H__
>> +#define __LIBCAMERA_INTERNAL_CONFIGURATION_H__
>> +
>> +#include <fstream>
>> +#include <string>
>> +
>> +/* https://nlohmann.github.io/json/home/exceptions/#switch-off-exceptions */
>> +#define JSON_NOEXCEPTION 1
>> +#include <nlohmann/json.hpp>
>> +
>> +using json = nlohmann::json;
>> +
>> +namespace libcamera {
>> +
>> +class Configuration
>> +{
>> +public:
>> +	int open(std::string filename);
>> +
>> +	json &data() { return json_; }
>> +
>> +private:
>> +	std::string findFile(std::string filename);
>> +
>> +	json json_;
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_INTERNAL_CONFIGURATION_H__ */
>> +
>> diff --git a/src/libcamera/configuration.cpp b/src/libcamera/configuration.cpp
>> new file mode 100644
>> index 000000000000..f849088bbc45
>> --- /dev/null
>> +++ b/src/libcamera/configuration.cpp
>> @@ -0,0 +1,91 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * configuration.cpp - Parsing configuration files
>> + */
>> +#include "libcamera/internal/configuration.h"
>> +
>> +#include "libcamera/internal/file.h"
>> +#include "libcamera/internal/log.h"
>> +#include "libcamera/internal/utils.h"
>> +
>> +#include <iostream>
>> +#include <stdlib.h>
>> +
>> +/**
>> + * \file configuration.h
>> + * \brief Read interface for configuration files
>> + */
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(Configuration)
>> +
>> +/*
>> + * Configuration files can be stored in system paths, which are identified
>> + * through the build configuration.
>> + *
>> + * However, when running uninstalled - the source location takes precedence.
>> + */
>> +std::string Configuration::findFile(std::string filename)
>> +{
>> +	static std::array<std::string, 2> searchPaths = {
>> +		LIBCAMERA_SYSCONF_DIR,
>> +		LIBCAMERA_DATA_DIR,
>> +	};
>> +
>> +	std::string root = utils::libcameraSourcePath();
>> +	if (!root.empty()) {
>> +		std::string configurationPath = root + "data/" + filename;
>> +
>> +		if (File::exists(configurationPath))
>> +			return configurationPath;
>> +	}
>> +
>> +	for (std::string &path : searchPaths) {
>> +		std::string configurationPath = path + "/" + filename;
>> +		if (File::exists(configurationPath))
>> +			return configurationPath;
>> +	}
>> +
>> +	return "";
>> +}
>> +
>> +/**
>> + * \brief Open and parse a configuration file.
>> + *
>> + * The filename will be searched for on the libcamera configuration and paths,
>> + * and then parsed.
>> + *
>> + * Successfully parsed files will present the data contained therein through the
>> + * json object exposed from data();
>> + */
>> +int Configuration::open(std::string filename)
>> +{
>> +	std::string data = findFile(filename);
>> +	if (data.empty()) {
>> +		LOG(Configuration, Warning)
>> +			<< "file: \"" << filename
>> +			<< "\" was not found.";
>> +		return -ENOENT;
>> +	}
>> +
>> +	LOG(Configuration, Debug) << "Reading configuration from " << data;
>> +
>> +	/* Parse with no error callbacks and exceptions disabled. */
>> +	std::ifstream input(data);
>> +	json j = json::parse(input, nullptr, false);
>> +	if (j.is_discarded()) {
>> +		LOG(Configuration, Error)
>> +			<< "file: \"" << data
>> +			<< "\" was not parsable.";
>> +		return -EINVAL;
>> +	}
>> +
>> +	json_ = std::move(j);
>> +
>> +	return 0;
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 387d5d88ecae..5d655c87a7a0 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -9,6 +9,7 @@ libcamera_sources = files([
>>      'camera_controls.cpp',
>>      'camera_manager.cpp',
>>      'camera_sensor.cpp',
>> +    'configuration.cpp',
>>      'controls.cpp',
>>      'control_serializer.cpp',
>>      'control_validator.cpp',
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran