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

10477 diff mbox series

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@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

Hi Kieran,


On 23/11/2020 16:43, 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.
> 

Ahem, that was an earlier design intention, which now that this class
directly exposes a json object - does not currently hold true.

Of course all of this is internal, so things can be refactored later -
but right now - Configuration() supports only JSON.

--
Kieran


> Signed-off-by: Kieran Bingham <kieran.bingham@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
>  
>  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',
>

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@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 :)

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).

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.

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

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.

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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

On Wed, Nov 25, 2020 at 09:11:10AM +0000, Kieran Bingham wrote:
> 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@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.
>

I would go in that direction too probably

>
> > 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...)
>

In general, knowing we can rely on known version would bring quite
some peace of mind in regard to versioning and available features

>
> > 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.
>

Right, 3.7.0 as I read it

> 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.
>

Indeed, I'm not aware of what the process is to do so though

> 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.

Probably so. I'm thinking of a use case where all the dependencies and
libcamera sources are packaged on a rootfs and -then- libcamera is
built on a live target, but I hardly find one.

>
>
>
> > 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 ;-)

You've done the background investigations, so I assume you chose
nlohmann_json for valid reasons. Also, jsoncpp seems not to be an
header only library, so integration might become more problematic.
They're both MIT-licensed project, in one case we will need to only
include un-touched MIT-licensed code headers in LGPL code. If I'm not
mistaken with json-cpp we would need to link against it, so either
build it as a .so or rely on pre-packaged versions (unless we go the
'amalgamated' way:
https://github.com/open-source-parsers/jsoncpp/wiki/Amalgamated-(Possibly-outdated)
but we would have to build it ourselves to obtain the amalgamated
version)

Is this your understanding as well ?

Thanks
  j

>
> 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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran

Hi Jacopo,

On 25/11/2020 10:18, Jacopo Mondi wrote:
> On Wed, Nov 25, 2020 at 09:11:10AM +0000, Kieran Bingham wrote:
>> 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@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.
>>
> 
> I would go in that direction too probably
> 
>>
>>> 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...)
>>
> 
> In general, knowing we can rely on known version would bring quite
> some peace of mind in regard to versioning and available features
> 
>>
>>> 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.
>>
> 
> Right, 3.7.0 as I read it
> 
>> 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.
>>
> 
> Indeed, I'm not aware of what the process is to do so though
> 
>> 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.
> 
> Probably so. I'm thinking of a use case where all the dependencies and
> libcamera sources are packaged on a rootfs and -then- libcamera is
> built on a live target, but I hardly find one.
> 
>>
>>
>>
>>> 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 ;-)
> 
> You've done the background investigations, so I assume you chose
> nlohmann_json for valid reasons. Also, jsoncpp seems not to be an

Part of the reasons were that nlohmann is also reported to have better
performances.

> header only library, so integration might become more problematic.
> They're both MIT-licensed project, in one case we will need to only
> include un-touched MIT-licensed code headers in LGPL code. If I'm not
> mistaken with json-cpp we would need to link against it, so either
> build it as a .so or rely on pre-packaged versions (unless we go the
> 'amalgamated' way:
> https://github.com/open-source-parsers/jsoncpp/wiki/Amalgamated-(Possibly-outdated)
> but we would have to build it ourselves to obtain the amalgamated
> version)

I hadn't looked at any of the amalgamated options. I assumed we'd have
to link against it.

From a license point of view, I don't think there's any conflict for
using an MIT licences either by linking, or by including the headers.

> Is this your understanding as well ?

Close enough.

> 
> Thanks
>   j
> 
>>
>> 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;
>>>> +	}
>>>> +

We might also want to expose an environment variable to add to the paths
here, as we do for IPA configuration files (LIBCAMERA_IPA_CONFIG_PATH)


Hrm ... I should tackle the IPA configuration files as part of this
series too.

