[libcamera-devel] [PATCH v7 08/14] ipa: raspberrypi: Use YamlParser to replace dependency on boost
Naushir Patuck
naush at raspberrypi.com
Wed Jul 27 19:28:16 CEST 2022
Hi Laurent,
On Wed, 27 Jul 2022, 6:10 pm Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
Feel free to leave the changes as is, I just wanted to check for my
understanding.
Naush
> > > +
> > > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220727/fcbbcf05/attachment.htm>
More information about the libcamera-devel
mailing list