[{"id":29891,"web_url":"https://patchwork.libcamera.org/comment/29891/","msgid":"","date":"2024-06-13T08:10:45","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jun 13, 2024 at 04:39:43AM +0300, Laurent Pinchart wrote:\n> Now that deserializing a Pwl object from YAML data is possible using the\n> YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> prepare for its removal.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> 8 files changed, 20 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> index 304629d6d4e5..5ca76dd98b4b 100644\n> --- a/src/ipa/rpi/controller/rpi/af.cpp\n> +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> \treadNumber(skipFrames, params, \"skip_frames\");\n> \n> \tif (params.contains(\"map\"))\n> -\t\tmap.readYaml(params[\"map\"]);\n> +\t\tmap = params[\"map\"].get(ipa::Pwl{});\n> \telse\n> \t\tLOG(RPiAf, Warning) << \"No map defined\";\n> \n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index a381dd972215..cf2565a83836 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> \t\treturn -EINVAL;\n> \tqHi = *value;\n> \n> -\treturn yTarget.readYaml(params[\"y_target\"]);\n> +\tyTarget = params[\"y_target\"].get(ipa::Pwl{});\n> +\treturn yTarget.empty() ? -EINVAL : 0;\n> }\n> \n> static std::tuple\n> @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> \t\t\treturn ret;\n> \t}\n> \n> -\tret = yTarget.readYaml(params[\"y_target\"]);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tyTarget = params[\"y_target\"].get(ipa::Pwl{});\n> +\tif (yTarget.empty())\n> +\t\treturn -EINVAL;\n> \n> \tspeed = params[\"speed\"].get(0.2);\n> \tstartupFrames = params[\"startup_frames\"].get(10);\n> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> index 603953d7d863..003c8fa137f3 100644\n> --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)\n> \t\treturn -EINVAL;\n> \tlux = *value;\n> \n> -\treturn prior.readYaml(params[\"prior\"]);\n> +\tprior = params[\"prior\"].get(ipa::Pwl{});\n> +\treturn prior.empty() ? -EINVAL : 0;\n> }\n> \n> static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)\n> diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> index 3272a1416ffa..e673964c1856 100644\n> --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> \tint ret;\n> \n> \tif (params.contains(\"saturation\")) {\n> -\t\tret = config_.saturation.readYaml(params[\"saturation\"]);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tconfig_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> +\t\tif (config_.saturation.empty())\n> +\t\t\treturn -EINVAL;\n> \t}\n> \n> \tfor (auto &p : params[\"ccms\"].asList()) {\n> diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp\n> index 66871a61ed28..9b37943ae9c9 100644\n> --- a/src/ipa/rpi/controller/rpi/contrast.cpp\n> +++ b/src/ipa/rpi/controller/rpi/contrast.cpp\n> @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)\n> \tconfig_.hiHistogram = params[\"hi_histogram\"].get(0.95);\n> \tconfig_.hiLevel = params[\"hi_level\"].get(0.95);\n> \tconfig_.hiMax = params[\"hi_max\"].get(2000);\n> -\treturn config_.gammaCurve.readYaml(params[\"gamma_curve\"]);\n> +\n> +\tconfig_.gammaCurve = params[\"gamma_curve\"].get(ipa::Pwl{});\n> +\treturn config_.gammaCurve.empty() ? -EINVAL : 0;\n> }\n> \n> void Contrast::setBrightness(double brightness)\n> diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp\n> index c9c38ebff5ba..40e7191ba16a 100644\n> --- a/src/ipa/rpi/controller/rpi/geq.cpp\n> +++ b/src/ipa/rpi/controller/rpi/geq.cpp\n> @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)\n> \t}\n> \n> \tif (params.contains(\"strength\")) {\n> -\t\tint ret = config_.strength.readYaml(params[\"strength\"]);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tconfig_.strength = params[\"strength\"].get(ipa::Pwl{});\n> +\t\tif (config_.strength.empty())\n> +\t\t\treturn -EINVAL;\n> \t}\n> \n> \treturn 0;\n> diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> index d533a4ea4e65..f3da8291bf5d 100644\n> --- a/src/ipa/rpi/controller/rpi/hdr.cpp\n> +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> \n> \t/* Lens shading related parameters. */\n> \tif (params.contains(\"spatial_gain_curve\")) {\n> -\t\tspatialGainCurve.readYaml(params[\"spatial_gain_curve\"]);\n> +\t\tspatialGainCurve = params[\"spatial_gain_curve\"].get(ipa::Pwl{});\n> \t} else if (params.contains(\"spatial_gain\")) {\n> \t\tdouble spatialGain = params[\"spatial_gain\"].get(2.0);\n> \t\tspatialGainCurve.append(0.0, spatialGain);\n> @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> \tiirStrength = params[\"iir_strength\"].get(8.0);\n> \tstrength = params[\"strength\"].get(1.5);\n> \tif (tonemapEnable)\n> -\t\ttonemap.readYaml(params[\"tonemap\"]);\n> +\t\ttonemap = params[\"tonemap\"].get(ipa::Pwl{});\n> \tspeed = params[\"speed\"].get(1.0);\n> \tif (params.contains(\"hi_quantile_targets\")) {\n> \t\thiQuantileTargets = params[\"hi_quantile_targets\"].getList().value();\n> diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> index 2dc50dfc8d2d..3422adfe7dee 100644\n> --- a/src/ipa/rpi/controller/rpi/tonemap.cpp\n> +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)\n> \tconfig_.detailSlope = params[\"detail_slope\"].get(0.1);\n> \tconfig_.iirStrength = params[\"iir_strength\"].get(1.0);\n> \tconfig_.strength = params[\"strength\"].get(1.0);\n> -\tconfig_.tonemap.readYaml(params[\"tone_curve\"]);\n> +\tconfig_.tonemap = params[\"tone_curve\"].get(ipa::Pwl{});\n> \treturn 0;\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CC647C3237\n\tfor ;\n\tThu, 13 Jun 2024 08:10:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F30416548E;\n\tThu, 13 Jun 2024 10:10:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C456165458\n\tfor ;\n\tThu, 13 Jun 2024 10:10:53 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6F125BEB;\n\tThu, 13 Jun 2024 10:10:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RxHsatqE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718266239;\n\tbh=eZEylgRipMjRQ+5ipF9ZGe2lLzsW3HJ5OfwKdl5hgwg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RxHsatqEc9Ud3vSCQRzZMyyr+4XsaDGvSQLc339yfwUgoz8TuLd2heZX9Jdw+ZHIK\n\t529ZAQ46RY4tK8D1eXEat/mc9Q8KW2/xpum2y/tAGOgczsIoipT/DAANpby9bjQJ6U\n\tHuwVESwnsEBAJlLAEcDdDvpHhvCPJvBuYOMp8fuI=","Date":"Thu, 13 Jun 2024 17:10:45 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDavid Plowman ,\n\tNaushir Patuck ","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","Message-ID":"","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29909,"web_url":"https://patchwork.libcamera.org/comment/29909/","msgid":"<171827679365.2248009.12219371291957233818@ping.linuxembedded.co.uk>","date":"2024-06-13T11:06:33","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-13 02:39:43)\n> Now that deserializing a Pwl object from YAML data is possible using the\n> YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> prepare for its removal.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> 8 files changed, 20 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> index 304629d6d4e5..5ca76dd98b4b 100644\n> --- a/src/ipa/rpi/controller/rpi/af.cpp\n> +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> readNumber(skipFrames, params, \"skip_frames\");\n> \n> if (params.contains(\"map\"))\n> - map.readYaml(params[\"map\"]);\n> + map = params[\"map\"].get(ipa::Pwl{});\n\nI look forward to your resurrection of the default constructor candidate\nin the optionals!\n\n> else\n> LOG(RPiAf, Warning) << \"No map defined\";\n> \n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index a381dd972215..cf2565a83836 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> return -EINVAL;\n> qHi = *value;\n> \n> - return yTarget.readYaml(params[\"y_target\"]);\n> + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> + return yTarget.empty() ? -EINVAL : 0;\n\nUtlimately this is the biggest change here. Instead of returning an\nerror - it will return a default construction or the result. Pwl is fine\nhere as we can check that it's empty or not - but I wonder what happens\non more complex structures like a Matrix where we default construct to\nan identity ...\n\nHow would that be handled?\n\n\nIt seems because Vector::readYaml wasn't used yet - I can't see an\nequivelent implemention for that concept.\n\nI'm curious how we make this expressive for the three conditions though:\n\n 1. Get succeeds, value returned.\n 2. Get fails desired value returned.\n 3. Get fails Do not want a default value, and knowing it failed is\n important.\n\nIs 3. a condition we will require?\n\nBut ... that aside - this works for Pwl ... so on that basis ...\n\nReviewed-by: Kieran Bingham \n\n> }\n> \n> static std::tuple\n> @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> return ret;\n> }\n> \n> - ret = yTarget.readYaml(params[\"y_target\"]);\n> - if (ret)\n> - return ret;\n> + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> + if (yTarget.empty())\n> + return -EINVAL;\n> \n> speed = params[\"speed\"].get(0.2);\n> startupFrames = params[\"startup_frames\"].get(10);\n> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> index 603953d7d863..003c8fa137f3 100644\n> --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)\n> return -EINVAL;\n> lux = *value;\n> \n> - return prior.readYaml(params[\"prior\"]);\n> + prior = params[\"prior\"].get(ipa::Pwl{});\n> + return prior.empty() ? -EINVAL : 0;\n> }\n> \n> static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)\n> diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> index 3272a1416ffa..e673964c1856 100644\n> --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> int ret;\n> \n> if (params.contains(\"saturation\")) {\n> - ret = config_.saturation.readYaml(params[\"saturation\"]);\n> - if (ret)\n> - return ret;\n> + config_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> + if (config_.saturation.empty())\n> + return -EINVAL;\n> }\n> \n> for (auto &p : params[\"ccms\"].asList()) {\n> diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp\n> index 66871a61ed28..9b37943ae9c9 100644\n> --- a/src/ipa/rpi/controller/rpi/contrast.cpp\n> +++ b/src/ipa/rpi/controller/rpi/contrast.cpp\n> @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)\n> config_.hiHistogram = params[\"hi_histogram\"].get(0.95);\n> config_.hiLevel = params[\"hi_level\"].get(0.95);\n> config_.hiMax = params[\"hi_max\"].get(2000);\n> - return config_.gammaCurve.readYaml(params[\"gamma_curve\"]);\n> +\n> + config_.gammaCurve = params[\"gamma_curve\"].get(ipa::Pwl{});\n> + return config_.gammaCurve.empty() ? -EINVAL : 0;\n> }\n> \n> void Contrast::setBrightness(double brightness)\n> diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp\n> index c9c38ebff5ba..40e7191ba16a 100644\n> --- a/src/ipa/rpi/controller/rpi/geq.cpp\n> +++ b/src/ipa/rpi/controller/rpi/geq.cpp\n> @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)\n> }\n> \n> if (params.contains(\"strength\")) {\n> - int ret = config_.strength.readYaml(params[\"strength\"]);\n> - if (ret)\n> - return ret;\n> + config_.strength = params[\"strength\"].get(ipa::Pwl{});\n> + if (config_.strength.empty())\n> + return -EINVAL;\n> }\n> \n> return 0;\n> diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> index d533a4ea4e65..f3da8291bf5d 100644\n> --- a/src/ipa/rpi/controller/rpi/hdr.cpp\n> +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> \n> /* Lens shading related parameters. */\n> if (params.contains(\"spatial_gain_curve\")) {\n> - spatialGainCurve.readYaml(params[\"spatial_gain_curve\"]);\n> + spatialGainCurve = params[\"spatial_gain_curve\"].get(ipa::Pwl{});\n> } else if (params.contains(\"spatial_gain\")) {\n> double spatialGain = params[\"spatial_gain\"].get(2.0);\n> spatialGainCurve.append(0.0, spatialGain);\n> @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> iirStrength = params[\"iir_strength\"].get(8.0);\n> strength = params[\"strength\"].get(1.5);\n> if (tonemapEnable)\n> - tonemap.readYaml(params[\"tonemap\"]);\n> + tonemap = params[\"tonemap\"].get(ipa::Pwl{});\n> speed = params[\"speed\"].get(1.0);\n> if (params.contains(\"hi_quantile_targets\")) {\n> hiQuantileTargets = params[\"hi_quantile_targets\"].getList().value();\n> diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> index 2dc50dfc8d2d..3422adfe7dee 100644\n> --- a/src/ipa/rpi/controller/rpi/tonemap.cpp\n> +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)\n> config_.detailSlope = params[\"detail_slope\"].get(0.1);\n> config_.iirStrength = params[\"iir_strength\"].get(1.0);\n> config_.strength = params[\"strength\"].get(1.0);\n> - config_.tonemap.readYaml(params[\"tone_curve\"]);\n> + config_.tonemap = params[\"tone_curve\"].get(ipa::Pwl{});\n> return 0;\n> }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B3B7BBD87C\n\tfor ;\n\tThu, 13 Jun 2024 11:06:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18BC765491;\n\tThu, 13 Jun 2024 13:06:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CEA76548D\n\tfor ;\n\tThu, 13 Jun 2024 13:06:37 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32F004CF;\n\tThu, 13 Jun 2024 13:06:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dW8C7hCf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718276783;\n\tbh=zabARqgJHMtGKGqAxc9bDlbtXQwuy5SRrKLaz1Au0rg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=dW8C7hCf//bx6uDmtNzgMS+ptvEsKsZ3The7CpF2D48Hgw0ZkuHzQCmJUzEkeKCY5\n\tr0/vaJtL67SiPNTESxwLiStlQWB6WgJW1jv4g1OcTwI2vYo4SK2oCJLhdoJYnaRciN\n\tWYBrRfVNikQZ1nQlkPD7afLMfouBK87kVQthDL5g=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tDavid Plowman ,\n\tNaushir Patuck ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 13 Jun 2024 12:06:33 +0100","Message-ID":"<171827679365.2248009.12219371291957233818@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29915,"web_url":"https://patchwork.libcamera.org/comment/29915/","msgid":"<20240613111729.GE6019@pendragon.ideasonboard.com>","date":"2024-06-13T11:17:29","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jun 13, 2024 at 12:06:33PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-06-13 02:39:43)\n> > Now that deserializing a Pwl object from YAML data is possible using the\n> > YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> > prepare for its removal.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> > src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> > src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> > src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> > src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> > src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> > src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> > src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> > 8 files changed, 20 insertions(+), 16 deletions(-)\n> > \n> > diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> > index 304629d6d4e5..5ca76dd98b4b 100644\n> > --- a/src/ipa/rpi/controller/rpi/af.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> > @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> > readNumber(skipFrames, params, \"skip_frames\");\n> > \n> > if (params.contains(\"map\"))\n> > - map.readYaml(params[\"map\"]);\n> > + map = params[\"map\"].get(ipa::Pwl{});\n> \n> I look forward to your resurrection of the default constructor candidate\n> in the optionals!\n\nMe too :-) The above would then be written\n\n\t\tmap = params[\"map\"].get().value_or(utils::defopt);\n\nI'll let you decide if you like it better or not.\n\n> > else\n> > LOG(RPiAf, Warning) << \"No map defined\";\n> > \n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > index a381dd972215..cf2565a83836 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> > return -EINVAL;\n> > qHi = *value;\n> > \n> > - return yTarget.readYaml(params[\"y_target\"]);\n> > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > + return yTarget.empty() ? -EINVAL : 0;\n> \n> Utlimately this is the biggest change here. Instead of returning an\n> error - it will return a default construction or the result. Pwl is fine\n> here as we can check that it's empty or not - but I wonder what happens\n> on more complex structures like a Matrix where we default construct to\n> an identity ...\n> \n> How would that be handled?\n\nWith a tiny bit more code:\n\n\tstd::optional pwl = params[\"y_target\"].get();\n\tif (!pwl)\n\t\treturn -EINVAL;\n\n\tyTarget = *pwl;\n\n> It seems because Vector::readYaml wasn't used yet - I can't see an\n> equivelent implemention for that concept.\n> \n> I'm curious how we make this expressive for the three conditions though:\n> \n> 1. Get succeeds, value returned.\n\n\tstd::optional pwl = params[\"y_target\"].get();\n\tif (pwl)\n\t\t/* Success */\n\nFor types that include a way to check if they are valid, you can also\n\n\tipa::Pwl pwl = params[\"y_target\"].get(ipa::Pwl{});\n\tif (!pwl.empty())\n\t\t/* Success */\n\n> 2. Get fails desired value returned.\n\nI assume you mean default value returned ?\n\n\tipa::Pwl pwl = params[\"y_target\"].get(ipa::Pwl{});\n\n> 3. Get fails Do not want a default value, and knowing it failed is\n> important.\n\n\tstd::optional pwl = params[\"y_target\"].get();\n\tif (!pwl)\n\t\t/* Failure */\n\n> \n> Is 3. a condition we will require?\n> \n> But ... that aside - this works for Pwl ... so on that basis ...\n> \n> Reviewed-by: Kieran Bingham \n> \n> > }\n> > \n> > static std::tuple\n> > @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> > return ret;\n> > }\n> > \n> > - ret = yTarget.readYaml(params[\"y_target\"]);\n> > - if (ret)\n> > - return ret;\n> > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > + if (yTarget.empty())\n> > + return -EINVAL;\n> > \n> > speed = params[\"speed\"].get(0.2);\n> > startupFrames = params[\"startup_frames\"].get(10);\n> > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> > index 603953d7d863..003c8fa137f3 100644\n> > --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> > @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)\n> > return -EINVAL;\n> > lux = *value;\n> > \n> > - return prior.readYaml(params[\"prior\"]);\n> > + prior = params[\"prior\"].get(ipa::Pwl{});\n> > + return prior.empty() ? -EINVAL : 0;\n> > }\n> > \n> > static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)\n> > diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > index 3272a1416ffa..e673964c1856 100644\n> > --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> > int ret;\n> > \n> > if (params.contains(\"saturation\")) {\n> > - ret = config_.saturation.readYaml(params[\"saturation\"]);\n> > - if (ret)\n> > - return ret;\n> > + config_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> > + if (config_.saturation.empty())\n> > + return -EINVAL;\n> > }\n> > \n> > for (auto &p : params[\"ccms\"].asList()) {\n> > diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > index 66871a61ed28..9b37943ae9c9 100644\n> > --- a/src/ipa/rpi/controller/rpi/contrast.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)\n> > config_.hiHistogram = params[\"hi_histogram\"].get(0.95);\n> > config_.hiLevel = params[\"hi_level\"].get(0.95);\n> > config_.hiMax = params[\"hi_max\"].get(2000);\n> > - return config_.gammaCurve.readYaml(params[\"gamma_curve\"]);\n> > +\n> > + config_.gammaCurve = params[\"gamma_curve\"].get(ipa::Pwl{});\n> > + return config_.gammaCurve.empty() ? -EINVAL : 0;\n> > }\n> > \n> > void Contrast::setBrightness(double brightness)\n> > diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp\n> > index c9c38ebff5ba..40e7191ba16a 100644\n> > --- a/src/ipa/rpi/controller/rpi/geq.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/geq.cpp\n> > @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)\n> > }\n> > \n> > if (params.contains(\"strength\")) {\n> > - int ret = config_.strength.readYaml(params[\"strength\"]);\n> > - if (ret)\n> > - return ret;\n> > + config_.strength = params[\"strength\"].get(ipa::Pwl{});\n> > + if (config_.strength.empty())\n> > + return -EINVAL;\n> > }\n> > \n> > return 0;\n> > diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > index d533a4ea4e65..f3da8291bf5d 100644\n> > --- a/src/ipa/rpi/controller/rpi/hdr.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> > \n> > /* Lens shading related parameters. */\n> > if (params.contains(\"spatial_gain_curve\")) {\n> > - spatialGainCurve.readYaml(params[\"spatial_gain_curve\"]);\n> > + spatialGainCurve = params[\"spatial_gain_curve\"].get(ipa::Pwl{});\n> > } else if (params.contains(\"spatial_gain\")) {\n> > double spatialGain = params[\"spatial_gain\"].get(2.0);\n> > spatialGainCurve.append(0.0, spatialGain);\n> > @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> > iirStrength = params[\"iir_strength\"].get(8.0);\n> > strength = params[\"strength\"].get(1.5);\n> > if (tonemapEnable)\n> > - tonemap.readYaml(params[\"tonemap\"]);\n> > + tonemap = params[\"tonemap\"].get(ipa::Pwl{});\n> > speed = params[\"speed\"].get(1.0);\n> > if (params.contains(\"hi_quantile_targets\")) {\n> > hiQuantileTargets = params[\"hi_quantile_targets\"].getList().value();\n> > diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > index 2dc50dfc8d2d..3422adfe7dee 100644\n> > --- a/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)\n> > config_.detailSlope = params[\"detail_slope\"].get(0.1);\n> > config_.iirStrength = params[\"iir_strength\"].get(1.0);\n> > config_.strength = params[\"strength\"].get(1.0);\n> > - config_.tonemap.readYaml(params[\"tone_curve\"]);\n> > + config_.tonemap = params[\"tone_curve\"].get(ipa::Pwl{});\n> > return 0;\n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8023FC3237\n\tfor ;\n\tThu, 13 Jun 2024 11:17:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3397D6549A;\n\tThu, 13 Jun 2024 13:17:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF7AA65490\n\tfor ;\n\tThu, 13 Jun 2024 13:17:49 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D535E4CF;\n\tThu, 13 Jun 2024 13:17:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZvLoJssn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718277456;\n\tbh=8PlphE87a6ud/dIPKsBqO5bkmYHotZeAqabPCLh64NQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZvLoJssnKm0klXYK2cnDP0N7veW+PJlNX7pKG2murJRl8CDGE6mxDk1aw2eaHYMqD\n\tHIGpDeazC9BoaemBcuA4+ceVRCoGB69wnHHPcEU3QwnvQmB7QESbe64HBB4eKrYvaM\n\tTHHzg2qtOdh5yfMheCLeRiTxFqTYby6QqvqQyyXE=","Date":"Thu, 13 Jun 2024 14:17:29 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ,\n\tDavid Plowman ,\n\tNaushir Patuck ","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","Message-ID":"<20240613111729.GE6019@pendragon.ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>\n\t<171827679365.2248009.12219371291957233818@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171827679365.2248009.12219371291957233818@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29917,"web_url":"https://patchwork.libcamera.org/comment/29917/","msgid":"<171827836580.1550852.8774357496630913805@ping.linuxembedded.co.uk>","date":"2024-06-13T11:32:45","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-13 12:17:29)\n> On Thu, Jun 13, 2024 at 12:06:33PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2024-06-13 02:39:43)\n> > > Now that deserializing a Pwl object from YAML data is possible using the\n> > > YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> > > prepare for its removal.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> > > src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> > > src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> > > src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> > > src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> > > src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> > > src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> > > 8 files changed, 20 insertions(+), 16 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> > > index 304629d6d4e5..5ca76dd98b4b 100644\n> > > --- a/src/ipa/rpi/controller/rpi/af.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> > > @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> > > readNumber(skipFrames, params, \"skip_frames\");\n> > > \n> > > if (params.contains(\"map\"))\n> > > - map.readYaml(params[\"map\"]);\n> > > + map = params[\"map\"].get(ipa::Pwl{});\n> > \n> > I look forward to your resurrection of the default constructor candidate\n> > in the optionals!\n> \n> Me too :-) The above would then be written\n> \n> map = params[\"map\"].get().value_or(utils::defopt);\n> \n> I'll let you decide if you like it better or not.\n\nHrm ... it's ... longer ;-) I thought it would mean we could write:\n\t\tmap = params[\"map\"].get();\n\nBut oh well, lets see later.\n\n\n> \n> > > else\n> > > LOG(RPiAf, Warning) << \"No map defined\";\n> > > \n> > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > index a381dd972215..cf2565a83836 100644\n> > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> > > return -EINVAL;\n> > > qHi = *value;\n> > > \n> > > - return yTarget.readYaml(params[\"y_target\"]);\n> > > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > > + return yTarget.empty() ? -EINVAL : 0;\n> > \n> > Utlimately this is the biggest change here. Instead of returning an\n> > error - it will return a default construction or the result. Pwl is fine\n> > here as we can check that it's empty or not - but I wonder what happens\n> > on more complex structures like a Matrix where we default construct to\n> > an identity ...\n> > \n> > How would that be handled?\n> \n> With a tiny bit more code:\n> \n> std::optional pwl = params[\"y_target\"].get();\n> if (!pwl)\n> return -EINVAL;\n> \n> yTarget = *pwl;\n> \n> > It seems because Vector::readYaml wasn't used yet - I can't see an\n> > equivelent implemention for that concept.\n> > \n> > I'm curious how we make this expressive for the three conditions though:\n> > \n> > 1. Get succeeds, value returned.\n> \n> std::optional pwl = params[\"y_target\"].get();\n> if (pwl)\n> /* Success */\n> \n> For types that include a way to check if they are valid, you can also\n> \n> ipa::Pwl pwl = params[\"y_target\"].get(ipa::Pwl{});\n> if (!pwl.empty())\n> /* Success */\n> \n> > 2. Get fails desired value returned.\n> \n> I assume you mean default value returned ?\n\nYes,\n\n\n> \n> ipa::Pwl pwl = params[\"y_target\"].get(ipa::Pwl{});\n> \n> > 3. Get fails Do not want a default value, and knowing it failed is\n> > important.\n> \n> std::optional pwl = params[\"y_target\"].get();\n> if (!pwl)\n> /* Failure */\n> \n\nOk - perfect - all cases handled!\n\nThanks for the clarification.\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 73F9FBD87C\n\tfor ;\n\tThu, 13 Jun 2024 11:32:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C73365494;\n\tThu, 13 Jun 2024 13:32:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA06B6548D\n\tfor ;\n\tThu, 13 Jun 2024 13:32:48 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B74D4CF;\n\tThu, 13 Jun 2024 13:32:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"o5kzHqHl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718278354;\n\tbh=REn56Yky2eDleI2OSYEnV6dArM6ChGaaTDyv4a7YkHA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=o5kzHqHl0O6PZmRVHnV+ut7JYHViarWaZ3kcIfaV6gbKiRDvdh8lxoHGUvchGI0VY\n\tt6BpvaLDTYH2CbwF7B6Oib6mc55kA2mF5VFBX5oB052ccfdfzkrjuAR9WOXEC5iNGZ\n\teEyzyV+1bB5v7gQKf8MZYojTJquBhzIdelwG54Mc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240613111729.GE6019@pendragon.ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>\n\t<171827679365.2248009.12219371291957233818@ping.linuxembedded.co.uk>\n\t<20240613111729.GE6019@pendragon.ideasonboard.com>","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ,\n\tDavid Plowman ,\n\tNaushir Patuck ","To":"Laurent Pinchart ","Date":"Thu, 13 Jun 2024 12:32:45 +0100","Message-ID":"<171827836580.1550852.8774357496630913805@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30004,"web_url":"https://patchwork.libcamera.org/comment/30004/","msgid":"<20240617134914.GC10134@pendragon.ideasonboard.com>","date":"2024-06-17T13:49:14","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush, David,\n\nCould you give this patch a review when you get the chance ?\n\nOn Thu, Jun 13, 2024 at 04:39:43AM +0300, Laurent Pinchart wrote:\n> Now that deserializing a Pwl object from YAML data is possible using the\n> YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> prepare for its removal.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> 8 files changed, 20 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> index 304629d6d4e5..5ca76dd98b4b 100644\n> --- a/src/ipa/rpi/controller/rpi/af.cpp\n> +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> \treadNumber(skipFrames, params, \"skip_frames\");\n> \n> \tif (params.contains(\"map\"))\n> -\t\tmap.readYaml(params[\"map\"]);\n> +\t\tmap = params[\"map\"].get(ipa::Pwl{});\n> \telse\n> \t\tLOG(RPiAf, Warning) << \"No map defined\";\n> \n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index a381dd972215..cf2565a83836 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> \t\treturn -EINVAL;\n> \tqHi = *value;\n> \n> -\treturn yTarget.readYaml(params[\"y_target\"]);\n> +\tyTarget = params[\"y_target\"].get(ipa::Pwl{});\n> +\treturn yTarget.empty() ? -EINVAL : 0;\n> }\n> \n> static std::tuple\n> @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> \t\t\treturn ret;\n> \t}\n> \n> -\tret = yTarget.readYaml(params[\"y_target\"]);\n> -\tif (ret)\n> -\t\treturn ret;\n> +\tyTarget = params[\"y_target\"].get(ipa::Pwl{});\n> +\tif (yTarget.empty())\n> +\t\treturn -EINVAL;\n> \n> \tspeed = params[\"speed\"].get(0.2);\n> \tstartupFrames = params[\"startup_frames\"].get(10);\n> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> index 603953d7d863..003c8fa137f3 100644\n> --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)\n> \t\treturn -EINVAL;\n> \tlux = *value;\n> \n> -\treturn prior.readYaml(params[\"prior\"]);\n> +\tprior = params[\"prior\"].get(ipa::Pwl{});\n> +\treturn prior.empty() ? -EINVAL : 0;\n> }\n> \n> static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)\n> diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> index 3272a1416ffa..e673964c1856 100644\n> --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> \tint ret;\n> \n> \tif (params.contains(\"saturation\")) {\n> -\t\tret = config_.saturation.readYaml(params[\"saturation\"]);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tconfig_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> +\t\tif (config_.saturation.empty())\n> +\t\t\treturn -EINVAL;\n> \t}\n> \n> \tfor (auto &p : params[\"ccms\"].asList()) {\n> diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp\n> index 66871a61ed28..9b37943ae9c9 100644\n> --- a/src/ipa/rpi/controller/rpi/contrast.cpp\n> +++ b/src/ipa/rpi/controller/rpi/contrast.cpp\n> @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)\n> \tconfig_.hiHistogram = params[\"hi_histogram\"].get(0.95);\n> \tconfig_.hiLevel = params[\"hi_level\"].get(0.95);\n> \tconfig_.hiMax = params[\"hi_max\"].get(2000);\n> -\treturn config_.gammaCurve.readYaml(params[\"gamma_curve\"]);\n> +\n> +\tconfig_.gammaCurve = params[\"gamma_curve\"].get(ipa::Pwl{});\n> +\treturn config_.gammaCurve.empty() ? -EINVAL : 0;\n> }\n> \n> void Contrast::setBrightness(double brightness)\n> diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp\n> index c9c38ebff5ba..40e7191ba16a 100644\n> --- a/src/ipa/rpi/controller/rpi/geq.cpp\n> +++ b/src/ipa/rpi/controller/rpi/geq.cpp\n> @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)\n> \t}\n> \n> \tif (params.contains(\"strength\")) {\n> -\t\tint ret = config_.strength.readYaml(params[\"strength\"]);\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> +\t\tconfig_.strength = params[\"strength\"].get(ipa::Pwl{});\n> +\t\tif (config_.strength.empty())\n> +\t\t\treturn -EINVAL;\n> \t}\n> \n> \treturn 0;\n> diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> index d533a4ea4e65..f3da8291bf5d 100644\n> --- a/src/ipa/rpi/controller/rpi/hdr.cpp\n> +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> \n> \t/* Lens shading related parameters. */\n> \tif (params.contains(\"spatial_gain_curve\")) {\n> -\t\tspatialGainCurve.readYaml(params[\"spatial_gain_curve\"]);\n> +\t\tspatialGainCurve = params[\"spatial_gain_curve\"].get(ipa::Pwl{});\n> \t} else if (params.contains(\"spatial_gain\")) {\n> \t\tdouble spatialGain = params[\"spatial_gain\"].get(2.0);\n> \t\tspatialGainCurve.append(0.0, spatialGain);\n> @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> \tiirStrength = params[\"iir_strength\"].get(8.0);\n> \tstrength = params[\"strength\"].get(1.5);\n> \tif (tonemapEnable)\n> -\t\ttonemap.readYaml(params[\"tonemap\"]);\n> +\t\ttonemap = params[\"tonemap\"].get(ipa::Pwl{});\n> \tspeed = params[\"speed\"].get(1.0);\n> \tif (params.contains(\"hi_quantile_targets\")) {\n> \t\thiQuantileTargets = params[\"hi_quantile_targets\"].getList().value();\n> diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> index 2dc50dfc8d2d..3422adfe7dee 100644\n> --- a/src/ipa/rpi/controller/rpi/tonemap.cpp\n> +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)\n> \tconfig_.detailSlope = params[\"detail_slope\"].get(0.1);\n> \tconfig_.iirStrength = params[\"iir_strength\"].get(1.0);\n> \tconfig_.strength = params[\"strength\"].get(1.0);\n> -\tconfig_.tonemap.readYaml(params[\"tone_curve\"]);\n> +\tconfig_.tonemap = params[\"tone_curve\"].get(ipa::Pwl{});\n> \treturn 0;\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C66D4C3237\n\tfor ;\n\tMon, 17 Jun 2024 13:49:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18C6B65496;\n\tMon, 17 Jun 2024 15:49:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 36F9C6548B\n\tfor ;\n\tMon, 17 Jun 2024 15:49:36 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1C3F439F;\n\tMon, 17 Jun 2024 15:49:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"n4U5FbIL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718632159;\n\tbh=goEWe78Qjds/zlklVaUfpHnfIvDd5h+Ir8av1ONLcKY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n4U5FbILeXVp4WK2VUvA/MWS7aa8K2VtFNVvGMAjL+tNgZ92RoJfuyIkl7ChfK0dy\n\t4K4GvvbaPZn75JBlsvV85NdZITR+9JIHyRCdTc//cbxnwSW2i1k1b8cQXmsfHuZu0q\n\tCRiOq6H7btxzFuNK/GXDGwapwJgjNkD0v4UY62AQ=","Date":"Mon, 17 Jun 2024 16:49:14 +0300","From":"Laurent Pinchart ","To":"David Plowman ,\n\tNaushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","Message-ID":"<20240617134914.GC10134@pendragon.ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30005,"web_url":"https://patchwork.libcamera.org/comment/30005/","msgid":"","date":"2024-06-17T14:40:37","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the patch.\n\nOn Mon, 17 Jun 2024 at 14:49, Laurent Pinchart\n wrote:\n>\n> Hi Naush, David,\n>\n> Could you give this patch a review when you get the chance ?\n>\n> On Thu, Jun 13, 2024 at 04:39:43AM +0300, Laurent Pinchart wrote:\n> > Now that deserializing a Pwl object from YAML data is possible using the\n> > YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> > prepare for its removal.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> > src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> > src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> > src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> > src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> > src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> > src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> > src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> > 8 files changed, 20 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> > index 304629d6d4e5..5ca76dd98b4b 100644\n> > --- a/src/ipa/rpi/controller/rpi/af.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> > @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> > readNumber(skipFrames, params, \"skip_frames\");\n> >\n> > if (params.contains(\"map\"))\n> > - map.readYaml(params[\"map\"]);\n> > + map = params[\"map\"].get(ipa::Pwl{});\n\nJust wondering how this works... YamlObject::get calls\nYamlObject::Getter::get, and if the latter returns std::nullopt, then\nthe former returns the value that was passed in? (Scratching my head\never so slightly...!)\n\nLooking at this, I suspect we should have been checking the return\ncode from readYaml (equivalently, an empty Pwl from get), but that's\nclearly a thing for later, and not for this patch.\n\n> > else\n> > LOG(RPiAf, Warning) << \"No map defined\";\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > index a381dd972215..cf2565a83836 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> > return -EINVAL;\n> > qHi = *value;\n> >\n> > - return yTarget.readYaml(params[\"y_target\"]);\n> > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > + return yTarget.empty() ? -EINVAL : 0;\n> > }\n> >\n> > static std::tuple\n> > @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> > return ret;\n> > }\n> >\n> > - ret = yTarget.readYaml(params[\"y_target\"]);\n> > - if (ret)\n> > - return ret;\n> > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > + if (yTarget.empty())\n> > + return -EINVAL;\n> >\n> > speed = params[\"speed\"].get(0.2);\n> > startupFrames = params[\"startup_frames\"].get(10);\n> > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> > index 603953d7d863..003c8fa137f3 100644\n> > --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> > @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)\n> > return -EINVAL;\n> > lux = *value;\n> >\n> > - return prior.readYaml(params[\"prior\"]);\n> > + prior = params[\"prior\"].get(ipa::Pwl{});\n> > + return prior.empty() ? -EINVAL : 0;\n> > }\n> >\n> > static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)\n> > diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > index 3272a1416ffa..e673964c1856 100644\n> > --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> > int ret;\n> >\n> > if (params.contains(\"saturation\")) {\n> > - ret = config_.saturation.readYaml(params[\"saturation\"]);\n> > - if (ret)\n> > - return ret;\n> > + config_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> > + if (config_.saturation.empty())\n> > + return -EINVAL;\n> > }\n> >\n> > for (auto &p : params[\"ccms\"].asList()) {\n> > diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > index 66871a61ed28..9b37943ae9c9 100644\n> > --- a/src/ipa/rpi/controller/rpi/contrast.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)\n> > config_.hiHistogram = params[\"hi_histogram\"].get(0.95);\n> > config_.hiLevel = params[\"hi_level\"].get(0.95);\n> > config_.hiMax = params[\"hi_max\"].get(2000);\n> > - return config_.gammaCurve.readYaml(params[\"gamma_curve\"]);\n> > +\n> > + config_.gammaCurve = params[\"gamma_curve\"].get(ipa::Pwl{});\n> > + return config_.gammaCurve.empty() ? -EINVAL : 0;\n> > }\n> >\n> > void Contrast::setBrightness(double brightness)\n> > diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp\n> > index c9c38ebff5ba..40e7191ba16a 100644\n> > --- a/src/ipa/rpi/controller/rpi/geq.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/geq.cpp\n> > @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)\n> > }\n> >\n> > if (params.contains(\"strength\")) {\n> > - int ret = config_.strength.readYaml(params[\"strength\"]);\n> > - if (ret)\n> > - return ret;\n> > + config_.strength = params[\"strength\"].get(ipa::Pwl{});\n> > + if (config_.strength.empty())\n> > + return -EINVAL;\n> > }\n> >\n> > return 0;\n> > diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > index d533a4ea4e65..f3da8291bf5d 100644\n> > --- a/src/ipa/rpi/controller/rpi/hdr.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> >\n> > /* Lens shading related parameters. */\n> > if (params.contains(\"spatial_gain_curve\")) {\n> > - spatialGainCurve.readYaml(params[\"spatial_gain_curve\"]);\n> > + spatialGainCurve = params[\"spatial_gain_curve\"].get(ipa::Pwl{});\n> > } else if (params.contains(\"spatial_gain\")) {\n> > double spatialGain = params[\"spatial_gain\"].get(2.0);\n> > spatialGainCurve.append(0.0, spatialGain);\n> > @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> > iirStrength = params[\"iir_strength\"].get(8.0);\n> > strength = params[\"strength\"].get(1.5);\n> > if (tonemapEnable)\n> > - tonemap.readYaml(params[\"tonemap\"]);\n> > + tonemap = params[\"tonemap\"].get(ipa::Pwl{});\n\nHmm, probably more return codes that weren't being checked...\n\n> > speed = params[\"speed\"].get(1.0);\n> > if (params.contains(\"hi_quantile_targets\")) {\n> > hiQuantileTargets = params[\"hi_quantile_targets\"].getList().value();\n> > diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > index 2dc50dfc8d2d..3422adfe7dee 100644\n> > --- a/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)\n> > config_.detailSlope = params[\"detail_slope\"].get(0.1);\n> > config_.iirStrength = params[\"iir_strength\"].get(1.0);\n> > config_.strength = params[\"strength\"].get(1.0);\n> > - config_.tonemap.readYaml(params[\"tone_curve\"]);\n> > + config_.tonemap = params[\"tone_curve\"].get(ipa::Pwl{});\n\nSo yes, that all looks good to me. Possibly worth running it on a Pi\njust to double check!\n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\n> > return 0;\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 13164BD87C\n\tfor ;\n\tMon, 17 Jun 2024 14:40:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF3976548F;\n\tMon, 17 Jun 2024 16:40:50 +0200 (CEST)","from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com\n\t[IPv6:2607:f8b0:4864:20::b35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99FA265489\n\tfor ;\n\tMon, 17 Jun 2024 16:40:49 +0200 (CEST)","by mail-yb1-xb35.google.com with SMTP id\n\t3f1490d57ef6-dff0c685371so3793255276.2\n\tfor ;\n\tMon, 17 Jun 2024 07:40:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"kwSDtdAS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1718635248; x=1719240048;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=0OhM9quiuC84rAKicYkEoMN/lwuqG70rhjSRZMde8Hg=;\n\tb=kwSDtdASAFbuoxktvZfkFVjO7oBMiLBCqgiHAn7DsooGLoX9WgO+MPgBALX2CnLi2l\n\ttqML+tQnTQ04IB5OsL3JLvIPPVqpVadvWv9vIBufNkqAt/JlCue5jH2B7MpjO/fKv0zx\n\tO1wK513x7obNIBmjX/MVnuqiHM8eM1Zs0Ugym0WPqyipBtNRNCuspj1CdLhP9Yv9CaFt\n\tloXS3ITwVOIf4OqDhKlu947PVcjnsRAI039jQRMo48PBrJH8mQr6jZXqZliDuMHE6O2p\n\t2A7ajkq22Sav+yZ6rVp36Q/IBNklr2+bo4IGgFw0b/jGRjhPrwq+OVPquOsIWLrPLILf\n\tuTQA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1718635248; x=1719240048;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=0OhM9quiuC84rAKicYkEoMN/lwuqG70rhjSRZMde8Hg=;\n\tb=MeDMFGZ+iN6oTatAuD20rsd/aKaO/SnMv3xeknpuokQ8qMBJj+zxas3uxXx17d2/1p\n\t1U7jJfZwc/ALonuvRNbDfhSbWGmjZazCGTkdxRhVHpNQA9h7g8THBx+yhMS4Rfb9b9mT\n\tsOd5zANi5Rvdi0sSQzEG6e/Z041Qtc/OqA5lsxl4z89xWTcXHbj+OnFmHpWlfQ5tPHuT\n\tH+APqVhSi5wDkP96G1LtE1dJuKLnZ/He6GgKVHnqs8Y+1ZsCvQD1kYODuPr06BPcgYoN\n\txchuvZDD9YUyv5rJ0Mvkc7LDXzyz9gYfiJwnhGRDQfjsN2bTzu02OglP5EP8bQkb0YJO\n\t9Zsw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCW30Pg7xRkCWvPec4vKofBUWz7CJRgr+vq1oHSDyyRPKOBCW874Z7xuU+02RqSC+bG5TA0kMQr4RjwnMoJz61Jq5XycJpZdP1ieIHeOI98Wt3HAPQ==","X-Gm-Message-State":"AOJu0Ywac2QoblFuck0oXe+w8T5qEGKDFcCq61dZR0I7w6sda1kT9Ktd\n\tkUCVevvHnBpOz9dNOtIC5D9FvzsLBoaU2bBeJQvGL5TgNfin/DuqzaTKKnPVvHaRknVyoib5RYC\n\txhVsNjSVIXTwcvjM2yO3JAaH2w7ALOLJrng0njw==","X-Google-Smtp-Source":"AGHT+IFZEwYuXeRo9wc9dFylFqJjPxV9L+EkeHd3t+Tz6GfIPmIz6LE0c3DB9YGNypJIFMHnv1HnX0Z3dr7y5HH1Vi8=","X-Received":"by 2002:a25:26ce:0:b0:dfa:4a4e:a601 with SMTP id\n\t3f1490d57ef6-dff154914edmr8122294276.41.1718635248282;\n\tMon, 17 Jun 2024 07:40:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>\n\t<20240617134914.GC10134@pendragon.ideasonboard.com>","In-Reply-To":"<20240617134914.GC10134@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Mon, 17 Jun 2024 15:40:37 +0100","Message-ID":"","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","To":"Laurent Pinchart ","Cc":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30007,"web_url":"https://patchwork.libcamera.org/comment/30007/","msgid":"<20240617162332.GA17726@pendragon.ideasonboard.com>","date":"2024-06-17T16:23:32","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Mon, Jun 17, 2024 at 03:40:37PM +0100, David Plowman wrote:\n> On Mon, 17 Jun 2024 at 14:49, Laurent Pinchart wrote:\n> > Hi Naush, David,\n> >\n> > Could you give this patch a review when you get the chance ?\n> >\n> > On Thu, Jun 13, 2024 at 04:39:43AM +0300, Laurent Pinchart wrote:\n> > > Now that deserializing a Pwl object from YAML data is possible using the\n> > > YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> > > prepare for its removal.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> > > src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> > > src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> > > src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> > > src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> > > src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> > > src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> > > 8 files changed, 20 insertions(+), 16 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> > > index 304629d6d4e5..5ca76dd98b4b 100644\n> > > --- a/src/ipa/rpi/controller/rpi/af.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> > > @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> > > readNumber(skipFrames, params, \"skip_frames\");\n> > >\n> > > if (params.contains(\"map\"))\n> > > - map.readYaml(params[\"map\"]);\n> > > + map = params[\"map\"].get(ipa::Pwl{});\n> \n> Just wondering how this works... YamlObject::get calls\n> YamlObject::Getter::get, and if the latter returns std::nullopt, then\n> the former returns the value that was passed in? (Scratching my head\n> ever so slightly...!)\n\nCorrect, the YamlObject::get(U &def) function return 'def' when the\nstd::optional is empty.\n\n> Looking at this, I suspect we should have been checking the return\n> code from readYaml (equivalently, an empty Pwl from get), but that's\n> clearly a thing for later, and not for this patch.\n\nI believe so. I went for bug-to-bug compatibility here :-)\n\n> > > else\n> > > LOG(RPiAf, Warning) << \"No map defined\";\n> > >\n> > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > index a381dd972215..cf2565a83836 100644\n> > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> > > return -EINVAL;\n> > > qHi = *value;\n> > >\n> > > - return yTarget.readYaml(params[\"y_target\"]);\n> > > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > > + return yTarget.empty() ? -EINVAL : 0;\n> > > }\n> > >\n> > > static std::tuple\n> > > @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> > > return ret;\n> > > }\n> > >\n> > > - ret = yTarget.readYaml(params[\"y_target\"]);\n> > > - if (ret)\n> > > - return ret;\n> > > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > > + if (yTarget.empty())\n> > > + return -EINVAL;\n> > >\n> > > speed = params[\"speed\"].get(0.2);\n> > > startupFrames = params[\"startup_frames\"].get(10);\n> > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> > > index 603953d7d863..003c8fa137f3 100644\n> > > --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> > > @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)\n> > > return -EINVAL;\n> > > lux = *value;\n> > >\n> > > - return prior.readYaml(params[\"prior\"]);\n> > > + prior = params[\"prior\"].get(ipa::Pwl{});\n> > > + return prior.empty() ? -EINVAL : 0;\n> > > }\n> > >\n> > > static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)\n> > > diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > > index 3272a1416ffa..e673964c1856 100644\n> > > --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > > @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> > > int ret;\n> > >\n> > > if (params.contains(\"saturation\")) {\n> > > - ret = config_.saturation.readYaml(params[\"saturation\"]);\n> > > - if (ret)\n> > > - return ret;\n> > > + config_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> > > + if (config_.saturation.empty())\n> > > + return -EINVAL;\n> > > }\n> > >\n> > > for (auto &p : params[\"ccms\"].asList()) {\n> > > diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > > index 66871a61ed28..9b37943ae9c9 100644\n> > > --- a/src/ipa/rpi/controller/rpi/contrast.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > > @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)\n> > > config_.hiHistogram = params[\"hi_histogram\"].get(0.95);\n> > > config_.hiLevel = params[\"hi_level\"].get(0.95);\n> > > config_.hiMax = params[\"hi_max\"].get(2000);\n> > > - return config_.gammaCurve.readYaml(params[\"gamma_curve\"]);\n> > > +\n> > > + config_.gammaCurve = params[\"gamma_curve\"].get(ipa::Pwl{});\n> > > + return config_.gammaCurve.empty() ? -EINVAL : 0;\n> > > }\n> > >\n> > > void Contrast::setBrightness(double brightness)\n> > > diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp\n> > > index c9c38ebff5ba..40e7191ba16a 100644\n> > > --- a/src/ipa/rpi/controller/rpi/geq.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/geq.cpp\n> > > @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)\n> > > }\n> > >\n> > > if (params.contains(\"strength\")) {\n> > > - int ret = config_.strength.readYaml(params[\"strength\"]);\n> > > - if (ret)\n> > > - return ret;\n> > > + config_.strength = params[\"strength\"].get(ipa::Pwl{});\n> > > + if (config_.strength.empty())\n> > > + return -EINVAL;\n> > > }\n> > >\n> > > return 0;\n> > > diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > > index d533a4ea4e65..f3da8291bf5d 100644\n> > > --- a/src/ipa/rpi/controller/rpi/hdr.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > > @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> > >\n> > > /* Lens shading related parameters. */\n> > > if (params.contains(\"spatial_gain_curve\")) {\n> > > - spatialGainCurve.readYaml(params[\"spatial_gain_curve\"]);\n> > > + spatialGainCurve = params[\"spatial_gain_curve\"].get(ipa::Pwl{});\n> > > } else if (params.contains(\"spatial_gain\")) {\n> > > double spatialGain = params[\"spatial_gain\"].get(2.0);\n> > > spatialGainCurve.append(0.0, spatialGain);\n> > > @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> > > iirStrength = params[\"iir_strength\"].get(8.0);\n> > > strength = params[\"strength\"].get(1.5);\n> > > if (tonemapEnable)\n> > > - tonemap.readYaml(params[\"tonemap\"]);\n> > > + tonemap = params[\"tonemap\"].get(ipa::Pwl{});\n> \n> Hmm, probably more return codes that weren't being checked...\n> \n> > > speed = params[\"speed\"].get(1.0);\n> > > if (params.contains(\"hi_quantile_targets\")) {\n> > > hiQuantileTargets = params[\"hi_quantile_targets\"].getList().value();\n> > > diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > > index 2dc50dfc8d2d..3422adfe7dee 100644\n> > > --- a/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > > +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > > @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)\n> > > config_.detailSlope = params[\"detail_slope\"].get(0.1);\n> > > config_.iirStrength = params[\"iir_strength\"].get(1.0);\n> > > config_.strength = params[\"strength\"].get(1.0);\n> > > - config_.tonemap.readYaml(params[\"tone_curve\"]);\n> > > + config_.tonemap = params[\"tone_curve\"].get(ipa::Pwl{});\n> \n> So yes, that all looks good to me. Possibly worth running it on a Pi\n> just to double check!\n> \n> Reviewed-by: David Plowman \n\nThank you.\n\nI'll test it on a Pi 4.\n\n> > > return 0;\n> > > }\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5E22DBD87C\n\tfor ;\n\tMon, 17 Jun 2024 16:23:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B53C6548F;\n\tMon, 17 Jun 2024 18:23:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 52C1D65489\n\tfor ;\n\tMon, 17 Jun 2024 18:23:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 664FE29A;\n\tMon, 17 Jun 2024 18:23:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"saP6VL5k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718641416;\n\tbh=JtAARNivO5TmAQESusc3tE5IoqIoVcrovkgGQbtlCnY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=saP6VL5kc4vQf1Ez7IqwoH7xzLlQ+E4YLilnj9PjjX4dfb5S8/mc6Hh51b7N5h2he\n\t8jm4nd4M22oNlwWvmwxoqSg38cziJJRcTugS/NtQKltb99GUOgKDXg37NdTBISLLwl\n\tJ7J90InL5rbmoQq0EOx2l5tNyOSVZO+Xt4TIGfgs=","Date":"Mon, 17 Jun 2024 19:23:32 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","Message-ID":"<20240617162332.GA17726@pendragon.ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>\n\t<20240617134914.GC10134@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30012,"web_url":"https://patchwork.libcamera.org/comment/30012/","msgid":"<20240617215738.GB17148@pendragon.ideasonboard.com>","date":"2024-06-17T21:57:38","subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 17, 2024 at 07:23:32PM +0300, Laurent Pinchart wrote:\n> On Mon, Jun 17, 2024 at 03:40:37PM +0100, David Plowman wrote:\n> > On Mon, 17 Jun 2024 at 14:49, Laurent Pinchart wrote:\n> > > Hi Naush, David,\n> > >\n> > > Could you give this patch a review when you get the chance ?\n> > >\n> > > On Thu, Jun 13, 2024 at 04:39:43AM +0300, Laurent Pinchart wrote:\n> > > > Now that deserializing a Pwl object from YAML data is possible using the\n> > > > YamlObject::get() function, replace all usage of Pwl::readYaml() to\n> > > > prepare for its removal.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/ipa/rpi/controller/rpi/af.cpp | 2 +-\n> > > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 9 +++++----\n> > > > src/ipa/rpi/controller/rpi/awb.cpp | 3 ++-\n> > > > src/ipa/rpi/controller/rpi/ccm.cpp | 6 +++---\n> > > > src/ipa/rpi/controller/rpi/contrast.cpp | 4 +++-\n> > > > src/ipa/rpi/controller/rpi/geq.cpp | 6 +++---\n> > > > src/ipa/rpi/controller/rpi/hdr.cpp | 4 ++--\n> > > > src/ipa/rpi/controller/rpi/tonemap.cpp | 2 +-\n> > > > 8 files changed, 20 insertions(+), 16 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rpi/controller/rpi/af.cpp b/src/ipa/rpi/controller/rpi/af.cpp\n> > > > index 304629d6d4e5..5ca76dd98b4b 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/af.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/af.cpp\n> > > > @@ -139,7 +139,7 @@ int Af::CfgParams::read(const libcamera::YamlObject ¶ms)\n> > > > readNumber(skipFrames, params, \"skip_frames\");\n> > > >\n> > > > if (params.contains(\"map\"))\n> > > > - map.readYaml(params[\"map\"]);\n> > > > + map = params[\"map\"].get(ipa::Pwl{});\n> > \n> > Just wondering how this works... YamlObject::get calls\n> > YamlObject::Getter::get, and if the latter returns std::nullopt, then\n> > the former returns the value that was passed in? (Scratching my head\n> > ever so slightly...!)\n> \n> Correct, the YamlObject::get(U &def) function return 'def' when the\n> std::optional is empty.\n> \n> > Looking at this, I suspect we should have been checking the return\n> > code from readYaml (equivalently, an empty Pwl from get), but that's\n> > clearly a thing for later, and not for this patch.\n> \n> I believe so. I went for bug-to-bug compatibility here :-)\n> \n> > > > else\n> > > > LOG(RPiAf, Warning) << \"No map defined\";\n> > > >\n> > > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > > index a381dd972215..cf2565a83836 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > > > @@ -130,7 +130,8 @@ int AgcConstraint::read(const libcamera::YamlObject ¶ms)\n> > > > return -EINVAL;\n> > > > qHi = *value;\n> > > >\n> > > > - return yTarget.readYaml(params[\"y_target\"]);\n> > > > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > > > + return yTarget.empty() ? -EINVAL : 0;\n> > > > }\n> > > >\n> > > > static std::tuple\n> > > > @@ -237,9 +238,9 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> > > > return ret;\n> > > > }\n> > > >\n> > > > - ret = yTarget.readYaml(params[\"y_target\"]);\n> > > > - if (ret)\n> > > > - return ret;\n> > > > + yTarget = params[\"y_target\"].get(ipa::Pwl{});\n> > > > + if (yTarget.empty())\n> > > > + return -EINVAL;\n> > > >\n> > > > speed = params[\"speed\"].get(0.2);\n> > > > startupFrames = params[\"startup_frames\"].get(10);\n> > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp\n> > > > index 603953d7d863..003c8fa137f3 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/awb.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp\n> > > > @@ -49,7 +49,8 @@ int AwbPrior::read(const libcamera::YamlObject ¶ms)\n> > > > return -EINVAL;\n> > > > lux = *value;\n> > > >\n> > > > - return prior.readYaml(params[\"prior\"]);\n> > > > + prior = params[\"prior\"].get(ipa::Pwl{});\n> > > > + return prior.empty() ? -EINVAL : 0;\n> > > > }\n> > > >\n> > > > static int readCtCurve(ipa::Pwl &ctR, ipa::Pwl &ctB, const libcamera::YamlObject ¶ms)\n> > > > diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > > > index 3272a1416ffa..e673964c1856 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/ccm.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/ccm.cpp\n> > > > @@ -71,9 +71,9 @@ int Ccm::read(const libcamera::YamlObject ¶ms)\n> > > > int ret;\n> > > >\n> > > > if (params.contains(\"saturation\")) {\n> > > > - ret = config_.saturation.readYaml(params[\"saturation\"]);\n> > > > - if (ret)\n> > > > - return ret;\n> > > > + config_.saturation = params[\"saturation\"].get(ipa::Pwl{});\n> > > > + if (config_.saturation.empty())\n> > > > + return -EINVAL;\n> > > > }\n> > > >\n> > > > for (auto &p : params[\"ccms\"].asList()) {\n> > > > diff --git a/src/ipa/rpi/controller/rpi/contrast.cpp b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > > > index 66871a61ed28..9b37943ae9c9 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/contrast.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/contrast.cpp\n> > > > @@ -53,7 +53,9 @@ int Contrast::read(const libcamera::YamlObject ¶ms)\n> > > > config_.hiHistogram = params[\"hi_histogram\"].get(0.95);\n> > > > config_.hiLevel = params[\"hi_level\"].get(0.95);\n> > > > config_.hiMax = params[\"hi_max\"].get(2000);\n> > > > - return config_.gammaCurve.readYaml(params[\"gamma_curve\"]);\n> > > > +\n> > > > + config_.gammaCurve = params[\"gamma_curve\"].get(ipa::Pwl{});\n> > > > + return config_.gammaCurve.empty() ? -EINVAL : 0;\n> > > > }\n> > > >\n> > > > void Contrast::setBrightness(double brightness)\n> > > > diff --git a/src/ipa/rpi/controller/rpi/geq.cpp b/src/ipa/rpi/controller/rpi/geq.cpp\n> > > > index c9c38ebff5ba..40e7191ba16a 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/geq.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/geq.cpp\n> > > > @@ -44,9 +44,9 @@ int Geq::read(const libcamera::YamlObject ¶ms)\n> > > > }\n> > > >\n> > > > if (params.contains(\"strength\")) {\n> > > > - int ret = config_.strength.readYaml(params[\"strength\"]);\n> > > > - if (ret)\n> > > > - return ret;\n> > > > + config_.strength = params[\"strength\"].get(ipa::Pwl{});\n> > > > + if (config_.strength.empty())\n> > > > + return -EINVAL;\n> > > > }\n> > > >\n> > > > return 0;\n> > > > diff --git a/src/ipa/rpi/controller/rpi/hdr.cpp b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > > > index d533a4ea4e65..f3da8291bf5d 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/hdr.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/hdr.cpp\n> > > > @@ -42,7 +42,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> > > >\n> > > > /* Lens shading related parameters. */\n> > > > if (params.contains(\"spatial_gain_curve\")) {\n> > > > - spatialGainCurve.readYaml(params[\"spatial_gain_curve\"]);\n> > > > + spatialGainCurve = params[\"spatial_gain_curve\"].get(ipa::Pwl{});\n> > > > } else if (params.contains(\"spatial_gain\")) {\n> > > > double spatialGain = params[\"spatial_gain\"].get(2.0);\n> > > > spatialGainCurve.append(0.0, spatialGain);\n> > > > @@ -66,7 +66,7 @@ void HdrConfig::read(const libcamera::YamlObject ¶ms, const std::string &mod\n> > > > iirStrength = params[\"iir_strength\"].get(8.0);\n> > > > strength = params[\"strength\"].get(1.5);\n> > > > if (tonemapEnable)\n> > > > - tonemap.readYaml(params[\"tonemap\"]);\n> > > > + tonemap = params[\"tonemap\"].get(ipa::Pwl{});\n> > \n> > Hmm, probably more return codes that weren't being checked...\n> > \n> > > > speed = params[\"speed\"].get(1.0);\n> > > > if (params.contains(\"hi_quantile_targets\")) {\n> > > > hiQuantileTargets = params[\"hi_quantile_targets\"].getList().value();\n> > > > diff --git a/src/ipa/rpi/controller/rpi/tonemap.cpp b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > > > index 2dc50dfc8d2d..3422adfe7dee 100644\n> > > > --- a/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > > > +++ b/src/ipa/rpi/controller/rpi/tonemap.cpp\n> > > > @@ -33,7 +33,7 @@ int Tonemap::read(const libcamera::YamlObject ¶ms)\n> > > > config_.detailSlope = params[\"detail_slope\"].get(0.1);\n> > > > config_.iirStrength = params[\"iir_strength\"].get(1.0);\n> > > > config_.strength = params[\"strength\"].get(1.0);\n> > > > - config_.tonemap.readYaml(params[\"tone_curve\"]);\n> > > > + config_.tonemap = params[\"tone_curve\"].get(ipa::Pwl{});\n> > \n> > So yes, that all looks good to me. Possibly worth running it on a Pi\n> > just to double check!\n> > \n> > Reviewed-by: David Plowman \n> \n> Thank you.\n> \n> I'll test it on a Pi 4.\n\nTested-by: Laurent Pinchart \n\n> > > > return 0;\n> > > > }\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AB7BEC3237\n\tfor ;\n\tMon, 17 Jun 2024 21:58:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9519A6548B;\n\tMon, 17 Jun 2024 23:58:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32DCD61A1C\n\tfor ;\n\tMon, 17 Jun 2024 23:58:00 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C7902183;\n\tMon, 17 Jun 2024 23:57:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ie93QC0H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718661463;\n\tbh=/G2PBy1F06NdWmE+HHqrRUhF8uxEuuLAqbPaWMfQ2Hc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ie93QC0Hk7GmBhpFArO7f//AcEFKo9XB+3RTcs7e8blNPAdUYs1wCMgf4I/vJtUEz\n\tmz5oZRh/gLVs2gR3Daz0DGHaLDqMR8BOaU7ibpZRpiVCxxZ7/L+Y+vuNCZa6w1Ox3r\n\toJ14EQpI1W1PUtRb7y7ajkKOmcKW0wZhLBj3IbxE=","Date":"Tue, 18 Jun 2024 00:57:38 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 10/11] ipa: rpi: controller: Replace Pwl::readYaml() with\n\tYamlObject::get()","Message-ID":"<20240617215738.GB17148@pendragon.ideasonboard.com>","References":"<20240613013944.23344-1-laurent.pinchart@ideasonboard.com>\n\t<20240613013944.23344-11-laurent.pinchart@ideasonboard.com>\n\t<20240617134914.GC10134@pendragon.ideasonboard.com>\n\t\n\t<20240617162332.GA17726@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240617162332.GA17726@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]