[{"id":29054,"web_url":"https://patchwork.libcamera.org/comment/29054/","msgid":"","date":"2024-03-25T16:48:07","subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan, Paul\n\nOn Fri, Mar 22, 2024 at 01:14:44PM +0000, Daniel Scally wrote:\n> From: Paul Elder \n>\n> Add a helper for managing exposure modes and splitting exposure times\n> into shutter and gain values.\n>\n> Signed-off-by: Paul Elder \n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++\n> src/ipa/libipa/exposure_mode_helper.h | 61 +++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 370 insertions(+)\n> create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp\n> create mode 100644 src/ipa/libipa/exposure_mode_helper.h\n>\n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> new file mode 100644\n> index 00000000..9e01f908\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -0,0 +1,307 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n\n exposure_mode_helper.cpp\n\n> + */\n> +#include \"exposure_mode_helper.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file exposure_mode_helper.h\n> + * \\brief Helper class that performs computations relating to exposure\n> + *\n> + * Exposure modes contain a list of shutter and gain values to determine how to\n> + * split the supplied exposure time into shutter and gain. As this function is\n> + * expected to be replicated in various IPAs, this helper class factors it\n> + * away.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ExposureModeHelper)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class ExposureModeHelper\n> + * \\brief Class for splitting exposure time into shutter and gain\n> + */\n> +\n> +/**\n> + * \\brief Initialize an ExposureModeHelper instance\n> + */\n> +ExposureModeHelper::ExposureModeHelper()\n> +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> +{\n> +}\n> +\n> +ExposureModeHelper::~ExposureModeHelper()\n> +{\n> +}\n\nYou probably don't need this and can rely on the compiler generated\none\n\n> +\n> +/**\n> + * \\brief Initialize an ExposureModeHelper instance\n> + * \\param[in] shutter The list of shutter values\n> + * \\param[in] gain The list of gain values\n> + *\n> + * When splitting an exposure time into shutter and gain, the shutter will be\n> + * increased first before increasing the gain. This is done in stages, where\n> + * each stage is an index into both lists. Both lists consequently need to be\n> + * the same length.\n\nIs this expected to be called one time only ? At what time ? when the\nlibrary is loaded and the configuration file is parsed ? Is it worth\nmentioning it ?\n\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + */\n> +int ExposureModeHelper::init(std::vector &shutter, std::vector &gain)\n> +{\n> +\tif (shutter.size() != gain.size()) {\n> +\t\tLOG(ExposureModeHelper, Error)\n> +\t\t\t<< \"Invalid exposure mode:\"\n> +\t\t\t<< \" expected size of 'shutter' and 'gain to be equal,\"\n> +\t\t\t<< \" got \" << shutter.size() << \" and \" << gain.size()\n> +\t\t\t<< \" respectively\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));\n> +\tstd::copy(gain.begin(), gain.end(), std::back_inserter(gains_));\n> +\n> +\t/*\n> +\t * Initialize the max shutter and gain if they aren't initialized yet.\n> +\t * This is to protect against the event that configure() is not called\n> +\t * before splitExposure().\n> +\t */\n\nIf init() has to be called one time only, can you record it with an\ninitialized_ flag and make sure that\n1) init cannot be called twice\n2) configure() can be called after init() only\n\n> +\tif (!maxShutter_) {\n\nso you can initialize the max values unconditionally\n\n> +\t\tif (shutters_.size() > 0)\n\nCan this happen ? If not you can\n\n\tif (shutter.size() != gain.size() ||\n shutter.size() == 0) {\n\t\tLOG(ExposureModeHelper, Error)\n\t\t\t<< \"Failed to initialize ExposureModeHelper. \"\n\t\t\t<< \" Number of shutter times:\" << shutter.size()\n\t\t\t<< \" Number of gain values:\" << gain.size()\n\t\treturn -EINVAL;\n\t}\n\nAt the beginning of the function\n\n> +\t\t\tmaxShutter_ = shutter.back();\n> +\t}\n> +\n> +\tif (!maxGain_) {\n> +\t\tif (gains_.size() > 0)\n> +\t\t\tmaxGain_ = gain.back();\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the ExposureModeHelper\n\n * \\brief Configure the ExposureModeHelper limits ?\n\n> + * \\param[in] minShutter The minimum shutter time\n> + * \\param[in] maxShutter The maximum shutter time\n> + * \\param[in] minGain The minimum gain\n> + * \\param[in] maxGain The maximum gain\n> + *\n> + * Note that the ExposureModeHelper needs to be reconfigured when\n> + * FrameDurationLimits is passed, and not just at IPA configuration time.\n\nI would rather\n\n * The ExposureModeHelper limits define the min/max shutter times and\n * gain values value the helpers class can select. The limits need to\n * be initialized when the IPA is configured and everytime an\n * application applies a constraint to the selectable values range, in\n * example when FrameDurationLimits is passed in.\n\n> + *\n> + * This function configures the maximum shutter and maximum gain.\n> + */\n> +void ExposureModeHelper::configure(utils::Duration minShutter,\n> +\t\t\t\t utils::Duration maxShutter,\n> +\t\t\t\t double minGain,\n> +\t\t\t\t double maxGain)\n> +{\n> +\tminShutter_ = minShutter;\n> +\tmaxShutter_ = maxShutter;\n> +\tminGain_ = minGain;\n> +\tmaxGain_ = maxGain;\n> +}\n> +\n> +utils::Duration ExposureModeHelper::clampShutter(utils::Duration shutter)\n> +{\n> +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n> +}\n> +\n> +double ExposureModeHelper::clampGain(double gain)\n> +{\n> +\treturn std::clamp(gain, minGain_, maxGain_);\n> +}\n> +\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure,\n> +\t\t\t\t utils::Duration shutter, bool shutterFixed,\n> +\t\t\t\t double gain, bool gainFixed)\n> +{\n> +\tshutter = clampShutter(shutter);\n> +\tgain = clampGain(gain);\n> +\n> +\t/* Not sure why you'd want to do this... */\n> +\tif (shutterFixed && gainFixed)\n> +\t\treturn { shutter, gain, 1 };\n> +\n> +\t/* Initial shutter and gain settings are sufficient */\n> +\tif (shutter * gain >= exposure) {\n> +\t\t/* Both shutter and gain cannot go lower */\n> +\t\tif (shutter == minShutter_ && gain == minGain_)\n> +\t\t\treturn { shutter, gain, 1 };\n> +\n> +\t\t/* Shutter cannot go lower */\n> +\t\tif (shutter == minShutter_ || shutterFixed)\n> +\t\t\treturn { shutter,\n> +\t\t\t\t gainFixed ? gain : clampGain(exposure / shutter),\n> +\t\t\t\t 1 };\n> +\n> +\t\t/* Gain cannot go lower */\n> +\t\tif (gain == minGain_ || gainFixed)\n> +\t\t\treturn {\n> +\t\t\t\tshutterFixed ? shutter : clampShutter(exposure / gain),\n> +\t\t\t\tgain,\n> +\t\t\t\t1\n> +\t\t\t};\n> +\n> +\t\t/* Both can go lower */\n> +\t\treturn { clampShutter(exposure / minGain_),\n> +\t\t\t exposure / clampShutter(exposure / minGain_),\n> +\t\t\t 1 };\n\nI trust your calculations here :)\n\n> +\t}\n> +\n> +\tunsigned int stage;\n> +\tutils::Duration stageShutter;\n> +\tdouble stageGain;\n> +\tdouble lastStageGain;\n> +\n> +\t/* We've already done stage 0 above so we start at 1 */\n> +\tfor (stage = 1; stage < gains_.size(); stage++) {\n> +\t\tstageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);\n> +\t\tstageGain = gainFixed ? gain : clampGain(gains_[stage]);\n> +\t\tlastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);\n> +\n> +\t\t/*\n> +\t\t * If the product of the new stage shutter and the old stage\n> +\t\t * gain is sufficient and we can change the shutter, reduce it.\n> +\t\t */\n> +\t\tif (!shutterFixed && stageShutter * lastStageGain >= exposure)\n> +\t\t\treturn { clampShutter(exposure / lastStageGain), lastStageGain, 1 };\n> +\n> +\t\t/*\n> +\t\t * The new stage shutter with old stage gain were insufficient,\n> +\t\t * so try the new stage shutter with new stage gain. If it is\n> +\t\t * sufficient and we can change the shutter, reduce it.\n> +\t\t */\n> +\t\tif (!shutterFixed && stageShutter * stageGain >= exposure)\n> +\t\t\treturn { clampShutter(exposure / stageGain), stageGain, 1 };\n> +\n> +\t\t/*\n> +\t\t * Same as above, but we can't change the shutter, so change\n> +\t\t * the gain instead.\n> +\t\t *\n> +\t\t * Note that at least one of !shutterFixed and !gainFixed is\n> +\t\t * guaranteed.\n> +\t\t */\n> +\t\tif (!gainFixed && stageShutter * stageGain >= exposure)\n> +\t\t\treturn { stageShutter, clampGain(exposure / stageShutter), 1 };\n> +\t}\n> +\n> +\t/* From here on we're going to try to max out shutter then gain */\n> +\tshutter = shutterFixed ? shutter : maxShutter_;\n> +\tgain = gainFixed ? gain : maxGain_;\n> +\n> +\t/*\n> +\t * We probably don't want to use the actual maximum analogue gain (as\n> +\t * it'll be unreasonably high), so we'll at least try to max out the\n> +\t * shutter, which is expected to be a bit more reasonable, as it is\n> +\t * limited by FrameDurationLimits and/or the sensor configuration.\n> +\t */\n> +\tif (!shutterFixed && shutter * stageGain >= exposure)\n> +\t\treturn { clampShutter(exposure / stageGain), stageGain, 1 };\n> +\n> +\t/*\n> +\t * If that's still not enough exposure, or if shutter is fixed, then\n> +\t * we'll max out the analogue gain before using digital gain.\n> +\t */\n> +\tif (!gainFixed && shutter * gain >= exposure)\n> +\t\treturn { shutter, clampGain(exposure / shutter), 1 };\n> +\n> +\t/*\n> +\t * We're out of shutter time and analogue gain; send the rest of the\n> +\t * exposure time to digital gain.\n> +\t */\n> +\treturn { shutter, gain, exposure / (shutter * gain) };\n\nThis really seems helpful!\n\n> +}\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain\n> + * \\param[in] exposure Exposure time\n> + * \\return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure)\n> +{\n> +\tASSERT(maxShutter_);\n> +\tASSERT(maxGain_);\n> +\tutils::Duration shutter;\n> +\tdouble gain;\n> +\n> +\tif (shutters_.size()) {\n\nWait, if you have no shutter times\n\n> +\t\tshutter = shutters_.at(0);\n> +\t\tgain = gains_.at(0);\n> +\t} else {\n> +\t\tshutter = maxShutter_;\n\nHow can you have a maxShutter ?\n\nOr is it allowed to call configure() without init() ?\n\n> +\t\tgain = maxGain_;\n> +\t}\n> +\n> +\treturn splitExposure(exposure, shutter, false, gain, false);\n> +}\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain, with fixed shutter\n> + * \\param[in] exposure Exposure time\n> + * \\param[in] fixedShutter Fixed shutter time\n> + *\n> + * Same as the base splitExposure, but with a fixed shutter (aka \"shutter priority\").\n> + *\n> + * \\return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)\n> +{\n> +\tASSERT(maxGain_);\n> +\tdouble gain = gains_.size() ? gains_.at(0) : maxGain_;\n> +\n> +\treturn splitExposure(exposure, fixedShutter, true, gain, false);\n> +}\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain, with fixed gain\n> + * \\param[in] exposure Exposure time\n> + * \\param[in] fixedGain Fixed gain\n> + *\n> + * Same as the base splitExposure, but with a fixed gain (aka \"gain priority\").\n> + *\n> + * \\return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)\n> +{\n> +\tASSERT(maxShutter_);\n> +\tutils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;\n> +\n> +\treturn splitExposure(exposure, shutter, false, fixedGain, true);\n> +}\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minShutter()\n> + * \\brief Retrieve the configured minimum shutter time\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxShutter()\n> + * \\brief Retrieve the configured maximum shutter time\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minGain()\n> + * \\brief Retrieve the configured minimum gain\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxGain()\n> + * \\brief Retrieve the configured maximum gain\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> new file mode 100644\n> index 00000000..d576c952\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -0,0 +1,61 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n\nIncluded by .cpp\n\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class ExposureModeHelper\n> +{\n> +public:\n> +\tExposureModeHelper();\n> +\t~ExposureModeHelper();\n> +\n> +\tint init(std::vector &shutters, std::vector &gains);\n> +\tvoid configure(utils::Duration minShutter, utils::Duration maxShutter,\n> +\t\t double minGain, double maxGain);\n> +\n> +\tstd::tuple splitExposure(utils::Duration exposure);\n> +\tstd::tuple splitExposure(utils::Duration exposure,\n> +\t\t\t\t\t\t\t\t utils::Duration fixedShutter);\n> +\tstd::tuple splitExposure(utils::Duration exposure,\n> +\t\t\t\t\t\t\t\t double fixedGain);\n> +\n> +\tutils::Duration minShutter() { return minShutter_; };\n> +\tutils::Duration maxShutter() { return maxShutter_; };\n> +\tdouble minGain() { return minGain_; };\n> +\tdouble maxGain() { return maxGain_; };\n> +\n> +private:\n> +\tutils::Duration clampShutter(utils::Duration shutter);\n> +\tdouble clampGain(double gain);\n> +\n> +\tstd::tuple\n> +\tsplitExposure(utils::Duration exposure,\n> +\t\t utils::Duration shutter, bool shutterFixed,\n> +\t\t double gain, bool gainFixed);\n> +\n> +\tstd::vector shutters_;\n> +\tstd::vector gains_;\n> +\n> +\tutils::Duration minShutter_;\n> +\tutils::Duration maxShutter_;\n> +\tdouble minGain_;\n> +\tdouble maxGain_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 016b8e0e..37fbd177 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -3,6 +3,7 @@\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> + 'exposure_mode_helper.h',\n> 'fc_queue.h',\n> 'histogram.h',\n> 'module.h',\n> @@ -11,6 +12,7 @@ libipa_headers = files([\n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> + 'exposure_mode_helper.cpp',\n> 'fc_queue.cpp',\n> 'histogram.cpp',\n> 'module.cpp',\n\nThis seems a really helpful addition overall !\n\n> --\n> 2.34.1\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 50F18BD160\n\tfor ;\n\tMon, 25 Mar 2024 16:48:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B976632EA;\n\tMon, 25 Mar 2024 17:48:13 +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 5257063036\n\tfor ;\n\tMon, 25 Mar 2024 17:48:11 +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 044487E4;\n\tMon, 25 Mar 2024 17:47:39 +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=\"caBFiYud\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711385260;\n\tbh=3p/IEFcHRat/7K+oSlWUkXSz22khwOBhpesekJVV2eA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=caBFiYud+WCtyNX0Dr89OF4rp0hpkTM4AiezGTkokINgjBpolExvquCNUP2fesQrz\n\tJ6qUFgTXQMBPnT2cFZWxoTyLVNulnxJ3ZMmtW1ae94UokuPPBACGshEbwblJCF7O/l\n\tIGb2ddF7p2RoO0Y8k4y9KoE8lEN5cnOjvSm2z2m8=","Date":"Mon, 25 Mar 2024 17:48:07 +0100","From":"Jacopo Mondi ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","Message-ID":"","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-4-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240322131451.3092931-4-dan.scally@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":29063,"web_url":"https://patchwork.libcamera.org/comment/29063/","msgid":"<20240326085818.ix5axvq6ygc63hx5@jasper>","date":"2024-03-26T08:58:18","subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul, hi Daniel,\n\nThanks for the patch. \n\nJacopo already did a thorough review. I can only add little bits\n\n\nOn Fri, Mar 22, 2024 at 01:14:44PM +0000, Daniel Scally wrote:\n> From: Paul Elder \n> \n> Add a helper for managing exposure modes and splitting exposure times\n> into shutter and gain values.\n> \n> Signed-off-by: Paul Elder \n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++\n> src/ipa/libipa/exposure_mode_helper.h | 61 +++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 370 insertions(+)\n> create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp\n> create mode 100644 src/ipa/libipa/exposure_mode_helper.h\n> \n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> new file mode 100644\n> index 00000000..9e01f908\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -0,0 +1,307 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n> + */\n> +#include \"exposure_mode_helper.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\file exposure_mode_helper.h\n> + * \\brief Helper class that performs computations relating to exposure\n> + *\n> + * Exposure modes contain a list of shutter and gain values to determine how to\n> + * split the supplied exposure time into shutter and gain. As this function is\n> + * expected to be replicated in various IPAs, this helper class factors it\n> + * away.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ExposureModeHelper)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class ExposureModeHelper\n> + * \\brief Class for splitting exposure time into shutter and gain\n> + */\n> +\n> +/**\n> + * \\brief Initialize an ExposureModeHelper instance\n> + */\n> +ExposureModeHelper::ExposureModeHelper()\n> +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> +{\n> +}\n> +\n> +ExposureModeHelper::~ExposureModeHelper()\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Initialize an ExposureModeHelper instance\n> + * \\param[in] shutter The list of shutter values\n> + * \\param[in] gain The list of gain values\n> + *\n> + * When splitting an exposure time into shutter and gain, the shutter will be\n> + * increased first before increasing the gain. This is done in stages, where\n> + * each stage is an index into both lists. Both lists consequently need to be\n> + * the same length.\n> + *\n> + * \\return Zero on success, negative error code otherwise\n> + */\n> +int ExposureModeHelper::init(std::vector &shutter, std::vector &gain)\n> +{\n> +\tif (shutter.size() != gain.size()) {\n> +\t\tLOG(ExposureModeHelper, Error)\n> +\t\t\t<< \"Invalid exposure mode:\"\n> +\t\t\t<< \" expected size of 'shutter' and 'gain to be equal,\"\n> +\t\t\t<< \" got \" << shutter.size() << \" and \" << gain.size()\n> +\t\t\t<< \" respectively\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));\n> +\tstd::copy(gain.begin(), gain.end(), std::back_inserter(gains_));\n> +\n> +\t/*\n> +\t * Initialize the max shutter and gain if they aren't initialized yet.\n> +\t * This is to protect against the event that configure() is not called\n> +\t * before splitExposure().\n> +\t */\n> +\tif (!maxShutter_) {\n> +\t\tif (shutters_.size() > 0)\n> +\t\t\tmaxShutter_ = shutter.back();\n> +\t}\n> +\n> +\tif (!maxGain_) {\n> +\t\tif (gains_.size() > 0)\n> +\t\t\tmaxGain_ = gain.back();\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the ExposureModeHelper\n> + * \\param[in] minShutter The minimum shutter time\n> + * \\param[in] maxShutter The maximum shutter time\n> + * \\param[in] minGain The minimum gain\n> + * \\param[in] maxGain The maximum gain\n> + *\n> + * Note that the ExposureModeHelper needs to be reconfigured when\n> + * FrameDurationLimits is passed, and not just at IPA configuration time.\n> + *\n> + * This function configures the maximum shutter and maximum gain.\n> + */\n> +void ExposureModeHelper::configure(utils::Duration minShutter,\n> +\t\t\t\t utils::Duration maxShutter,\n> +\t\t\t\t double minGain,\n> +\t\t\t\t double maxGain)\n> +{\n> +\tminShutter_ = minShutter;\n> +\tmaxShutter_ = maxShutter;\n> +\tminGain_ = minGain;\n> +\tmaxGain_ = maxGain;\n> +}\n> +\n> +utils::Duration ExposureModeHelper::clampShutter(utils::Duration shutter)\n> +{\n> +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n> +}\n> +\n> +double ExposureModeHelper::clampGain(double gain)\n> +{\n> +\treturn std::clamp(gain, minGain_, maxGain_);\n> +}\n> +\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure,\n> +\t\t\t\t utils::Duration shutter, bool shutterFixed,\n> +\t\t\t\t double gain, bool gainFixed)\n> +{\n> +\tshutter = clampShutter(shutter);\n> +\tgain = clampGain(gain);\n> +\n> +\t/* Not sure why you'd want to do this... */\n> +\tif (shutterFixed && gainFixed)\n> +\t\treturn { shutter, gain, 1 };\n> +\n> +\t/* Initial shutter and gain settings are sufficient */\n> +\tif (shutter * gain >= exposure) {\n> +\t\t/* Both shutter and gain cannot go lower */\n> +\t\tif (shutter == minShutter_ && gain == minGain_)\n> +\t\t\treturn { shutter, gain, 1 };\n> +\n> +\t\t/* Shutter cannot go lower */\n> +\t\tif (shutter == minShutter_ || shutterFixed)\n> +\t\t\treturn { shutter,\n> +\t\t\t\t gainFixed ? gain : clampGain(exposure / shutter),\n> +\t\t\t\t 1 };\n> +\n> +\t\t/* Gain cannot go lower */\n> +\t\tif (gain == minGain_ || gainFixed)\n> +\t\t\treturn {\n> +\t\t\t\tshutterFixed ? shutter : clampShutter(exposure / gain),\n> +\t\t\t\tgain,\n> +\t\t\t\t1\n> +\t\t\t};\n> +\n> +\t\t/* Both can go lower */\n> +\t\treturn { clampShutter(exposure / minGain_),\n> +\t\t\t exposure / clampShutter(exposure / minGain_),\n> +\t\t\t 1 };\n\nIsn't this missing the clampGain()? Might be easier to read if the two\ncalls to clampShutter would be reduced to one and a variable.\n\n> +\t}\n> +\n> +\tunsigned int stage;\n> +\tutils::Duration stageShutter;\n> +\tdouble stageGain;\n> +\tdouble lastStageGain;\n> +\n> +\t/* We've already done stage 0 above so we start at 1 */\n> +\tfor (stage = 1; stage < gains_.size(); stage++) {\n> +\t\tstageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);\n> +\t\tstageGain = gainFixed ? gain : clampGain(gains_[stage]);\n> +\t\tlastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);\n> +\n> +\t\t/*\n> +\t\t * If the product of the new stage shutter and the old stage\n> +\t\t * gain is sufficient and we can change the shutter, reduce it.\n> +\t\t */\n> +\t\tif (!shutterFixed && stageShutter * lastStageGain >= exposure)\n> +\t\t\treturn { clampShutter(exposure / lastStageGain), lastStageGain, 1 };\n> +\n> +\t\t/*\n> +\t\t * The new stage shutter with old stage gain were insufficient,\n> +\t\t * so try the new stage shutter with new stage gain. If it is\n> +\t\t * sufficient and we can change the shutter, reduce it.\n> +\t\t */\n> +\t\tif (!shutterFixed && stageShutter * stageGain >= exposure)\n> +\t\t\treturn { clampShutter(exposure / stageGain), stageGain, 1 };\n> +\n> +\t\t/*\n> +\t\t * Same as above, but we can't change the shutter, so change\n> +\t\t * the gain instead.\n> +\t\t *\n> +\t\t * Note that at least one of !shutterFixed and !gainFixed is\n> +\t\t * guaranteed.\n> +\t\t */\n> +\t\tif (!gainFixed && stageShutter * stageGain >= exposure)\n> +\t\t\treturn { stageShutter, clampGain(exposure / stageShutter), 1 };\n> +\t}\n> +\n> +\t/* From here on we're going to try to max out shutter then gain */\n> +\tshutter = shutterFixed ? shutter : maxShutter_;\n> +\tgain = gainFixed ? gain : maxGain_;\n> +\n> +\t/*\n> +\t * We probably don't want to use the actual maximum analogue gain (as\n> +\t * it'll be unreasonably high), so we'll at least try to max out the\n> +\t * shutter, which is expected to be a bit more reasonable, as it is\n> +\t * limited by FrameDurationLimits and/or the sensor configuration.\n> +\t */\n> +\tif (!shutterFixed && shutter * stageGain >= exposure)\n> +\t\treturn { clampShutter(exposure / stageGain), stageGain, 1 };\n> +\n> +\t/*\n> +\t * If that's still not enough exposure, or if shutter is fixed, then\n> +\t * we'll max out the analogue gain before using digital gain.\n> +\t */\n> +\tif (!gainFixed && shutter * gain >= exposure)\n> +\t\treturn { shutter, clampGain(exposure / shutter), 1 };\n> +\n> +\t/*\n> +\t * We're out of shutter time and analogue gain; send the rest of the\n> +\t * exposure time to digital gain.\n> +\t */\n> +\treturn { shutter, gain, exposure / (shutter * gain) };\n> +}\n\nI wonder how this would work with flicker mitigation. But that's\npropably out of scope.\n\nCheers,\nStefan\n\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain\n> + * \\param[in] exposure Exposure time\n> + * \\return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure)\n> +{\n> +\tASSERT(maxShutter_);\n> +\tASSERT(maxGain_);\n> +\tutils::Duration shutter;\n> +\tdouble gain;\n> +\n> +\tif (shutters_.size()) {\n> +\t\tshutter = shutters_.at(0);\n> +\t\tgain = gains_.at(0);\n> +\t} else {\n> +\t\tshutter = maxShutter_;\n> +\t\tgain = maxGain_;\n> +\t}\n> +\n> +\treturn splitExposure(exposure, shutter, false, gain, false);\n> +}\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain, with fixed shutter\n> + * \\param[in] exposure Exposure time\n> + * \\param[in] fixedShutter Fixed shutter time\n> + *\n> + * Same as the base splitExposure, but with a fixed shutter (aka \"shutter priority\").\n> + *\n> + * \\return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)\n> +{\n> +\tASSERT(maxGain_);\n> +\tdouble gain = gains_.size() ? gains_.at(0) : maxGain_;\n> +\n> +\treturn splitExposure(exposure, fixedShutter, true, gain, false);\n> +}\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain, with fixed gain\n> + * \\param[in] exposure Exposure time\n> + * \\param[in] fixedGain Fixed gain\n> + *\n> + * Same as the base splitExposure, but with a fixed gain (aka \"gain priority\").\n> + *\n> + * \\return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)\n> +{\n> +\tASSERT(maxShutter_);\n> +\tutils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;\n> +\n> +\treturn splitExposure(exposure, shutter, false, fixedGain, true);\n> +}\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minShutter()\n> + * \\brief Retrieve the configured minimum shutter time\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxShutter()\n> + * \\brief Retrieve the configured maximum shutter time\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minGain()\n> + * \\brief Retrieve the configured minimum gain\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxGain()\n> + * \\brief Retrieve the configured maximum gain\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> new file mode 100644\n> index 00000000..d576c952\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -0,0 +1,61 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class ExposureModeHelper\n> +{\n> +public:\n> +\tExposureModeHelper();\n> +\t~ExposureModeHelper();\n> +\n> +\tint init(std::vector &shutters, std::vector &gains);\n> +\tvoid configure(utils::Duration minShutter, utils::Duration maxShutter,\n> +\t\t double minGain, double maxGain);\n> +\n> +\tstd::tuple splitExposure(utils::Duration exposure);\n> +\tstd::tuple splitExposure(utils::Duration exposure,\n> +\t\t\t\t\t\t\t\t utils::Duration fixedShutter);\n> +\tstd::tuple splitExposure(utils::Duration exposure,\n> +\t\t\t\t\t\t\t\t double fixedGain);\n> +\n> +\tutils::Duration minShutter() { return minShutter_; };\n> +\tutils::Duration maxShutter() { return maxShutter_; };\n> +\tdouble minGain() { return minGain_; };\n> +\tdouble maxGain() { return maxGain_; };\n> +\n> +private:\n> +\tutils::Duration clampShutter(utils::Duration shutter);\n> +\tdouble clampGain(double gain);\n> +\n> +\tstd::tuple\n> +\tsplitExposure(utils::Duration exposure,\n> +\t\t utils::Duration shutter, bool shutterFixed,\n> +\t\t double gain, bool gainFixed);\n> +\n> +\tstd::vector shutters_;\n> +\tstd::vector gains_;\n> +\n> +\tutils::Duration minShutter_;\n> +\tutils::Duration maxShutter_;\n> +\tdouble minGain_;\n> +\tdouble maxGain_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 016b8e0e..37fbd177 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -3,6 +3,7 @@\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> + 'exposure_mode_helper.h',\n> 'fc_queue.h',\n> 'histogram.h',\n> 'module.h',\n> @@ -11,6 +12,7 @@ libipa_headers = files([\n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> + 'exposure_mode_helper.cpp',\n> 'fc_queue.cpp',\n> 'histogram.cpp',\n> 'module.cpp',\n> -- \n> 2.34.1\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 A0B88C0DA4\n\tfor ;\n\tTue, 26 Mar 2024 08:58:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A47DA63311;\n\tTue, 26 Mar 2024 09:58:22 +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 9ECAC6303D\n\tfor ;\n\tTue, 26 Mar 2024 09:58:21 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:a916:1c71:357:e10c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC95563B;\n\tTue, 26 Mar 2024 09:57:49 +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=\"lR593i3d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711443470;\n\tbh=B1pVlsI2/oMi0QmQMw6j68l22bVcYay5ENie3ngCwzQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lR593i3dNPPtWx/Mvy5OwiDU3h12tdyliEFxy7Y4qeHNet2NAaKY0DHGSWgvIkqBx\n\tF5aV787R+I20E47O09LFy5O7NYsDZLRavwcAsc9oXeYCZI5xxoxK0OFcbXI9/Y0iJO\n\tUxWpidq2BR5lxEFXvtVCNpu5kGG7qOj8vmkgiZ1k=","Date":"Tue, 26 Mar 2024 09:58:18 +0100","From":"Stefan Klug ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","Message-ID":"<20240326085818.ix5axvq6ygc63hx5@jasper>","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-4-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240322131451.3092931-4-dan.scally@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":29065,"web_url":"https://patchwork.libcamera.org/comment/29065/","msgid":"<171144676800.3566204.2932849165119788292@ping.linuxembedded.co.uk>","date":"2024-03-26T09:52:48","subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-03-26 08:58:18)\n> Hi Paul, hi Daniel,\n> \n> Thanks for the patch. \n> \n> Jacopo already did a thorough review. I can only add little bits\n> \n> \n> On Fri, Mar 22, 2024 at 01:14:44PM +0000, Daniel Scally wrote:\n> > From: Paul Elder \n> > \n> > Add a helper for managing exposure modes and splitting exposure times\n> > into shutter and gain values.\n> > \n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Daniel Scally \n> > ---\n> > src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++\n> > src/ipa/libipa/exposure_mode_helper.h | 61 +++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 370 insertions(+)\n> > create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp\n> > create mode 100644 src/ipa/libipa/exposure_mode_helper.h\n> > \n> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> > new file mode 100644\n> > index 00000000..9e01f908\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > @@ -0,0 +1,307 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n> > + */\n> > +#include \"exposure_mode_helper.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file exposure_mode_helper.h\n> > + * \\brief Helper class that performs computations relating to exposure\n> > + *\n> > + * Exposure modes contain a list of shutter and gain values to determine how to\n> > + * split the supplied exposure time into shutter and gain. As this function is\n> > + * expected to be replicated in various IPAs, this helper class factors it\n> > + * away.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(ExposureModeHelper)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\class ExposureModeHelper\n> > + * \\brief Class for splitting exposure time into shutter and gain\n> > + */\n> > +\n> > +/**\n> > + * \\brief Initialize an ExposureModeHelper instance\n> > + */\n> > +ExposureModeHelper::ExposureModeHelper()\n> > + : minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> > +{\n> > +}\n> > +\n> > +ExposureModeHelper::~ExposureModeHelper()\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Initialize an ExposureModeHelper instance\n> > + * \\param[in] shutter The list of shutter values\n> > + * \\param[in] gain The list of gain values\n> > + *\n> > + * When splitting an exposure time into shutter and gain, the shutter will be\n> > + * increased first before increasing the gain. This is done in stages, where\n> > + * each stage is an index into both lists. Both lists consequently need to be\n> > + * the same length.\n> > + *\n> > + * \\return Zero on success, negative error code otherwise\n> > + */\n> > +int ExposureModeHelper::init(std::vector &shutter, std::vector &gain)\n> > +{\n> > + if (shutter.size() != gain.size()) {\n> > + LOG(ExposureModeHelper, Error)\n> > + << \"Invalid exposure mode:\"\n> > + << \" expected size of 'shutter' and 'gain to be equal,\"\n> > + << \" got \" << shutter.size() << \" and \" << gain.size()\n> > + << \" respectively\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + std::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));\n> > + std::copy(gain.begin(), gain.end(), std::back_inserter(gains_));\n> > +\n> > + /*\n> > + * Initialize the max shutter and gain if they aren't initialized yet.\n> > + * This is to protect against the event that configure() is not called\n> > + * before splitExposure().\n> > + */\n> > + if (!maxShutter_) {\n> > + if (shutters_.size() > 0)\n> > + maxShutter_ = shutter.back();\n> > + }\n> > +\n> > + if (!maxGain_) {\n> > + if (gains_.size() > 0)\n> > + maxGain_ = gain.back();\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Configure the ExposureModeHelper\n> > + * \\param[in] minShutter The minimum shutter time\n> > + * \\param[in] maxShutter The maximum shutter time\n> > + * \\param[in] minGain The minimum gain\n> > + * \\param[in] maxGain The maximum gain\n> > + *\n> > + * Note that the ExposureModeHelper needs to be reconfigured when\n> > + * FrameDurationLimits is passed, and not just at IPA configuration time.\n> > + *\n> > + * This function configures the maximum shutter and maximum gain.\n> > + */\n> > +void ExposureModeHelper::configure(utils::Duration minShutter,\n> > + utils::Duration maxShutter,\n> > + double minGain,\n> > + double maxGain)\n> > +{\n> > + minShutter_ = minShutter;\n> > + maxShutter_ = maxShutter;\n> > + minGain_ = minGain;\n> > + maxGain_ = maxGain;\n> > +}\n> > +\n> > +utils::Duration ExposureModeHelper::clampShutter(utils::Duration shutter)\n> > +{\n> > + return std::clamp(shutter, minShutter_, maxShutter_);\n> > +}\n> > +\n> > +double ExposureModeHelper::clampGain(double gain)\n> > +{\n> > + return std::clamp(gain, minGain_, maxGain_);\n> > +}\n> > +\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure,\n> > + utils::Duration shutter, bool shutterFixed,\n> > + double gain, bool gainFixed)\n> > +{\n> > + shutter = clampShutter(shutter);\n> > + gain = clampGain(gain);\n> > +\n> > + /* Not sure why you'd want to do this... */\n> > + if (shutterFixed && gainFixed)\n> > + return { shutter, gain, 1 };\n> > +\n> > + /* Initial shutter and gain settings are sufficient */\n> > + if (shutter * gain >= exposure) {\n> > + /* Both shutter and gain cannot go lower */\n> > + if (shutter == minShutter_ && gain == minGain_)\n> > + return { shutter, gain, 1 };\n> > +\n> > + /* Shutter cannot go lower */\n> > + if (shutter == minShutter_ || shutterFixed)\n> > + return { shutter,\n> > + gainFixed ? gain : clampGain(exposure / shutter),\n> > + 1 };\n> > +\n> > + /* Gain cannot go lower */\n> > + if (gain == minGain_ || gainFixed)\n> > + return {\n> > + shutterFixed ? shutter : clampShutter(exposure / gain),\n> > + gain,\n> > + 1\n> > + };\n> > +\n> > + /* Both can go lower */\n> > + return { clampShutter(exposure / minGain_),\n> > + exposure / clampShutter(exposure / minGain_),\n> > + 1 };\n> \n> Isn't this missing the clampGain()? Might be easier to read if the two\n> calls to clampShutter would be reduced to one and a variable.\n> \n> > + }\n> > +\n> > + unsigned int stage;\n> > + utils::Duration stageShutter;\n> > + double stageGain;\n> > + double lastStageGain;\n> > +\n> > + /* We've already done stage 0 above so we start at 1 */\n> > + for (stage = 1; stage < gains_.size(); stage++) {\n> > + stageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);\n> > + stageGain = gainFixed ? gain : clampGain(gains_[stage]);\n> > + lastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);\n> > +\n> > + /*\n> > + * If the product of the new stage shutter and the old stage\n> > + * gain is sufficient and we can change the shutter, reduce it.\n> > + */\n> > + if (!shutterFixed && stageShutter * lastStageGain >= exposure)\n> > + return { clampShutter(exposure / lastStageGain), lastStageGain, 1 };\n> > +\n> > + /*\n> > + * The new stage shutter with old stage gain were insufficient,\n> > + * so try the new stage shutter with new stage gain. If it is\n> > + * sufficient and we can change the shutter, reduce it.\n> > + */\n> > + if (!shutterFixed && stageShutter * stageGain >= exposure)\n> > + return { clampShutter(exposure / stageGain), stageGain, 1 };\n> > +\n> > + /*\n> > + * Same as above, but we can't change the shutter, so change\n> > + * the gain instead.\n> > + *\n> > + * Note that at least one of !shutterFixed and !gainFixed is\n> > + * guaranteed.\n> > + */\n> > + if (!gainFixed && stageShutter * stageGain >= exposure)\n> > + return { stageShutter, clampGain(exposure / stageShutter), 1 };\n> > + }\n> > +\n> > + /* From here on we're going to try to max out shutter then gain */\n> > + shutter = shutterFixed ? shutter : maxShutter_;\n> > + gain = gainFixed ? gain : maxGain_;\n> > +\n> > + /*\n> > + * We probably don't want to use the actual maximum analogue gain (as\n> > + * it'll be unreasonably high), so we'll at least try to max out the\n> > + * shutter, which is expected to be a bit more reasonable, as it is\n> > + * limited by FrameDurationLimits and/or the sensor configuration.\n> > + */\n> > + if (!shutterFixed && shutter * stageGain >= exposure)\n> > + return { clampShutter(exposure / stageGain), stageGain, 1 };\n> > +\n> > + /*\n> > + * If that's still not enough exposure, or if shutter is fixed, then\n> > + * we'll max out the analogue gain before using digital gain.\n> > + */\n> > + if (!gainFixed && shutter * gain >= exposure)\n> > + return { shutter, clampGain(exposure / shutter), 1 };\n> > +\n> > + /*\n> > + * We're out of shutter time and analogue gain; send the rest of the\n> > + * exposure time to digital gain.\n> > + */\n> > + return { shutter, gain, exposure / (shutter * gain) };\n> > +}\n> \n> I wonder how this would work with flicker mitigation. But that's\n> propably out of scope.\n\nTackling flicker avoidance centrally here definitely makes sense, but as\nneither IPU3 nor RKISP1 even attempt this yet - I think it's fine to do\nso on top.\n\n--\nKieran\n\n\n> \n> Cheers,\n> Stefan\n> \n> > +\n> > +/**\n> > + * \\brief Split exposure time into shutter and gain\n> > + * \\param[in] exposure Exposure time\n> > + * \\return Tuple of shutter time, analogue gain, and digital gain\n> > + */\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure)\n> > +{\n> > + ASSERT(maxShutter_);\n> > + ASSERT(maxGain_);\n> > + utils::Duration shutter;\n> > + double gain;\n> > +\n> > + if (shutters_.size()) {\n> > + shutter = shutters_.at(0);\n> > + gain = gains_.at(0);\n> > + } else {\n> > + shutter = maxShutter_;\n> > + gain = maxGain_;\n> > + }\n> > +\n> > + return splitExposure(exposure, shutter, false, gain, false);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Split exposure time into shutter and gain, with fixed shutter\n> > + * \\param[in] exposure Exposure time\n> > + * \\param[in] fixedShutter Fixed shutter time\n> > + *\n> > + * Same as the base splitExposure, but with a fixed shutter (aka \"shutter priority\").\n> > + *\n> > + * \\return Tuple of shutter time, analogue gain, and digital gain\n> > + */\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)\n> > +{\n> > + ASSERT(maxGain_);\n> > + double gain = gains_.size() ? gains_.at(0) : maxGain_;\n> > +\n> > + return splitExposure(exposure, fixedShutter, true, gain, false);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Split exposure time into shutter and gain, with fixed gain\n> > + * \\param[in] exposure Exposure time\n> > + * \\param[in] fixedGain Fixed gain\n> > + *\n> > + * Same as the base splitExposure, but with a fixed gain (aka \"gain priority\").\n> > + *\n> > + * \\return Tuple of shutter time, analogue gain, and digital gain\n> > + */\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)\n> > +{\n> > + ASSERT(maxShutter_);\n> > + utils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;\n> > +\n> > + return splitExposure(exposure, shutter, false, fixedGain, true);\n> > +}\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::minShutter()\n> > + * \\brief Retrieve the configured minimum shutter time\n> > + */\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::maxShutter()\n> > + * \\brief Retrieve the configured maximum shutter time\n> > + */\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::minGain()\n> > + * \\brief Retrieve the configured minimum gain\n> > + */\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::maxGain()\n> > + * \\brief Retrieve the configured maximum gain\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> > new file mode 100644\n> > index 00000000..d576c952\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > @@ -0,0 +1,61 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +class ExposureModeHelper\n> > +{\n> > +public:\n> > + ExposureModeHelper();\n> > + ~ExposureModeHelper();\n> > +\n> > + int init(std::vector &shutters, std::vector &gains);\n> > + void configure(utils::Duration minShutter, utils::Duration maxShutter,\n> > + double minGain, double maxGain);\n> > +\n> > + std::tuple splitExposure(utils::Duration exposure);\n> > + std::tuple splitExposure(utils::Duration exposure,\n> > + utils::Duration fixedShutter);\n> > + std::tuple splitExposure(utils::Duration exposure,\n> > + double fixedGain);\n> > +\n> > + utils::Duration minShutter() { return minShutter_; };\n> > + utils::Duration maxShutter() { return maxShutter_; };\n> > + double minGain() { return minGain_; };\n> > + double maxGain() { return maxGain_; };\n> > +\n> > +private:\n> > + utils::Duration clampShutter(utils::Duration shutter);\n> > + double clampGain(double gain);\n> > +\n> > + std::tuple\n> > + splitExposure(utils::Duration exposure,\n> > + utils::Duration shutter, bool shutterFixed,\n> > + double gain, bool gainFixed);\n> > +\n> > + std::vector shutters_;\n> > + std::vector gains_;\n> > +\n> > + utils::Duration minShutter_;\n> > + utils::Duration maxShutter_;\n> > + double minGain_;\n> > + double maxGain_;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 016b8e0e..37fbd177 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -3,6 +3,7 @@\n> > libipa_headers = files([\n> > 'algorithm.h',\n> > 'camera_sensor_helper.h',\n> > + 'exposure_mode_helper.h',\n> > 'fc_queue.h',\n> > 'histogram.h',\n> > 'module.h',\n> > @@ -11,6 +12,7 @@ libipa_headers = files([\n> > libipa_sources = files([\n> > 'algorithm.cpp',\n> > 'camera_sensor_helper.cpp',\n> > + 'exposure_mode_helper.cpp',\n> > 'fc_queue.cpp',\n> > 'histogram.cpp',\n> > 'module.cpp',\n> > -- \n> > 2.34.1\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 6CB1BBD160\n\tfor ;\n\tTue, 26 Mar 2024 09:52:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 510DD632EA;\n\tTue, 26 Mar 2024 10:52:52 +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 3744963037\n\tfor ;\n\tTue, 26 Mar 2024 10:52:50 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93B80497;\n\tTue, 26 Mar 2024 10:52:18 +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=\"wJKfH6mr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711446738;\n\tbh=aafIndAV2V6Cfc7YGJokJzIUF77uhu2zJbDbYhOCHVc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=wJKfH6mrVRVu7R18CNURH0pYqRQSROcKskTwJroRLyGiH6ihL8GT1atfupYkKphXD\n\tRy8/AgiOLYZi5P6oStRJhb/m4KsnEvtzgqo6BeJuMe7sp0QnYEi3ZUGyp5wUeBSF6I\n\tOhGAyvp/jwJ00L8iMQWvhnEM4yBah9WRTZiJJaS0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240326085818.ix5axvq6ygc63hx5@jasper>","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-4-dan.scally@ideasonboard.com>\n\t<20240326085818.ix5axvq6ygc63hx5@jasper>","Subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","To":"Daniel Scally ,\n\tStefan Klug ","Date":"Tue, 26 Mar 2024 09:52:48 +0000","Message-ID":"<171144676800.3566204.2932849165119788292@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":29164,"web_url":"https://patchwork.libcamera.org/comment/29164/","msgid":"<20240405230046.GJ12507@pendragon.ideasonboard.com>","date":"2024-04-05T23:00:46","subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 25, 2024 at 05:48:07PM +0100, Jacopo Mondi wrote:\n> On Fri, Mar 22, 2024 at 01:14:44PM +0000, Daniel Scally wrote:\n> > From: Paul Elder \n> >\n> > Add a helper for managing exposure modes and splitting exposure times\n> > into shutter and gain values.\n> >\n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Daniel Scally \n> > ---\n> > src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++++++++++++\n> > src/ipa/libipa/exposure_mode_helper.h | 61 +++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 370 insertions(+)\n> > create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp\n> > create mode 100644 src/ipa/libipa/exposure_mode_helper.h\n> >\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> > new file mode 100644\n> > index 00000000..9e01f908\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > @@ -0,0 +1,307 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n> \n> exposure_mode_helper.cpp\n\nShould we drop the file name from the top-level headers project-wide ?\n\n> > + */\n> > +#include \"exposure_mode_helper.h\"\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file exposure_mode_helper.h\n> > + * \\brief Helper class that performs computations relating to exposure\n> > + *\n> > + * Exposure modes contain a list of shutter and gain values to determine how to\n> > + * split the supplied exposure time into shutter and gain.\n\n\"exposure time\" and \"shutter\" are the same. You probably meant\n\"exposure\" instead of \"exposure time\". Same below where applicable.\n\nOn a side note, I think it would be clearer to transition to\n\"integration time\" instead of \"exposure time\" or \"shutter\". This will\navoid confusing \"exposure\" and \"exposure time\", as well as free the term\n\"shutter\" for mechanical shutters if we ever need to support them.\n\nSetting shutter and gain ranges per mode is one possible implementation,\nnot something that libcamera dictates globally. Could you refactor this\ntext accordingly ? Something along the lines of \"there's a need for AEGC\nalgorithms to split exposure in shutter and gain, multiple IPA modules\ndo so based on , provide a helper to avoid\nduplicating the code\".\n\n> > As this function is\n> > + * expected to be replicated in various IPAs, this helper class factors it\n> > + * away.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(ExposureModeHelper)\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\class ExposureModeHelper\n> > + * \\brief Class for splitting exposure time into shutter and gain\n\nThis is where you need to explain how the class operates. After reading\nthis block, the reader should understand what the class is used for and\nif it matches its needs. The theory of operation of exposure modes and\nhow they dictate how to split exposure in shutter and gain belong here.\n\n> > + */\n> > +\n> > +/**\n> > + * \\brief Initialize an ExposureModeHelper instance\n\ns/Initialize/Construct/\n\n> > + */\n> > +ExposureModeHelper::ExposureModeHelper()\n> > +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> > +{\n> > +}\n> > +\n> > +ExposureModeHelper::~ExposureModeHelper()\n> > +{\n> > +}\n> \n> You probably don't need this and can rely on the compiler generated\n> one\n> \n> > +\n> > +/**\n> > + * \\brief Initialize an ExposureModeHelper instance\n> > + * \\param[in] shutter The list of shutter values\n> > + * \\param[in] gain The list of gain values\n\nIs this for the analog gain, the digital gain, or both ? Make it\nexplicit here and where it matters. Also explain in the class\ndescription that it handles both gains, and why there's no need to\nprovide configuration parameters (and limits) for both.\n\n> > + *\n> > + * When splitting an exposure time into shutter and gain, the shutter will be\n> > + * increased first before increasing the gain. This is done in stages, where\n> > + * each stage is an index into both lists. Both lists consequently need to be\n> > + * the same length.\n> \n> Is this expected to be called one time only ? At what time ? when the\n> library is loaded and the configuration file is parsed ? Is it worth\n> mentioning it ?\n\nIt's worth explaining it. Also, reading the documentation, I'm afraid I\ndon't understand what parameters to pass to this function.\n\n> > + *\n> > + * \\return Zero on success, negative error code otherwise\n> > + */\n> > +int ExposureModeHelper::init(std::vector &shutter, std::vector &gain)\n\nThe parameters are mutable, which I don't think is what you intended.\nAlso, nothing in the function strictly requires shutter and gain to be\nvectors. Use spans to relax the data storage requirements in the caller.\n\n> > +{\n> > +\tif (shutter.size() != gain.size()) {\n> > +\t\tLOG(ExposureModeHelper, Error)\n> > +\t\t\t<< \"Invalid exposure mode:\"\n> > +\t\t\t<< \" expected size of 'shutter' and 'gain to be equal,\"\n> > +\t\t\t<< \" got \" << shutter.size() << \" and \" << gain.size()\n> > +\t\t\t<< \" respectively\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n\nTry to design the API in a way that minimizes misuse. If the function\nneeds the same number of shutter and gain values, you can for instance\nstore them in a vector of pairs (std::pair or a custom structure), not a\npair of vectors.\n\n> > +\n> > +\tstd::copy(shutter.begin(), shutter.end(), std::back_inserter(shutters_));\n> > +\tstd::copy(gain.begin(), gain.end(), std::back_inserter(gains_));\n\nCalling the function multiple times will add values to shutters_ and\ngains_, not replace them. Is that by design ?\n\n> > +\n> > +\t/*\n> > +\t * Initialize the max shutter and gain if they aren't initialized yet.\n> > +\t * This is to protect against the event that configure() is not called\n> > +\t * before splitExposure().\n> > +\t */\n> \n> If init() has to be called one time only, can you record it with an\n> initialized_ flag and make sure that\n> 1) init cannot be called twice\n> 2) configure() can be called after init() only\n\nIt seems difficult to use the class correctly, we need to aim for\nimpossible to use incorrectly instead :-)\n\n> > +\tif (!maxShutter_) {\n> \n> so you can initialize the max values unconditionally\n> \n> > +\t\tif (shutters_.size() > 0)\n> \n> Can this happen ? If not you can\n> \n> \tif (shutter.size() != gain.size() ||\n> shutter.size() == 0) {\n> \t\tLOG(ExposureModeHelper, Error)\n> \t\t\t<< \"Failed to initialize ExposureModeHelper. \"\n> \t\t\t<< \" Number of shutter times:\" << shutter.size()\n> \t\t\t<< \" Number of gain values:\" << gain.size()\n> \t\treturn -EINVAL;\n> \t}\n> \n> At the beginning of the function\n> \n> > +\t\t\tmaxShutter_ = shutter.back();\n> > +\t}\n> > +\n> > +\tif (!maxGain_) {\n> > +\t\tif (gains_.size() > 0)\n> > +\t\t\tmaxGain_ = gain.back();\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Configure the ExposureModeHelper\n> \n> * \\brief Configure the ExposureModeHelper limits ?\n\nAnd name the function accordingly. configure() doesn't tell me much.\nsetShutterGainLimits() is a much better name. You can then document it\nas\n\n * \\brief Set the shutter and gain limits\n\nWhen your \\brief can't tell much more than the function name without\nbecoming a non-brief text, you know you have a good name.\n\n> > + * \\param[in] minShutter The minimum shutter time\n> > + * \\param[in] maxShutter The maximum shutter time\n> > + * \\param[in] minGain The minimum gain\n> > + * \\param[in] maxGain The maximum gain\n> > + *\n> > + * Note that the ExposureModeHelper needs to be reconfigured when\n> > + * FrameDurationLimits is passed, and not just at IPA configuration time.\n> \n> I would rather\n> \n> * The ExposureModeHelper limits define the min/max shutter times and\n> * gain values value the helpers class can select. The limits need to\n> * be initialized when the IPA is configured and everytime an\n> * application applies a constraint to the selectable values range, in\n> * example when FrameDurationLimits is passed in.\n\nThat's information specific to the caller. Keep the class generic, tell\nif setting the limits is required before using the class, if the limits\ncan change (and if so when), and what the limits are used for.\n\n> > + *\n> > + * This function configures the maximum shutter and maximum gain.\n> > + */\n> > +void ExposureModeHelper::configure(utils::Duration minShutter,\n> > +\t\t\t\t utils::Duration maxShutter,\n> > +\t\t\t\t double minGain,\n> > +\t\t\t\t double maxGain)\n> > +{\n> > +\tminShutter_ = minShutter;\n> > +\tmaxShutter_ = maxShutter;\n> > +\tminGain_ = minGain;\n> > +\tmaxGain_ = maxGain;\n> > +}\n> > +\n> > +utils::Duration ExposureModeHelper::clampShutter(utils::Duration shutter)\n> > +{\n> > +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n> > +}\n> > +\n> > +double ExposureModeHelper::clampGain(double gain)\n> > +{\n> > +\treturn std::clamp(gain, minGain_, maxGain_);\n> > +}\n> > +\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure,\n> > +\t\t\t\t utils::Duration shutter, bool shutterFixed,\n> > +\t\t\t\t double gain, bool gainFixed)\n> > +{\n> > +\tshutter = clampShutter(shutter);\n> > +\tgain = clampGain(gain);\n> > +\n> > +\t/* Not sure why you'd want to do this... */\n> > +\tif (shutterFixed && gainFixed)\n> > +\t\treturn { shutter, gain, 1 };\n\nNo need to use digital gain here ?\n\n> > +\n> > +\t/* Initial shutter and gain settings are sufficient */\n> > +\tif (shutter * gain >= exposure) {\n> > +\t\t/* Both shutter and gain cannot go lower */\n> > +\t\tif (shutter == minShutter_ && gain == minGain_)\n> > +\t\t\treturn { shutter, gain, 1 };\n> > +\n> > +\t\t/* Shutter cannot go lower */\n> > +\t\tif (shutter == minShutter_ || shutterFixed)\n> > +\t\t\treturn { shutter,\n> > +\t\t\t\t gainFixed ? gain : clampGain(exposure / shutter),\n> > +\t\t\t\t 1 };\n> > +\n> > +\t\t/* Gain cannot go lower */\n> > +\t\tif (gain == minGain_ || gainFixed)\n> > +\t\t\treturn {\n> > +\t\t\t\tshutterFixed ? shutter : clampShutter(exposure / gain),\n> > +\t\t\t\tgain,\n> > +\t\t\t\t1\n> > +\t\t\t};\n> > +\n> > +\t\t/* Both can go lower */\n> > +\t\treturn { clampShutter(exposure / minGain_),\n> > +\t\t\t exposure / clampShutter(exposure / minGain_),\n> > +\t\t\t 1 };\n> \n> I trust your calculations here :)\n> \n> > +\t}\n> > +\n> > +\tunsigned int stage;\n> > +\tutils::Duration stageShutter;\n> > +\tdouble stageGain;\n> > +\tdouble lastStageGain;\n> > +\n> > +\t/* We've already done stage 0 above so we start at 1 */\n> > +\tfor (stage = 1; stage < gains_.size(); stage++) {\n> > +\t\tstageShutter = shutterFixed ? shutter : clampShutter(shutters_[stage]);\n> > +\t\tstageGain = gainFixed ? gain : clampGain(gains_[stage]);\n> > +\t\tlastStageGain = gainFixed ? gain : clampGain(gains_[stage - 1]);\n> > +\n> > +\t\t/*\n> > +\t\t * If the product of the new stage shutter and the old stage\n> > +\t\t * gain is sufficient and we can change the shutter, reduce it.\n> > +\t\t */\n> > +\t\tif (!shutterFixed && stageShutter * lastStageGain >= exposure)\n> > +\t\t\treturn { clampShutter(exposure / lastStageGain), lastStageGain, 1 };\n> > +\n> > +\t\t/*\n> > +\t\t * The new stage shutter with old stage gain were insufficient,\n> > +\t\t * so try the new stage shutter with new stage gain. If it is\n> > +\t\t * sufficient and we can change the shutter, reduce it.\n> > +\t\t */\n> > +\t\tif (!shutterFixed && stageShutter * stageGain >= exposure)\n> > +\t\t\treturn { clampShutter(exposure / stageGain), stageGain, 1 };\n> > +\n> > +\t\t/*\n> > +\t\t * Same as above, but we can't change the shutter, so change\n> > +\t\t * the gain instead.\n> > +\t\t *\n> > +\t\t * Note that at least one of !shutterFixed and !gainFixed is\n> > +\t\t * guaranteed.\n> > +\t\t */\n> > +\t\tif (!gainFixed && stageShutter * stageGain >= exposure)\n> > +\t\t\treturn { stageShutter, clampGain(exposure / stageShutter), 1 };\n> > +\t}\n> > +\n> > +\t/* From here on we're going to try to max out shutter then gain */\n> > +\tshutter = shutterFixed ? shutter : maxShutter_;\n> > +\tgain = gainFixed ? gain : maxGain_;\n> > +\n> > +\t/*\n> > +\t * We probably don't want to use the actual maximum analogue gain (as\n> > +\t * it'll be unreasonably high), so we'll at least try to max out the\n> > +\t * shutter, which is expected to be a bit more reasonable, as it is\n> > +\t * limited by FrameDurationLimits and/or the sensor configuration.\n> > +\t */\n> > +\tif (!shutterFixed && shutter * stageGain >= exposure)\n> > +\t\treturn { clampShutter(exposure / stageGain), stageGain, 1 };\n> > +\n> > +\t/*\n> > +\t * If that's still not enough exposure, or if shutter is fixed, then\n> > +\t * we'll max out the analogue gain before using digital gain.\n> > +\t */\n> > +\tif (!gainFixed && shutter * gain >= exposure)\n> > +\t\treturn { shutter, clampGain(exposure / shutter), 1 };\n> > +\n> > +\t/*\n> > +\t * We're out of shutter time and analogue gain; send the rest of the\n> > +\t * exposure time to digital gain.\n> > +\t */\n> > +\treturn { shutter, gain, exposure / (shutter * gain) };\n> \n> This really seems helpful!\n> \n> > +}\n> > +\n> > +/**\n> > + * \\brief Split exposure time into shutter and gain\n> > + * \\param[in] exposure Exposure time\n> > + * \\return Tuple of shutter time, analogue gain, and digital gain\n> > + */\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure)\n> > +{\n> > +\tASSERT(maxShutter_);\n> > +\tASSERT(maxGain_);\n> > +\tutils::Duration shutter;\n> > +\tdouble gain;\n> > +\n> > +\tif (shutters_.size()) {\n> \n> Wait, if you have no shutter times\n> \n> > +\t\tshutter = shutters_.at(0);\n> > +\t\tgain = gains_.at(0);\n> > +\t} else {\n> > +\t\tshutter = maxShutter_;\n> \n> How can you have a maxShutter ?\n> \n> Or is it allowed to call configure() without init() ?\n> \n> > +\t\tgain = maxGain_;\n> > +\t}\n> > +\n> > +\treturn splitExposure(exposure, shutter, false, gain, false);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Split exposure time into shutter and gain, with fixed shutter\n> > + * \\param[in] exposure Exposure time\n> > + * \\param[in] fixedShutter Fixed shutter time\n> > + *\n> > + * Same as the base splitExposure, but with a fixed shutter (aka \"shutter priority\").\n\nThis can be achieved by setting the min and max shutter limits to the\nfixed shutter value. What is the reason for having two different ways to\ndo the same ?\n\n> > + *\n> > + * \\return Tuple of shutter time, analogue gain, and digital gain\n> > + */\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure, utils::Duration fixedShutter)\n> > +{\n> > +\tASSERT(maxGain_);\n> > +\tdouble gain = gains_.size() ? gains_.at(0) : maxGain_;\n> > +\n> > +\treturn splitExposure(exposure, fixedShutter, true, gain, false);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Split exposure time into shutter and gain, with fixed gain\n> > + * \\param[in] exposure Exposure time\n> > + * \\param[in] fixedGain Fixed gain\n> > + *\n> > + * Same as the base splitExposure, but with a fixed gain (aka \"gain priority\").\n> > + *\n> > + * \\return Tuple of shutter time, analogue gain, and digital gain\n> > + */\n> > +std::tuple\n> > +ExposureModeHelper::splitExposure(utils::Duration exposure, double fixedGain)\n> > +{\n> > +\tASSERT(maxShutter_);\n> > +\tutils::Duration shutter = shutters_.size() ? shutters_.at(0) : maxShutter_;\n> > +\n> > +\treturn splitExposure(exposure, shutter, false, fixedGain, true);\n> > +}\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::minShutter()\n> > + * \\brief Retrieve the configured minimum shutter time\n\nMissing \\return. Same below. Update the text to mention limits if you\nrename configure() to setShutterGainLimits(). Reference the\nsetShutterGainLimits() function explicitly.\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::maxShutter()\n> > + * \\brief Retrieve the configured maximum shutter time\n> > + */\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::minGain()\n> > + * \\brief Retrieve the configured minimum gain\n> > + */\n> > +\n> > +/**\n> > + * \\fn ExposureModeHelper::maxGain()\n> > + * \\brief Retrieve the configured maximum gain\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> > new file mode 100644\n> > index 00000000..d576c952\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > @@ -0,0 +1,61 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Paul Elder \n> > + *\n> > + * exposure_mode_helper.h - Helper class that performs computations relating to exposure\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> \n> Included by .cpp\n> \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +class ExposureModeHelper\n> > +{\n> > +public:\n> > +\tExposureModeHelper();\n> > +\t~ExposureModeHelper();\n> > +\n> > +\tint init(std::vector &shutters, std::vector &gains);\n> > +\tvoid configure(utils::Duration minShutter, utils::Duration maxShutter,\n> > +\t\t double minGain, double maxGain);\n> > +\n> > +\tstd::tuple splitExposure(utils::Duration exposure);\n\nThis function and all the public functions below are const.\n\n> > +\tstd::tuple splitExposure(utils::Duration exposure,\n> > +\t\t\t\t\t\t\t\t utils::Duration fixedShutter);\n> > +\tstd::tuple splitExposure(utils::Duration exposure,\n> > +\t\t\t\t\t\t\t\t double fixedGain);\n> > +\n> > +\tutils::Duration minShutter() { return minShutter_; };\n\nExtra semicolumn after }. Same below.\n\n> > +\tutils::Duration maxShutter() { return maxShutter_; };\n> > +\tdouble minGain() { return minGain_; };\n> > +\tdouble maxGain() { return maxGain_; };\n> > +\n> > +private:\n> > +\tutils::Duration clampShutter(utils::Duration shutter);\n> > +\tdouble clampGain(double gain);\n> > +\n> > +\tstd::tuple\n> > +\tsplitExposure(utils::Duration exposure,\n> > +\t\t utils::Duration shutter, bool shutterFixed,\n> > +\t\t double gain, bool gainFixed);\n> > +\n> > +\tstd::vector shutters_;\n> > +\tstd::vector gains_;\n> > +\n> > +\tutils::Duration minShutter_;\n> > +\tutils::Duration maxShutter_;\n> > +\tdouble minGain_;\n> > +\tdouble maxGain_;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 016b8e0e..37fbd177 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -3,6 +3,7 @@\n> > libipa_headers = files([\n> > 'algorithm.h',\n> > 'camera_sensor_helper.h',\n> > + 'exposure_mode_helper.h',\n> > 'fc_queue.h',\n> > 'histogram.h',\n> > 'module.h',\n> > @@ -11,6 +12,7 @@ libipa_headers = files([\n> > libipa_sources = files([\n> > 'algorithm.cpp',\n> > 'camera_sensor_helper.cpp',\n> > + 'exposure_mode_helper.cpp',\n> > 'fc_queue.cpp',\n> > 'histogram.cpp',\n> > 'module.cpp',\n> \n> This seems a really helpful addition overall !","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 37F94C0DA4\n\tfor ;\n\tFri, 5 Apr 2024 23:01:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1747262CA0;\n\tSat, 6 Apr 2024 01:01:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87F7961C2F\n\tfor ;\n\tSat, 6 Apr 2024 01:00:57 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D09CDD9;\n\tSat, 6 Apr 2024 01:00: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=\"JZ3Gmg/q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712358018;\n\tbh=c+/7jLuqEtKPXTnueKQqoicaIsqyrPGL+LYYlJ4Kn0U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JZ3Gmg/q4RdINMVDDgi/urOkFNS6d26LQagblr+pUZI2yiDr7NXC6jCXV2y2N0fAD\n\t1INw1ltm5hhTYe/O1i6iH/lp/deFI1jKoXF8KWMvnhXh7bTBxdr1BoDiRLiTo/pi8D\n\t4555MrmHRcTM/25B2ShFm6xxRWAKIo3gBinix+RE=","Date":"Sat, 6 Apr 2024 02:00:46 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Daniel Scally ,\n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH 03/10] ipa: libipa: Add ExposureModeHelper","Message-ID":"<20240405230046.GJ12507@pendragon.ideasonboard.com>","References":"<20240322131451.3092931-1-dan.scally@ideasonboard.com>\n\t<20240322131451.3092931-4-dan.scally@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]