| Message ID | 20240613013944.23344-11-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@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{}); > 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{}); > 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{}); > return 0; > } >
Quoting Laurent Pinchart (2024-06-13 02:39:43) > 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@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{}); I look forward to your resurrection of the default constructor candidate in the optionals! > 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; Utlimately this is the biggest change here. Instead of returning an error - it will return a default construction or the result. Pwl is fine here as we can check that it's empty or not - but I wonder what happens on more complex structures like a Matrix where we default construct to an identity ... How would that be handled? It seems because Vector::readYaml wasn't used yet - I can't see an equivelent implemention for that concept. I'm curious how we make this expressive for the three conditions though: 1. Get succeeds, value returned. 2. Get fails desired value returned. 3. Get fails Do not want a default value, and knowing it failed is important. Is 3. a condition we will require? But ... that aside - this works for Pwl ... so on that basis ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > 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{}); > 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{}); > return 0; > } > > -- > Regards, > > Laurent Pinchart >
On Thu, Jun 13, 2024 at 12:06:33PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2024-06-13 02:39:43) > > 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@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{}); > > I look forward to your resurrection of the default constructor candidate > in the optionals! Me too :-) The above would then be written map = params["map"].get<ipa::Pwl>().value_or(utils::defopt); I'll let you decide if you like it better or not. > > 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; > > Utlimately this is the biggest change here. Instead of returning an > error - it will return a default construction or the result. Pwl is fine > here as we can check that it's empty or not - but I wonder what happens > on more complex structures like a Matrix where we default construct to > an identity ... > > How would that be handled? With a tiny bit more code: std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>(); if (!pwl) return -EINVAL; yTarget = *pwl; > It seems because Vector::readYaml wasn't used yet - I can't see an > equivelent implemention for that concept. > > I'm curious how we make this expressive for the three conditions though: > > 1. Get succeeds, value returned. std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>(); if (pwl) /* Success */ For types that include a way to check if they are valid, you can also ipa::Pwl pwl = params["y_target"].get<ipa::Pwl>(ipa::Pwl{}); if (!pwl.empty()) /* Success */ > 2. Get fails desired value returned. I assume you mean default value returned ? ipa::Pwl pwl = params["y_target"].get<ipa::Pwl>(ipa::Pwl{}); > 3. Get fails Do not want a default value, and knowing it failed is > important. std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>(); if (!pwl) /* Failure */ > > Is 3. a condition we will require? > > But ... that aside - this works for Pwl ... so on that basis ... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > } > > > > 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{}); > > 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{}); > > return 0; > > } > >
Quoting Laurent Pinchart (2024-06-13 12:17:29) > On Thu, Jun 13, 2024 at 12:06:33PM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2024-06-13 02:39:43) > > > 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@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{}); > > > > I look forward to your resurrection of the default constructor candidate > > in the optionals! > > Me too :-) The above would then be written > > map = params["map"].get<ipa::Pwl>().value_or(utils::defopt); > > I'll let you decide if you like it better or not. Hrm ... it's ... longer ;-) I thought it would mean we could write: map = params["map"].get<ipa::Pwl>(); But oh well, lets see later. > > > > 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; > > > > Utlimately this is the biggest change here. Instead of returning an > > error - it will return a default construction or the result. Pwl is fine > > here as we can check that it's empty or not - but I wonder what happens > > on more complex structures like a Matrix where we default construct to > > an identity ... > > > > How would that be handled? > > With a tiny bit more code: > > std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>(); > if (!pwl) > return -EINVAL; > > yTarget = *pwl; > > > It seems because Vector::readYaml wasn't used yet - I can't see an > > equivelent implemention for that concept. > > > > I'm curious how we make this expressive for the three conditions though: > > > > 1. Get succeeds, value returned. > > std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>(); > if (pwl) > /* Success */ > > For types that include a way to check if they are valid, you can also > > ipa::Pwl pwl = params["y_target"].get<ipa::Pwl>(ipa::Pwl{}); > if (!pwl.empty()) > /* Success */ > > > 2. Get fails desired value returned. > > I assume you mean default value returned ? Yes, > > ipa::Pwl pwl = params["y_target"].get<ipa::Pwl>(ipa::Pwl{}); > > > 3. Get fails Do not want a default value, and knowing it failed is > > important. > > std::optional<ipa::Pwl> pwl = params["y_target"].get<ipa::Pwl>(); > if (!pwl) > /* Failure */ > Ok - perfect - all cases handled! Thanks for the clarification. -- Kieran
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@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{}); > 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{}); > 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{}); > return 0; > } >
Hi Laurent Thanks for the patch. On Mon, 17 Jun 2024 at 14:49, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@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...!) 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. > > 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@raspberrypi.com> Thanks David > > return 0; > > } > > > > -- > Regards, > > Laurent Pinchart
Hi David, 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@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@raspberrypi.com> Thank you. I'll test it on a Pi 4. > > > return 0; > > > } > > >
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@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@raspberrypi.com> > > Thank you. > > I'll test it on a Pi 4. Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > return 0; > > > > } > > > >
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{}); 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{}); 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{}); return 0; }
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@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(-)