It looks like that code path isn't really being used much yet though -
except for a dummy file at src/ipa/vimc/data/vimc.conf

More things for me to look at ;-)

--
Kieran


>>>> +	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@lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> --
>> Regards
>> --
>> Kieran

Hi Kieran,

Thank you for the patch.

On Wed, Nov 25, 2020 at 10:37:15AM +0000, Kieran Bingham wrote:
> On 25/11/2020 10:18, Jacopo Mondi wrote:
> > On Wed, Nov 25, 2020 at 09:11:10AM +0000, Kieran Bingham wrote:
> >> On 25/11/2020 08:52, Jacopo Mondi wrote:
> >>> 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@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.

We also have the option of using a C library, as we wrap it in a custom
class anyway. https://github.com/json-c/json-c/wiki and
https://www.digip.org/jansson/ come to mind, there are probably other
options.  I'm not advocating switching to a different library, just
wanted to make sure that all options are known by everybody.

> >> 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.

YAML is more complicated, otherwise I would have proposed going directly
for it (I *really* dislike how JSON doesn't support comments).

> >>> 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.
> > 
> > I would go in that direction too probably

That or using a wrap, yes. Relying on the distro version is likely not a
good idea if we can't rely on a recent enough version. It however also
means that we'll be responsible for tracking upstream changes
(especially security fixes) and bringing them in.

> >>> 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...)
> > 
> > In general, knowing we can rely on known version would bring quite
> > some peace of mind in regard to versioning and available features
> > 
> >>> 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.
> > 
> > Right, 3.7.0 as I read it
> > 
> >> 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.
> > 
> > Indeed, I'm not aware of what the process is to do so though
> > 
> >> 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.
> > 
> > Probably so. I'm thinking of a use case where all the dependencies and
> > libcamera sources are packaged on a rootfs and -then- libcamera is
> > built on a live target, but I hardly find one.

I believe meson wrap can be pointed to a cache directory with
pre-downloaded zip files. That would be my preference, if we go for a
wrap.

> >>> 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 ;-)

That won't change much though, we'll still link against MIT code,
regardless of whether we carry a copy of it or not. The MIT license is
compatible with the LGPL as far as I can tell, so that's not an issue.

If we want to use an LGPL JSON parser, there's json-glib, but I don't
think that's a good idea :-) I'll also mention simdjson, just to share
the "interesting" idea of SIMD-accelerated JSON parsing.

> >> 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 ;-)
> > 
> > You've done the background investigations, so I assume you chose
> > nlohmann_json for valid reasons. Also, jsoncpp seems not to be an
> 
> Part of the reasons were that nlohmann is also reported to have better
> performances.
> 
> > header only library, so integration might become more problematic.
> > They're both MIT-licensed project, in one case we will need to only
> > include un-touched MIT-licensed code headers in LGPL code. If I'm not
> > mistaken with json-cpp we would need to link against it, so either
> > build it as a .so or rely on pre-packaged versions (unless we go the
> > 'amalgamated' way:
> > https://github.com/open-source-parsers/jsoncpp/wiki/Amalgamated-(Possibly-outdated)
> > but we would have to build it ourselves to obtain the amalgamated
> > version)
> 
> I hadn't looked at any of the amalgamated options. I assumed we'd have
> to link against it.
> 
> From a license point of view, I don't think there's any conflict for
> using an MIT licences either by linking, or by including the headers.
> 
> > Is this your understanding as well ?
> 
> Close enough.
> 
> >>>>  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>

Is this needed ?

> >>>> +#include <string>
> >>>> +
> >>>> +/* https://nlohmann.github.io/json/home/exceptions/#switch-off-exceptions */
> >>>> +#define JSON_NOEXCEPTION 1
> >>>> +#include <nlohmann/json.hpp>

