[{"id":33935,"web_url":"https://patchwork.libcamera.org/comment/33935/","msgid":"<174446250045.722505.11233046066604471234@ping.linuxembedded.co.uk>","date":"2025-04-12T12:55:00","subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-04-11 13:36:36)\n> Exposure compensation allows to over- or under-expose an\n> image by a given value. Add support for that in agc_mean_luminance.\n> \n> The added exposure compensation can lead to luminance target values that\n> are close or above saturation and are therefore never reachable. Add a\n> fix for that by limiting the maximum luminance target to 0.95.\n> \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Daniel Scally \n> \n> ---\n> \n> Changes in v2:\n> - Fixed compiler error that slipped through in v1\n> - Improved commit message to explain the luminance target limitation\n> - Collected tag\n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--\n> src/ipa/libipa/agc_mean_luminance.h | 6 ++++++\n> 2 files changed, 27 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 9154f083a510..f6cb7144b31a 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> */\n> static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> \n> +/*\n> + * Maximum relative luminance target\n> + *\n> + * This value limits the relative luminance target after applying the exposure\n> + * compensation. Targeting a value above this limit results in saturation\n> + * and the inability to regulate properly.\n> + */\n> +static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> +\n> /**\n> * \\struct AgcMeanLuminance::AgcConstraint\n> * \\brief The boundaries and target for an AeConstraintMode constraint\n> @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> */\n> \n> AgcMeanLuminance::AgcMeanLuminance()\n> - : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> + : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> {\n> }\n> \n> @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> return parseExposureModes(tuningData);\n> }\n> \n> +/**\n> + * \\fn AgcMeanLuminance::setExposureCompensation()\n> + * \\brief Set the exposure compensation value\n> + * \\param[in] gain The exposure compensation gain\n> + *\n> + * This function sets the exposure compensation value to be used in the\n> + * AGC calculations. It is expressed as gain instead of EV.\n> + */\n> +\n> /**\n> * \\brief Set the ExposureModeHelper limits for this class\n> * \\param[in] minExposureTime Minimum exposure time to allow\n> @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,\n> */\n> double AgcMeanLuminance::estimateInitialGain() const\n> {\n> - double yTarget = relativeLuminanceTarget_;\n> + double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> + kMaxRelativeLuminanceTarget);\n> double yGain = 1.0;\n> \n> /*\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index c41391cb0b73..cad7ef845487 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -44,6 +44,11 @@ public:\n> \n> int parseTuningData(const YamlObject &tuningData);\n> \n> + void setExposureCompensation(double gain)\n> + {\n> + exposureCompensation_ = gain;\n> + }\n> +\n> void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> double minGain, double maxGain);\n> \n> @@ -84,6 +89,7 @@ private:\n> double gain);\n> utils::Duration filterExposure(utils::Duration exposureValue);\n> \n> + double exposureCompensation_;\n\nThis seems like a 'streaming session' configuration parameter, but\nstoring this here means it will persist even between\nstop/start/reconfigure cycles.\n\nShould it be reset at configure() time or such ?\n\nOr do you expect each helper user to reset it at configure time ? Maybe\nthis means we need to bring the same basic IPA interface to the helpers\ndirectly to maintain the same call hierarchy/events through the whole\narchitecture (and I think even the metadata/control handling could then\nbe de-duplicated when the controls modify the helpers rather than\nplatform specific parameters ...)\n\n\n\n> uint64_t frameCount_;\n> utils::Duration filteredExposure_;\n> double relativeLuminanceTarget_;\n> -- \n> 2.43.0\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 9CB0CC3213\n\tfor ;\n\tSat, 12 Apr 2025 12:55:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D865F68AB2;\n\tSat, 12 Apr 2025 14:55:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C67B68AA9\n\tfor ;\n\tSat, 12 Apr 2025 14:55:04 +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 A521575A;\n\tSat, 12 Apr 2025 14:53:03 +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=\"SczT0MlA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1744462383;\n\tbh=3IMzEGi99tzdet2nXwVFVl0fAOHTChQRPZDnPLF0tZY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SczT0MlAf1LazN2s+j4XSU71fdNTdKT8WAAsspQPzvGDQnbvKwxEIT56lINXiIxd3\n\t7VYJGCOqEQh5h33eNdI/hupuyrQXZ1yGbZa0ahJRCobMIOAbnqid0kwUNdpwMM+Ysc\n\t+bji8T3deMpQklVPs8AjO7vJH6tQ2jE2hAnG9dKs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250411123641.2144530-9-stefan.klug@ideasonboard.com>","References":"<20250411123641.2144530-1-stefan.klug@ideasonboard.com>\n\t<20250411123641.2144530-9-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","From":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tDaniel Scally ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 12 Apr 2025 13:55:00 +0100","Message-ID":"<174446250045.722505.11233046066604471234@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":34194,"web_url":"https://patchwork.libcamera.org/comment/34194/","msgid":"<174700412969.116187.15560451914730282755@pyrite.rasen.tech>","date":"2025-05-11T22:55:29","subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-04-11 14:36:36)\n> Exposure compensation allows to over- or under-expose an\n> image by a given value. Add support for that in agc_mean_luminance.\n> \n> The added exposure compensation can lead to luminance target values that\n> are close or above saturation and are therefore never reachable. Add a\n> fix for that by limiting the maximum luminance target to 0.95.\n> \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Daniel Scally \n\nReviewed-by: Paul Elder \n\n> \n> ---\n> \n> Changes in v2:\n> - Fixed compiler error that slipped through in v1\n> - Improved commit message to explain the luminance target limitation\n> - Collected tag\n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--\n> src/ipa/libipa/agc_mean_luminance.h | 6 ++++++\n> 2 files changed, 27 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 9154f083a510..f6cb7144b31a 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> */\n> static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> \n> +/*\n> + * Maximum relative luminance target\n> + *\n> + * This value limits the relative luminance target after applying the exposure\n> + * compensation. Targeting a value above this limit results in saturation\n> + * and the inability to regulate properly.\n> + */\n> +static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> +\n> /**\n> * \\struct AgcMeanLuminance::AgcConstraint\n> * \\brief The boundaries and target for an AeConstraintMode constraint\n> @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> */\n> \n> AgcMeanLuminance::AgcMeanLuminance()\n> - : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> + : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> {\n> }\n> \n> @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> return parseExposureModes(tuningData);\n> }\n> \n> +/**\n> + * \\fn AgcMeanLuminance::setExposureCompensation()\n> + * \\brief Set the exposure compensation value\n> + * \\param[in] gain The exposure compensation gain\n> + *\n> + * This function sets the exposure compensation value to be used in the\n> + * AGC calculations. It is expressed as gain instead of EV.\n> + */\n> +\n> /**\n> * \\brief Set the ExposureModeHelper limits for this class\n> * \\param[in] minExposureTime Minimum exposure time to allow\n> @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,\n> */\n> double AgcMeanLuminance::estimateInitialGain() const\n> {\n> - double yTarget = relativeLuminanceTarget_;\n> + double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> + kMaxRelativeLuminanceTarget);\n> double yGain = 1.0;\n> \n> /*\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index c41391cb0b73..cad7ef845487 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -44,6 +44,11 @@ public:\n> \n> int parseTuningData(const YamlObject &tuningData);\n> \n> + void setExposureCompensation(double gain)\n> + {\n> + exposureCompensation_ = gain;\n> + }\n> +\n> void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> double minGain, double maxGain);\n> \n> @@ -84,6 +89,7 @@ private:\n> double gain);\n> utils::Duration filterExposure(utils::Duration exposureValue);\n> \n> + double exposureCompensation_;\n> uint64_t frameCount_;\n> utils::Duration filteredExposure_;\n> double relativeLuminanceTarget_;\n> -- \n> 2.43.0\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 597A8C3226\n\tfor ;\n\tSun, 11 May 2025 22:55:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 423D368D47;\n\tMon, 12 May 2025 00:55:35 +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 29D0A68B60\n\tfor ;\n\tMon, 12 May 2025 00:55:33 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2001:861:3a80:3300:4f2f:8c2c:b3ef:17d4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6AFA08FA;\n\tMon, 12 May 2025 00:55:18 +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=\"c/A2TcE5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747004118;\n\tbh=VTdJzDPGAZrYlKnYfnk/vtAfEMIqSv2cGufz7qy4Qqk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=c/A2TcE56dyPiFah23wSe4Gk3ozGR5Op5lfkBnv09z0Vb78cESv3YVxTueglMn3gm\n\tMM10QNYKoclb3BYPle5ImfVI9ccfwZmdc6xJt8bRlCqWJOzAW+Wc1hIFMaRwYVl35a\n\teOYv2gYRvChiZBWjWqkkcm8Z/kg1y5V2FOi3mVF4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250411123641.2144530-9-stefan.klug@ideasonboard.com>","References":"<20250411123641.2144530-1-stefan.klug@ideasonboard.com>\n\t<20250411123641.2144530-9-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","From":"Paul Elder ","Cc":"Stefan Klug ,\n\tDaniel Scally ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 12 May 2025 00:55:29 +0200","Message-ID":"<174700412969.116187.15560451914730282755@pyrite.rasen.tech>","User-Agent":"alot/0.0.0","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":34196,"web_url":"https://patchwork.libcamera.org/comment/34196/","msgid":"<20250511230817.GA29184@pendragon.ideasonboard.com>","date":"2025-05-11T23:08:17","subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Apr 11, 2025 at 02:36:36PM +0200, Stefan Klug wrote:\n> Exposure compensation allows to over- or under-expose an\n> image by a given value. Add support for that in agc_mean_luminance.\n> \n> The added exposure compensation can lead to luminance target values that\n> are close or above saturation and are therefore never reachable. Add a\n> fix for that by limiting the maximum luminance target to 0.95.\n> \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Daniel Scally \n> \n> ---\n> \n> Changes in v2:\n> - Fixed compiler error that slipped through in v1\n> - Improved commit message to explain the luminance target limitation\n> - Collected tag\n> ---\n> src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--\n> src/ipa/libipa/agc_mean_luminance.h | 6 ++++++\n> 2 files changed, 27 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 9154f083a510..f6cb7144b31a 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> */\n> static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> \n> +/*\n> + * Maximum relative luminance target\n> + *\n> + * This value limits the relative luminance target after applying the exposure\n> + * compensation. Targeting a value above this limit results in saturation\n> + * and the inability to regulate properly.\n> + */\n> +static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> +\n> /**\n> * \\struct AgcMeanLuminance::AgcConstraint\n> * \\brief The boundaries and target for an AeConstraintMode constraint\n> @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> */\n> \n> AgcMeanLuminance::AgcMeanLuminance()\n> -\t: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> +\t: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n\nLine wrap please.\n\n> {\n> }\n> \n> @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> \treturn parseExposureModes(tuningData);\n> }\n> \n> +/**\n> + * \\fn AgcMeanLuminance::setExposureCompensation()\n> + * \\brief Set the exposure compensation value\n> + * \\param[in] gain The exposure compensation gain\n> + *\n> + * This function sets the exposure compensation value to be used in the\n> + * AGC calculations. It is expressed as gain instead of EV.\n\nWhat's the rationale for expressing it as a gain instead of EV ?\n\n> + */\n> +\n> /**\n> * \\brief Set the ExposureModeHelper limits for this class\n> * \\param[in] minExposureTime Minimum exposure time to allow\n> @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,\n> */\n> double AgcMeanLuminance::estimateInitialGain() const\n> {\n> -\tdouble yTarget = relativeLuminanceTarget_;\n> +\tdouble yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> +\t\t\t\t kMaxRelativeLuminanceTarget);\n> \tdouble yGain = 1.0;\n> \n> \t/*\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index c41391cb0b73..cad7ef845487 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -44,6 +44,11 @@ public:\n> \n> \tint parseTuningData(const YamlObject &tuningData);\n> \n> +\tvoid setExposureCompensation(double gain)\n> +\t{\n> +\t\texposureCompensation_ = gain;\n> +\t}\n> +\n> \tvoid setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> \t\t double minGain, double maxGain);\n> \n> @@ -84,6 +89,7 @@ private:\n> \t\t\t\t double gain);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> \n> +\tdouble exposureCompensation_;\n> \tuint64_t frameCount_;\n> \tutils::Duration filteredExposure_;\n> \tdouble relativeLuminanceTarget_;","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 6EFFCC3226\n\tfor ;\n\tSun, 11 May 2025 23:08:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B85768D58;\n\tMon, 12 May 2025 01:08:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5A6268B70\n\tfor ;\n\tMon, 12 May 2025 01:08:24 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(noh63-h01-176-133-119-130.dsl.sta.abo.bbox.fr [176.133.119.130])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1C87D605;\n\tMon, 12 May 2025 01:08:10 +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=\"EkdzdlTp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747004890;\n\tbh=WcTTfPshTpUOIKdVoBRwr6oHUN4TZx13bNNQgR3+vTI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EkdzdlTpjS6Q1wGMGgP0VaaxYqGWkxGWWDhY5fw6147vy4FHzOnFBOMSBgRDG0QOS\n\tM7+f6YyklAf2oZd2xqGNpNdVI9mujI8ZDislWKYwKop+sn+t+0oy/4Fv5Xhja3F8eX\n\t3QjnCQDpWjkc/3SAFAd+CyPI9V5ax5+KIsouVg5U=","Date":"Mon, 12 May 2025 01:08:17 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDaniel Scally ","Subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","Message-ID":"<20250511230817.GA29184@pendragon.ideasonboard.com>","References":"<20250411123641.2144530-1-stefan.klug@ideasonboard.com>\n\t<20250411123641.2144530-9-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250411123641.2144530-9-stefan.klug@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":34619,"web_url":"https://patchwork.libcamera.org/comment/34619/","msgid":"<175069456026.159456.7155957609753088722@localhost>","date":"2025-06-23T16:02:40","subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nQuoting Laurent Pinchart (2025-05-12 01:08:17)\n> On Fri, Apr 11, 2025 at 02:36:36PM +0200, Stefan Klug wrote:\n> > Exposure compensation allows to over- or under-expose an\n> > image by a given value. Add support for that in agc_mean_luminance.\n> > \n> > The added exposure compensation can lead to luminance target values that\n> > are close or above saturation and are therefore never reachable. Add a\n> > fix for that by limiting the maximum luminance target to 0.95.\n> > \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Daniel Scally \n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Fixed compiler error that slipped through in v1\n> > - Improved commit message to explain the luminance target limitation\n> > - Collected tag\n> > ---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--\n> > src/ipa/libipa/agc_mean_luminance.h | 6 ++++++\n> > 2 files changed, 27 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 9154f083a510..f6cb7144b31a 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> > */\n> > static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > \n> > +/*\n> > + * Maximum relative luminance target\n> > + *\n> > + * This value limits the relative luminance target after applying the exposure\n> > + * compensation. Targeting a value above this limit results in saturation\n> > + * and the inability to regulate properly.\n> > + */\n> > +static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > +\n> > /**\n> > * \\struct AgcMeanLuminance::AgcConstraint\n> > * \\brief The boundaries and target for an AeConstraintMode constraint\n> > @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > */\n> > \n> > AgcMeanLuminance::AgcMeanLuminance()\n> > - : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> > + : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> \n> Line wrap please.\n> \n> > {\n> > }\n> > \n> > @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > return parseExposureModes(tuningData);\n> > }\n> > \n> > +/**\n> > + * \\fn AgcMeanLuminance::setExposureCompensation()\n> > + * \\brief Set the exposure compensation value\n> > + * \\param[in] gain The exposure compensation gain\n> > + *\n> > + * This function sets the exposure compensation value to be used in the\n> > + * AGC calculations. It is expressed as gain instead of EV.\n> \n> What's the rationale for expressing it as a gain instead of EV ?\n\nI still owe you an answer on this one. The idea was that everything\ninside AgcMeanLuminance is handled as gains. To me EV is more of a\nphotograpy related unit than a technical unit. But other than that,\nI don't have a real argument. So I can as well express that as EV. Maybe\nthat is even cleaner...\n\nOpinions?\n\nBest regards,\nStefan\n\n> \n> > + */\n> > +\n> > /**\n> > * \\brief Set the ExposureModeHelper limits for this class\n> > * \\param[in] minExposureTime Minimum exposure time to allow\n> > @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,\n> > */\n> > double AgcMeanLuminance::estimateInitialGain() const\n> > {\n> > - double yTarget = relativeLuminanceTarget_;\n> > + double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > + kMaxRelativeLuminanceTarget);\n> > double yGain = 1.0;\n> > \n> > /*\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index c41391cb0b73..cad7ef845487 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -44,6 +44,11 @@ public:\n> > \n> > int parseTuningData(const YamlObject &tuningData);\n> > \n> > + void setExposureCompensation(double gain)\n> > + {\n> > + exposureCompensation_ = gain;\n> > + }\n> > +\n> > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > double minGain, double maxGain);\n> > \n> > @@ -84,6 +89,7 @@ private:\n> > double gain);\n> > utils::Duration filterExposure(utils::Duration exposureValue);\n> > \n> > + double exposureCompensation_;\n> > uint64_t frameCount_;\n> > utils::Duration filteredExposure_;\n> > double relativeLuminanceTarget_;\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 16EE4BDE6B\n\tfor ;\n\tMon, 23 Jun 2025 16:02:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D279768DE3;\n\tMon, 23 Jun 2025 18:02:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5EE0B68DC9\n\tfor ;\n\tMon, 23 Jun 2025 18:02:43 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:8dc5:f4ae:2b23:86a3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7523F6F3;\n\tMon, 23 Jun 2025 18:02:26 +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=\"anFmLAJS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1750694546;\n\tbh=2AV86if3E1vFRM8KaUwrz/m5tPSOOzC1RPSTJQabA7U=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=anFmLAJSjvtLNQGf/UwuQSEoELC5mbSuKHlCXXVdFfkJ8dgRdL9ZsCTktAFMvQ+UJ\n\tD/1vYb1V6l53iCrvcLA2/3LPM8A6segB2h6KksgS4hCcY8I2zaBZB3cI/xrfX+y80M\n\tnixOOw4eCfR++iH0QRZZD4tzI92mNRr0LFKK8EOM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250511230817.GA29184@pendragon.ideasonboard.com>","References":"<20250411123641.2144530-1-stefan.klug@ideasonboard.com>\n\t<20250411123641.2144530-9-stefan.klug@ideasonboard.com>\n\t<20250511230817.GA29184@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDaniel Scally ","To":"Laurent Pinchart ","Date":"Mon, 23 Jun 2025 18:02:40 +0200","Message-ID":"<175069456026.159456.7155957609753088722@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":"","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":34620,"web_url":"https://patchwork.libcamera.org/comment/34620/","msgid":"<175069493347.159456.5617277299563061835@localhost>","date":"2025-06-23T16:08:53","subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","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-04-12 14:55:00)\n> Quoting Stefan Klug (2025-04-11 13:36:36)\n> > Exposure compensation allows to over- or under-expose an\n> > image by a given value. Add support for that in agc_mean_luminance.\n> > \n> > The added exposure compensation can lead to luminance target values that\n> > are close or above saturation and are therefore never reachable. Add a\n> > fix for that by limiting the maximum luminance target to 0.95.\n> > \n> > Signed-off-by: Stefan Klug \n> > Reviewed-by: Daniel Scally \n> > \n> > ---\n> > \n> > Changes in v2:\n> > - Fixed compiler error that slipped through in v1\n> > - Improved commit message to explain the luminance target limitation\n> > - Collected tag\n> > ---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--\n> > src/ipa/libipa/agc_mean_luminance.h | 6 ++++++\n> > 2 files changed, 27 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 9154f083a510..f6cb7144b31a 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> > */\n> > static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > \n> > +/*\n> > + * Maximum relative luminance target\n> > + *\n> > + * This value limits the relative luminance target after applying the exposure\n> > + * compensation. Targeting a value above this limit results in saturation\n> > + * and the inability to regulate properly.\n> > + */\n> > +static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > +\n> > /**\n> > * \\struct AgcMeanLuminance::AgcConstraint\n> > * \\brief The boundaries and target for an AeConstraintMode constraint\n> > @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;\n> > */\n> > \n> > AgcMeanLuminance::AgcMeanLuminance()\n> > - : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> > + : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)\n> > {\n> > }\n> > \n> > @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)\n> > return parseExposureModes(tuningData);\n> > }\n> > \n> > +/**\n> > + * \\fn AgcMeanLuminance::setExposureCompensation()\n> > + * \\brief Set the exposure compensation value\n> > + * \\param[in] gain The exposure compensation gain\n> > + *\n> > + * This function sets the exposure compensation value to be used in the\n> > + * AGC calculations. It is expressed as gain instead of EV.\n> > + */\n> > +\n> > /**\n> > * \\brief Set the ExposureModeHelper limits for this class\n> > * \\param[in] minExposureTime Minimum exposure time to allow\n> > @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,\n> > */\n> > double AgcMeanLuminance::estimateInitialGain() const\n> > {\n> > - double yTarget = relativeLuminanceTarget_;\n> > + double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,\n> > + kMaxRelativeLuminanceTarget);\n> > double yGain = 1.0;\n> > \n> > /*\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index c41391cb0b73..cad7ef845487 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -44,6 +44,11 @@ public:\n> > \n> > int parseTuningData(const YamlObject &tuningData);\n> > \n> > + void setExposureCompensation(double gain)\n> > + {\n> > + exposureCompensation_ = gain;\n> > + }\n> > +\n> > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > double minGain, double maxGain);\n> > \n> > @@ -84,6 +89,7 @@ private:\n> > double gain);\n> > utils::Duration filterExposure(utils::Duration exposureValue);\n> > \n> > + double exposureCompensation_;\n> \n> This seems like a 'streaming session' configuration parameter, but\n> storing this here means it will persist even between\n> stop/start/reconfigure cycles.\n> \n> Should it be reset at configure() time or such ?\n> \n> Or do you expect each helper user to reset it at configure time ? Maybe\n> this means we need to bring the same basic IPA interface to the helpers\n> directly to maintain the same call hierarchy/events through the whole\n> architecture (and I think even the metadata/control handling could then\n> be de-duplicated when the controls modify the helpers rather than\n> platform specific parameters ...)\n\nI don't know if I'd like to push the same interface to the helpers. I\nlike to think of them as small building blocks for the actual pipeline.\nBut then when we want to have all the pipelines to behave similarly it\nmight make sense... oh my.\n\nRegarding the actual issue. In the actual implementation (next patch)\nthe function gets called unconditionally with data from the frame\ncontext. So I don't think there is a practical problem here.\n\nBest regards,\nStefan\n\n> \n> \n> \n> > uint64_t frameCount_;\n> > utils::Duration filteredExposure_;\n> > double relativeLuminanceTarget_;\n> > -- \n> > 2.43.0\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 D930AC3237\n\tfor ;\n\tMon, 23 Jun 2025 16:08:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A5D268DCE;\n\tMon, 23 Jun 2025 18:08:59 +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 EFD8E68DC9\n\tfor ;\n\tMon, 23 Jun 2025 18:08:56 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:8dc5:f4ae:2b23:86a3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 1FEFE6F3;\n\tMon, 23 Jun 2025 18:08:40 +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=\"AlBq9Inw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1750694920;\n\tbh=NjtfaMnG5GbN6qKITgBYeG+dIyXgXVaBmleWRA/zrqU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=AlBq9Inw83pee0dceKTMqNL9vc27YrKOG+4PA42zdjr/fssa2hABKHawIbvY3asgW\n\tKRVaI65VQ3ps+YjNLxLLVj2XyUgvB5oeBO3L5/rgeOWWuxKH6h+oPSwPkOgmNiCoFa\n\t6FwPh6q+wyQ/pMTJ3hVPsIe24iwN2C8XM13eIv0o=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<174446250045.722505.11233046066604471234@ping.linuxembedded.co.uk>","References":"<20250411123641.2144530-1-stefan.klug@ideasonboard.com>\n\t<20250411123641.2144530-9-stefan.klug@ideasonboard.com>\n\t<174446250045.722505.11233046066604471234@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v2 8/9] libipa: agc_mean_luminance: Add exposure\n\tcompensation support","From":"Stefan Klug ","Cc":"Daniel Scally ","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 23 Jun 2025 18:08:53 +0200","Message-ID":"<175069493347.159456.5617277299563061835@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":"","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\" "}}]