[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:56:54 CEST 2022
Hi Naush,
On Thu, Jul 14, 2022 at 06:45:28PM +0300, Laurent Pinchart via libcamera-devel wrote:
> 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.
It's actually needed, dropping boost from headers removes indirect
inclusion of cstring, which is needed for a std::memcpy() call in this
file.
> > > #include <fcntl.h>
> > > #include <math.h>
> > > #include <stdint.h>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list