This is what I was hoping to avoid :-S We're pulling nearly 1MB of
header in every file that includes configuration.h. As everything is
inline in headers, it will mean quite a bit of code duplication. At
least the parser is isolated in a .cpp file and won't get duplicated
(but will still get parsed by the compiler, with some impact on build
time), but access to the data will always be inlined :-(

> >>>> +
> >>>> +using json = nlohmann::json;
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +class Configuration

The name Configuration is very generic, especially given
CameraConfiguration that is often referred to as just "configuration".
How about ConfigurationFile ?

> >>>> +{
> >>>> +public:
> >>>> +	int open(std::string filename);

const &

s/open/parse/ ? I also wonder if we couldn't just have a static parse()
function that would return a json object.

> >>>> +
> >>>> +	json &data() { return json_; }

Ooohhhh so json is a class and contains member types. It's not very nice
to write json without a prefix, and json::parse() or json::iterator that
looks like namespace-qualified names :-S

> >>>> +
> >>>> +private:
> >>>> +	std::string findFile(std::string filename);

const &

> >>>> +
> >>>> +	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>

This should go above the previous three headers.

> >>>> +
> >>>> +/**
> >>>> + * \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.
> >>>> + */

I don't think thins should be part of this class. I'd rather have a
JSON parser that only parses files, and deal with paths externally,
especially given that different types of configuration files may be
stored in different locations. For instance I expect we'll have a global
configuration file stored in /etc/libcamera/ with an override in
$HOME/.libcamera/, IPA configuration files in /usr/share/libcamera/
(likely with overrides, including through an environment variable), and
probably more. The mechanisms to locate files will differ, and shouldn't
be in scope for the parser.

> >>>> +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;
> >>>> +	}
> >>>> +
> 
> We might also want to expose an environment variable to add to the paths
> here, as we do for IPA configuration files (LIBCAMERA_IPA_CONFIG_PATH)
> 
> 
> Hrm ... I should tackle the IPA configuration files as part of this
> series too.
> 
> It looks like that code path isn't really being used much yet though -
> except for a dummy file at src/ipa/vimc/data/vimc.conf
> 
> More things for me to look at ;-)
> 
> >>>> +	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',

Hi Laurent,

On 01/12/2020 18:02, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Nov 25, 2020 at 10:37:15AM +0000, Kieran Bingham wrote:
>> On 25/11/2020 10:18, Jacopo Mondi wrote:
>>> On Wed, Nov 25, 2020 at 09:11:10AM +0000, Kieran Bingham wrote:
>>>> On 25/11/2020 08:52, Jacopo Mondi wrote:
>>>>> 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@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.
> 
> We also have the option of using a C library, as we wrap it in a custom
> class anyway. https://github.com/json-c/json-c/wiki and
> https://www.digip.org/jansson/ come to mind, there are probably other
> options.  I'm not advocating switching to a different library, just
> wanted to make sure that all options are known by everybody.


You know, I hadn't actually considered starting at a c-library (because
we're in C++), so I hadn't looked at those.



>>>> 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.
> 
> YAML is more complicated, otherwise I would have proposed going directly
> for it (I *really* dislike how JSON doesn't support comments).

Comments are always helpful anywhere.
Restricting them is certainly frustrating.



>>>>> 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.
>>>
>>> I would go in that direction too probably
> 
> That or using a wrap, yes. Relying on the distro version is likely not a
> good idea if we can't rely on a recent enough version. It however also
> means that we'll be responsible for tracking upstream changes
> (especially security fixes) and bringing them in.
> 
>>>>> 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...)
>>>
>>> In general, knowing we can rely on known version would bring quite
>>> some peace of mind in regard to versioning and available features
>>>
>>>>> 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.
>>>
>>> Right, 3.7.0 as I read it
>>>
>>>> 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.
>>>
>>> Indeed, I'm not aware of what the process is to do so though
>>>
>>>> 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.
>>>
>>> Probably so. I'm thinking of a use case where all the dependencies and
>>> libcamera sources are packaged on a rootfs and -then- libcamera is
>>> built on a live target, but I hardly find one.
> 
> I believe meson wrap can be pointed to a cache directory with
> pre-downloaded zip files. That would be my preference, if we go for a
> wrap.
> 
>>>>> 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 ;-)
> 
> That won't change much though, we'll still link against MIT code,
> regardless of whether we carry a copy of it or not. The MIT license is
> compatible with the LGPL as far as I can tell, so that's not an issue.
> 
> If we want to use an LGPL JSON parser, there's json-glib, but I don't
> think that's a good idea :-) I'll also mention simdjson, just to share
> the "interesting" idea of SIMD-accelerated JSON parsing.


