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

[Laurent Pinchart]

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 at ideasonboard.com>
> >>>> ---
> >>>>  README.rst                                 |  2 +-
> >>>>  include/libcamera/internal/configuration.h | 37 +++++++++
> >>>>  src/libcamera/configuration.cpp            | 91 ++++++++++++++++++++++
> >>>>  src/libcamera/meson.build                  |  1 +
> >>>>  4 files changed, 130 insertions(+), 1 deletion(-)
> >>>>  create mode 100644 include/libcamera/internal/configuration.h
> >>>>  create mode 100644 src/libcamera/configuration.cpp
> >>>>
> >>>> diff --git a/README.rst b/README.rst
> >>>> index 251291b77c62..c09e262fcc43 100644
> >>>> --- a/README.rst
> >>>> +++ b/README.rst
> >>>> @@ -58,7 +58,7 @@ Meson Build system: [required]
> >>>>              pip3 install --user --upgrade meson
> >>>>
> >>>>  for the libcamera core: [required]
> >>>> -        python3-yaml python3-ply python3-jinja2
> >>>> +        python3-yaml python3-ply python3-jinja2 nlohmann-json3-dev
> >>>
> >>> First question I have is about the dependency on this package.
> >>>
> >>> I have no real reasons to doubt about the availability of a packaged
> >>> version in most linux distro, nor about their stability and update
> >>> rate. It might be that projects named after a single person always
> >>> sound fragile to me, might be the rather low bus factor :)
> >>
> >> Yes, I find the 'named after me' packaging a little ... awkward. But
> >> there's really two choices for json libraries if we don't want boost.
> >> nlohmann-json and json-cpp.

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',

-- 
Regards,

Laurent Pinchart