[libcamera-devel] [RFC PATCH 6/8] libcamera: Add configuration interface
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Nov 25 11:37:15 CET 2020
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 at ideasonboard.com>
>>>> ---
>>>> README.rst | 2 +-
>>>> include/libcamera/internal/configuration.h | 37 +++++++++
>>>> src/libcamera/configuration.cpp | 91 ++++++++++++++++++++++
>>>> src/libcamera/meson.build | 1 +
>>>> 4 files changed, 130 insertions(+), 1 deletion(-)
>>>> create mode 100644 include/libcamera/internal/configuration.h
>>>> create mode 100644 src/libcamera/configuration.cpp
>>>>
>>>> diff --git a/README.rst b/README.rst
>>>> index 251291b77c62..c09e262fcc43 100644
>>>> --- a/README.rst
>>>> +++ b/README.rst
>>>> @@ -58,7 +58,7 @@ Meson Build system: [required]
>>>> pip3 install --user --upgrade meson
>>>>
>>>> for the libcamera core: [required]
>>>> - python3-yaml python3-ply python3-jinja2
>>>> + python3-yaml python3-ply python3-jinja2 nlohmann-json3-dev
>>>
>>> First question I have is about the dependency on this package.
>>>
>>> I have no real reasons to doubt about the availability of a packaged
>>> version in most linux distro, nor about their stability and update
>>> rate. It might be that projects named after a single person always
>>> sound fragile to me, might be the rather low bus factor :)
>>
>>
>> Yes, I find the 'named after me' packaging a little ... awkward. But
>> there's really two choices for json libraries if we don't want boost.
>> nlohmann-json and json-cpp.
>>
>> For bus factor, nlohmann-json has 178 contributors ... so I don't think
>> it's just 'one guy'.
>>
>> I had planned to try both in earnest, and subclass Configuration so that
>> they could be swapped at will, but the nlohmann was seemingly easier to
>> get started with in the end, and subclassing to swap would mean wrapping
>> the nlohmann generic container types in a generic container type and
>> creating my own tree structures.
>>
>> We can do that of course, but that will take longer, and I honestly
>> couldn't see the point of wrapping it for the internal usages (yet).
>>
>> If this was public API then it would be different.
>>
>> And maybe we'll update/change later - but I wanted to get things
>> actually going so we can start figuring out what and where we store data
>> in the first place.
>>
>> Who knows, maybe Configuration() will sometime need to parse yaml files
>> so it will build a ConfigurationJSON or a ConfigurationYAML depending on
>> the data - but I don't want to over engineer just to be able to read
>> some key-values at this stage.
>>
>>> Joking aside this seems to be packaged in most distros
>>> https://repology.org/project/nlohmann-json/versions
>>> with rather different versions (3.9.1 on Arch, 2.1.1 on Ubuntu 18.04
>>> which is an LTS if I'm not mistaken).
>>
>> It did seem to be quite well packaged, but I would probably lean towards
>> bringing the headers in under third_party/ or such as it's header only
>> to prevent any potential issues.
>>
>
> 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 at lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> --
>> Regards
>> --
>> Kieran
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list