https://github.com/zserge/jsmn/

(jasmine == Json Minimalist) also looks quite reasonable as a c-parser

But we could pull in any of a hundred external libraries. The fact is,
what we want isn't provided by the standard libraries, so we pull in
something else or write our own. And I don't intend to write a full json
parser.


Though I must say, jsmn does it in 400 lines of header, so JSON really
is quite simple ;-)

>>>> 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 ;-)
>>>
>>> You've done the background investigations, so I assume you chose
>>> nlohmann_json for valid reasons. Also, jsoncpp seems not to be an
>>
>> Part of the reasons were that nlohmann is also reported to have better
>> performances.
>>
>>> header only library, so integration might become more problematic.
>>> They're both MIT-licensed project, in one case we will need to only
>>> include un-touched MIT-licensed code headers in LGPL code. If I'm not
>>> mistaken with json-cpp we would need to link against it, so either
>>> build it as a .so or rely on pre-packaged versions (unless we go the
>>> 'amalgamated' way:
>>> https://github.com/open-source-parsers/jsoncpp/wiki/Amalgamated-(Possibly-outdated)
>>> but we would have to build it ourselves to obtain the amalgamated
>>> version)
>>
>> I hadn't looked at any of the amalgamated options. I assumed we'd have
>> to link against it.
>>
>> From a license point of view, I don't think there's any conflict for
>> using an MIT licences either by linking, or by including the headers.
>>
>>> Is this your understanding as well ?
>>
>> Close enough.
>>
>>>>>>  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>
> 
> Is this needed ?
> 

Not anymore,

