[libcamera-devel] [PATCH v4 4/8] ipa: raspberrypi: Use YamlParser to replace dependency on boost
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 14 17:45:28 CEST 2022
Hi Naush,
On Wed, Jul 13, 2022 at 10:35:07AM +0100, Naushir Patuck wrote:
> On Wed, 13 Jul 2022 at 10:22, Naushir Patuck wrote:
>
> > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > The Raspberry Pi IPA module depends on boost only to parse the JSON
> > tuning data files. As libcamera depends on libyaml, use the YamlParser
> > class to parse those files and drop the dependency on boost.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > README.rst | 6 --
> > src/ipa/raspberrypi/controller/algorithm.cpp | 2 +-
> > src/ipa/raspberrypi/controller/algorithm.hpp | 6 +-
> > src/ipa/raspberrypi/controller/controller.cpp | 27 ++++--
> > src/ipa/raspberrypi/controller/pwl.cpp | 12 ++-
> > src/ipa/raspberrypi/controller/pwl.hpp | 5 +-
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 94 +++++++++----------
> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 10 +-
> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 94 +++++++++----------
> > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +-
> > src/ipa/raspberrypi/controller/rpi/awb.cpp | 89 +++++++++---------
> > src/ipa/raspberrypi/controller/rpi/awb.hpp | 8 +-
> > .../controller/rpi/black_level.cpp | 12 +--
> > .../controller/rpi/black_level.hpp | 2 +-
> > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 28 +++---
> > src/ipa/raspberrypi/controller/rpi/ccm.hpp | 4 +-
> > .../raspberrypi/controller/rpi/contrast.cpp | 18 ++--
> > .../raspberrypi/controller/rpi/contrast.hpp | 2 +-
> > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 4 +-
> > src/ipa/raspberrypi/controller/rpi/dpc.hpp | 2 +-
> > src/ipa/raspberrypi/controller/rpi/geq.cpp | 10 +-
> > src/ipa/raspberrypi/controller/rpi/geq.hpp | 2 +-
> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 +--
> > src/ipa/raspberrypi/controller/rpi/lux.hpp | 2 +-
> > src/ipa/raspberrypi/controller/rpi/noise.cpp | 6 +-
> > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +-
> > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 6 +-
> > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 2 +-
> > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +-
> > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +-
> > src/ipa/raspberrypi/meson.build | 1 -
> > src/ipa/raspberrypi/raspberrypi.cpp | 1 +
> > 32 files changed, 241 insertions(+), 240 deletions(-)
[snip]
> > diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp
> > index d3433ad2e7e8..67d650ef0c1b 100644
> > --- a/src/ipa/raspberrypi/controller/controller.cpp
> > +++ b/src/ipa/raspberrypi/controller/controller.cpp
> > @@ -5,14 +5,16 @@
> > * controller.cpp - ISP controller
> > */
> >
> > +#include <assert.h>
> > +
> > +#include <libcamera/base/file.h>
> > #include <libcamera/base/log.h>
> >
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > #include "algorithm.hpp"
> > #include "controller.hpp"
> >
> > -#include <boost/property_tree/json_parser.hpp>
> > -#include <boost/property_tree/ptree.hpp>
> > -
> > using namespace RPiController;
> > using namespace libcamera;
> >
> > @@ -32,16 +34,23 @@ Controller::~Controller() {}
> >
> > void Controller::Read(char const *filename)
> > {
> > - boost::property_tree::ptree root;
> > - boost::property_tree::read_json(filename, root);
> > - for (auto const &key_and_value : root) {
> > - Algorithm *algo = CreateAlgorithm(key_and_value.first.c_str());
> > + File file(filename);
> > + if (!file.open(File::OpenModeFlag::ReadOnly)) {
> > + LOG(RPiController, Warning)
> > + << "Failed to open tuning file '" << filename << "'";
> > + return;
> > + }
> > +
> > + std::unique_ptr<YamlObject> root = YamlParser::parse(file);
> > +
> > + for (auto const &[key, value] : root->asDict()) {
> > + Algorithm *algo = CreateAlgorithm(key.c_str());
> > if (algo) {
> > - algo->Read(key_and_value.second);
> > + algo->Read(value);
> > algorithms_.push_back(AlgorithmPtr(algo));
> > } else
> > LOG(RPiController, Warning)
> > - << "No algorithm found for \"" << key_and_value.first << "\"";
> > + << "No algorithm found for \"" << key << "\"";
> > }
> > }
> >
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp
> > b/src/ipa/raspberrypi/controller/pwl.cpp
> > index 130c820b559f..9c7bc94dd484 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > @@ -12,13 +12,15 @@
> >
> > using namespace RPiController;
> >
> > -void Pwl::Read(boost::property_tree::ptree const ¶ms)
> > +void Pwl::Read(const libcamera::YamlObject ¶ms)
> > {
> > - for (auto it = params.begin(); it != params.end(); it++) {
> > - double x = it->second.get_value<double>();
> > - assert(it == params.begin() || x > points_.back().x);
> > + const auto &list = params.asList();
> > +
> > + for (auto it = list.begin(); it != list.end(); it++) {
> > + double x = it->get<double>(0.0);
> > + assert(it == list.begin() || x > points_.back().x);
> > it++;
> > - double y = it->second.get_value<double>();
> > + double y = it->get<double>(0.0);
> >
>
> Only one minor point of concern here (and throughout this patch) for me is the
> use of default values. In the Boost parser, if a default value was not passed
> into the get_value() call, there would be an exception thrown. This behavior
> may be desirable since it would catch if some vital parameters were missing.
>
> With libyaml, we always provide default values so we cannot catch if vital
> parameters are missing. Perhaps we should use the "ok" return value to flag
> this? Not sure how much of a problem this will be in practice, perhaps David
> (when he is back from holiday) can give his thoughts?
That's a good point, and I think it makes sense to fail instead of
ignoring issues silently. Adding an &ok to all calls is one way, but
maybe there's a better way to express this in the YamlObject API. I'm
thinking about returning an std::optional<> from get() for instance.
> > points_.push_back(Point(x, y));
> > }
> > assert(points_.size() >= 2);
[snip]
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f8d37b876c54..952a6ace2911 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -7,6 +7,7 @@
> >
> > #include <algorithm>
> > #include <array>
> > +#include <cstring>
>
> Looks unrelated to this work.
> I'm ok to have it in this patch, but perhaps a comment in the commit
> message saying
> it's a drive-by?
Indeed. I'll double-check where this came from.
> > #include <fcntl.h>
> > #include <math.h>
> > #include <stdint.h>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list