[libcamera-devel] [RFC PATCH 6/8] libcamera: Add configuration interface
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Dec 7 17:32:38 CET 2020
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 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.
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',
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list