>>>>>> +#include <string>
>>>>>> +
>>>>>> +/* https://nlohmann.github.io/json/home/exceptions/#switch-off-exceptions */
>>>>>> +#define JSON_NOEXCEPTION 1
>>>>>> +#include <nlohmann/json.hpp>
> 
> This is what I was hoping to avoid :-S We're pulling nearly 1MB of
> header in every file that includes configuration.h. As everything is
> inline in headers, it will mean quite a bit of code duplication. At
> least the parser is isolated in a .cpp file and won't get duplicated
> (but will still get parsed by the compiler, with some impact on build
> time), but access to the data will always be inlined :-(

Ok, so then we'll have to create our own 'types' or structures to pass
around or store data tokens.

I went this route to avoid exactly that in the end. But I guess I'm back
to the other path.


>>>>>> +
>>>>>> +using json = nlohmann::json;
>>>>>> +
>>>>>> +namespace libcamera {
>>>>>> +
>>>>>> +class Configuration
> 
> The name Configuration is very generic, especially given
> CameraConfiguration that is often referred to as just "configuration".
> How about ConfigurationFile ?

ConfigurationFile sounds good.


> 
>>>>>> +{
>>>>>> +public:
>>>>>> +	int open(std::string filename);
> 
> const &
> 
> s/open/parse/ ? I also wonder if we couldn't just have a static parse()
> function that would return a json object.

parse sounds good actually. I thought the open/then data() felt a bit
awkward.


> 
>>>>>> +
>>>>>> +	json &data() { return json_; }
> 
> Ooohhhh so json is a class and contains member types. It's not very nice
> to write json without a prefix, and json::parse() or json::iterator that
> looks like namespace-qualified names :-S

Well actually its:

nlohmann_json::json::iterator

I shorten it above with:

using json = nlohmann::json;

as prefixing nlohmann everywhere felt quite heavyweight.

>>>>>> +
>>>>>> +private:
>>>>>> +	std::string findFile(std::string filename);
> 
> const &
> 
>>>>>> +
>>>>>> +	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>
> 
> This should go above the previous three headers.
> 
>>>>>> +
>>>>>> +/**
>>>>>> + * \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.
>>>>>> + */
> 
> I don't think thins should be part of this class. I'd rather have a
> JSON parser that only parses files, and deal with paths externally,
> especially given that different types of configuration files may be

Ok - so I think that's the split.

To me this Configuration class was dealing with obtaining the file
locations, and the json library was dealing with the parsing (as it was
included in the components that use it).


> stored in different locations. For instance I expect we'll have a global
> configuration file stored in /etc/libcamera/ with an override in
> $HOME/.libcamera/, IPA configuration files in /usr/share/libcamera/
> (likely with overrides, including through an environment variable), and
> probably more. The mechanisms to locate files will differ, and shouldn't
> be in scope for the parser.

No - but that is precisely what is being handled by findFile().

So really, the semantics you're saying is that findFile should be separated.

If so - where would you like it to be ?


> 
>>>>>> +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;
>>>>>> +	}
>>>>>> +
>>
>> We might also want to expose an environment variable to add to the paths
>> here, as we do for IPA configuration files (LIBCAMERA_IPA_CONFIG_PATH)
>>
>>
>> Hrm ... I should tackle the IPA configuration files as part of this
>> series too.
>>
>> It looks like that code path isn't really being used much yet though -
>> except for a dummy file at src/ipa/vimc/data/vimc.conf
>>
>> More things for me to look at ;-)
>>
>>>>>> +	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',
>

Hi Kieran,

On Mon, Dec 07, 2020 at 04:32:38PM +0000, Kieran Bingham wrote:
> On 01/12/2020 18:02, Laurent Pinchart wrote:
> > On Wed, Nov 25, 2020 at 10:37:15AM +0000, Kieran Bingham wrote:
> >> On 25/11/2020 10:18, Jacopo Mondi wrote:
> >>> On Wed, Nov 25, 2020 at 09:11:10AM +0000, Kieran Bingham wrote:
> >>>> On 25/11/2020 08:52, Jacopo Mondi wrote:
> >>>>> 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@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.
> > 
> > We also have the option of using a C library, as we wrap it in a custom
> > class anyway. https://github.com/json-c/json-c/wiki and
> > https://www.digip.org/jansson/ come to mind, there are probably other
> > options.  I'm not advocating switching to a different library, just
> > wanted to make sure that all options are known by everybody.
> 
> You know, I hadn't actually considered starting at a c-library (because
> we're in C++), so I hadn't looked at those.

I would have gone for a C++ library too, it certainly makes sense. I
wonder why none of the available libraries seem to have been developed
in a sensical way though. And I also wonder if people are telling the
same about libcamera :-)

