[libcamera-devel] [PATCH v7 08/14] ipa: raspberrypi: Use YamlParser to replace dependency on boost
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 27 19:10:28 CEST 2022
Hi Naush,
On Wed, Jul 27, 2022 at 01:27:21PM +0100, Naushir Patuck wrote:
> On Wed, 27 Jul 2022 at 03:38, Laurent Pinchart wrote:
>
> > 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>
> > ---
> > Changes since v6:
> >
> > - Propagate tuning data read errors
> > ---
> > README.rst | 6 -
> > src/ipa/raspberrypi/controller/algorithm.cpp | 2 +-
> > src/ipa/raspberrypi/controller/algorithm.h | 6 +-
> > src/ipa/raspberrypi/controller/controller.cpp | 27 ++--
> > src/ipa/raspberrypi/controller/pwl.cpp | 12 +-
> > src/ipa/raspberrypi/controller/pwl.h | 4 +-
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 136 ++++++++++--------
> > src/ipa/raspberrypi/controller/rpi/agc.h | 10 +-
> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 105 +++++++-------
> > src/ipa/raspberrypi/controller/rpi/alsc.h | 2 +-
> > src/ipa/raspberrypi/controller/rpi/awb.cpp | 134 ++++++++++-------
> > src/ipa/raspberrypi/controller/rpi/awb.h | 8 +-
> > .../controller/rpi/black_level.cpp | 12 +-
> > .../raspberrypi/controller/rpi/black_level.h | 2 +-
> > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 47 +++---
> > src/ipa/raspberrypi/controller/rpi/ccm.h | 4 +-
> > .../raspberrypi/controller/rpi/contrast.cpp | 28 ++--
> > src/ipa/raspberrypi/controller/rpi/contrast.h | 2 +-
> > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 7 +-
> > src/ipa/raspberrypi/controller/rpi/dpc.h | 2 +-
> > src/ipa/raspberrypi/controller/rpi/geq.cpp | 10 +-
> > src/ipa/raspberrypi/controller/rpi/geq.h | 2 +-
> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 30 +++-
> > src/ipa/raspberrypi/controller/rpi/lux.h | 2 +-
> > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 +-
> > src/ipa/raspberrypi/controller/rpi/noise.h | 2 +-
> > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 6 +-
> > src/ipa/raspberrypi/controller/rpi/sdn.h | 2 +-
> > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +-
> > src/ipa/raspberrypi/controller/rpi/sharpen.h | 2 +-
> > src/ipa/raspberrypi/meson.build | 1 -
> > src/ipa/raspberrypi/raspberrypi.cpp | 1 +
> > 32 files changed, 362 insertions(+), 274 deletions(-)
[snip]
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp b/src/ipa/raspberrypi/controller/pwl.cpp
> > index fde0b298c6ce..8e6201920062 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > @@ -12,13 +12,15 @@
> >
> > using namespace RPiController;
> >
> > -int Pwl::read(boost::property_tree::ptree const ¶ms)
> > +int 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);
>
> This would be a candidate to not provide a default value and test
> the std::optional validity. If we have an odd number of points, that
> would be a malformed PWL.
You're right, that was a leftover of the previous version. I'll add
error checks.
> > points_.push_back(Point(x, y));
> > }
> > assert(points_.size() >= 2);
[snip]
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > index 10c49c126341..d6d312993cdd 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
[snip]
> > -static int readCtCurve(Pwl &ctR, Pwl &ctB,
> > - boost::property_tree::ptree const ¶ms)
> > +static int readCtCurve(Pwl &ctR, Pwl &ctB, const libcamera::YamlObject ¶ms)
> > {
> > - int num = 0;
> > - for (auto it = params.begin(); it != params.end(); it++) {
> > - double ct = it->second.get_value<double>();
> > - assert(it == params.begin() || ct != ctR.domain().end);
> > - if (++it == params.end()) {
> > - LOG(RPiAwb, Error) << "AwbConfig: incomplete CT curve entry";
> > - return -EINVAL;
> > - }
> > - ctR.append(ct, it->second.get_value<double>());
> > - if (++it == params.end()) {
> > - LOG(RPiAwb, Error) << "AwbConfig: incomplete CT curve entry";
> > - return -EINVAL;
> > - }
> > - ctB.append(ct, it->second.get_value<double>());
> > - num++;
> > + if (params.size() % 3) {
> > + LOG(RPiAwb, Error) << "AwbConfig: incomplete CT curve entry";
> > + return -EINVAL;
> > }
> > - if (num < 2) {
> > +
> > + if (params.size() < 6) {
> > LOG(RPiAwb, Error) << "AwbConfig: insufficient points in CT curve";
> > return -EINVAL;
> > }
> > +
> > + const auto &list = params.asList();
> > +
> > + for (auto it = list.begin(); it != list.end(); it++) {
> > + auto value = it->get<double>();
> > + if (!value)
> > + return -EINVAL;
> > + double ct = *value;
> > +
> > + assert(it == list.begin() || ct != ctR.domain().end);
> > +
> > + value = (++it)->get<double>();
> > + if (!value)
> > + return -EINVAL;
> > + ctR.append(ct, *value);
> > +
> > + value = (++it)->get<double>();
> > + if (!value)
> > + return -EINVAL;
> > + ctB.append(ct, *value);
> > + }
>
> The if (!value) tests are probably unneeded in this loop as you test
> if (params.size() % 3) earlier. Or are we also guarding against values
> that may not be numeric, in which case, the test is needed?
That was the idea, yes. I don't want to write
ctR.append(ct, *(++it)->get<double>());
as it will crash if the value isn't convertible to double. If you think
the only problem we should protect against is an incorrect number of
values, but incorrect values should be blamed on the user and silently
ignored, then I would write
ctR.append(ct, (++it)->get<double>(0.0));
What would you prefer ? I'll apply the same to the Pwl::read() above,
and will either only test that the number of elements is even, or test
each element, and to Matrix::read() below.
> > +
> > return 0;
> > }
[snip]
> > diff --git a/src/ipa/raspberrypi/controller/rpi/ccm.cpp b/src/ipa/raspberrypi/controller/rpi/ccm.cpp
> > index 9588e94adeb1..2e2e66647e86 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/ccm.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/ccm.cpp
> > @@ -39,21 +39,22 @@ Matrix::Matrix(double m0, double m1, double m2, double m3, double m4, double m5,
> > m[0][0] = m0, m[0][1] = m1, m[0][2] = m2, m[1][0] = m3, m[1][1] = m4,
> > m[1][2] = m5, m[2][0] = m6, m[2][1] = m7, m[2][2] = m8;
> > }
> > -int Matrix::read(boost::property_tree::ptree const ¶ms)
> > +int Matrix::read(const libcamera::YamlObject ¶ms)
> > {
> > double *ptr = (double *)m;
> > - int n = 0;
> > - for (auto it = params.begin(); it != params.end(); it++) {
> > - if (n++ == 9) {
> > - LOG(RPiCcm, Error) << "Too many values in CCM";
> > - return -EINVAL;
> > - }
> > - *ptr++ = it->second.get_value<double>();
> > - }
> > - if (n < 9) {
> > - LOG(RPiCcm, Error) << "Too few values in CCM";
> > +
> > + if (params.size() != 9) {
> > + LOG(RPiCcm, Error) << "Wrong number of values in CCM";
> > return -EINVAL;
> > }
> > +
> > + for (const auto ¶m : params.asList()) {
> > + auto value = param.get<double>();
> > + if (!value)
> > + return -EINVAL;
> > + *ptr++ = *value;
> > + }
> > +
>
> Again, if (!value) not needed here (with the same provision as earlier)?
>
> > return 0;
> > }
> >
[snip]
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > index ca1e827191fd..ea1623dda345 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp
> > @@ -38,14 +38,30 @@ char const *Lux::name() const
> > return NAME;
> > }
> >
> > -int Lux::read(boost::property_tree::ptree const ¶ms)
> > +int Lux::read(const libcamera::YamlObject ¶ms)
> > {
> > - referenceShutterSpeed_ =
> > - params.get<double>("reference_shutter_speed") * 1.0us;
> > - referenceGain_ = params.get<double>("reference_gain");
> > - referenceAperture_ = params.get<double>("reference_aperture", 1.0);
> > - referenceY_ = params.get<double>("reference_Y");
> > - referenceLux_ = params.get<double>("reference_lux");
> > + auto value = params["reference_shutter_speed"].get<double>();
> > + if (!value)
> > + return -EINVAL;
> > + referenceShutterSpeed_ = *value * 1.0us;
> > +
> > + value = params["reference_gain"].get<double>();
> > + if (!value)
> > + return -EINVAL;
> > + referenceGain_ = *value;
> > +
> > + referenceAperture_ = params["reference_aperture"].get<double>(1.0);
> > +
> > + value = params["reference_Y"].get<double>(0.0);
> > + if (!value)
> > + return -EINVAL;
> > + referenceY_ = *value;
> > +
> > + value = params["reference_lux"].get<double>(0.0);
> > + if (!value)
> > + return -EINVAL;
> > + referenceLux_ = *value;
> > +
>
> reference_Y and reference_lux should not take in default values above.
I'll fix that too.
> > currentAperture_ = referenceAperture_;
> > return 0;
> > }
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list