[{"id":36740,"web_url":"https://patchwork.libcamera.org/comment/36740/","msgid":"<176245123168.2421127.5648927085034468452@ping.linuxembedded.co.uk>","date":"2025-11-06T17:47:11","subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-11-06 16:42:26)\n> In some situations it is necessary to specify the target brightness\n> value depending on the overall lux level. This is a rework [1] to fit\n> current master. As the PWL loading code loads a plain value as single\n> point PWL, backwards compatibility to existing tuning files is ensured.\n> \n> [1] https://patchwork.libcamera.org/patch/20231/\n\nreworking [1] to master should not be in the commit message. When this\nis merged, old versions are irrelevant.\n\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n> \n> ---\n> \n> Changes in v2:\n> - Collected tags\n> - Dropped special handling of plain values, as this is now part of the\n>   yaml PWL loader.\n> ---\n>  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----\n>  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--\n>  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +\n>  3 files changed, 55 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 64f36bd75dd2..b80af92a2c0f 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n>   */\n>  static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>  \n> +/*\n> + * Default lux level\n> + *\n> + * If no lux level or a zero lux level is specified, but PWLs are used to\n> + * specify luminance targets, this default level is used.\n> + */\n> +static constexpr unsigned int kDefaultLuxLevel = 500;\n> +\n>  /**\n>   * \\struct AgcMeanLuminance::AgcConstraint\n>   * \\brief The boundaries and target for an AeConstraintMode constraint\n> @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>  \n>  AgcMeanLuminance::AgcMeanLuminance()\n>         : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> -         relativeLuminanceTarget_(0)\n> +         lux_(0)\n>  {\n>  }\n>  \n>  AgcMeanLuminance::~AgcMeanLuminance() = default;\n>  \n> -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n>  {\n> -       relativeLuminanceTarget_ =\n> -               tuningData[\"relativeLuminanceTarget\"].get<double>(kDefaultRelativeLuminanceTarget);\n> +       auto &target = tuningData[\"relativeLuminanceTarget\"];\n> +       auto pwl = target.get<Pwl>();\n> +       if (!pwl) {\n> +               LOG(AgcMeanLuminance, Error)\n> +                       << \"Failed to load relative luminance target.\";\n> +               return -EINVAL;\n> +       }\n> +\n> +       relativeLuminanceTarget_ = std::move(*pwl);\n> +       return 0;\n>  }\n>  \n>  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n>  {\n>         int ret;\n>  \n> -       parseRelativeLuminanceTarget(tuningData);\n> +       ret = parseRelativeLuminanceTarget(tuningData);\n> +       if (ret)\n> +               return ret;\n>  \n>         ret = parseConstraintModes(tuningData);\n>         if (ret)\n> @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n>   * AGC calculations. It is expressed as gain instead of EV.\n>   */\n>  \n> +/**\n> + * \\fn AgcMeanLuminance::setLux(int lux)\n> + * \\brief Set the lux level\n> + * \\param[in] lux The lux level\n> + *\n> + * This function sets the lux level to be used in the AGC calculations. A value\n> + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> + * if necessary.\n> + */\n> +\n>  /**\n>   * \\brief Set the ExposureModeHelper limits for this class\n>   * \\param[in] minExposureTime Minimum exposure time to allow\n> @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n>   */\n>  double AgcMeanLuminance::effectiveYTarget() const\n>  {\n> -       return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> +       double lux = lux_;\n> +       if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> +               LOG(AgcMeanLuminance, Debug)\n> +                       << \"Missing lux value for luminance target calculation, default to \"\n> +                       << kDefaultLuxLevel;\n> +               lux = kDefaultLuxLevel;\n> +       }\n> +\n> +       double luminanceTarget = relativeLuminanceTarget_.eval(\n> +               relativeLuminanceTarget_.domain().clamp(lux));\n> +\n> +       return std::min(luminanceTarget * exposureCompensation_,\n>                         kMaxRelativeLuminanceTarget);\n>  }\n>  \n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index d7ec548e3e58..1fbff304a72b 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -20,6 +20,7 @@\n>  \n>  #include \"exposure_mode_helper.h\"\n>  #include \"histogram.h\"\n> +#include \"pwl.h\"\n>  \n>  namespace libcamera {\n>  \n> @@ -50,6 +51,11 @@ public:\n>                 exposureCompensation_ = gain;\n>         }\n>  \n> +       void setLux(unsigned int lux)\n> +       {\n> +               lux_ = lux;\n> +       }\n> +\n>         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n>                        double minGain, double maxGain, std::vector<AgcConstraint> constraints);\n>  \n> @@ -81,8 +87,8 @@ public:\n>  \n>  private:\n>         virtual double estimateLuminance(const double gain) const = 0;\n> +       int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n>  \n> -       void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n>         void parseConstraint(const YamlObject &modeDict, int32_t id);\n>         int parseConstraintModes(const YamlObject &tuningData);\n>         int parseExposureModes(const YamlObject &tuningData);\n> @@ -95,7 +101,8 @@ private:\n>         double exposureCompensation_;\n>         uint64_t frameCount_;\n>         utils::Duration filteredExposure_;\n> -       double relativeLuminanceTarget_;\n> +       unsigned int lux_;\n> +       Pwl relativeLuminanceTarget_;\n>  \n>         std::vector<AgcConstraint> additionalConstraints_;\n>         std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb69..1ecaff680978 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>                 effectiveExposureValue *= frameContext.agc.quantizationGain;\n>  \n>         setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> +       setLux(frameContext.lux.lux);\n\nShould this be from the active state (the most current/latest known lux\nlevel) rather than the frame context? Or do we know that the lux is run\nbefore this and is correct for this frame ?\n\n(Especially as Lux comes after Agc alphabetically)\n\nDo we need to do anything to enforce that lux is run /before/ AGC\notherwise?\n\nOtherwise I expect we'd see it default to zero and therefore default to\nthe kDefaultLux level 'always'?\n\n--\nKieran\n\n\n>  \n>         utils::Duration newExposureTime;\n>         double aGain, qGain, dGain;\n> -- \n> 2.51.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 85DC5BDE4C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  6 Nov 2025 17:47:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA577609D8;\n\tThu,  6 Nov 2025 18:47:16 +0100 (CET)","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 AB82E606E6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  6 Nov 2025 18:47:14 +0100 (CET)","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 745A9591;\n\tThu,  6 Nov 2025 18:45:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jhn6ZNse\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762451119;\n\tbh=/fF/7Hxf4mss2ZUzLUaWl/mZwWgaiCcucoY+olTV5tg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=jhn6ZNseV+aCFjvhH21EC/jiThiaTFC6Zj623Hj5LHkT4u3RHEimjiJyHCYZeO7/D\n\tZ7yz/545lJW/CbJSjbRHqAge3zeg3Fp3E9v/F00YoN2Ih62JWbaujw5N+VhoMZCxus\n\tjWst+1ZXbBtg9VCUMmOaWctc/gcRXkBzw7NSa1ak=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251106164239.460738-3-stefan.klug@ideasonboard.com>","References":"<20251106164239.460738-1-stefan.klug@ideasonboard.com>\n\t<20251106164239.460738-3-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tPaul Elder <paul.elder@ideasonboard.com>,\n\tDaniel Scally <dan.scally@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 06 Nov 2025 17:47:11 +0000","Message-ID":"<176245123168.2421127.5648927085034468452@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36824,"web_url":"https://patchwork.libcamera.org/comment/36824/","msgid":"<pb6ibcddou4xoezzvdj5zk76uqnjdrojsazybdzllwopaffdgn@czyiadxtladz>","date":"2025-11-14T16:51:19","subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:\n> In some situations it is necessary to specify the target brightness\n> value depending on the overall lux level. This is a rework [1] to fit\n> current master. As the PWL loading code loads a plain value as single\n> point PWL, backwards compatibility to existing tuning files is ensured.\n>\n> [1] https://patchwork.libcamera.org/patch/20231/\n>\nAs Kieran said\n\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n>\n> ---\n>\n> Changes in v2:\n> - Collected tags\n> - Dropped special handling of plain values, as this is now part of the\n>   yaml PWL loader.\n> ---\n>  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----\n>  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--\n>  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +\n>  3 files changed, 55 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 64f36bd75dd2..b80af92a2c0f 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n>   */\n>  static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>\n> +/*\n> + * Default lux level\n> + *\n> + * If no lux level or a zero lux level is specified, but PWLs are used to\n> + * specify luminance targets, this default level is used.\n> + */\n> +static constexpr unsigned int kDefaultLuxLevel = 500;\n> +\n>  /**\n>   * \\struct AgcMeanLuminance::AgcConstraint\n>   * \\brief The boundaries and target for an AeConstraintMode constraint\n> @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>\n>  AgcMeanLuminance::AgcMeanLuminance()\n>  \t: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> -\t  relativeLuminanceTarget_(0)\n> +\t  lux_(0)\n>  {\n>  }\n>\n>  AgcMeanLuminance::~AgcMeanLuminance() = default;\n>\n> -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n>  {\n> -\trelativeLuminanceTarget_ =\n> -\t\ttuningData[\"relativeLuminanceTarget\"].get<double>(kDefaultRelativeLuminanceTarget);\n> +\tauto &target = tuningData[\"relativeLuminanceTarget\"];\n> +\tauto pwl = target.get<Pwl>();\n> +\tif (!pwl) {\n> +\t\tLOG(AgcMeanLuminance, Error)\n> +\t\t\t<< \"Failed to load relative luminance target.\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\trelativeLuminanceTarget_ = std::move(*pwl);\n> +\treturn 0;\n>  }\n>\n>  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n>  {\n>  \tint ret;\n>\n> -\tparseRelativeLuminanceTarget(tuningData);\n> +\tret = parseRelativeLuminanceTarget(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n>\n>  \tret = parseConstraintModes(tuningData);\n>  \tif (ret)\n> @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n>   * AGC calculations. It is expressed as gain instead of EV.\n>   */\n>\n> +/**\n> + * \\fn AgcMeanLuminance::setLux(int lux)\n> + * \\brief Set the lux level\n> + * \\param[in] lux The lux level\n> + *\n> + * This function sets the lux level to be used in the AGC calculations. A value\n> + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> + * if necessary.\n> + */\n> +\n>  /**\n>   * \\brief Set the ExposureModeHelper limits for this class\n>   * \\param[in] minExposureTime Minimum exposure time to allow\n> @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n>   */\n>  double AgcMeanLuminance::effectiveYTarget() const\n>  {\n> -\treturn std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> +\tdouble lux = lux_;\n> +\tif (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n\nI might have missed why lux is mandatory only if\nrelativeLuminanceTarget_.size() > 1\nbut that's just me not knowing well how this work.\n\n\n> +\t\tLOG(AgcMeanLuminance, Debug)\n> +\t\t\t<< \"Missing lux value for luminance target calculation, default to \"\n> +\t\t\t<< kDefaultLuxLevel;\n\nBut I wonder if this message will appear for every frame ?\n\n> +\t\tlux = kDefaultLuxLevel;\n> +\t}\n> +\n> +\tdouble luminanceTarget = relativeLuminanceTarget_.eval(\n> +\t\trelativeLuminanceTarget_.domain().clamp(lux));\n> +\n> +\treturn std::min(luminanceTarget * exposureCompensation_,\n>  \t\t\tkMaxRelativeLuminanceTarget);\n>  }\n>\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index d7ec548e3e58..1fbff304a72b 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -20,6 +20,7 @@\n>\n>  #include \"exposure_mode_helper.h\"\n>  #include \"histogram.h\"\n> +#include \"pwl.h\"\n>\n>  namespace libcamera {\n>\n> @@ -50,6 +51,11 @@ public:\n>  \t\texposureCompensation_ = gain;\n>  \t}\n>\n> +\tvoid setLux(unsigned int lux)\n> +\t{\n> +\t\tlux_ = lux;\n> +\t}\n> +\n>  \tvoid setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n>  \t\t       double minGain, double maxGain, std::vector<AgcConstraint> constraints);\n>\n> @@ -81,8 +87,8 @@ public:\n>\n>  private:\n>  \tvirtual double estimateLuminance(const double gain) const = 0;\n> +\tint parseRelativeLuminanceTarget(const YamlObject &tuningData);\n>\n> -\tvoid parseRelativeLuminanceTarget(const YamlObject &tuningData);\n>  \tvoid parseConstraint(const YamlObject &modeDict, int32_t id);\n>  \tint parseConstraintModes(const YamlObject &tuningData);\n>  \tint parseExposureModes(const YamlObject &tuningData);\n> @@ -95,7 +101,8 @@ private:\n>  \tdouble exposureCompensation_;\n>  \tuint64_t frameCount_;\n>  \tutils::Duration filteredExposure_;\n> -\tdouble relativeLuminanceTarget_;\n> +\tunsigned int lux_;\n> +\tPwl relativeLuminanceTarget_;\n\nvery minor nit: it's nicer if you invert the two declarations :)\n\nquestions apart\nReviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nThanks\n  j\n\n>\n>  \tstd::vector<AgcConstraint> additionalConstraints_;\n>  \tstd::map<int32_t, std::vector<AgcConstraint>> constraintModes_;\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb69..1ecaff680978 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n>  \t\teffectiveExposureValue *= frameContext.agc.quantizationGain;\n>\n>  \tsetExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> +\tsetLux(frameContext.lux.lux);\n>\n>  \tutils::Duration newExposureTime;\n>  \tdouble aGain, qGain, dGain;\n> --\n> 2.51.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 279ACC3241\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 14 Nov 2025 16:51:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27BC960A80;\n\tFri, 14 Nov 2025 17:51:24 +0100 (CET)","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 98426606E6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Nov 2025 17:51:22 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F267664;\n\tFri, 14 Nov 2025 17:49:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YA53Fppj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763138961;\n\tbh=8amy0GqwlTAHRSlYZvDt51ouZRdHIYDWpuibygQ+bBs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YA53FppjflHv1CYckCvgorPERKwHv1XrJXneqhqlAMynDHWitgaZvXtK/71qEoOVw\n\tFNywWIEWTE4Z+DBcUGxlEj+Orvh2WQMWNCAWswuJajJBUdS8fBLvyjE5I97xfVoGzm\n\tv03e3xo+pUB0nySz+ynuz1UvPQiXNW6ZceMrAjO0=","Date":"Fri, 14 Nov 2025 17:51:19 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder <paul.elder@ideasonboard.com>,\n\tDaniel Scally <dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","Message-ID":"<pb6ibcddou4xoezzvdj5zk76uqnjdrojsazybdzllwopaffdgn@czyiadxtladz>","References":"<20251106164239.460738-1-stefan.klug@ideasonboard.com>\n\t<20251106164239.460738-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251106164239.460738-3-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36826,"web_url":"https://patchwork.libcamera.org/comment/36826/","msgid":"<176313994524.83626.1391057866390371007@localhost>","date":"2025-11-14T17:05:45","subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nQuoting Kieran Bingham (2025-11-06 18:47:11)\n> Quoting Stefan Klug (2025-11-06 16:42:26)\n> > In some situations it is necessary to specify the target brightness\n> > value depending on the overall lux level. This is a rework [1] to fit\n> > current master. As the PWL loading code loads a plain value as single\n> > point PWL, backwards compatibility to existing tuning files is ensured.\n> > \n> > [1] https://patchwork.libcamera.org/patch/20231/\n> \n> reworking [1] to master should not be in the commit message. When this\n> is merged, old versions are irrelevant.\n\nRight, I'll drop that or move it to the changelog.\n\n> \n> > \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Collected tags\n> > - Dropped special handling of plain values, as this is now part of the\n> >   yaml PWL loader.\n> > ---\n> >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----\n> >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--\n> >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +\n> >  3 files changed, 55 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 64f36bd75dd2..b80af92a2c0f 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> >   */\n> >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> >  \n> > +/*\n> > + * Default lux level\n> > + *\n> > + * If no lux level or a zero lux level is specified, but PWLs are used to\n> > + * specify luminance targets, this default level is used.\n> > + */\n> > +static constexpr unsigned int kDefaultLuxLevel = 500;\n> > +\n> >  /**\n> >   * \\struct AgcMeanLuminance::AgcConstraint\n> >   * \\brief The boundaries and target for an AeConstraintMode constraint\n> > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> >  \n> >  AgcMeanLuminance::AgcMeanLuminance()\n> >         : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> > -         relativeLuminanceTarget_(0)\n> > +         lux_(0)\n> >  {\n> >  }\n> >  \n> >  AgcMeanLuminance::~AgcMeanLuminance() = default;\n> >  \n> > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> >  {\n> > -       relativeLuminanceTarget_ =\n> > -               tuningData[\"relativeLuminanceTarget\"].get<double>(kDefaultRelativeLuminanceTarget);\n> > +       auto &target = tuningData[\"relativeLuminanceTarget\"];\n> > +       auto pwl = target.get<Pwl>();\n> > +       if (!pwl) {\n> > +               LOG(AgcMeanLuminance, Error)\n> > +                       << \"Failed to load relative luminance target.\";\n> > +               return -EINVAL;\n> > +       }\n> > +\n> > +       relativeLuminanceTarget_ = std::move(*pwl);\n> > +       return 0;\n> >  }\n> >  \n> >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> >  {\n> >         int ret;\n> >  \n> > -       parseRelativeLuminanceTarget(tuningData);\n> > +       ret = parseRelativeLuminanceTarget(tuningData);\n> > +       if (ret)\n> > +               return ret;\n> >  \n> >         ret = parseConstraintModes(tuningData);\n> >         if (ret)\n> > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> >   * AGC calculations. It is expressed as gain instead of EV.\n> >   */\n> >  \n> > +/**\n> > + * \\fn AgcMeanLuminance::setLux(int lux)\n> > + * \\brief Set the lux level\n> > + * \\param[in] lux The lux level\n> > + *\n> > + * This function sets the lux level to be used in the AGC calculations. A value\n> > + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> > + * if necessary.\n> > + */\n> > +\n> >  /**\n> >   * \\brief Set the ExposureModeHelper limits for this class\n> >   * \\param[in] minExposureTime Minimum exposure time to allow\n> > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> >   */\n> >  double AgcMeanLuminance::effectiveYTarget() const\n> >  {\n> > -       return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > +       double lux = lux_;\n> > +       if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> > +               LOG(AgcMeanLuminance, Debug)\n> > +                       << \"Missing lux value for luminance target calculation, default to \"\n> > +                       << kDefaultLuxLevel;\n> > +               lux = kDefaultLuxLevel;\n> > +       }\n> > +\n> > +       double luminanceTarget = relativeLuminanceTarget_.eval(\n> > +               relativeLuminanceTarget_.domain().clamp(lux));\n> > +\n> > +       return std::min(luminanceTarget * exposureCompensation_,\n> >                         kMaxRelativeLuminanceTarget);\n> >  }\n> >  \n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index d7ec548e3e58..1fbff304a72b 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -20,6 +20,7 @@\n> >  \n> >  #include \"exposure_mode_helper.h\"\n> >  #include \"histogram.h\"\n> > +#include \"pwl.h\"\n> >  \n> >  namespace libcamera {\n> >  \n> > @@ -50,6 +51,11 @@ public:\n> >                 exposureCompensation_ = gain;\n> >         }\n> >  \n> > +       void setLux(unsigned int lux)\n> > +       {\n> > +               lux_ = lux;\n> > +       }\n> > +\n> >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> >                        double minGain, double maxGain, std::vector<AgcConstraint> constraints);\n> >  \n> > @@ -81,8 +87,8 @@ public:\n> >  \n> >  private:\n> >         virtual double estimateLuminance(const double gain) const = 0;\n> > +       int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> >  \n> > -       void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> >         void parseConstraint(const YamlObject &modeDict, int32_t id);\n> >         int parseConstraintModes(const YamlObject &tuningData);\n> >         int parseExposureModes(const YamlObject &tuningData);\n> > @@ -95,7 +101,8 @@ private:\n> >         double exposureCompensation_;\n> >         uint64_t frameCount_;\n> >         utils::Duration filteredExposure_;\n> > -       double relativeLuminanceTarget_;\n> > +       unsigned int lux_;\n> > +       Pwl relativeLuminanceTarget_;\n> >  \n> >         std::vector<AgcConstraint> additionalConstraints_;\n> >         std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb69..1ecaff680978 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> >                 effectiveExposureValue *= frameContext.agc.quantizationGain;\n> >  \n> >         setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> > +       setLux(frameContext.lux.lux);\n> \n> Should this be from the active state (the most current/latest known lux\n> level) rather than the frame context? Or do we know that the lux is run\n> before this and is correct for this frame ?\n> \n> (Especially as Lux comes after Agc alphabetically)\n> \n> Do we need to do anything to enforce that lux is run /before/ AGC\n> otherwise?\n> \n> Otherwise I expect we'd see it default to zero and therefore default to\n> the kDefaultLux level 'always'?\n\nDang, I didn't look to closely at the logic here, as it needs rework in\nthe context of the regulation series anyways. But that fix wasn't ready\nyet (but in use on all of my testpaths). But you're right the current\nlogic is too flaky. So I'll look again at the missing patch and will\ninclude it in this series even before the regulation rework.\n\nBest regards,\nStefan\n\n> \n> --\n> Kieran\n> \n> \n> >  \n> >         utils::Duration newExposureTime;\n> >         double aGain, qGain, dGain;\n> > -- \n> > 2.51.0\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 F29E7C3263\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 14 Nov 2025 17:05:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 279D360A8B;\n\tFri, 14 Nov 2025 18:05:49 +0100 (CET)","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 A5712606E6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Nov 2025 18:05:47 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3cb4:651a:fe0:73ab])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7B179161;\n\tFri, 14 Nov 2025 18:03:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DYz5U6I+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763139826;\n\tbh=kwGTZb2wOdSaMqKzf5AD16Cj1dcJvCgSdLMxKI7Krlc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=DYz5U6I+OJWlik5NFlQV0fDN+8s5F0O4xuTnn6bH/wHR7YMgJohmcqax8WRl2CTGI\n\tpewKAmrn/2Rc4FEREhfwkG9x2AicvvOULjxVFU+I5XTk404p8ragMWn9zTbIeSHwsW\n\txl0LgjoGJ7177BzmhL0rIEwuUd+Hwd77+heJ6Y0c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176245123168.2421127.5648927085034468452@ping.linuxembedded.co.uk>","References":"<20251106164239.460738-1-stefan.klug@ideasonboard.com>\n\t<20251106164239.460738-3-stefan.klug@ideasonboard.com>\n\t<176245123168.2421127.5648927085034468452@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","From":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"Paul Elder <paul.elder@ideasonboard.com>,\n\tDaniel Scally <dan.scally@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 14 Nov 2025 18:05:45 +0100","Message-ID":"<176313994524.83626.1391057866390371007@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36828,"web_url":"https://patchwork.libcamera.org/comment/36828/","msgid":"<176314096983.83626.1537374219841635497@localhost>","date":"2025-11-14T17:22:49","subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nQuoting Jacopo Mondi (2025-11-14 17:51:19)\n> Hi Stefan\n> \n> On Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:\n> > In some situations it is necessary to specify the target brightness\n> > value depending on the overall lux level. This is a rework [1] to fit\n> > current master. As the PWL loading code loads a plain value as single\n> > point PWL, backwards compatibility to existing tuning files is ensured.\n> >\n> > [1] https://patchwork.libcamera.org/patch/20231/\n> >\n> As Kieran said\n> \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n> >\n> > ---\n> >\n> > Changes in v2:\n> > - Collected tags\n> > - Dropped special handling of plain values, as this is now part of the\n> >   yaml PWL loader.\n> > ---\n> >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----\n> >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--\n> >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +\n> >  3 files changed, 55 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 64f36bd75dd2..b80af92a2c0f 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> >   */\n> >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> >\n> > +/*\n> > + * Default lux level\n> > + *\n> > + * If no lux level or a zero lux level is specified, but PWLs are used to\n> > + * specify luminance targets, this default level is used.\n> > + */\n> > +static constexpr unsigned int kDefaultLuxLevel = 500;\n> > +\n> >  /**\n> >   * \\struct AgcMeanLuminance::AgcConstraint\n> >   * \\brief The boundaries and target for an AeConstraintMode constraint\n> > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> >\n> >  AgcMeanLuminance::AgcMeanLuminance()\n> >       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> > -       relativeLuminanceTarget_(0)\n> > +       lux_(0)\n> >  {\n> >  }\n> >\n> >  AgcMeanLuminance::~AgcMeanLuminance() = default;\n> >\n> > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> >  {\n> > -     relativeLuminanceTarget_ =\n> > -             tuningData[\"relativeLuminanceTarget\"].get<double>(kDefaultRelativeLuminanceTarget);\n> > +     auto &target = tuningData[\"relativeLuminanceTarget\"];\n> > +     auto pwl = target.get<Pwl>();\n> > +     if (!pwl) {\n> > +             LOG(AgcMeanLuminance, Error)\n> > +                     << \"Failed to load relative luminance target.\";\n> > +             return -EINVAL;\n> > +     }\n> > +\n> > +     relativeLuminanceTarget_ = std::move(*pwl);\n> > +     return 0;\n> >  }\n> >\n> >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> >  {\n> >       int ret;\n> >\n> > -     parseRelativeLuminanceTarget(tuningData);\n> > +     ret = parseRelativeLuminanceTarget(tuningData);\n> > +     if (ret)\n> > +             return ret;\n> >\n> >       ret = parseConstraintModes(tuningData);\n> >       if (ret)\n> > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> >   * AGC calculations. It is expressed as gain instead of EV.\n> >   */\n> >\n> > +/**\n> > + * \\fn AgcMeanLuminance::setLux(int lux)\n> > + * \\brief Set the lux level\n> > + * \\param[in] lux The lux level\n> > + *\n> > + * This function sets the lux level to be used in the AGC calculations. A value\n> > + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> > + * if necessary.\n> > + */\n> > +\n> >  /**\n> >   * \\brief Set the ExposureModeHelper limits for this class\n> >   * \\param[in] minExposureTime Minimum exposure time to allow\n> > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> >   */\n> >  double AgcMeanLuminance::effectiveYTarget() const\n> >  {\n> > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > +     double lux = lux_;\n> > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> \n> I might have missed why lux is mandatory only if\n> relativeLuminanceTarget_.size() > 1\n> but that's just me not knowing well how this work.\n\nIf in the tuning file a single value is provided instead of an array, the\nPWL is reduced to a single-point PWL returning always the same value. So\nthe relativeLuminanceTarget is no longer lux dependent.\n\n> \n> \n> > +             LOG(AgcMeanLuminance, Debug)\n> > +                     << \"Missing lux value for luminance target calculation, default to \"\n> > +                     << kDefaultLuxLevel;\n> \n> But I wonder if this message will appear for every frame ?\n\nYes, it would appear on every frame. It is debug only so it usually\ndoesn't show up. I was thinking about making it info level, as it\nbasically shows a error in the tuning file. Why would one create a\ntuning file with a lux dependant luminance target but not include the\nlux algorithm?\n\nIdeally I'd like to have a LOG_ONCE and then make this a warning. But\nthat would be another rabbit hole :-)\n\n> \n> > +             lux = kDefaultLuxLevel;\n> > +     }\n> > +\n> > +     double luminanceTarget = relativeLuminanceTarget_.eval(\n> > +             relativeLuminanceTarget_.domain().clamp(lux));\n> > +\n> > +     return std::min(luminanceTarget * exposureCompensation_,\n> >                       kMaxRelativeLuminanceTarget);\n> >  }\n> >\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index d7ec548e3e58..1fbff304a72b 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -20,6 +20,7 @@\n> >\n> >  #include \"exposure_mode_helper.h\"\n> >  #include \"histogram.h\"\n> > +#include \"pwl.h\"\n> >\n> >  namespace libcamera {\n> >\n> > @@ -50,6 +51,11 @@ public:\n> >               exposureCompensation_ = gain;\n> >       }\n> >\n> > +     void setLux(unsigned int lux)\n> > +     {\n> > +             lux_ = lux;\n> > +     }\n> > +\n> >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> >                      double minGain, double maxGain, std::vector<AgcConstraint> constraints);\n> >\n> > @@ -81,8 +87,8 @@ public:\n> >\n> >  private:\n> >       virtual double estimateLuminance(const double gain) const = 0;\n> > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> >\n> > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> >       void parseConstraint(const YamlObject &modeDict, int32_t id);\n> >       int parseConstraintModes(const YamlObject &tuningData);\n> >       int parseExposureModes(const YamlObject &tuningData);\n> > @@ -95,7 +101,8 @@ private:\n> >       double exposureCompensation_;\n> >       uint64_t frameCount_;\n> >       utils::Duration filteredExposure_;\n> > -     double relativeLuminanceTarget_;\n> > +     unsigned int lux_;\n> > +     Pwl relativeLuminanceTarget_;\n> \n> very minor nit: it's nicer if you invert the two declarations :)\n\nI often struggle with the order of members. When to add a blank line,\nwhen to keep them in block and whether to order by logic or\nalphabetically. Alphabetically is the hint I heard most often so I went\nfor that in this case. What is your hint here?\n\n> \n> questions apart\n> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nThanks,\nStefan\n\n> \n> Thanks\n>   j\n> \n> >\n> >       std::vector<AgcConstraint> additionalConstraints_;\n> >       std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb69..1ecaff680978 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> >               effectiveExposureValue *= frameContext.agc.quantizationGain;\n> >\n> >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> > +     setLux(frameContext.lux.lux);\n> >\n> >       utils::Duration newExposureTime;\n> >       double aGain, qGain, dGain;\n> > --\n> > 2.51.0\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 ECEF8C3241\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 14 Nov 2025 17:22:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 917ED60A9E;\n\tFri, 14 Nov 2025 18:22:54 +0100 (CET)","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 2753B609DE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Nov 2025 18:22:53 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3cb4:651a:fe0:73ab])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 372E5169;\n\tFri, 14 Nov 2025 18:20:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DThuWJPm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763140852;\n\tbh=0eUkV/PSSB0CnKWWKCr0Xzg69WmLzxyxYZMuejcAFIE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=DThuWJPmyQkpYoBCzKYTtSeqsEzw7aK8RoTLLCfFv/97UEndghP4RC5bA9HOrOXSm\n\tG/pXSziOhDjbDNdRWP8oaHpgSvR0Hli2KgQSJtc6UVPD5wfErGEG74xY9B4fp/hM0m\n\t/THGJtp3aQFiiZ1OQobmOVLkW42mJjeTxBQlHi08=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<pb6ibcddou4xoezzvdj5zk76uqnjdrojsazybdzllwopaffdgn@czyiadxtladz>","References":"<20251106164239.460738-1-stefan.klug@ideasonboard.com>\n\t<20251106164239.460738-3-stefan.klug@ideasonboard.com>\n\t<pb6ibcddou4xoezzvdj5zk76uqnjdrojsazybdzllwopaffdgn@czyiadxtladz>","Subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","From":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder <paul.elder@ideasonboard.com>,\n\tDaniel Scally <dan.scally@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Date":"Fri, 14 Nov 2025 18:22:49 +0100","Message-ID":"<176314096983.83626.1537374219841635497@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36843,"web_url":"https://patchwork.libcamera.org/comment/36843/","msgid":"<gii3lgud2cz2owg34ef7wuagj7cxswarz4rnnvpyv5nm5rqgq5@ghvpjdlyg75n>","date":"2025-11-14T19:30:43","subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Fri, Nov 14, 2025 at 06:22:49PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Quoting Jacopo Mondi (2025-11-14 17:51:19)\n> > Hi Stefan\n> >\n> > On Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:\n> > > In some situations it is necessary to specify the target brightness\n> > > value depending on the overall lux level. This is a rework [1] to fit\n> > > current master. As the PWL loading code loads a plain value as single\n> > > point PWL, backwards compatibility to existing tuning files is ensured.\n> > >\n> > > [1] https://patchwork.libcamera.org/patch/20231/\n> > >\n> > As Kieran said\n> >\n> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n> > >\n> > > ---\n> > >\n> > > Changes in v2:\n> > > - Collected tags\n> > > - Dropped special handling of plain values, as this is now part of the\n> > >   yaml PWL loader.\n> > > ---\n> > >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----\n> > >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--\n> > >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +\n> > >  3 files changed, 55 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > index 64f36bd75dd2..b80af92a2c0f 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > >   */\n> > >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > >\n> > > +/*\n> > > + * Default lux level\n> > > + *\n> > > + * If no lux level or a zero lux level is specified, but PWLs are used to\n> > > + * specify luminance targets, this default level is used.\n> > > + */\n> > > +static constexpr unsigned int kDefaultLuxLevel = 500;\n> > > +\n> > >  /**\n> > >   * \\struct AgcMeanLuminance::AgcConstraint\n> > >   * \\brief The boundaries and target for an AeConstraintMode constraint\n> > > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > >\n> > >  AgcMeanLuminance::AgcMeanLuminance()\n> > >       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> > > -       relativeLuminanceTarget_(0)\n> > > +       lux_(0)\n> > >  {\n> > >  }\n> > >\n> > >  AgcMeanLuminance::~AgcMeanLuminance() = default;\n> > >\n> > > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > >  {\n> > > -     relativeLuminanceTarget_ =\n> > > -             tuningData[\"relativeLuminanceTarget\"].get<double>(kDefaultRelativeLuminanceTarget);\n> > > +     auto &target = tuningData[\"relativeLuminanceTarget\"];\n> > > +     auto pwl = target.get<Pwl>();\n> > > +     if (!pwl) {\n> > > +             LOG(AgcMeanLuminance, Error)\n> > > +                     << \"Failed to load relative luminance target.\";\n> > > +             return -EINVAL;\n> > > +     }\n> > > +\n> > > +     relativeLuminanceTarget_ = std::move(*pwl);\n> > > +     return 0;\n> > >  }\n> > >\n> > >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > >  {\n> > >       int ret;\n> > >\n> > > -     parseRelativeLuminanceTarget(tuningData);\n> > > +     ret = parseRelativeLuminanceTarget(tuningData);\n> > > +     if (ret)\n> > > +             return ret;\n> > >\n> > >       ret = parseConstraintModes(tuningData);\n> > >       if (ret)\n> > > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > >   * AGC calculations. It is expressed as gain instead of EV.\n> > >   */\n> > >\n> > > +/**\n> > > + * \\fn AgcMeanLuminance::setLux(int lux)\n> > > + * \\brief Set the lux level\n> > > + * \\param[in] lux The lux level\n> > > + *\n> > > + * This function sets the lux level to be used in the AGC calculations. A value\n> > > + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> > > + * if necessary.\n> > > + */\n> > > +\n> > >  /**\n> > >   * \\brief Set the ExposureModeHelper limits for this class\n> > >   * \\param[in] minExposureTime Minimum exposure time to allow\n> > > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > >   */\n> > >  double AgcMeanLuminance::effectiveYTarget() const\n> > >  {\n> > > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > > +     double lux = lux_;\n> > > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> >\n> > I might have missed why lux is mandatory only if\n> > relativeLuminanceTarget_.size() > 1\n> > but that's just me not knowing well how this work.\n>\n> If in the tuning file a single value is provided instead of an array, the\n> PWL is reduced to a single-point PWL returning always the same value. So\n> the relativeLuminanceTarget is no longer lux dependent.\n>\n> >\n> >\n> > > +             LOG(AgcMeanLuminance, Debug)\n> > > +                     << \"Missing lux value for luminance target calculation, default to \"\n> > > +                     << kDefaultLuxLevel;\n> >\n> > But I wonder if this message will appear for every frame ?\n>\n> Yes, it would appear on every frame. It is debug only so it usually\n> doesn't show up. I was thinking about making it info level, as it\n> basically shows a error in the tuning file. Why would one create a\n> tuning file with a lux dependant luminance target but not include the\n> lux algorithm?\n>\n> Ideally I'd like to have a LOG_ONCE and then make this a warning. But\n> that would be another rabbit hole :-)\n\nmmm, I'll let you judge if every frame is too much or not (I think it\nis :) as this a config file error and could be reported once only.\n\nYou can use a static variable or some other trick to rate-limit the\nmessage up to you.\n\n>\n> >\n> > > +             lux = kDefaultLuxLevel;\n> > > +     }\n> > > +\n> > > +     double luminanceTarget = relativeLuminanceTarget_.eval(\n> > > +             relativeLuminanceTarget_.domain().clamp(lux));\n> > > +\n> > > +     return std::min(luminanceTarget * exposureCompensation_,\n> > >                       kMaxRelativeLuminanceTarget);\n> > >  }\n> > >\n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > > index d7ec548e3e58..1fbff304a72b 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > > @@ -20,6 +20,7 @@\n> > >\n> > >  #include \"exposure_mode_helper.h\"\n> > >  #include \"histogram.h\"\n> > > +#include \"pwl.h\"\n> > >\n> > >  namespace libcamera {\n> > >\n> > > @@ -50,6 +51,11 @@ public:\n> > >               exposureCompensation_ = gain;\n> > >       }\n> > >\n> > > +     void setLux(unsigned int lux)\n> > > +     {\n> > > +             lux_ = lux;\n> > > +     }\n> > > +\n> > >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > >                      double minGain, double maxGain, std::vector<AgcConstraint> constraints);\n> > >\n> > > @@ -81,8 +87,8 @@ public:\n> > >\n> > >  private:\n> > >       virtual double estimateLuminance(const double gain) const = 0;\n> > > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > >\n> > > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > >       void parseConstraint(const YamlObject &modeDict, int32_t id);\n> > >       int parseConstraintModes(const YamlObject &tuningData);\n> > >       int parseExposureModes(const YamlObject &tuningData);\n> > > @@ -95,7 +101,8 @@ private:\n> > >       double exposureCompensation_;\n> > >       uint64_t frameCount_;\n> > >       utils::Duration filteredExposure_;\n> > > -     double relativeLuminanceTarget_;\n> > > +     unsigned int lux_;\n> > > +     Pwl relativeLuminanceTarget_;\n> >\n> > very minor nit: it's nicer if you invert the two declarations :)\n>\n> I often struggle with the order of members. When to add a blank line,\n> when to keep them in block and whether to order by logic or\n> alphabetically. Alphabetically is the hint I heard most often so I went\n> for that in this case. What is your hint here?\n\nSince you asked...\n\nReverse xmas tree!\n\nisn't it much nicer\nto have lines\nsorted as a\nreverse\nx-mas\ntree\n\n?\n\nThis is just OCD though. I used to suggest it for Linux drivers, but\npeople not always apreciate it, so I only do that when the ordering\nis already somewhat like this and someones adds a new variable\n\nand messes up\nmy\nonce perfect\nx-xmas\ntree\n\n>\n> >\n> > questions apart\n> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n>\n> Thanks,\n> Stefan\n>\n> >\n> > Thanks\n> >   j\n> >\n> > >\n> > >       std::vector<AgcConstraint> additionalConstraints_;\n> > >       std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index f5a3c917cb69..1ecaff680978 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > >               effectiveExposureValue *= frameContext.agc.quantizationGain;\n> > >\n> > >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> > > +     setLux(frameContext.lux.lux);\n> > >\n> > >       utils::Duration newExposureTime;\n> > >       double aGain, qGain, dGain;\n> > > --\n> > > 2.51.0\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 B4770C3241\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 14 Nov 2025 19:30:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB54A60A80;\n\tFri, 14 Nov 2025 20:30:48 +0100 (CET)","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 664C8606E6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 14 Nov 2025 20:30:47 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15ED7397;\n\tFri, 14 Nov 2025 20:28:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PvSF0g2N\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763148526;\n\tbh=qf+8Ldss29agXKqoHP4ybyzrIkk1V1BWkUfWzBKbOq0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PvSF0g2NQMasELETZFtPkBDCuuwo/M6ZepLcy6T4ZKcLc33HhxctBWDwycRKmjlrZ\n\tCnxd2yhJyAFM1Na4FoVGAqs42tcg5BShmZCC3WMWSB41BRDzWwQ2zDx3JxIjpdrqoy\n\tJfgheAYK7cdGITDB4B7nIiwBfzIGzlffExg7N4Eg=","Date":"Fri, 14 Nov 2025 20:30:43 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Stefan Klug <stefan.klug@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder <paul.elder@ideasonboard.com>, \n\tDaniel Scally <dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","Message-ID":"<gii3lgud2cz2owg34ef7wuagj7cxswarz4rnnvpyv5nm5rqgq5@ghvpjdlyg75n>","References":"<20251106164239.460738-1-stefan.klug@ideasonboard.com>\n\t<20251106164239.460738-3-stefan.klug@ideasonboard.com>\n\t<pb6ibcddou4xoezzvdj5zk76uqnjdrojsazybdzllwopaffdgn@czyiadxtladz>\n\t<176314096983.83626.1537374219841635497@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176314096983.83626.1537374219841635497@localhost>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36846,"web_url":"https://patchwork.libcamera.org/comment/36846/","msgid":"<1c0f329c-17ee-49c8-85c1-45a63c2ae10b@oss.qualcomm.com>","date":"2025-11-16T10:07:35","subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":242,"url":"https://patchwork.libcamera.org/api/people/242/","name":"Hans de Goede","email":"johannes.goede@oss.qualcomm.com"},"content":"Hi,\n\nOn 14-Nov-25 8:30 PM, Jacopo Mondi wrote:\n> Hi Stefan\n> \n> On Fri, Nov 14, 2025 at 06:22:49PM +0100, Stefan Klug wrote:\n\n...\n\n>>>> @@ -95,7 +101,8 @@ private:\n>>>>       double exposureCompensation_;\n>>>>       uint64_t frameCount_;\n>>>>       utils::Duration filteredExposure_;\n>>>> -     double relativeLuminanceTarget_;\n>>>> +     unsigned int lux_;\n>>>> +     Pwl relativeLuminanceTarget_;\n>>>\n>>> very minor nit: it's nicer if you invert the two declarations :)\n>>\n>> I often struggle with the order of members. When to add a blank line,\n>> when to keep them in block and whether to order by logic or\n>> alphabetically. Alphabetically is the hint I heard most often so I went\n>> for that in this case. What is your hint here?\n> \n> Since you asked...\n> \n> Reverse xmas tree!\n> \n> isn't it much nicer\n> to have lines\n> sorted as a\n> reverse\n> x-mas\n> tree\n> \n> ?\n> \n> This is just OCD though. I used to suggest it for Linux drivers, but\n> people not always apreciate it, so I only do that when the ordering\n> is already somewhat like this and someones adds a new variable\n\nUsing reverse xmas tree is actually somewhat of an unwritten rule for\nvariable declarations in the kernel which gets requested by lots of\nkernel reviewers.\n\nSince AFAICT libcamera tends to mostly follow the kernel coding style\nJacopo's suggestion to swap these 2 gets a +1 from me.\n\nRegards,\n\nHans","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E57D0C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 16 Nov 2025 10:07:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDB6160A8B;\n\tSun, 16 Nov 2025 11:07:41 +0100 (CET)","from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com\n\t[205.220.180.131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBD7E606D5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 16 Nov 2025 11:07:39 +0100 (CET)","from pps.filterd (m0279868.ppops.net [127.0.0.1])\n\tby mx0a-0031df01.pphosted.com (8.18.1.11/8.18.1.11) with ESMTP id\n\t5AG7Jo3v465453 for <libcamera-devel@lists.libcamera.org>;\n\tSun, 16 Nov 2025 10:07:38 GMT","from mail-qt1-f198.google.com (mail-qt1-f198.google.com\n\t[209.85.160.198])\n\tby mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 4aejh01yba-1\n\t(version=TLSv1.3 cipher=TLS_AES_128_GCM_SHA256 bits=128 verify=NOT)\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 16 Nov 2025 10:07:38 +0000 (GMT)","by mail-qt1-f198.google.com with SMTP id\n\td75a77b69052e-4e884663b25so93789911cf.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 16 Nov 2025 02:07:38 -0800 (PST)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-b734fa80bf5sm819808466b.3.2025.11.16.02.07.36\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 16 Nov 2025 02:07:36 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=qualcomm.com header.i=@qualcomm.com\n\theader.b=\"UUO7yXa1\"; dkim=pass (2048-bit key;\n\tunprotected) header.d=oss.qualcomm.com header.i=@oss.qualcomm.com\n\theader.b=\"fHi1tCyY\"; dkim-atps=neutral","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=qualcomm.com; h=\n\tcc:content-transfer-encoding:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to; s=qcppdkim1; bh=\n\tmgq2zm/Vd5Askg/o0jCHB3i+5E8wLki/gLmvGjS1Sz4=; b=UUO7yXa15HUKPkA8\n\tQO7X5uJqwM/BkBq/a+L5+EUl6+mlqf3QDXmzP29dP36C0UC/huI1DzPVVrw8T236\n\t6FnsxSr/hSQNjOC1jOUyLpT2+HDDgTwzLwD7ZCNNdPS2yKTEcgy4uO1llFH3nPr1\n\trSArqb1jWVuYa6A4a/2LDn9GeHOjeTopg1cVeNKccvPT1Z9a0nZxzRBoiC7iFyR5\n\ts3sobKuI0Mw8ZS50Ut6KNOB1FCvVRE6fyhh/giauB8HyfdiLJv3EYMmslAYoX2fR\n\tWeJ/QRvZRIoNdTpxp/VBA+CbcbhySVNHWe7bXUIqbfkiw9LfrVDVtjIF+C6Yzh97\n\tSQDnng==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=oss.qualcomm.com; s=google; t=1763287658; x=1763892458;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:content-language:references\n\t:cc:to:subject:from:user-agent:mime-version:date:message-id:from:to\n\t:cc:subject:date:message-id:reply-to;\n\tbh=mgq2zm/Vd5Askg/o0jCHB3i+5E8wLki/gLmvGjS1Sz4=;\n\tb=fHi1tCyYo7GFun9DkdmCVaCtPSb263BWzECaZo5119E4+J64dO+i43cvudGvLaKMHJ\n\t7zu1T6MTaoMgOTRjo0YnWFMaJmFePwOmITSgEMIQTchLa2nvoixKtAG+SZ+1qDruamAe\n\t/B/cGF28CU0SA5k6NY+9J9gjJJuWlgNKNWTx+lsb6seMgtXaTdi2ZGQyCvJsQEB8KrF2\n\taXZV9gaVqiZPCmsVv1fjZ8RDU99wr6D8DcIXIVQMaTw6LlONn84CuUCiqorlES6ynCEB\n\tg3Zbr16uAwuY7wMyc6KDn2uBfvxusaeASM8RpheXn2jsM32ndJ3qtMXohhTsTXpQ5NPX\n\tOk8A=="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1763287658; x=1763892458;\n\th=content-transfer-encoding:in-reply-to:content-language:references\n\t:cc:to:subject:from:user-agent:mime-version:date:message-id:x-gm-gg\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=mgq2zm/Vd5Askg/o0jCHB3i+5E8wLki/gLmvGjS1Sz4=;\n\tb=rvZB+ZER0weaUnobqSNIr7Nar2v2+B0TALagcyPK501itx+3suB0FsjRRkeuIRugIO\n\t3QUdqEkMLH9bNT2GEq/s1OeYZi1IPxlY4zQUS+pKuKSbZekPOpdDv5EMOY3GUF/tWq7V\n\tXmuXYs5LzJs9pZFSHLpLk/KgcWnKI7QsP1Cir19dYmpaaBqHbKIpU9IL7Qc8WVmJGZpD\n\tKEtxm+GcVdbZf0tA4bhpWIseBAd/GYcwvBVSKKxLhQIRfRXbbbhhxCWOj2Y2lUKrjCae\n\tsckWq/dYL8ZR4HdiuwBFnpcOYuaJlGVa8X7j1ojxwsY5Ipn5sEKX/UvEOVPX9JxnBHc6\n\tSI4A==","X-Gm-Message-State":"AOJu0Yzo+3hhYpOP6S1GAhqy3K8IL0FPSHnPXklWHgCLE6nVe6lB5qQI\n\ta9CEEWb8Um4jPjOd1bfJMQgqdKe8DX0LsyVq7Vln/5OhL00Jq9npBy8xNgl6ki/E4QUxsgPT5z8\n\t0zxRiO1gGcN/yteDV4zIvAkA1En+hgTQKfuXZd+pBZ11+wtvbtyFsFrwDVG9CBKMnStM/8hImSz\n\tSL","X-Gm-Gg":"ASbGncvnTmUD32ZCKkvsrgsNJrlBiFNjJnzo4g4kmSkl4CK8zlb8/qdGCn1BelY/Dv8\n\tl8tE/8rrUyzUBbkx3vLaFH3sROYJ2ZDMYpKcLpcLzuyhaaO3fv55Y+rj6le+zAGUD9Wkib9I6O+\n\tOdjiCurG42edxIiPr7Jo8+HBaTXOr3MO6A6tK9kmPBARnPdQ/N1U3ylsNW0PIDdx/x9c8mwgETg\n\tFMqcprO4lzcvBDovHJaBqAZICM66H0T6k5OBmtV6V09aeBUL6fZvcHCcgVrP9kt3zkKuVM3ztud\n\ttRxwgXQnXD/ZZ+1ponN2Y/rpI8ZcSIUqbp/bQ63tCnxQ1J+LGiudwvrYAqw20YYzThQF04Q2rzV\n\tuhNYNLcQE2EnBJF3TkCffWJf/g0QyL0FhEfS7OiMbvPy/zULW1Dmxabb4ROfaAsQIRCV7ufAHhO\n\tmwe95r1tmbspzHbV3rlXSXqFS7T9vWwbEY5qo/XSEeh2kk560JJbbbvHwOoE5YIljikw==","X-Received":["by 2002:a05:622a:11c8:b0:4eb:a0aa:28e with SMTP id\n\td75a77b69052e-4edf21610c7mr113153911cf.64.1763287657815; \n\tSun, 16 Nov 2025 02:07:37 -0800 (PST)","by 2002:a05:622a:11c8:b0:4eb:a0aa:28e with SMTP id\n\td75a77b69052e-4edf21610c7mr113153691cf.64.1763287657293; \n\tSun, 16 Nov 2025 02:07:37 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFZJyoTf00K+eBJI7iWAkSmw40etEVJZt/La1kpmeOr5DvlW46ymw6QhZW4nR77vAV6JK2/sg==","Message-ID":"<1c0f329c-17ee-49c8-85c1-45a63c2ae10b@oss.qualcomm.com>","Date":"Sun, 16 Nov 2025 11:07:35 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","From":"johannes.goede@oss.qualcomm.com","Subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tStefan Klug <stefan.klug@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder <paul.elder@ideasonboard.com>,\n\tDaniel Scally <dan.scally@ideasonboard.com>","References":"<20251106164239.460738-1-stefan.klug@ideasonboard.com>\n\t<20251106164239.460738-3-stefan.klug@ideasonboard.com>\n\t<pb6ibcddou4xoezzvdj5zk76uqnjdrojsazybdzllwopaffdgn@czyiadxtladz>\n\t<176314096983.83626.1537374219841635497@localhost>\n\t<gii3lgud2cz2owg34ef7wuagj7cxswarz4rnnvpyv5nm5rqgq5@ghvpjdlyg75n>","Content-Language":"en-US, nl","In-Reply-To":"<gii3lgud2cz2owg34ef7wuagj7cxswarz4rnnvpyv5nm5rqgq5@ghvpjdlyg75n>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-Authority-Analysis":"v=2.4 cv=W9U1lBWk c=1 sm=1 tr=0 ts=6919a26a cx=c_pps\n\ta=mPf7EqFMSY9/WdsSgAYMbA==:117 a=xqWC_Br6kY4A:10 a=IkcTkHD0fZMA:10\n\ta=6UeiqGixMTsA:10 a=s4-Qcg_JpJYA:10 a=VkNPw1HP01LnGYTKEx00:22\n\ta=XdXo_1psR5xiht4M68YA:9 a=QEXdDO2ut3YA:10 a=dawVfQjAaf238kedN5IG:22\n\ta=HhbK4dLum7pmb74im6QT:22","X-Proofpoint-GUID":"NZbNfsXCyKRlaGVjPpPCKqM1D4MGFhBi","X-Proofpoint-ORIG-GUID":"NZbNfsXCyKRlaGVjPpPCKqM1D4MGFhBi","X-Proofpoint-Spam-Details-Enc":"AW1haW4tMjUxMTE2MDA4NCBTYWx0ZWRfX0D/puwRtF+pN\n\t4pCks3L4Dbgrgk7IF0+j4DRJKypFfx9+ZTtACHtcxvPOgWz7+AmjIKmPqLR+Ww2PuIuNb3/XgpZ\n\t/Kf/LABysazIqjKsDDAJknQ/Nr+ZWK9m7vBi7f/mato/47TKZ3l4+ksMF1aoC8U5JFzNJ1E5O5i\n\tvuIpjCm6zsw4sulQdNWsYhVqLfMVMJkGj0Z86WKoa9SOZ85QTFCrNflKNqHyK5iZdEEpHBib8wZ\n\tH+8LtvIuLaefQf9b2n6h09+kbLmw8tCTBbcMKvry6oVQXS2uKdCoVwmlIb3JGQMhEcEPFbwR/yd\n\tg3hGc0q2Bq77UJAvi8IyJVtnG7KHBjf9n+A4AjMIdB9II8j79GAVNBGQDTGZhrwRUMK1sJ8tbp6\n\t6ehndqAoDpjrTsUfwvzxkScqqFQRaw==","X-Proofpoint-Virus-Version":"vendor=baseguard\n\tengine=ICAP:2.0.293, Aquarius:18.0.1121, Hydra:6.1.9,\n\tFMLib:17.12.100.49\n\tdefinitions=2025-11-16_04,2025-11-13_02,2025-10-01_01","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tsuspectscore=0 lowpriorityscore=0 impostorscore=0 adultscore=0\n\tmalwarescore=0\n\tspamscore=0 clxscore=1015 phishscore=0 priorityscore=1501 bulkscore=0\n\tclassifier=typeunknown authscore=0 authtc= authcc= route=outbound\n\tadjust=0\n\treason=mlx scancount=1 engine=8.22.0-2510240001\n\tdefinitions=main-2511160084","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36847,"web_url":"https://patchwork.libcamera.org/comment/36847/","msgid":"<20251116140241.GD31986@pendragon.ideasonboard.com>","date":"2025-11-16T14:02:41","subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Nov 14, 2025 at 08:30:43PM +0100, Jacopo Mondi wrote:\n> On Fri, Nov 14, 2025 at 06:22:49PM +0100, Stefan Klug wrote:\n> > Quoting Jacopo Mondi (2025-11-14 17:51:19)\n> > > On Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:\n> > > > In some situations it is necessary to specify the target brightness\n> > > > value depending on the overall lux level. This is a rework [1] to fit\n> > > > current master. As the PWL loading code loads a plain value as single\n> > > > point PWL, backwards compatibility to existing tuning files is ensured.\n> > > >\n> > > > [1] https://patchwork.libcamera.org/patch/20231/\n> > > >\n> > > As Kieran said\n> > >\n> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>\n> > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n> > > >\n> > > > ---\n> > > >\n> > > > Changes in v2:\n> > > > - Collected tags\n> > > > - Dropped special handling of plain values, as this is now part of the\n> > > >   yaml PWL loader.\n> > > > ---\n> > > >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----\n> > > >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--\n> > > >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +\n> > > >  3 files changed, 55 insertions(+), 8 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > index 64f36bd75dd2..b80af92a2c0f 100644\n> > > > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > > >   */\n> > > >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > > >\n> > > > +/*\n> > > > + * Default lux level\n> > > > + *\n> > > > + * If no lux level or a zero lux level is specified, but PWLs are used to\n> > > > + * specify luminance targets, this default level is used.\n> > > > + */\n> > > > +static constexpr unsigned int kDefaultLuxLevel = 500;\n> > > > +\n> > > >  /**\n> > > >   * \\struct AgcMeanLuminance::AgcConstraint\n> > > >   * \\brief The boundaries and target for an AeConstraintMode constraint\n> > > > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > > >\n> > > >  AgcMeanLuminance::AgcMeanLuminance()\n> > > >       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),\n> > > > -       relativeLuminanceTarget_(0)\n> > > > +       lux_(0)\n> > > >  {\n> > > >  }\n> > > >\n> > > >  AgcMeanLuminance::~AgcMeanLuminance() = default;\n> > > >\n> > > > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > > > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)\n> > > >  {\n> > > > -     relativeLuminanceTarget_ =\n> > > > -             tuningData[\"relativeLuminanceTarget\"].get<double>(kDefaultRelativeLuminanceTarget);\n> > > > +     auto &target = tuningData[\"relativeLuminanceTarget\"];\n> > > > +     auto pwl = target.get<Pwl>();\n> > > > +     if (!pwl) {\n> > > > +             LOG(AgcMeanLuminance, Error)\n> > > > +                     << \"Failed to load relative luminance target.\";\n> > > > +             return -EINVAL;\n> > > > +     }\n> > > > +\n> > > > +     relativeLuminanceTarget_ = std::move(*pwl);\n> > > > +     return 0;\n> > > >  }\n> > > >\n> > > >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)\n> > > > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > > >  {\n> > > >       int ret;\n> > > >\n> > > > -     parseRelativeLuminanceTarget(tuningData);\n> > > > +     ret = parseRelativeLuminanceTarget(tuningData);\n> > > > +     if (ret)\n> > > > +             return ret;\n> > > >\n> > > >       ret = parseConstraintModes(tuningData);\n> > > >       if (ret)\n> > > > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > > >   * AGC calculations. It is expressed as gain instead of EV.\n> > > >   */\n> > > >\n> > > > +/**\n> > > > + * \\fn AgcMeanLuminance::setLux(int lux)\n> > > > + * \\brief Set the lux level\n> > > > + * \\param[in] lux The lux level\n> > > > + *\n> > > > + * This function sets the lux level to be used in the AGC calculations. A value\n> > > > + * of 0 means no measurement and a default value of \\a kDefaultLuxLevel is used\n> > > > + * if necessary.\n> > > > + */\n> > > > +\n> > > >  /**\n> > > >   * \\brief Set the ExposureModeHelper limits for this class\n> > > >   * \\param[in] minExposureTime Minimum exposure time to allow\n> > > > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,\n> > > >   */\n> > > >  double AgcMeanLuminance::effectiveYTarget() const\n> > > >  {\n> > > > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > > > +     double lux = lux_;\n> > > > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {\n> > >\n> > > I might have missed why lux is mandatory only if\n> > > relativeLuminanceTarget_.size() > 1\n> > > but that's just me not knowing well how this work.\n> >\n> > If in the tuning file a single value is provided instead of an array, the\n> > PWL is reduced to a single-point PWL returning always the same value. So\n> > the relativeLuminanceTarget is no longer lux dependent.\n> >\n> > > > +             LOG(AgcMeanLuminance, Debug)\n> > > > +                     << \"Missing lux value for luminance target calculation, default to \"\n> > > > +                     << kDefaultLuxLevel;\n> > >\n> > > But I wonder if this message will appear for every frame ?\n> >\n> > Yes, it would appear on every frame. It is debug only so it usually\n> > doesn't show up. I was thinking about making it info level, as it\n> > basically shows a error in the tuning file. Why would one create a\n> > tuning file with a lux dependant luminance target but not include the\n> > lux algorithm?\n> >\n> > Ideally I'd like to have a LOG_ONCE and then make this a warning. But\n> > that would be another rabbit hole :-)\n> \n> mmm, I'll let you judge if every frame is too much or not (I think it\n> is :) as this a config file error and could be reported once only.\n> \n> You can use a static variable or some other trick to rate-limit the\n> message up to you.\n> \n> > > > +             lux = kDefaultLuxLevel;\n> > > > +     }\n> > > > +\n> > > > +     double luminanceTarget = relativeLuminanceTarget_.eval(\n> > > > +             relativeLuminanceTarget_.domain().clamp(lux));\n> > > > +\n> > > > +     return std::min(luminanceTarget * exposureCompensation_,\n> > > >                       kMaxRelativeLuminanceTarget);\n> > > >  }\n> > > >\n> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > > > index d7ec548e3e58..1fbff304a72b 100644\n> > > > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > > > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > > > @@ -20,6 +20,7 @@\n> > > >\n> > > >  #include \"exposure_mode_helper.h\"\n> > > >  #include \"histogram.h\"\n> > > > +#include \"pwl.h\"\n> > > >\n> > > >  namespace libcamera {\n> > > >\n> > > > @@ -50,6 +51,11 @@ public:\n> > > >               exposureCompensation_ = gain;\n> > > >       }\n> > > >\n> > > > +     void setLux(unsigned int lux)\n> > > > +     {\n> > > > +             lux_ = lux;\n> > > > +     }\n> > > > +\n> > > >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > > >                      double minGain, double maxGain, std::vector<AgcConstraint> constraints);\n> > > >\n> > > > @@ -81,8 +87,8 @@ public:\n> > > >\n> > > >  private:\n> > > >       virtual double estimateLuminance(const double gain) const = 0;\n> > > > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > > >\n> > > > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);\n> > > >       void parseConstraint(const YamlObject &modeDict, int32_t id);\n> > > >       int parseConstraintModes(const YamlObject &tuningData);\n> > > >       int parseExposureModes(const YamlObject &tuningData);\n> > > > @@ -95,7 +101,8 @@ private:\n> > > >       double exposureCompensation_;\n> > > >       uint64_t frameCount_;\n> > > >       utils::Duration filteredExposure_;\n> > > > -     double relativeLuminanceTarget_;\n> > > > +     unsigned int lux_;\n> > > > +     Pwl relativeLuminanceTarget_;\n> > >\n> > > very minor nit: it's nicer if you invert the two declarations :)\n> >\n> > I often struggle with the order of members. When to add a blank line,\n> > when to keep them in block and whether to order by logic or\n> > alphabetically. Alphabetically is the hint I heard most often so I went\n> > for that in this case. What is your hint here?\n> \n> Since you asked...\n> \n> Reverse xmas tree!\n> \n> isn't it much nicer\n> to have lines\n> sorted as a\n> reverse\n> x-mas\n> tree\n> \n> ?\n\nThat rule comes from the Linux kernel and meant for local variables. For\nC++ member variables, which are similar to C struct members, the kernel\nusually groups them by purpose, and we tend to separate groups by blank\nlines. There is no clear documented rule.\n\nWithin a group of related member variables, if there are no specific\nlogical order that makes sense, the reverse christmas tree order is a\ngood default.\n\nAll those rules are meant to improve readability, and that's the goal we\nshould target here. In addition, attention can also be given to\nperformance constraints (avoiding holes in structures, packing related\ndata in a single cache line, ...). That's mostly applicable to either\nstructures that are instantiated a very large number of times (to save\nmemory) or that are used in hot paths (to improve performance).\n\n> This is just OCD though. I used to suggest it for Linux drivers, but\n> people not always apreciate it, so I only do that when the ordering\n> is already somewhat like this and someones adds a new variable\n> \n> and messes up\n> my\n> once perfect\n> x-xmas\n> tree\n> \n> > > questions apart\n> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> >\n> > > >\n> > > >       std::vector<AgcConstraint> additionalConstraints_;\n> > > >       std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index f5a3c917cb69..1ecaff680978 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > >               effectiveExposureValue *= frameContext.agc.quantizationGain;\n> > > >\n> > > >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));\n> > > > +     setLux(frameContext.lux.lux);\n> > > >\n> > > >       utils::Duration newExposureTime;\n> > > >       double aGain, qGain, dGain;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7416ABD80A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 16 Nov 2025 14:02:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6DA3F60A8B;\n\tSun, 16 Nov 2025 15:02:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1559E6096B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 16 Nov 2025 15:02:56 +0100 (CET)","from pendragon.ideasonboard.com (82-203-165-222.bb.dnainternet.fi\n\t[82.203.165.222])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 66F748CB;\n\tSun, 16 Nov 2025 15:00:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LhRigrsJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763301653;\n\tbh=/9uCTFeUI2/XB8rxyGz/C1NT3LhwyVIAR59E42rAxQs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LhRigrsJirQWnXmaMTuhyZi+hIq/dgJlrWG1VVnCMg5l5qagyGrUBaGiAkiI2NL9D\n\t+x9X81XrK816AIHVz8p23q4eeF7gva3A9f+wYtC1FIoQOOwg7krnOg/roWC8W0EYRh\n\tFCI8pEemRs7ZavD2usBogvUkz5n/rtzOLmcSvFv8=","Date":"Sun, 16 Nov 2025 16:02:41 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Stefan Klug <stefan.klug@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder <paul.elder@ideasonboard.com>,\n\tDaniel Scally <dan.scally@ideasonboard.com>","Subject":"Re: [PATCH v2 2/3] ipa: libipa: agc_mean_luminance: Change luminance\n\ttarget to piecewise linear function","Message-ID":"<20251116140241.GD31986@pendragon.ideasonboard.com>","References":"<20251106164239.460738-1-stefan.klug@ideasonboard.com>\n\t<20251106164239.460738-3-stefan.klug@ideasonboard.com>\n\t<pb6ibcddou4xoezzvdj5zk76uqnjdrojsazybdzllwopaffdgn@czyiadxtladz>\n\t<176314096983.83626.1537374219841635497@localhost>\n\t<gii3lgud2cz2owg34ef7wuagj7cxswarz4rnnvpyv5nm5rqgq5@ghvpjdlyg75n>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<gii3lgud2cz2owg34ef7wuagj7cxswarz4rnnvpyv5nm5rqgq5@ghvpjdlyg75n>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]