> >>>> 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.
> > 
> > YAML is more complicated, otherwise I would have proposed going directly
> > for it (I *really* dislike how JSON doesn't support comments).
> 
> Comments are always helpful anywhere.
> Restricting them is certainly frustrating.

I wonder if some parsers may support comments, even if they're not
standard. I guess we could preprocess too (for instance if a parser
takes its input from an istream, we could create an ifstream that opens
the file, and wraps it in an istream that filters comments out).

> >>>>> 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.
> >>>
> >>> I would go in that direction too probably
> > 
> > That or using a wrap, yes. Relying on the distro version is likely not a
> > good idea if we can't rely on a recent enough version. It however also
> > means that we'll be responsible for tracking upstream changes
> > (especially security fixes) and bringing them in.
> > 
> >>>>> 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...)
> >>>
> >>> In general, knowing we can rely on known version would bring quite
> >>> some peace of mind in regard to versioning and available features
> >>>
> >>>>> 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.
> >>>
> >>> Right, 3.7.0 as I read it
> >>>
> >>>> 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.
> >>>
> >>> Indeed, I'm not aware of what the process is to do so though
> >>>
> >>>> 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.
> >>>
> >>> Probably so. I'm thinking of a use case where all the dependencies and
> >>> libcamera sources are packaged on a rootfs and -then- libcamera is
> >>> built on a live target, but I hardly find one.
> > 
> > I believe meson wrap can be pointed to a cache directory with
> > pre-downloaded zip files. That would be my preference, if we go for a
> > wrap.
> > 
> >>>>> 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 ;-)
> > 
> > That won't change much though, we'll still link against MIT code,
> > regardless of whether we carry a copy of it or not. The MIT license is
> > compatible with the LGPL as far as I can tell, so that's not an issue.
> > 
> > If we want to use an LGPL JSON parser, there's json-glib, but I don't
> > think that's a good idea :-) I'll also mention simdjson, just to share
> > the "interesting" idea of SIMD-accelerated JSON parsing.
> 
> https://github.com/zserge/jsmn/
> 
> (jasmine == Json Minimalist) also looks quite reasonable as a c-parser
> 
> But we could pull in any of a hundred external libraries. The fact is,
> what we want isn't provided by the standard libraries, so we pull in
> something else or write our own. And I don't intend to write a full json
> parser.
> 
> Though I must say, jsmn does it in 400 lines of header, so JSON really
> is quite simple ;-)

My go for a 400 lines implementation when there's a 1MB header available
to do the same :-)

