[libcamera-devel] [PATCH v7 07/14] ipa: raspberrypi: Replace Fatal log by error propagation
Naushir Patuck
naush at raspberrypi.com
Wed Jul 27 12:54:54 CEST 2022
Hi Laurent,
Thank you for your patch.
On Wed, 27 Jul 2022 at 03:38, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Replace the Fatal log messages that cause an abort during tuning data
> read with Error messages and proper error propagation to the caller.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 36 ++++++++++----
> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 52 ++++++++++++++-------
> src/ipa/raspberrypi/controller/rpi/awb.cpp | 42 +++++++++++------
> src/ipa/raspberrypi/controller/rpi/ccm.cpp | 24 ++++++----
> src/ipa/raspberrypi/controller/rpi/dpc.cpp | 6 ++-
> src/ipa/raspberrypi/controller/rpi/geq.cpp | 6 ++-
> 6 files changed, 113 insertions(+), 53 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index 93f966a1d5ce..153493f8b8e2 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -34,13 +34,20 @@ static constexpr unsigned int PipelineBits = 13; /*
> seems to be a 13-bit pipelin
> int AgcMeteringMode::read(boost::property_tree::ptree const ¶ms)
> {
> int num = 0;
> +
> for (auto &p : params.get_child("weights")) {
> - if (num == AgcStatsSize)
> - LOG(RPiAgc, Fatal) << "AgcMeteringMode: too many
> weights";
> + if (num == AgcStatsSize) {
> + LOG(RPiAgc, Error) << "AgcMeteringMode: too many
> weights";
> + return -EINVAL;
> + }
> weights[num++] = p.second.get_value<double>();
> }
> - if (num != AgcStatsSize)
> - LOG(RPiAgc, Fatal) << "AgcMeteringMode: insufficient
> weights";
> +
> + if (num != AgcStatsSize) {
> + LOG(RPiAgc, Error) << "AgcMeteringMode: insufficient
> weights";
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> @@ -85,12 +92,19 @@ int AgcExposureMode::read(boost::property_tree::ptree
> const ¶ms)
> {
> int numShutters = readList(shutter, params.get_child("shutter"));
> int numAgs = readList(gain, params.get_child("gain"));
> - if (numShutters < 2 || numAgs < 2)
> - LOG(RPiAgc, Fatal)
> +
> + if (numShutters < 2 || numAgs < 2) {
> + LOG(RPiAgc, Error)
> << "AgcExposureMode: must have at least two
> entries in exposure profile";
> - if (numShutters != numAgs)
> - LOG(RPiAgc, Fatal)
> + return -EINVAL;
> + }
> +
> + if (numShutters != numAgs) {
> + LOG(RPiAgc, Error)
> << "AgcExposureMode: expect same number of
> exposure and gain entries in exposure profile";
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> @@ -120,8 +134,10 @@ int AgcConstraint::read(boost::property_tree::ptree
> const ¶ms)
> std::string boundString = params.get<std::string>("bound", "");
> transform(boundString.begin(), boundString.end(),
> boundString.begin(), ::toupper);
> - if (boundString != "UPPER" && boundString != "LOWER")
> - LOG(RPiAgc, Fatal) << "AGC constraint type should be UPPER
> or LOWER";
> + if (boundString != "UPPER" && boundString != "LOWER") {
> + LOG(RPiAgc, Error) << "AGC constraint type should be UPPER
> or LOWER";
> + return -EINVAL;
> + }
> bound = boundString == "UPPER" ? Bound::UPPER : Bound::LOWER;
> qLo = params.get<double>("q_lo");
> qHi = params.get<double>("q_hi");
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index b36277696a52..49aaf6b74603 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -53,11 +53,17 @@ char const *Alsc::name() const
> static int generateLut(double *lut, boost::property_tree::ptree const
> ¶ms)
> {
> double cstrength = params.get<double>("corner_strength", 2.0);
> - if (cstrength <= 1.0)
> - LOG(RPiAlsc, Fatal) << "Alsc: corner_strength must be >
> 1.0";
> + if (cstrength <= 1.0) {
> + LOG(RPiAlsc, Error) << "corner_strength must be > 1.0";
> + return -EINVAL;
> + }
> +
> double asymmetry = params.get<double>("asymmetry", 1.0);
> - if (asymmetry < 0)
> - LOG(RPiAlsc, Fatal) << "Alsc: asymmetry must be >= 0";
> + if (asymmetry < 0) {
> + LOG(RPiAlsc, Error) << "asymmetry must be >= 0";
> + return -EINVAL;
> + }
> +
> double f1 = cstrength - 1, f2 = 1 + sqrt(cstrength);
> double R2 = X * Y / 4 * (1 + asymmetry * asymmetry);
> int num = 0;
> @@ -78,13 +84,19 @@ static int readLut(double *lut,
> boost::property_tree::ptree const ¶ms)
> {
> int num = 0;
> const int maxNum = XY;
> +
> for (auto &p : params) {
> - if (num == maxNum)
> - LOG(RPiAlsc, Fatal) << "Alsc: too many entries in
> LSC table";
> + if (num == maxNum) {
> + LOG(RPiAlsc, Error) << "Too many entries in LSC
> table";
> + return -EINVAL;
> + }
> lut[num++] = p.second.get_value<double>();
> }
> - if (num < maxNum)
> - LOG(RPiAlsc, Fatal) << "Alsc: too few entries in LSC
> table";
> +
> + if (num < maxNum) {
> + LOG(RPiAlsc, Error) << "Too few entries in LSC table";
> + return -EINVAL;
> + }
> return 0;
> }
>
> @@ -96,24 +108,30 @@ static int
> readCalibrations(std::vector<AlscCalibration> &calibrations,
> double lastCt = 0;
> for (auto &p : params.get_child(name)) {
> double ct = p.second.get<double>("ct");
> - if (ct <= lastCt)
> - LOG(RPiAlsc, Fatal)
> - << "Alsc: entries in " << name <<
> " must be in increasing ct order";
> + if (ct <= lastCt) {
> + LOG(RPiAlsc, Error)
> + << "Entries in " << name << " must
> be in increasing ct order";
> + return -EINVAL;
> + }
> AlscCalibration calibration;
> calibration.ct = lastCt = ct;
> boost::property_tree::ptree const &table =
> p.second.get_child("table");
> int num = 0;
> for (auto it = table.begin(); it != table.end();
> it++) {
> - if (num == XY)
> - LOG(RPiAlsc, Fatal)
> - << "Alsc: too many values
> for ct " << ct << " in " << name;
> + if (num == XY) {
> + LOG(RPiAlsc, Error)
> + << "Too many values for ct
> " << ct << " in " << name;
> + return -EINVAL;
> + }
> calibration.table[num++] =
> it->second.get_value<double>();
> }
> - if (num != XY)
> - LOG(RPiAlsc, Fatal)
> - << "Alsc: too few values for ct "
> << ct << " in " << name;
> + if (num != XY) {
> + LOG(RPiAlsc, Error)
> + << "Too few values for ct " << ct
> << " in " << name;
> + return -EINVAL;
> + }
> calibrations.push_back(calibration);
> LOG(RPiAlsc, Debug)
> << "Read " << name << " calibration for ct
> " << ct;
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> index a9e776a344a1..10c49c126341 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> @@ -46,16 +46,22 @@ static int readCtCurve(Pwl &ctR, Pwl &ctB,
> 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, Fatal) << "AwbConfig: incomplete CT
> curve entry";
> + 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, Fatal) << "AwbConfig: incomplete CT
> curve entry";
> + if (++it == params.end()) {
> + LOG(RPiAwb, Error) << "AwbConfig: incomplete CT
> curve entry";
> + return -EINVAL;
> + }
> ctB.append(ct, it->second.get_value<double>());
> num++;
> }
> - if (num < 2)
> - LOG(RPiAwb, Fatal) << "AwbConfig: insufficient points in
> CT curve";
> + if (num < 2) {
> + LOG(RPiAwb, Error) << "AwbConfig: insufficient points in
> CT curve";
> + return -EINVAL;
> + }
> return 0;
> }
>
> @@ -78,12 +84,16 @@ int AwbConfig::read(boost::property_tree::ptree const
> ¶ms)
> ret = prior.read(p.second);
> if (ret)
> return ret;
> - if (!priors.empty() && prior.lux <=
> priors.back().lux)
> - LOG(RPiAwb, Fatal) << "AwbConfig: Prior
> must be ordered in increasing lux value";
> + if (!priors.empty() && prior.lux <=
> priors.back().lux) {
> + LOG(RPiAwb, Error) << "AwbConfig: Prior
> must be ordered in increasing lux value";
> + return -EINVAL;
> + }
> priors.push_back(prior);
> }
> - if (priors.empty())
> - LOG(RPiAwb, Fatal) << "AwbConfig: no AWB priors
> configured";
> + if (priors.empty()) {
> + LOG(RPiAwb, Error) << "AwbConfig: no AWB priors
> configured";
> + return ret;
> + }
> }
> if (params.get_child_optional("modes")) {
> for (auto &p : params.get_child("modes")) {
> @@ -93,8 +103,10 @@ int AwbConfig::read(boost::property_tree::ptree const
> ¶ms)
> if (defaultMode == nullptr)
> defaultMode = &modes[p.first];
> }
> - if (defaultMode == nullptr)
> - LOG(RPiAwb, Fatal) << "AwbConfig: no AWB modes
> configured";
> + if (defaultMode == nullptr) {
> + LOG(RPiAwb, Error) << "AwbConfig: no AWB modes
> configured";
> + return -EINVAL;
> + }
> }
> minPixels = params.get<double>("min_pixels", 16.0);
> minG = params.get<uint16_t>("min_G", 32);
> @@ -103,8 +115,10 @@ int AwbConfig::read(boost::property_tree::ptree const
> ¶ms)
> coarseStep = params.get<double>("coarse_step", 0.2);
> transversePos = params.get<double>("transverse_pos", 0.01);
> transverseNeg = params.get<double>("transverse_neg", 0.01);
> - if (transversePos <= 0 || transverseNeg <= 0)
> - LOG(RPiAwb, Fatal) << "AwbConfig: transverse_pos/neg must
> be > 0";
> + if (transversePos <= 0 || transverseNeg <= 0) {
> + LOG(RPiAwb, Error) << "AwbConfig: transverse_pos/neg must
> be > 0";
> + return -EINVAL;
> + }
> sensitivityR = params.get<double>("sensitivity_r", 1.0);
> sensitivityB = params.get<double>("sensitivity_b", 1.0);
> if (bayes) {
> diff --git a/src/ipa/raspberrypi/controller/rpi/ccm.cpp
> b/src/ipa/raspberrypi/controller/rpi/ccm.cpp
> index f0110d38f548..9588e94adeb1 100644
> --- a/src/ipa/raspberrypi/controller/rpi/ccm.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/ccm.cpp
> @@ -44,12 +44,16 @@ int Matrix::read(boost::property_tree::ptree const
> ¶ms)
> double *ptr = (double *)m;
> int n = 0;
> for (auto it = params.begin(); it != params.end(); it++) {
> - if (n++ == 9)
> - LOG(RPiCcm, Fatal) << "Ccm: too many values in
> CCM";
> + if (n++ == 9) {
> + LOG(RPiCcm, Error) << "Too many values in CCM";
> + return -EINVAL;
> + }
> *ptr++ = it->second.get_value<double>();
> }
> - if (n < 9)
> - LOG(RPiCcm, Fatal) << "Ccm: too few values in CCM";
> + if (n < 9) {
> + LOG(RPiCcm, Error) << "Too few values in CCM";
> + return -EINVAL;
> + }
> return 0;
> }
>
> @@ -78,13 +82,17 @@ int Ccm::read(boost::property_tree::ptree const
> ¶ms)
> if (ret)
> return ret;
> if (!config_.ccms.empty() &&
> - ctCcm.ct <= config_.ccms.back().ct)
> - LOG(RPiCcm, Fatal) << "Ccm: CCM not in increasing
> colour temperature order";
> + ctCcm.ct <= config_.ccms.back().ct) {
> + LOG(RPiCcm, Error) << "CCM not in increasing
> colour temperature order";
> + return -EINVAL;
> + }
> config_.ccms.push_back(std::move(ctCcm));
> }
>
> - if (config_.ccms.empty())
> - LOG(RPiCcm, Fatal) << "Ccm: no CCMs specified";
> + if (config_.ccms.empty()) {
> + LOG(RPiCcm, Error) << "No CCMs specified";
> + return -EINVAL;
> + }
>
> return 0;
> }
> diff --git a/src/ipa/raspberrypi/controller/rpi/dpc.cpp
> b/src/ipa/raspberrypi/controller/rpi/dpc.cpp
> index be014a05fd41..def39bb7416a 100644
> --- a/src/ipa/raspberrypi/controller/rpi/dpc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/dpc.cpp
> @@ -34,8 +34,10 @@ char const *Dpc::name() const
> int Dpc::read(boost::property_tree::ptree const ¶ms)
> {
> config_.strength = params.get<int>("strength", 1);
> - if (config_.strength < 0 || config_.strength > 2)
> - LOG(RPiDpc, Fatal) << "Dpc: bad strength value";
> + if (config_.strength < 0 || config_.strength > 2) {
> + LOG(RPiDpc, Error) << "bad strength value";
> + return -EINVAL;
> + }
> return 0;
> }
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/geq.cpp
> b/src/ipa/raspberrypi/controller/rpi/geq.cpp
> index a74447877bf8..106f0a40dfc1 100644
> --- a/src/ipa/raspberrypi/controller/rpi/geq.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/geq.cpp
> @@ -39,8 +39,10 @@ int Geq::read(boost::property_tree::ptree const ¶ms)
> {
> config_.offset = params.get<uint16_t>("offset", 0);
> config_.slope = params.get<double>("slope", 0.0);
> - if (config_.slope < 0.0 || config_.slope >= 1.0)
> - LOG(RPiGeq, Fatal) << "Geq: bad slope value";
> + if (config_.slope < 0.0 || config_.slope >= 1.0) {
> + LOG(RPiGeq, Error) << "Bad slope value";
> + return -EINVAL;
> + }
>
> if (params.get_child_optional("strength")) {
> int ret =
> config_.strength.read(params.get_child("strength"));
> --
> Regards,
>
> Laurent Pinchart
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220727/13e06237/attachment.htm>
More information about the libcamera-devel
mailing list