[PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with YamlObject::get()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 17 23:57:38 CEST 2024
On Mon, Jun 17, 2024 at 07:23:32PM +0300, Laurent Pinchart wrote:
> On Mon, Jun 17, 2024 at 03:40:37PM +0100, David Plowman wrote:
> > On Mon, 17 Jun 2024 at 14:49, Laurent Pinchart wrote:
> > > Hi Naush, David,
> > >
> > > Could you give this patch a review when you get the chance ?
> > >
> > > On Thu, Jun 13, 2024 at 04:39:43AM +0300, Laurent Pinchart wrote:
> > > > Now that deserializing a Pwl object from YAML data is possible using the
> > > > YamlObject::get() function, replace all usage of Pwl::readYaml() to
> > > > prepare for its removal.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > > src/ipa/rpi/controller/rpi/af.cpp | 2 +-
> > > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----
> > > > src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-
> > > > src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---
> > > > src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-
> > > > src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---
> > > > src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--
> > > > src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-
> > > > 8 files changed, 20 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp
> > > > index 304629d6d4e5..5ca76dd98b4b 100644
> > > > --- a/src/ipa/rpi/controller/rpi/af.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/af.cpp
> > > > @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)
> > > > readNumber<uint32_t>(skipFrames, params, "skip_frames");
> > > >
> > > > if (params.contains("map"))
> > > > - map.readYaml(params["map"]);
> > > > + map = params["map"].get<ipa::Pwl>(ipa::Pwl{});
> >
> > Just wondering how this works... YamlObject::get calls
> > YamlObject::Getter::get, and if the latter returns std::nullopt, then
> > the former returns the value that was passed in? (Scratching my head
> > ever so slightly...!)
>
> Correct, the YamlObject::get<T>(U &def) function return 'def' when the
> std::optional<T> is empty.
>
> > Looking at this, I suspect we should have been checking the return
> > code from readYaml (equivalently, an empty Pwl from get), but that's
> > clearly a thing for later, and not for this patch.
>
> I believe so. I went for bug-to-bug compatibility here :-)
>
> > > > else
> > > > LOG(RPiAf, Warning) << "No map defined";
> > > >
> > > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > > > index a381dd972215..cf2565a83836 100644
> > > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > > > @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)
> > > > return -EINVAL;
> > > > qHi = *value;
> > > >
> > > > - return yTarget.readYaml(params["y_target"]);
> > > > + yTarget = params["y_target"].get<ipa::Pwl>(ipa::Pwl{});
> > > > + return yTarget.empty() ? -EINVAL : 0;
> > > > }
> > > >
> > > > static std::tuple<int, AgcConstraintMode>
> > > > @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)
> > > > return ret;
> > > > }
> > > >
> > > > - ret = yTarget.readYaml(params["y_target"]);
> > > > - if (ret)
> > > > - return ret;
> > > > + yTarget = params["y_target"].get<ipa::Pwl>(ipa::Pwl{});
> > > > + if (yTarget.empty())
> > > > + return -EINVAL;
> > > >
> > > > speed = params["speed"].get<double>(0.2);
> > > > startupFrames = params["startup_frames"].get<uint16_t>(10);
> > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> > > > index 603953d7d863..003c8fa137f3 100644
> > > > --- a/src/ipa/rpi/controller/rpi/awb.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> > > > @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)
> > > > return -EINVAL;
> > > > lux = *value;
> > > >
> > > > - return prior.readYaml(params["prior"]);
> > > > + prior = params["prior"].get<ipa::Pwl>(ipa::Pwl{});
> > > > + return prior.empty() ? -EINVAL : 0;
> > > > }
> > > >
> > > > static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)
> > > > diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp
> > > > index 3272a1416ffa..e673964c1856 100644
> > > > --- a/src/ipa/rpi/controller/rpi/ccm.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/ccm.cpp
> > > > @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)
> > > > int ret;
> > > >
> > > > if (params.contains("saturation")) {
> > > > - ret = config_.saturation.readYaml(params["saturation"]);
> > > > - if (ret)
> > > > - return ret;
> > > > + config_.saturation = params["saturation"].get<ipa::Pwl>(ipa::Pwl{});
> > > > + if (config_.saturation.empty())
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > for (auto &p : params["ccms"].asList()) {
> > > > diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp
> > > > index 66871a61ed28..9b37943ae9c9 100644
> > > > --- a/src/ipa/rpi/controller/rpi/contrast.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/contrast.cpp
> > > > @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)
> > > > config_.hiHistogram = params["hi_histogram"].get<double>(0.95);
> > > > config_.hiLevel = params["hi_level"].get<double>(0.95);
> > > > config_.hiMax = params["hi_max"].get<double>(2000);
> > > > - return config_.gammaCurve.readYaml(params["gamma_curve"]);
> > > > +
> > > > + config_.gammaCurve = params["gamma_curve"].get<ipa::Pwl>(ipa::Pwl{});
> > > > + return config_.gammaCurve.empty() ? -EINVAL : 0;
> > > > }
> > > >
> > > > void Contrast::setBrightness(double brightness)
> > > > diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp
> > > > index c9c38ebff5ba..40e7191ba16a 100644
> > > > --- a/src/ipa/rpi/controller/rpi/geq.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/geq.cpp
> > > > @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)
> > > > }
> > > >
> > > > if (params.contains("strength")) {
> > > > - int ret = config_.strength.readYaml(params["strength"]);
> > > > - if (ret)
> > > > - return ret;
> > > > + config_.strength = params["strength"].get<ipa::Pwl>(ipa::Pwl{});
> > > > + if (config_.strength.empty())
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > return 0;
> > > > diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp
> > > > index d533a4ea4e65..f3da8291bf5d 100644
> > > > --- a/src/ipa/rpi/controller/rpi/hdr.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp
> > > > @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod
> > > >
> > > > /* Lens shading related parameters. */
> > > > if (params.contains("spatial_gain_curve")) {
> > > > - spatialGainCurve.readYaml(params["spatial_gain_curve"]);
> > > > + spatialGainCurve = params["spatial_gain_curve"].get<ipa::Pwl>(ipa::Pwl{});
> > > > } else if (params.contains("spatial_gain")) {
> > > > double spatialGain = params["spatial_gain"].get<double>(2.0);
> > > > spatialGainCurve.append(0.0, spatialGain);
> > > > @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod
> > > > iirStrength = params["iir_strength"].get<double>(8.0);
> > > > strength = params["strength"].get<double>(1.5);
> > > > if (tonemapEnable)
> > > > - tonemap.readYaml(params["tonemap"]);
> > > > + tonemap = params["tonemap"].get<ipa::Pwl>(ipa::Pwl{});
> >
> > Hmm, probably more return codes that weren't being checked...
> >
> > > > speed = params["speed"].get<double>(1.0);
> > > > if (params.contains("hi_quantile_targets")) {
> > > > hiQuantileTargets = params["hi_quantile_targets"].getList<double>().value();
> > > > diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp
> > > > index 2dc50dfc8d2d..3422adfe7dee 100644
> > > > --- a/src/ipa/rpi/controller/rpi/tonemap.cpp
> > > > +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp
> > > > @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)
> > > > config_.detailSlope = params["detail_slope"].get<double>(0.1);
> > > > config_.iirStrength = params["iir_strength"].get<double>(1.0);
> > > > config_.strength = params["strength"].get<double>(1.0);
> > > > - config_.tonemap.readYaml(params["tone_curve"]);
> > > > + config_.tonemap = params["tone_curve"].get<ipa::Pwl>(ipa::Pwl{});
> >
> > So yes, that all looks good to me. Possibly worth running it on a Pi
> > just to double check!
> >
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thank you.
>
> I'll test it on a Pi 4.
Tested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > return 0;
> > > > }
> > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list