> >>>> 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 ;-)
> >>>
> >>> You've done the background investigations, so I assume you chose
> >>> nlohmann_json for valid reasons. Also, jsoncpp seems not to be an
> >>
> >> Part of the reasons were that nlohmann is also reported to have better
> >> performances.
> >>
> >>> header only library, so integration might become more problematic.
> >>> They're both MIT-licensed project, in one case we will need to only
> >>> include un-touched MIT-licensed code headers in LGPL code. If I'm not
> >>> mistaken with json-cpp we would need to link against it, so either
> >>> build it as a .so or rely on pre-packaged versions (unless we go the
> >>> 'amalgamated' way:
> >>> https://github.com/open-source-parsers/jsoncpp/wiki/Amalgamated-(Possibly-outdated)
> >>> but we would have to build it ourselves to obtain the amalgamated
> >>> version)
> >>
> >> I hadn't looked at any of the amalgamated options. I assumed we'd have
> >> to link against it.
> >>
> >> From a license point of view, I don't think there's any conflict for
> >> using an MIT licences either by linking, or by including the headers.
> >>
> >>> Is this your understanding as well ?
> >>
> >> Close enough.
> >>
> >>>>>>  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>
> > 
> > Is this needed ?
> 
> Not anymore,
> 
> >>>>>> +#include <string>
> >>>>>> +
> >>>>>> +/* https://nlohmann.github.io/json/home/exceptions/#switch-off-exceptions */
> >>>>>> +#define JSON_NOEXCEPTION 1
> >>>>>> +#include <nlohmann/json.hpp>
> > 
> > This is what I was hoping to avoid :-S We're pulling nearly 1MB of
> > header in every file that includes configuration.h. As everything is
> > inline in headers, it will mean quite a bit of code duplication. At
> > least the parser is isolated in a .cpp file and won't get duplicated
> > (but will still get parsed by the compiler, with some impact on build
> > time), but access to the data will always be inlined :-(
> 
> Ok, so then we'll have to create our own 'types' or structures to pass
> around or store data tokens.
> 
> I went this route to avoid exactly that in the end. But I guess I'm back
> to the other path.

That's the annoying part, I agree. We have done the same for
ControlValue, and at some point I wonder if we'll end up with a Value
class that will be used in different parts of libcamera. It's not
something to worry about now in any case.

> >>>>>> +
> >>>>>> +using json = nlohmann::json;
> >>>>>> +
> >>>>>> +namespace libcamera {
> >>>>>> +
> >>>>>> +class Configuration
> > 
> > The name Configuration is very generic, especially given
> > CameraConfiguration that is often referred to as just "configuration".
> > How about ConfigurationFile ?
> 
> ConfigurationFile sounds good.
> 
> >>>>>> +{
> >>>>>> +public:
> >>>>>> +	int open(std::string filename);
> > 
> > const &
> > 
> > s/open/parse/ ? I also wonder if we couldn't just have a static parse()
> > function that would return a json object.
> 
> parse sounds good actually. I thought the open/then data() felt a bit
> awkward.
> 
> >>>>>> +
> >>>>>> +	json &data() { return json_; }
> > 
> > Ooohhhh so json is a class and contains member types. It's not very nice
> > to write json without a prefix, and json::parse() or json::iterator that
> > looks like namespace-qualified names :-S
> 
> Well actually its:
> 
> nlohmann_json::json::iterator
> 
> I shorten it above with:
> 
> using json = nlohmann::json;
> 
> as prefixing nlohmann everywhere felt quite heavyweight.

I wish the nlohmann parser were split from the property tree
implementation :-S

> >>>>>> +
> >>>>>> +private:
> >>>>>> +	std::string findFile(std::string filename);
> > 
> > const &
> > 
> >>>>>> +
> >>>>>> +	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>
> > 
> > This should go above the previous three headers.
> > 
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * \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.
> >>>>>> + */
> > 
> > I don't think thins should be part of this class. I'd rather have a
> > JSON parser that only parses files, and deal with paths externally,
> > especially given that different types of configuration files may be
> 
> Ok - so I think that's the split.
> 
> To me this Configuration class was dealing with obtaining the file
> locations, and the json library was dealing with the parsing (as it was
> included in the components that use it).
> 
> > stored in different locations. For instance I expect we'll have a global
> > configuration file stored in /etc/libcamera/ with an override in
> > $HOME/.libcamera/, IPA configuration files in /usr/share/libcamera/
> > (likely with overrides, including through an environment variable), and
> > probably more. The mechanisms to locate files will differ, and shouldn't
> > be in scope for the parser.
> 
> No - but that is precisely what is being handled by findFile().
> 
> So really, the semantics you're saying is that findFile should be separated.
> 
> If so - where would you like it to be ?

Very good question :-) I see two options:

- We have some logic in IPAProxy::configurationFile() already, and we
  could extend other classes to support a similar function. For a global
  configuration file, that would likely be the CameraManager. A HAL
  configuration file would be handled in the HAL itself.

- Thinking more about this, maybe this class isn't such a bad location
  to store this code, in which case we would need to move
  IPAProxy::configurationFile() here, and likely add a configuration
  file type enum parameter to the findFile() function.

The latter could actually allow us to implement configuration file
merging, where asking the ConfigurationFile class to parse, for
instance, the global libcamera.json, would look for it in
/etc/libcamera/ first, and then would overwrite values with
$HOME/.libcamera/libcamera.json. I'm not sure if that would end up being
useful, but it seems like a nice feature.

> >>>>>> +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;
> >>>>>> +	}
> >>>>>> +
> >>
> >> We might also want to expose an environment variable to add to the paths
> >> here, as we do for IPA configuration files (LIBCAMERA_IPA_CONFIG_PATH)
> >>
> >> Hrm ... I should tackle the IPA configuration files as part of this
> >> series too.
> >>
> >> It looks like that code path isn't really being used much yet though -
> >> except for a dummy file at src/ipa/vimc/data/vimc.conf
> >>
> >> More things for me to look at ;-)
> >>
> >>>>>> +	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',