[{"id":29258,"web_url":"https://patchwork.libcamera.org/comment/29258/","msgid":"","date":"2024-04-18T11:35:38","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Dan,\n\nOn Wed, Apr 17, 2024 at 02:15:31PM +0100, 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> Changes in v2:\n> \n> \t- Expanded the documentation\n> \t- Dropped the overloads for fixed shutter / gain - the same\n> \t functionality is instead done by setting min and max shutter and gain\n> \t to the same value\n> \t- Changed ::init() to consume a vector of pairs instead of two separate\n> \t vectors\n> \t- Reworked splitExposure()\n> \n> src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n> src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 312 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..6463de18\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -0,0 +1,257 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * exposure_mode_helper.cpp - 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> + * AEGC algorithms have a need to split exposure between shutter and gain.\n> + * Multiple implementations do so based on paired sets of shutter and gain\n> + * limits; provide a helper to avoid duplicating the code.\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 into shutter and gain\n> + *\n> + * The ExposureModeHelper class provides a standard interface through which an\n> + * AEGC algorithm can divide exposure between shutter and gain. It is configured\n> + * with a set of shutter and gain pairs and works by initially fixing gain at\n> + * 1.0 and increasing shutter up to the shutter value from the first pair in the\n> + * set in an attempt to meet the required exposure value.\n> + *\n> + * If the required exposure is not achievable by the first shutter value alone\n> + * it ramps gain up to the value from the first pair in the set. If the required\n> + * exposure is still not met it then allows shutter to ramp up to the shutter\n> + * value from the second pair in the set, and continues in this vein until\n> + * either the required exposure value is met, or else the hardware's shutter or\n> + * gain limits are reached.\n> + *\n> + * This method allows users to strike a balance between a well-exposed image and\n> + * an acceptable frame-rate, as opposed to simply maximising shutter followed by\n> + * gain. The same helpers can be used to perform the latter operation if needed\n> + * by passing an empty set of pairs to the initialisation function.\n> + *\n> + * The gain values may exceed a camera sensor's analogue gain limits if either\n> + * it or the IPA is also capable of digital gain. The configure() function must\n> + * be called with the hardware's limits to inform the helper of those\n> + * constraints. Any gain that is needed will be applied as analogue gain first\n> + * until the hardware's limit is reached, following which digital gain will be\n> + * used.\n> + */\n> +\n> +/**\n> + * \\brief Construct an ExposureModeHelper instance\n> + */\n> +ExposureModeHelper::ExposureModeHelper()\n> +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Initialize an ExposureModeHelper instance\n> + * \\param[in] stages The vector of paired shutter time and gain limits\n> + *\n> + * This function is expected to be called a single time once the algorithm that\n> + * is using these helpers has built the necessary list of shutter and gain pairs\n> + * to use (archetypically by parsing them from a tuning file during the\n> + * algorithm's .init() call).\n> + *\n> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> + * analogue and digital gain.\n> + *\n> + * The vector of stages may be empty. In that case, the helper will simply use\n> + * the runtime limits set through setShutterGainLimits() instead.\n> + */\n> +void ExposureModeHelper::init(const std::vector> stages)\n> +{\n> +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n> +\tif (!shutters_.empty()) {\n> +\t\tLOG(ExposureModeHelper, Warning)\n> +\t\t\t<< \"Initialization attempted multiple times\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tfor (auto stage : stages) {\n> +\t\tshutters_.push_back(stage.first);\n> +\t\tgains_.push_back(stage.second);\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Set the shutter and gain limits\n> + * \\param[in] minShutter The minimum shutter time supported\n> + * \\param[in] maxShutter The maximum shutter time supported\n> + * \\param[in] minGain The minimum analogue gain supported\n> + * \\param[in] maxGain The maximum analogue gain supported\n> + *\n> + * This function configures the shutter and analogue gain limits that need to be\n> + * adhered to as the helper divides up exposure. Note that these function *must*\n\ns/these/this/\n\n> + * be called whenever those limits change and before splitExposure() is used.\n> + *\n> + * If the algorithm using the helpers needs to indicate that either shutter,\n> + * analogue gain or both should be fixed it can do so by setting both the minima\n> + * and maxima to the same value.\n> + */\n> +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,\n> +\t\t\t\t\t utils::Duration maxShutter,\n> +\t\t\t\t\t double minGain,\n> +\t\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) const\n> +{\n> +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n> +}\n> +\n> +double ExposureModeHelper::clampGain(double gain) const\n> +{\n> +\treturn std::clamp(gain, minGain_, maxGain_);\n> +}\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain\n> + * \\param[in] exposure Exposure time\n> + *\n> + * This function divides a given exposure value into shutter time, analogue and\n> + * digital gain by iterating through sets of shutter and gain limits. At each\n> + * stage the current stage's shutter limit is multiplied by the previous stage's\n> + * gain limit (or 1.0 initially) to see if the combination of the two can meet\n> + * the required exposure value. If they cannot then splitExpothe current stage's shutter\n\ntypo \"splitExpothe\"?\n\n> + * limit is multiplied by the same stage's gain limit to see if that combination\n> + * can meet the required exposure value. If they cannot then the function moves\n> + * to consider the next stage.\n> + *\n> + * When a combination of shutter and gain _stage_ limits are found that are\n> + * sufficient to meet the required exposure value, the function attempts to\n> + * reduce shutter time as much as possible whilst fixing gain and still meeting\n> + * the exposure value. If a _runtime_ limit prevents shutter time from being\n> + * lowered enough to meet the exposure value with gain fixed at the stage limit,\n> + * gain is also lowered to compensate.\n> + *\n> + * Once the shutter time and gain values are ascertained, gain is assigned as\n> + * analogue gain as much as possible, with digital gain only in use if the\n> + * maximum analogue gain runtime limit is unable to accomodate the exposure\n> + * value.\n> + *\n> + * If no combination of shutter and gain limits is found that meets the required\n> + * exposure value, the helper falls-back to simply maximising the shutter time\n> + * first, followed by analogue gain, followed by digital gain.\n> + *\n> + * @return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> +{\n> +\tASSERT(maxShutter_);\n> +\tASSERT(maxGain_);\n> +\n> +\tbool gainFixed = minGain_ == maxGain_;\n> +\tbool shutterFixed = minShutter_ == maxShutter_;\n> +\n> +\t/*\n> +\t * There's no point entering the loop if we cannot change either gain\n> +\t * nor shutter anyway.\n> +\t */\n> +\tif (shutterFixed && gainFixed)\n> +\t\treturn { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };\n> +\n> +\tutils::Duration shutter;\n> +\tdouble stageGain;\n> +\tdouble gain;\n> +\n> +\tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n> +\t\tdouble lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);\n> +\t\tutils::Duration stageShutter = clampShutter(shutters_[stage]);\n> +\t\tstageGain = clampGain(gains_[stage]);\n> +\n> +\t\t/*\n> +\t\t * We perform the clamping on both shutter and gain in case the\n> +\t\t * helper has had limits set that prevent those values being\n> +\t\t * lowered beyond a certain minimum...this can happen at runtime\n> +\t\t * for various reasons and so would not be known when the stage\n> +\t\t * limits are initialised.\n> +\t\t */\n> +\n> +\t\tif (stageShutter * lastStageGain >= exposure) {\n> +\t\t\tshutter = clampShutter(exposure / clampGain(lastStageGain));\n> +\t\t\tgain = clampGain(exposure / shutter);\n> +\n> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> +\t\t}\n> +\n> +\t\tif (stageShutter * stageGain >= exposure) {\n> +\t\t\tshutter = clampShutter(exposure / clampGain(stageGain));\n> +\t\t\tgain = clampGain(exposure / shutter);\n> +\n> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> +\t\t}\n\nThis is really nice and clean but if I'm not mistaken you're ignoring\nfixed gain and fixed shutter...\n\n\nPaul\n\n> +\t}\n> +\n> +\t/*\n> +\t * From here on all we can do is max out the shutter, followed by the\n> +\t * analogue gain. If we still haven't achieved the target we send the\n> +\t * rest of the exposure time to digital gain. If we were given no stages\n> +\t * to use then set stageGain to 1.0 so that shutter is maxed before gain\n> +\t * touched at all.\n> +\t */\n> +\tif (gains_.empty())\n> +\t\tstageGain = 1.0;\n> +\n> +\tshutter = clampShutter(exposure / clampGain(stageGain));\n> +\tgain = clampGain(exposure / shutter);\n> +\n> +\treturn { shutter, gain, exposure / (shutter * gain) };\n> +}\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minShutter()\n> + * \\brief Retrieve the configured minimum shutter time limit set through\n> + * setShutterGainLimits()\n> + * \\return The minShutter_ value\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxShutter()\n> + * \\brief Retrieve the configured maximum shutter time set through\n> + * setShutterGainLimits()\n> + * \\return The maxShutter_ value\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minGain()\n> + * \\brief Retrieve the configured minimum gain set through\n> + * setShutterGainLimits()\n> + * \\return The minGain_ value\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxGain()\n> + * \\brief Retrieve the configured maximum gain set through\n> + * setShutterGainLimits()\n> + * \\return The maxGain_ value\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..1f672135\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -0,0 +1,53 @@\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> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class ExposureModeHelper\n> +{\n> +public:\n> +\tExposureModeHelper();\n> +\t~ExposureModeHelper() = default;\n> +\n> +\tvoid init(const std::vector> stages);\n> +\tvoid setShutterGainLimits(utils::Duration minShutter,\n> +\t\t\t\t utils::Duration maxShutter,\n> +\t\t\t\t double minGain, double maxGain);\n> +\n> +\tstd::tuple\n> +\tsplitExposure(utils::Duration exposure) const;\n> +\n> +\tutils::Duration minShutter() const { return minShutter_; }\n> +\tutils::Duration maxShutter() const { return maxShutter_; }\n> +\tdouble minGain() const { return minGain_; }\n> +\tdouble maxGain() const { return maxGain_; }\n> +\n> +private:\n> +\tutils::Duration clampShutter(utils::Duration shutter) const;\n> +\tdouble clampGain(double gain) const;\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 B18D3C3200\n\tfor ;\n\tThu, 18 Apr 2024 11:35:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ABD6E633F3;\n\tThu, 18 Apr 2024 13:35:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C660C61B4F\n\tfor ;\n\tThu, 18 Apr 2024 13:35:45 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3EF3C8E;\n\tThu, 18 Apr 2024 13:34:56 +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=\"UWgfWBFw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713440098;\n\tbh=qu3caXR3ld6J/xAcMt52pIZb/0RCatILlfm5wK82aro=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UWgfWBFwvzhTXL8fwgR83etzF/UjsHWBTQPJ714Dsk5oWWdm8e+YXr6XL5RzjGfG3\n\ttg2P9P6b4JZ9pUBO2egTmc0oawx5lCC7TGgsYG+z6btuim8leAezlqbXpsihSh53cZ\n\tG1f9PilPkPKezVkMaKSpNA5KnwnOCZzsrUaLOEKk=","Date":"Thu, 18 Apr 2024 20:35:38 +0900","From":"Paul Elder ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240417131536.484129-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":29259,"web_url":"https://patchwork.libcamera.org/comment/29259/","msgid":"<8695d5f5-7319-443b-84f4-1e5095dafc4e@ideasonboard.com>","date":"2024-04-18T11:48:13","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Paul\n\nOn 18/04/2024 12:35, Paul Elder wrote:\n> Hi Dan,\n>\n> On Wed, Apr 17, 2024 at 02:15:31PM +0100, 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>> Changes in v2:\n>>\n>> \t- Expanded the documentation\n>> \t- Dropped the overloads for fixed shutter / gain - the same\n>> \t functionality is instead done by setting min and max shutter and gain\n>> \t to the same value\n>> \t- Changed ::init() to consume a vector of pairs instead of two separate\n>> \t vectors\n>> \t- Reworked splitExposure()\n>>\n>> src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n>> src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 312 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..6463de18\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n>> @@ -0,0 +1,257 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Paul Elder \n>> + *\n>> + * exposure_mode_helper.cpp - 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>> + * AEGC algorithms have a need to split exposure between shutter and gain.\n>> + * Multiple implementations do so based on paired sets of shutter and gain\n>> + * limits; provide a helper to avoid duplicating the code.\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 into shutter and gain\n>> + *\n>> + * The ExposureModeHelper class provides a standard interface through which an\n>> + * AEGC algorithm can divide exposure between shutter and gain. It is configured\n>> + * with a set of shutter and gain pairs and works by initially fixing gain at\n>> + * 1.0 and increasing shutter up to the shutter value from the first pair in the\n>> + * set in an attempt to meet the required exposure value.\n>> + *\n>> + * If the required exposure is not achievable by the first shutter value alone\n>> + * it ramps gain up to the value from the first pair in the set. If the required\n>> + * exposure is still not met it then allows shutter to ramp up to the shutter\n>> + * value from the second pair in the set, and continues in this vein until\n>> + * either the required exposure value is met, or else the hardware's shutter or\n>> + * gain limits are reached.\n>> + *\n>> + * This method allows users to strike a balance between a well-exposed image and\n>> + * an acceptable frame-rate, as opposed to simply maximising shutter followed by\n>> + * gain. The same helpers can be used to perform the latter operation if needed\n>> + * by passing an empty set of pairs to the initialisation function.\n>> + *\n>> + * The gain values may exceed a camera sensor's analogue gain limits if either\n>> + * it or the IPA is also capable of digital gain. The configure() function must\n>> + * be called with the hardware's limits to inform the helper of those\n>> + * constraints. Any gain that is needed will be applied as analogue gain first\n>> + * until the hardware's limit is reached, following which digital gain will be\n>> + * used.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct an ExposureModeHelper instance\n>> + */\n>> +ExposureModeHelper::ExposureModeHelper()\n>> +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Initialize an ExposureModeHelper instance\n>> + * \\param[in] stages The vector of paired shutter time and gain limits\n>> + *\n>> + * This function is expected to be called a single time once the algorithm that\n>> + * is using these helpers has built the necessary list of shutter and gain pairs\n>> + * to use (archetypically by parsing them from a tuning file during the\n>> + * algorithm's .init() call).\n>> + *\n>> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n>> + * analogue and digital gain.\n>> + *\n>> + * The vector of stages may be empty. In that case, the helper will simply use\n>> + * the runtime limits set through setShutterGainLimits() instead.\n>> + */\n>> +void ExposureModeHelper::init(const std::vector> stages)\n>> +{\n>> +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n>> +\tif (!shutters_.empty()) {\n>> +\t\tLOG(ExposureModeHelper, Warning)\n>> +\t\t\t<< \"Initialization attempted multiple times\";\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tfor (auto stage : stages) {\n>> +\t\tshutters_.push_back(stage.first);\n>> +\t\tgains_.push_back(stage.second);\n>> +\t}\n>> +}\n>> +\n>> +/**\n>> + * \\brief Set the shutter and gain limits\n>> + * \\param[in] minShutter The minimum shutter time supported\n>> + * \\param[in] maxShutter The maximum shutter time supported\n>> + * \\param[in] minGain The minimum analogue gain supported\n>> + * \\param[in] maxGain The maximum analogue gain supported\n>> + *\n>> + * This function configures the shutter and analogue gain limits that need to be\n>> + * adhered to as the helper divides up exposure. Note that these function *must*\n> s/these/this/\n>\n>> + * be called whenever those limits change and before splitExposure() is used.\n>> + *\n>> + * If the algorithm using the helpers needs to indicate that either shutter,\n>> + * analogue gain or both should be fixed it can do so by setting both the minima\n>> + * and maxima to the same value.\n>> + */\n>> +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,\n>> +\t\t\t\t\t utils::Duration maxShutter,\n>> +\t\t\t\t\t double minGain,\n>> +\t\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) const\n>> +{\n>> +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n>> +}\n>> +\n>> +double ExposureModeHelper::clampGain(double gain) const\n>> +{\n>> +\treturn std::clamp(gain, minGain_, maxGain_);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Split exposure time into shutter and gain\n>> + * \\param[in] exposure Exposure time\n>> + *\n>> + * This function divides a given exposure value into shutter time, analogue and\n>> + * digital gain by iterating through sets of shutter and gain limits. At each\n>> + * stage the current stage's shutter limit is multiplied by the previous stage's\n>> + * gain limit (or 1.0 initially) to see if the combination of the two can meet\n>> + * the required exposure value. If they cannot then splitExpothe current stage's shutter\n> typo \"splitExpothe\"?\nArgh, I've actually got a whole section duplicated there!\n>\n>> + * limit is multiplied by the same stage's gain limit to see if that combination\n>> + * can meet the required exposure value. If they cannot then the function moves\n>> + * to consider the next stage.\n>> + *\n>> + * When a combination of shutter and gain _stage_ limits are found that are\n>> + * sufficient to meet the required exposure value, the function attempts to\n>> + * reduce shutter time as much as possible whilst fixing gain and still meeting\n>> + * the exposure value. If a _runtime_ limit prevents shutter time from being\n>> + * lowered enough to meet the exposure value with gain fixed at the stage limit,\n>> + * gain is also lowered to compensate.\n>> + *\n>> + * Once the shutter time and gain values are ascertained, gain is assigned as\n>> + * analogue gain as much as possible, with digital gain only in use if the\n>> + * maximum analogue gain runtime limit is unable to accomodate the exposure\n>> + * value.\n>> + *\n>> + * If no combination of shutter and gain limits is found that meets the required\n>> + * exposure value, the helper falls-back to simply maximising the shutter time\n>> + * first, followed by analogue gain, followed by digital gain.\n>> + *\n>> + * @return Tuple of shutter time, analogue gain, and digital gain\n>> + */\n>> +std::tuple\n>> +ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>> +{\n>> +\tASSERT(maxShutter_);\n>> +\tASSERT(maxGain_);\n>> +\n>> +\tbool gainFixed = minGain_ == maxGain_;\n>> +\tbool shutterFixed = minShutter_ == maxShutter_;\n>> +\n>> +\t/*\n>> +\t * There's no point entering the loop if we cannot change either gain\n>> +\t * nor shutter anyway.\n>> +\t */\n>> +\tif (shutterFixed && gainFixed)\n>> +\t\treturn { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };\n>> +\n>> +\tutils::Duration shutter;\n>> +\tdouble stageGain;\n>> +\tdouble gain;\n>> +\n>> +\tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n>> +\t\tdouble lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);\n>> +\t\tutils::Duration stageShutter = clampShutter(shutters_[stage]);\n>> +\t\tstageGain = clampGain(gains_[stage]);\n>> +\n>> +\t\t/*\n>> +\t\t * We perform the clamping on both shutter and gain in case the\n>> +\t\t * helper has had limits set that prevent those values being\n>> +\t\t * lowered beyond a certain minimum...this can happen at runtime\n>> +\t\t * for various reasons and so would not be known when the stage\n>> +\t\t * limits are initialised.\n>> +\t\t */\n>> +\n>> +\t\tif (stageShutter * lastStageGain >= exposure) {\n>> +\t\t\tshutter = clampShutter(exposure / clampGain(lastStageGain));\n>> +\t\t\tgain = clampGain(exposure / shutter);\n>> +\n>> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n>> +\t\t}\n>> +\n>> +\t\tif (stageShutter * stageGain >= exposure) {\n>> +\t\t\tshutter = clampShutter(exposure / clampGain(stageGain));\n>> +\t\t\tgain = clampGain(exposure / shutter);\n>> +\n>> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n>> +\t\t}\n> This is really nice and clean but if I'm not mistaken you're ignoring\n> fixed gain and fixed shutter...\nThat should just be accounted for by the fact that clampShutter() and clampGain() will fix the value \nat that fixed shutter/gain - at least that's the idea. I tested by comparing the original \nimplementation to this one, including with fixed shutter/gain/both and the calculated shutter time \nand gains matched.\n>\n> Paul\n>\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * From here on all we can do is max out the shutter, followed by the\n>> +\t * analogue gain. If we still haven't achieved the target we send the\n>> +\t * rest of the exposure time to digital gain. If we were given no stages\n>> +\t * to use then set stageGain to 1.0 so that shutter is maxed before gain\n>> +\t * touched at all.\n>> +\t */\n>> +\tif (gains_.empty())\n>> +\t\tstageGain = 1.0;\n>> +\n>> +\tshutter = clampShutter(exposure / clampGain(stageGain));\n>> +\tgain = clampGain(exposure / shutter);\n>> +\n>> +\treturn { shutter, gain, exposure / (shutter * gain) };\n>> +}\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::minShutter()\n>> + * \\brief Retrieve the configured minimum shutter time limit set through\n>> + * setShutterGainLimits()\n>> + * \\return The minShutter_ value\n>> + */\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::maxShutter()\n>> + * \\brief Retrieve the configured maximum shutter time set through\n>> + * setShutterGainLimits()\n>> + * \\return The maxShutter_ value\n>> + */\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::minGain()\n>> + * \\brief Retrieve the configured minimum gain set through\n>> + * setShutterGainLimits()\n>> + * \\return The minGain_ value\n>> + */\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::maxGain()\n>> + * \\brief Retrieve the configured maximum gain set through\n>> + * setShutterGainLimits()\n>> + * \\return The maxGain_ value\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..1f672135\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n>> @@ -0,0 +1,53 @@\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>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class ExposureModeHelper\n>> +{\n>> +public:\n>> +\tExposureModeHelper();\n>> +\t~ExposureModeHelper() = default;\n>> +\n>> +\tvoid init(const std::vector> stages);\n>> +\tvoid setShutterGainLimits(utils::Duration minShutter,\n>> +\t\t\t\t utils::Duration maxShutter,\n>> +\t\t\t\t double minGain, double maxGain);\n>> +\n>> +\tstd::tuple\n>> +\tsplitExposure(utils::Duration exposure) const;\n>> +\n>> +\tutils::Duration minShutter() const { return minShutter_; }\n>> +\tutils::Duration maxShutter() const { return maxShutter_; }\n>> +\tdouble minGain() const { return minGain_; }\n>> +\tdouble maxGain() const { return maxGain_; }\n>> +\n>> +private:\n>> +\tutils::Duration clampShutter(utils::Duration shutter) const;\n>> +\tdouble clampGain(double gain) const;\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 479A8BE08B\n\tfor ;\n\tThu, 18 Apr 2024 11:48:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 630AF6334D;\n\tThu, 18 Apr 2024 13:48:19 +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 2D9AC61B4F\n\tfor ;\n\tThu, 18 Apr 2024 13:48:17 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 633FAC8E;\n\tThu, 18 Apr 2024 13:47:29 +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=\"sAhT7g3R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713440849;\n\tbh=X9mEqKHg2GtRqQtwaR/HwVajJwp8IK/BTFtw56WynK8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=sAhT7g3RNO04JZPzKQ2VZYzcWr/pQXSfUEaRA7fGvC8caAWpow65rRcGXxcIBtE9S\n\tRxYOMCcyB+jEo8AYnEKCotERyX+C/UdkOL3wkhy7UULJhHrPDPOolf+q/vCW7K/iBr\n\tXyU+MIi3YtnoEcki1eSw0kmN6k4oS1jEgltdzB1Y=","Message-ID":"<8695d5f5-7319-443b-84f4-1e5095dafc4e@ideasonboard.com>","Date":"Thu, 18 Apr 2024 12:48:13 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":29263,"web_url":"https://patchwork.libcamera.org/comment/29263/","msgid":"","date":"2024-04-18T12:50:44","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Apr 18, 2024 at 12:48:13PM +0100, Dan Scally wrote:\n> Hi Paul\n> \n> On 18/04/2024 12:35, Paul Elder wrote:\n> > Hi Dan,\n> > \n> > On Wed, Apr 17, 2024 at 02:15:31PM +0100, 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> > > Changes in v2:\n> > > \n> > > \t- Expanded the documentation\n> > > \t- Dropped the overloads for fixed shutter / gain - the same\n> > > \t functionality is instead done by setting min and max shutter and gain\n> > > \t to the same value\n> > > \t- Changed ::init() to consume a vector of pairs instead of two separate\n> > > \t vectors\n> > > \t- Reworked splitExposure()\n> > > \n> > > src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n> > > src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n> > > src/ipa/libipa/meson.build | 2 +\n> > > 3 files changed, 312 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..6463de18\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > > @@ -0,0 +1,257 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Paul Elder \n> > > + *\n> > > + * exposure_mode_helper.cpp - 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> > > + * AEGC algorithms have a need to split exposure between shutter and gain.\n> > > + * Multiple implementations do so based on paired sets of shutter and gain\n> > > + * limits; provide a helper to avoid duplicating the code.\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 into shutter and gain\n> > > + *\n> > > + * The ExposureModeHelper class provides a standard interface through which an\n> > > + * AEGC algorithm can divide exposure between shutter and gain. It is configured\n> > > + * with a set of shutter and gain pairs and works by initially fixing gain at\n> > > + * 1.0 and increasing shutter up to the shutter value from the first pair in the\n> > > + * set in an attempt to meet the required exposure value.\n> > > + *\n> > > + * If the required exposure is not achievable by the first shutter value alone\n> > > + * it ramps gain up to the value from the first pair in the set. If the required\n> > > + * exposure is still not met it then allows shutter to ramp up to the shutter\n> > > + * value from the second pair in the set, and continues in this vein until\n> > > + * either the required exposure value is met, or else the hardware's shutter or\n> > > + * gain limits are reached.\n> > > + *\n> > > + * This method allows users to strike a balance between a well-exposed image and\n> > > + * an acceptable frame-rate, as opposed to simply maximising shutter followed by\n> > > + * gain. The same helpers can be used to perform the latter operation if needed\n> > > + * by passing an empty set of pairs to the initialisation function.\n> > > + *\n> > > + * The gain values may exceed a camera sensor's analogue gain limits if either\n> > > + * it or the IPA is also capable of digital gain. The configure() function must\n> > > + * be called with the hardware's limits to inform the helper of those\n> > > + * constraints. Any gain that is needed will be applied as analogue gain first\n> > > + * until the hardware's limit is reached, following which digital gain will be\n> > > + * used.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct an ExposureModeHelper instance\n> > > + */\n> > > +ExposureModeHelper::ExposureModeHelper()\n> > > +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Initialize an ExposureModeHelper instance\n> > > + * \\param[in] stages The vector of paired shutter time and gain limits\n> > > + *\n> > > + * This function is expected to be called a single time once the algorithm that\n> > > + * is using these helpers has built the necessary list of shutter and gain pairs\n> > > + * to use (archetypically by parsing them from a tuning file during the\n> > > + * algorithm's .init() call).\n> > > + *\n> > > + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> > > + * analogue and digital gain.\n> > > + *\n> > > + * The vector of stages may be empty. In that case, the helper will simply use\n> > > + * the runtime limits set through setShutterGainLimits() instead.\n> > > + */\n> > > +void ExposureModeHelper::init(const std::vector> stages)\n> > > +{\n> > > +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n> > > +\tif (!shutters_.empty()) {\n> > > +\t\tLOG(ExposureModeHelper, Warning)\n> > > +\t\t\t<< \"Initialization attempted multiple times\";\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tfor (auto stage : stages) {\n> > > +\t\tshutters_.push_back(stage.first);\n> > > +\t\tgains_.push_back(stage.second);\n> > > +\t}\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Set the shutter and gain limits\n> > > + * \\param[in] minShutter The minimum shutter time supported\n> > > + * \\param[in] maxShutter The maximum shutter time supported\n> > > + * \\param[in] minGain The minimum analogue gain supported\n> > > + * \\param[in] maxGain The maximum analogue gain supported\n> > > + *\n> > > + * This function configures the shutter and analogue gain limits that need to be\n> > > + * adhered to as the helper divides up exposure. Note that these function *must*\n> > s/these/this/\n> > \n> > > + * be called whenever those limits change and before splitExposure() is used.\n> > > + *\n> > > + * If the algorithm using the helpers needs to indicate that either shutter,\n> > > + * analogue gain or both should be fixed it can do so by setting both the minima\n> > > + * and maxima to the same value.\n> > > + */\n> > > +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,\n> > > +\t\t\t\t\t utils::Duration maxShutter,\n> > > +\t\t\t\t\t double minGain,\n> > > +\t\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) const\n> > > +{\n> > > +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n> > > +}\n> > > +\n> > > +double ExposureModeHelper::clampGain(double gain) const\n> > > +{\n> > > +\treturn std::clamp(gain, minGain_, maxGain_);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Split exposure time into shutter and gain\n> > > + * \\param[in] exposure Exposure time\n> > > + *\n> > > + * This function divides a given exposure value into shutter time, analogue and\n> > > + * digital gain by iterating through sets of shutter and gain limits. At each\n> > > + * stage the current stage's shutter limit is multiplied by the previous stage's\n> > > + * gain limit (or 1.0 initially) to see if the combination of the two can meet\n> > > + * the required exposure value. If they cannot then splitExpothe current stage's shutter\n> > typo \"splitExpothe\"?\n> Argh, I've actually got a whole section duplicated there!\n> > \n> > > + * limit is multiplied by the same stage's gain limit to see if that combination\n> > > + * can meet the required exposure value. If they cannot then the function moves\n> > > + * to consider the next stage.\n> > > + *\n> > > + * When a combination of shutter and gain _stage_ limits are found that are\n> > > + * sufficient to meet the required exposure value, the function attempts to\n> > > + * reduce shutter time as much as possible whilst fixing gain and still meeting\n> > > + * the exposure value. If a _runtime_ limit prevents shutter time from being\n> > > + * lowered enough to meet the exposure value with gain fixed at the stage limit,\n> > > + * gain is also lowered to compensate.\n> > > + *\n> > > + * Once the shutter time and gain values are ascertained, gain is assigned as\n> > > + * analogue gain as much as possible, with digital gain only in use if the\n> > > + * maximum analogue gain runtime limit is unable to accomodate the exposure\n> > > + * value.\n> > > + *\n> > > + * If no combination of shutter and gain limits is found that meets the required\n> > > + * exposure value, the helper falls-back to simply maximising the shutter time\n> > > + * first, followed by analogue gain, followed by digital gain.\n> > > + *\n> > > + * @return Tuple of shutter time, analogue gain, and digital gain\n> > > + */\n> > > +std::tuple\n> > > +ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> > > +{\n> > > +\tASSERT(maxShutter_);\n> > > +\tASSERT(maxGain_);\n> > > +\n> > > +\tbool gainFixed = minGain_ == maxGain_;\n> > > +\tbool shutterFixed = minShutter_ == maxShutter_;\n> > > +\n> > > +\t/*\n> > > +\t * There's no point entering the loop if we cannot change either gain\n> > > +\t * nor shutter anyway.\n> > > +\t */\n> > > +\tif (shutterFixed && gainFixed)\n> > > +\t\treturn { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };\n> > > +\n> > > +\tutils::Duration shutter;\n> > > +\tdouble stageGain;\n> > > +\tdouble gain;\n> > > +\n> > > +\tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n> > > +\t\tdouble lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);\n> > > +\t\tutils::Duration stageShutter = clampShutter(shutters_[stage]);\n> > > +\t\tstageGain = clampGain(gains_[stage]);\n> > > +\n> > > +\t\t/*\n> > > +\t\t * We perform the clamping on both shutter and gain in case the\n> > > +\t\t * helper has had limits set that prevent those values being\n> > > +\t\t * lowered beyond a certain minimum...this can happen at runtime\n> > > +\t\t * for various reasons and so would not be known when the stage\n> > > +\t\t * limits are initialised.\n> > > +\t\t */\n> > > +\n> > > +\t\tif (stageShutter * lastStageGain >= exposure) {\n> > > +\t\t\tshutter = clampShutter(exposure / clampGain(lastStageGain));\n> > > +\t\t\tgain = clampGain(exposure / shutter);\n> > > +\n> > > +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> > > +\t\t}\n> > > +\n> > > +\t\tif (stageShutter * stageGain >= exposure) {\n> > > +\t\t\tshutter = clampShutter(exposure / clampGain(stageGain));\n> > > +\t\t\tgain = clampGain(exposure / shutter);\n> > > +\n> > > +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> > > +\t\t}\n> > This is really nice and clean but if I'm not mistaken you're ignoring\n> > fixed gain and fixed shutter...\n> That should just be accounted for by the fact that clampShutter() and\n> clampGain() will fix the value at that fixed shutter/gain - at least that's\n> the idea. I tested by comparing the original implementation to this one,\n> including with fixed shutter/gain/both and the calculated shutter time and\n> gains matched.\n\nAh, indeed you're right. That's efficient.\n\nIt feels weird rb-ing a patch that I authored but you've changed it\nenough imo...\n\nReviewed-by: Paul Elder \n\n> > \n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * From here on all we can do is max out the shutter, followed by the\n> > > +\t * analogue gain. If we still haven't achieved the target we send the\n> > > +\t * rest of the exposure time to digital gain. If we were given no stages\n> > > +\t * to use then set stageGain to 1.0 so that shutter is maxed before gain\n> > > +\t * touched at all.\n> > > +\t */\n> > > +\tif (gains_.empty())\n> > > +\t\tstageGain = 1.0;\n> > > +\n> > > +\tshutter = clampShutter(exposure / clampGain(stageGain));\n> > > +\tgain = clampGain(exposure / shutter);\n> > > +\n> > > +\treturn { shutter, gain, exposure / (shutter * gain) };\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::minShutter()\n> > > + * \\brief Retrieve the configured minimum shutter time limit set through\n> > > + * setShutterGainLimits()\n> > > + * \\return The minShutter_ value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::maxShutter()\n> > > + * \\brief Retrieve the configured maximum shutter time set through\n> > > + * setShutterGainLimits()\n> > > + * \\return The maxShutter_ value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::minGain()\n> > > + * \\brief Retrieve the configured minimum gain set through\n> > > + * setShutterGainLimits()\n> > > + * \\return The minGain_ value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::maxGain()\n> > > + * \\brief Retrieve the configured maximum gain set through\n> > > + * setShutterGainLimits()\n> > > + * \\return The maxGain_ value\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..1f672135\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > > @@ -0,0 +1,53 @@\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> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa {\n> > > +\n> > > +class ExposureModeHelper\n> > > +{\n> > > +public:\n> > > +\tExposureModeHelper();\n> > > +\t~ExposureModeHelper() = default;\n> > > +\n> > > +\tvoid init(const std::vector> stages);\n> > > +\tvoid setShutterGainLimits(utils::Duration minShutter,\n> > > +\t\t\t\t utils::Duration maxShutter,\n> > > +\t\t\t\t double minGain, double maxGain);\n> > > +\n> > > +\tstd::tuple\n> > > +\tsplitExposure(utils::Duration exposure) const;\n> > > +\n> > > +\tutils::Duration minShutter() const { return minShutter_; }\n> > > +\tutils::Duration maxShutter() const { return maxShutter_; }\n> > > +\tdouble minGain() const { return minGain_; }\n> > > +\tdouble maxGain() const { return maxGain_; }\n> > > +\n> > > +private:\n> > > +\tutils::Duration clampShutter(utils::Duration shutter) const;\n> > > +\tdouble clampGain(double gain) const;\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 9D273BE08B\n\tfor ;\n\tThu, 18 Apr 2024 12:50:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 55C776334D;\n\tThu, 18 Apr 2024 14:50:53 +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 E4D396334D\n\tfor ;\n\tThu, 18 Apr 2024 14:50:51 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4D92F2BC;\n\tThu, 18 Apr 2024 14:50: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=\"RF5cgoPO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713444604;\n\tbh=9iWSbtodK5hVnpyvA31g9tpDCAYe9l3iZ5nKtTVvq88=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RF5cgoPOwbny+sZ4ItL5bF0VjumlPSpLQQe/FjH214Ir/8rByoi0Bi0hIMfg88xZK\n\th9uT9xB97xfi52m0/NDvD40Z6qnkvtNXPW/OZ/hTtkzrhP0vSwp5gDuZXEEgUJ6a4S\n\t6W9/O2zqurUmIRMcOIASzNeWOs4j04bIqXPfTTQI=","Date":"Thu, 18 Apr 2024 21:50:44 +0900","From":"Paul Elder ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t\n\t<8695d5f5-7319-443b-84f4-1e5095dafc4e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<8695d5f5-7319-443b-84f4-1e5095dafc4e@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":29276,"web_url":"https://patchwork.libcamera.org/comment/29276/","msgid":"","date":"2024-04-19T14:10:35","subject":"Re: [PATCH v2 3/8] 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\n\nOn Wed, Apr 17, 2024 at 02:15:31PM +0100, 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> Changes in v2:\n>\n> \t- Expanded the documentation\n> \t- Dropped the overloads for fixed shutter / gain - the same\n> \t functionality is instead done by setting min and max shutter and gain\n> \t to the same value\n> \t- Changed ::init() to consume a vector of pairs instead of two separate\n> \t vectors\n> \t- Reworked splitExposure()\n>\n> src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n> src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 312 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..6463de18\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -0,0 +1,257 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * exposure_mode_helper.cpp - 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> + * AEGC algorithms have a need to split exposure between shutter and gain.\n\ns/shutter/shutter time ?\n\nThis and in the other occurences\n\nalso, \"gain\" which gain ? Analogue, digital or a combination of the\ntwo ?\n\n> + * Multiple implementations do so based on paired sets of shutter and gain\n> + * limits; provide a helper to avoid duplicating the code.\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 into shutter and gain\n> + *\n> + * The ExposureModeHelper class provides a standard interface through which an\n> + * AEGC algorithm can divide exposure between shutter and gain. It is configured\n> + * with a set of shutter and gain pairs and works by initially fixing gain at\n> + * 1.0 and increasing shutter up to the shutter value from the first pair in the\n> + * set in an attempt to meet the required exposure value.\n> + *\n> + * If the required exposure is not achievable by the first shutter value alone\n> + * it ramps gain up to the value from the first pair in the set. If the required\n> + * exposure is still not met it then allows shutter to ramp up to the shutter\n> + * value from the second pair in the set, and continues in this vein until\n> + * either the required exposure value is met, or else the hardware's shutter or\n> + * gain limits are reached.\n> + *\n> + * This method allows users to strike a balance between a well-exposed image and\n> + * an acceptable frame-rate, as opposed to simply maximising shutter followed by\n> + * gain. The same helpers can be used to perform the latter operation if needed\n> + * by passing an empty set of pairs to the initialisation function.\n> + *\n> + * The gain values may exceed a camera sensor's analogue gain limits if either\n> + * it or the IPA is also capable of digital gain. The configure() function must\n> + * be called with the hardware's limits to inform the helper of those\n> + * constraints. Any gain that is needed will be applied as analogue gain first\n> + * until the hardware's limit is reached, following which digital gain will be\n> + * used.\n> + */\n> +\n> +/**\n> + * \\brief Construct an ExposureModeHelper instance\n> + */\n> +ExposureModeHelper::ExposureModeHelper()\n> +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Initialize an ExposureModeHelper instance\n> + * \\param[in] stages The vector of paired shutter time and gain limits\n> + *\n> + * This function is expected to be called a single time once the algorithm that\n> + * is using these helpers has built the necessary list of shutter and gain pairs\n> + * to use (archetypically by parsing them from a tuning file during the\n\nIs archetypically intentional ? Isn't \"typically\" enough ?\n\n> + * algorithm's .init() call).\n> + *\n> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> + * analogue and digital gain.\n> + *\n\nShutter -time- ?\n\nAnd yes, now gain is better explained.\n\n> + * The vector of stages may be empty. In that case, the helper will simply use\n> + * the runtime limits set through setShutterGainLimits() instead.\n> + */\n> +void ExposureModeHelper::init(const std::vector> stages)\n> +{\n> +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n> +\tif (!shutters_.empty()) {\n> +\t\tLOG(ExposureModeHelper, Warning)\n> +\t\t\t<< \"Initialization attempted multiple times\";\n> +\t\treturn;\n\nAs we do this in a function instead of a constructor, we can return a\nvalue. Is this useful for users of this class ?\n\n> +\t}\n> +\n> +\tfor (auto stage : stages) {\n\nauto const &\n\n> +\t\tshutters_.push_back(stage.first);\n> +\t\tgains_.push_back(stage.second);\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Set the shutter and gain limits\n> + * \\param[in] minShutter The minimum shutter time supported\n> + * \\param[in] maxShutter The maximum shutter time supported\n> + * \\param[in] minGain The minimum analogue gain supported\n> + * \\param[in] maxGain The maximum analogue gain supported\n\nAh so this is only analogue ? but the steps are the total gain ?\n\n> + *\n> + * This function configures the shutter and analogue gain limits that need to be\n> + * adhered to as the helper divides up exposure. Note that these function *must*\n> + * be called whenever those limits change and before splitExposure() is used.\n> + *\n> + * If the algorithm using the helpers needs to indicate that either shutter,\n> + * analogue gain or both should be fixed it can do so by setting both the minima\n> + * and maxima to the same value.\n\nAre minima and maxima intentional ? Isn't this minimum and maximum ?\n\n> + */\n> +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,\n\nOr just setLimits()\n\n> +\t\t\t\t\t utils::Duration maxShutter,\n> +\t\t\t\t\t double minGain,\n> +\t\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) const\n> +{\n> +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n> +}\n> +\n> +double ExposureModeHelper::clampGain(double gain) const\n> +{\n> +\treturn std::clamp(gain, minGain_, maxGain_);\n> +}\n> +\n> +/**\n> + * \\brief Split exposure time into shutter and gain\n> + * \\param[in] exposure Exposure time\n> + *\n> + * This function divides a given exposure value into shutter time, analogue and\n\nexposure 'value' or exposure 'time' as in the function's brief ?\n\n> + * digital gain by iterating through sets of shutter and gain limits. At each\n\ndoes this iterate 'limits' or the stages populated at init() time ?\n\n> + * stage the current stage's shutter limit is multiplied by the previous stage's\n> + * gain limit (or 1.0 initially) to see if the combination of the two can meet\n> + * the required exposure value. If they cannot then splitExpothe current stage's shutter\n\nExposure definitely seems to be a value then (and this matches my\nunderstanding)\n\n'splitExposthe' ?\n\n\n> + * limit is multiplied by the same stage's gain limit to see if that combination\n> + * can meet the required exposure value. If they cannot then the function moves\n> + * to consider the next stage.\n\nI think something went wrong with formatting here\n\n> + *\n> + * When a combination of shutter and gain _stage_ limits are found that are\n\nwhy '_stage_' ? and why limits ?\n\nCan't it be simply:\n\n * When a combination of shutter time and gain are found to be sufficient\n\n> + * sufficient to meet the required exposure value, the function attempts to\n> + * reduce shutter time as much as possible whilst fixing gain and still meeting\n> + * the exposure value. If a _runtime_ limit prevents shutter time from being\n\nAgain, why _runtime_ ?\n\n> + * lowered enough to meet the exposure value with gain fixed at the stage limit,\n> + * gain is also lowered to compensate.\n> + *\n> + * Once the shutter time and gain values are ascertained, gain is assigned as\n> + * analogue gain as much as possible, with digital gain only in use if the\n> + * maximum analogue gain runtime limit is unable to accomodate the exposure\n> + * value.\n> + *\n> + * If no combination of shutter and gain limits is found that meets the required\n> + * exposure value, the helper falls-back to simply maximising the shutter time\n> + * first, followed by analogue gain, followed by digital gain.\n> + *\n> + * @return Tuple of shutter time, analogue gain, and digital gain\n> + */\n> +std::tuple\n> +ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> +{\n> +\tASSERT(maxShutter_);\n> +\tASSERT(maxGain_);\n> +\n> +\tbool gainFixed = minGain_ == maxGain_;\n> +\tbool shutterFixed = minShutter_ == maxShutter_;\n> +\n> +\t/*\n> +\t * There's no point entering the loop if we cannot change either gain\n> +\t * nor shutter anyway.\n> +\t */\n> +\tif (shutterFixed && gainFixed)\n> +\t\treturn { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };\n> +\n> +\tutils::Duration shutter;\n> +\tdouble stageGain;\n> +\tdouble gain;\n> +\n> +\tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n> +\t\tdouble lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);\n> +\t\tutils::Duration stageShutter = clampShutter(shutters_[stage]);\n> +\t\tstageGain = clampGain(gains_[stage]);\n> +\n> +\t\t/*\n> +\t\t * We perform the clamping on both shutter and gain in case the\n> +\t\t * helper has had limits set that prevent those values being\n> +\t\t * lowered beyond a certain minimum...this can happen at runtime\n> +\t\t * for various reasons and so would not be known when the stage\n> +\t\t * limits are initialised.\n> +\t\t */\n> +\n> +\t\tif (stageShutter * lastStageGain >= exposure) {\n> +\t\t\tshutter = clampShutter(exposure / clampGain(lastStageGain));\n> +\t\t\tgain = clampGain(exposure / shutter);\n> +\n> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> +\t\t}\n> +\n> +\t\tif (stageShutter * stageGain >= exposure) {\n> +\t\t\tshutter = clampShutter(exposure / clampGain(stageGain));\n> +\t\t\tgain = clampGain(exposure / shutter);\n> +\n> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * From here on all we can do is max out the shutter, followed by the\n> +\t * analogue gain. If we still haven't achieved the target we send the\n> +\t * rest of the exposure time to digital gain. If we were given no stages\n> +\t * to use then set stageGain to 1.0 so that shutter is maxed before gain\n> +\t * touched at all.\n> +\t */\n> +\tif (gains_.empty())\n> +\t\tstageGain = 1.0;\n> +\n> +\tshutter = clampShutter(exposure / clampGain(stageGain));\n> +\tgain = clampGain(exposure / shutter);\n> +\n> +\treturn { shutter, gain, exposure / (shutter * gain) };\n> +}\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minShutter()\n> + * \\brief Retrieve the configured minimum shutter time limit set through\n> + * setShutterGainLimits()\n\nNo alignement in briefs\n\n> + * \\return The minShutter_ value\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxShutter()\n> + * \\brief Retrieve the configured maximum shutter time set through\n> + * setShutterGainLimits()\n\nditto\n\n> + * \\return The maxShutter_ value\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::minGain()\n> + * \\brief Retrieve the configured minimum gain set through\n> + * setShutterGainLimits()\n> + * \\return The minGain_ value\n> + */\n> +\n> +/**\n> + * \\fn ExposureModeHelper::maxGain()\n> + * \\brief Retrieve the configured maximum gain set through\n> + * setShutterGainLimits()\n> + * \\return The maxGain_ value\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..1f672135\n> --- /dev/null\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -0,0 +1,53 @@\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\nMissing \n\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() = default;\n> +\n> +\tvoid init(const std::vector> stages);\n> +\tvoid setShutterGainLimits(utils::Duration minShutter,\n> +\t\t\t\t utils::Duration maxShutter,\n> +\t\t\t\t double minGain, double maxGain);\n> +\n> +\tstd::tuple\n> +\tsplitExposure(utils::Duration exposure) const;\n> +\n> +\tutils::Duration minShutter() const { return minShutter_; }\n> +\tutils::Duration maxShutter() const { return maxShutter_; }\n> +\tdouble minGain() const { return minGain_; }\n> +\tdouble maxGain() const { return maxGain_; }\n> +\n> +private:\n> +\tutils::Duration clampShutter(utils::Duration shutter) const;\n> +\tdouble clampGain(double gain) const;\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 C01D1BE08B\n\tfor ;\n\tFri, 19 Apr 2024 14:10:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9085B633F3;\n\tFri, 19 Apr 2024 16:10:39 +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 EC4EC61C15\n\tfor ;\n\tFri, 19 Apr 2024 16:10:37 +0200 (CEST)","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 511E363B;\n\tFri, 19 Apr 2024 16:09:49 +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=\"Bos4naOt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713535789;\n\tbh=sxI/eACuHCKY276zBLwMP596yPQMOXSmec1gXNL+Ibw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Bos4naOt0G9bGS5VVYGJKxPdLfZKkwGqPmduBlOMW8VIWp4THBFhUlNGWy+GdJ0C7\n\t+rDAuC/N2awgYYRUo0r0i+cD8tY+7LxHdZeNYC5wk6Pn5Ut3ngO2Tr3fmxzF3AkJX5\n\tHeXLulBCzAQtlHaGymUYRGinh/732ZwpnYRfCI20=","Date":"Fri, 19 Apr 2024 16:10:35 +0200","From":"Jacopo Mondi ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240417131536.484129-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":29309,"web_url":"https://patchwork.libcamera.org/comment/29309/","msgid":"","date":"2024-04-23T09:16:31","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 19/04/2024 15:10, Jacopo Mondi wrote:\n> Hi Dan\n>\n> On Wed, Apr 17, 2024 at 02:15:31PM +0100, 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>> Changes in v2:\n>>\n>> \t- Expanded the documentation\n>> \t- Dropped the overloads for fixed shutter / gain - the same\n>> \t functionality is instead done by setting min and max shutter and gain\n>> \t to the same value\n>> \t- Changed ::init() to consume a vector of pairs instead of two separate\n>> \t vectors\n>> \t- Reworked splitExposure()\n>>\n>> src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n>> src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 312 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..6463de18\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n>> @@ -0,0 +1,257 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Paul Elder \n>> + *\n>> + * exposure_mode_helper.cpp - 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>> + * AEGC algorithms have a need to split exposure between shutter and gain.\n> s/shutter/shutter time ?\n>\n> This and in the other occurences\n>\n> also, \"gain\" which gain ? Analogue, digital or a combination of the\n> two ?\n\n\nBoth\n\n>\n>> + * Multiple implementations do so based on paired sets of shutter and gain\n>> + * limits; provide a helper to avoid duplicating the code.\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 into shutter and gain\n>> + *\n>> + * The ExposureModeHelper class provides a standard interface through which an\n>> + * AEGC algorithm can divide exposure between shutter and gain. It is configured\n>> + * with a set of shutter and gain pairs and works by initially fixing gain at\n>> + * 1.0 and increasing shutter up to the shutter value from the first pair in the\n>> + * set in an attempt to meet the required exposure value.\n>> + *\n>> + * If the required exposure is not achievable by the first shutter value alone\n>> + * it ramps gain up to the value from the first pair in the set. If the required\n>> + * exposure is still not met it then allows shutter to ramp up to the shutter\n>> + * value from the second pair in the set, and continues in this vein until\n>> + * either the required exposure value is met, or else the hardware's shutter or\n>> + * gain limits are reached.\n>> + *\n>> + * This method allows users to strike a balance between a well-exposed image and\n>> + * an acceptable frame-rate, as opposed to simply maximising shutter followed by\n>> + * gain. The same helpers can be used to perform the latter operation if needed\n>> + * by passing an empty set of pairs to the initialisation function.\n>> + *\n>> + * The gain values may exceed a camera sensor's analogue gain limits if either\n>> + * it or the IPA is also capable of digital gain. The configure() function must\n>> + * be called with the hardware's limits to inform the helper of those\n>> + * constraints. Any gain that is needed will be applied as analogue gain first\n>> + * until the hardware's limit is reached, following which digital gain will be\n>> + * used.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct an ExposureModeHelper instance\n>> + */\n>> +ExposureModeHelper::ExposureModeHelper()\n>> +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Initialize an ExposureModeHelper instance\n>> + * \\param[in] stages The vector of paired shutter time and gain limits\n>> + *\n>> + * This function is expected to be called a single time once the algorithm that\n>> + * is using these helpers has built the necessary list of shutter and gain pairs\n>> + * to use (archetypically by parsing them from a tuning file during the\n> Is archetypically intentional ? Isn't \"typically\" enough ?\n\n\nIt's intentional yes; \"archetypically\" is like \"in the first implementation\" whereas \"typically\" is \nlike \"in most implementations\"...but I can switch to typically so it's less confusing, either works.\n\n>\n>> + * algorithm's .init() call).\n>> + *\n>> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n>> + * analogue and digital gain.\n>> + *\n> Shutter -time- ?\n>\n> And yes, now gain is better explained.\n>\n>> + * The vector of stages may be empty. In that case, the helper will simply use\n>> + * the runtime limits set through setShutterGainLimits() instead.\n>> + */\n>> +void ExposureModeHelper::init(const std::vector> stages)\n>> +{\n>> +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n>> +\tif (!shutters_.empty()) {\n>> +\t\tLOG(ExposureModeHelper, Warning)\n>> +\t\t\t<< \"Initialization attempted multiple times\";\n>> +\t\treturn;\n> As we do this in a function instead of a constructor, we can return a\n> value. Is this useful for users of this class ?\n\n\nI don't think there's any need to return anything in this function if that's what you mean\n\n>\n>> +\t}\n>> +\n>> +\tfor (auto stage : stages) {\n> auto const &\n>\n>> +\t\tshutters_.push_back(stage.first);\n>> +\t\tgains_.push_back(stage.second);\n>> +\t}\n>> +}\n>> +\n>> +/**\n>> + * \\brief Set the shutter and gain limits\n>> + * \\param[in] minShutter The minimum shutter time supported\n>> + * \\param[in] maxShutter The maximum shutter time supported\n>> + * \\param[in] minGain The minimum analogue gain supported\n>> + * \\param[in] maxGain The maximum analogue gain supported\n> Ah so this is only analogue ? but the steps are the total gain ?\n\n\nCorrect\n\n>\n>> + *\n>> + * This function configures the shutter and analogue gain limits that need to be\n>> + * adhered to as the helper divides up exposure. Note that these function *must*\n>> + * be called whenever those limits change and before splitExposure() is used.\n>> + *\n>> + * If the algorithm using the helpers needs to indicate that either shutter,\n>> + * analogue gain or both should be fixed it can do so by setting both the minima\n>> + * and maxima to the same value.\n> Are minima and maxima intentional ? Isn't this minimum and maximum ?\n\n\nMinima/maxima is the plural, meaning to refer to both \"minShutter\" and \"minGain\" for example\n\n>\n>> + */\n>> +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,\n> Or just setLimits()\n>\n>> +\t\t\t\t\t utils::Duration maxShutter,\n>> +\t\t\t\t\t double minGain,\n>> +\t\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) const\n>> +{\n>> +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n>> +}\n>> +\n>> +double ExposureModeHelper::clampGain(double gain) const\n>> +{\n>> +\treturn std::clamp(gain, minGain_, maxGain_);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Split exposure time into shutter and gain\n>> + * \\param[in] exposure Exposure time\n>> + *\n>> + * This function divides a given exposure value into shutter time, analogue and\n> exposure 'value' or exposure 'time' as in the function's brief ?\n\n\nIt's Exposure time rather than value, if by Exposure value you mean: \nhttps://en.wikipedia.org/wiki/Exposure_value\n\n\n\n>\n>> + * digital gain by iterating through sets of shutter and gain limits. At each\n> does this iterate 'limits' or the stages populated at init() time ?\n\n\nThe limits parsed from tuning data\n\n>\n>> + * stage the current stage's shutter limit is multiplied by the previous stage's\n>> + * gain limit (or 1.0 initially) to see if the combination of the two can meet\n>> + * the required exposure value. If they cannot then splitExpothe current stage's shutter\n> Exposure definitely seems to be a value then (and this matches my\n> understanding)\n\n\nI just mean the value of the exposure parameter; the input is the exposure time needed to attain the \ntarget\n\n>\n> 'splitExposthe' ?\n>\n>\n>> + * limit is multiplied by the same stage's gain limit to see if that combination\n>> + * can meet the required exposure value. If they cannot then the function moves\n>> + * to consider the next stage.\n> I think something went wrong with formatting here\n>\n>> + *\n>> + * When a combination of shutter and gain _stage_ limits are found that are\n> why '_stage_' ? and why limits ?\n>\n> Can't it be simply:\n>\n> * When a combination of shutter time and gain are found to be sufficient\n\n\nI was trying to make it clear that it's working on the limits from the sets parsed from tuning data \nrather than the runtime limits set through setShutterGainLimits.\n\n>\n>> + * sufficient to meet the required exposure value, the function attempts to\n>> + * reduce shutter time as much as possible whilst fixing gain and still meeting\n>> + * the exposure value. If a _runtime_ limit prevents shutter time from being\n> Again, why _runtime_ ?\n\n\nAnd here trying to emphasis that it's the limits from setShutterGainLimits rather than those parsed \nfrom Yaml\n\n>\n>> + * lowered enough to meet the exposure value with gain fixed at the stage limit,\n>> + * gain is also lowered to compensate.\n>> + *\n>> + * Once the shutter time and gain values are ascertained, gain is assigned as\n>> + * analogue gain as much as possible, with digital gain only in use if the\n>> + * maximum analogue gain runtime limit is unable to accomodate the exposure\n>> + * value.\n>> + *\n>> + * If no combination of shutter and gain limits is found that meets the required\n>> + * exposure value, the helper falls-back to simply maximising the shutter time\n>> + * first, followed by analogue gain, followed by digital gain.\n>> + *\n>> + * @return Tuple of shutter time, analogue gain, and digital gain\n>> + */\n>> +std::tuple\n>> +ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>> +{\n>> +\tASSERT(maxShutter_);\n>> +\tASSERT(maxGain_);\n>> +\n>> +\tbool gainFixed = minGain_ == maxGain_;\n>> +\tbool shutterFixed = minShutter_ == maxShutter_;\n>> +\n>> +\t/*\n>> +\t * There's no point entering the loop if we cannot change either gain\n>> +\t * nor shutter anyway.\n>> +\t */\n>> +\tif (shutterFixed && gainFixed)\n>> +\t\treturn { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };\n>> +\n>> +\tutils::Duration shutter;\n>> +\tdouble stageGain;\n>> +\tdouble gain;\n>> +\n>> +\tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n>> +\t\tdouble lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);\n>> +\t\tutils::Duration stageShutter = clampShutter(shutters_[stage]);\n>> +\t\tstageGain = clampGain(gains_[stage]);\n>> +\n>> +\t\t/*\n>> +\t\t * We perform the clamping on both shutter and gain in case the\n>> +\t\t * helper has had limits set that prevent those values being\n>> +\t\t * lowered beyond a certain minimum...this can happen at runtime\n>> +\t\t * for various reasons and so would not be known when the stage\n>> +\t\t * limits are initialised.\n>> +\t\t */\n>> +\n>> +\t\tif (stageShutter * lastStageGain >= exposure) {\n>> +\t\t\tshutter = clampShutter(exposure / clampGain(lastStageGain));\n>> +\t\t\tgain = clampGain(exposure / shutter);\n>> +\n>> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n>> +\t\t}\n>> +\n>> +\t\tif (stageShutter * stageGain >= exposure) {\n>> +\t\t\tshutter = clampShutter(exposure / clampGain(stageGain));\n>> +\t\t\tgain = clampGain(exposure / shutter);\n>> +\n>> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n>> +\t\t}\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * From here on all we can do is max out the shutter, followed by the\n>> +\t * analogue gain. If we still haven't achieved the target we send the\n>> +\t * rest of the exposure time to digital gain. If we were given no stages\n>> +\t * to use then set stageGain to 1.0 so that shutter is maxed before gain\n>> +\t * touched at all.\n>> +\t */\n>> +\tif (gains_.empty())\n>> +\t\tstageGain = 1.0;\n>> +\n>> +\tshutter = clampShutter(exposure / clampGain(stageGain));\n>> +\tgain = clampGain(exposure / shutter);\n>> +\n>> +\treturn { shutter, gain, exposure / (shutter * gain) };\n>> +}\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::minShutter()\n>> + * \\brief Retrieve the configured minimum shutter time limit set through\n>> + * setShutterGainLimits()\n> No alignement in briefs\n>\n>> + * \\return The minShutter_ value\n>> + */\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::maxShutter()\n>> + * \\brief Retrieve the configured maximum shutter time set through\n>> + * setShutterGainLimits()\n> ditto\n>\n>> + * \\return The maxShutter_ value\n>> + */\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::minGain()\n>> + * \\brief Retrieve the configured minimum gain set through\n>> + * setShutterGainLimits()\n>> + * \\return The minGain_ value\n>> + */\n>> +\n>> +/**\n>> + * \\fn ExposureModeHelper::maxGain()\n>> + * \\brief Retrieve the configured maximum gain set through\n>> + * setShutterGainLimits()\n>> + * \\return The maxGain_ value\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..1f672135\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n>> @@ -0,0 +1,53 @@\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> Missing \n>\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() = default;\n>> +\n>> +\tvoid init(const std::vector> stages);\n>> +\tvoid setShutterGainLimits(utils::Duration minShutter,\n>> +\t\t\t\t utils::Duration maxShutter,\n>> +\t\t\t\t double minGain, double maxGain);\n>> +\n>> +\tstd::tuple\n>> +\tsplitExposure(utils::Duration exposure) const;\n>> +\n>> +\tutils::Duration minShutter() const { return minShutter_; }\n>> +\tutils::Duration maxShutter() const { return maxShutter_; }\n>> +\tdouble minGain() const { return minGain_; }\n>> +\tdouble maxGain() const { return maxGain_; }\n>> +\n>> +private:\n>> +\tutils::Duration clampShutter(utils::Duration shutter) const;\n>> +\tdouble clampGain(double gain) const;\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 05AEEBE08B\n\tfor ;\n\tTue, 23 Apr 2024 09:16:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 051D263417;\n\tTue, 23 Apr 2024 11:16:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9426963416\n\tfor ;\n\tTue, 23 Apr 2024 11:16:34 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38B6A65D;\n\tTue, 23 Apr 2024 11:15:43 +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=\"BARDu1x4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713863743;\n\tbh=cmpUzEDv4F4VwB6sgShyKSiu/zgB+BFB2OiYXOshb10=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=BARDu1x4Tvb7t/C/czQFZj9zKKnOWP9EVjJolCWA1cLRRWB21XgSQHwMqpoBbWCLu\n\tKwKfeoA+DU4wcAAuZFRNvWlPxZLsfBkMXpqDe6dnuJs0kLllZe/yuljqAB02LOuZiR\n\t0beP5n6TgQZw2mW9bHGnXHn93vXhjnVlWovUIfk4=","Message-ID":"","Date":"Tue, 23 Apr 2024 10:16:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":29312,"web_url":"https://patchwork.libcamera.org/comment/29312/","msgid":"","date":"2024-04-23T10:19:55","subject":"Re: [PATCH v2 3/8] 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\n\nOn Tue, Apr 23, 2024 at 10:16:31AM +0100, Dan Scally wrote:\n> Hi Jacopo\n>\n> On 19/04/2024 15:10, Jacopo Mondi wrote:\n> > Hi Dan\n> >\n> > On Wed, Apr 17, 2024 at 02:15:31PM +0100, 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> > > Changes in v2:\n> > >\n> > > \t- Expanded the documentation\n> > > \t- Dropped the overloads for fixed shutter / gain - the same\n> > > \t functionality is instead done by setting min and max shutter and gain\n> > > \t to the same value\n> > > \t- Changed ::init() to consume a vector of pairs instead of two separate\n> > > \t vectors\n> > > \t- Reworked splitExposure()\n> > >\n> > > src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n> > > src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n> > > src/ipa/libipa/meson.build | 2 +\n> > > 3 files changed, 312 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..6463de18\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > > @@ -0,0 +1,257 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Paul Elder \n> > > + *\n> > > + * exposure_mode_helper.cpp - 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> > > + * AEGC algorithms have a need to split exposure between shutter and gain.\n> > s/shutter/shutter time ?\n> >\n> > This and in the other occurences\n> >\n> > also, \"gain\" which gain ? Analogue, digital or a combination of the\n> > two ?\n>\n>\n> Both\n>\n\nMaybe specify it then ? :)\n\n> >\n> > > + * Multiple implementations do so based on paired sets of shutter and gain\n> > > + * limits; provide a helper to avoid duplicating the code.\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 into shutter and gain\n> > > + *\n> > > + * The ExposureModeHelper class provides a standard interface through which an\n> > > + * AEGC algorithm can divide exposure between shutter and gain. It is configured\n> > > + * with a set of shutter and gain pairs and works by initially fixing gain at\n> > > + * 1.0 and increasing shutter up to the shutter value from the first pair in the\n> > > + * set in an attempt to meet the required exposure value.\n> > > + *\n> > > + * If the required exposure is not achievable by the first shutter value alone\n> > > + * it ramps gain up to the value from the first pair in the set. If the required\n> > > + * exposure is still not met it then allows shutter to ramp up to the shutter\n> > > + * value from the second pair in the set, and continues in this vein until\n> > > + * either the required exposure value is met, or else the hardware's shutter or\n> > > + * gain limits are reached.\n> > > + *\n> > > + * This method allows users to strike a balance between a well-exposed image and\n> > > + * an acceptable frame-rate, as opposed to simply maximising shutter followed by\n> > > + * gain. The same helpers can be used to perform the latter operation if needed\n> > > + * by passing an empty set of pairs to the initialisation function.\n> > > + *\n> > > + * The gain values may exceed a camera sensor's analogue gain limits if either\n> > > + * it or the IPA is also capable of digital gain. The configure() function must\n> > > + * be called with the hardware's limits to inform the helper of those\n> > > + * constraints. Any gain that is needed will be applied as analogue gain first\n> > > + * until the hardware's limit is reached, following which digital gain will be\n> > > + * used.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct an ExposureModeHelper instance\n> > > + */\n> > > +ExposureModeHelper::ExposureModeHelper()\n> > > +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Initialize an ExposureModeHelper instance\n> > > + * \\param[in] stages The vector of paired shutter time and gain limits\n> > > + *\n> > > + * This function is expected to be called a single time once the algorithm that\n> > > + * is using these helpers has built the necessary list of shutter and gain pairs\n> > > + * to use (archetypically by parsing them from a tuning file during the\n> > Is archetypically intentional ? Isn't \"typically\" enough ?\n>\n>\n> It's intentional yes; \"archetypically\" is like \"in the first implementation\"\n> whereas \"typically\" is like \"in most implementations\"...but I can switch to\n> typically so it's less confusing, either works.\n>\n\nas long as it's intentional and makes sense to a native speaker, I'm\nfine!\n\n> >\n> > > + * algorithm's .init() call).\n> > > + *\n> > > + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> > > + * analogue and digital gain.\n> > > + *\n> > Shutter -time- ?\n> >\n> > And yes, now gain is better explained.\n> >\n> > > + * The vector of stages may be empty. In that case, the helper will simply use\n> > > + * the runtime limits set through setShutterGainLimits() instead.\n> > > + */\n> > > +void ExposureModeHelper::init(const std::vector> stages)\n> > > +{\n> > > +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n> > > +\tif (!shutters_.empty()) {\n> > > +\t\tLOG(ExposureModeHelper, Warning)\n> > > +\t\t\t<< \"Initialization attempted multiple times\";\n> > > +\t\treturn;\n> > As we do this in a function instead of a constructor, we can return a\n> > value. Is this useful for users of this class ?\n>\n>\n> I don't think there's any need to return anything in this function if that's what you mean\n>\n\nBut this is an error condition which means the class user is not\nrespecting the intended API usage. Can we make the above Fatal ?\n\n> >\n> > > +\t}\n> > > +\n> > > +\tfor (auto stage : stages) {\n> > auto const &\n> >\n> > > +\t\tshutters_.push_back(stage.first);\n> > > +\t\tgains_.push_back(stage.second);\n> > > +\t}\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Set the shutter and gain limits\n> > > + * \\param[in] minShutter The minimum shutter time supported\n> > > + * \\param[in] maxShutter The maximum shutter time supported\n> > > + * \\param[in] minGain The minimum analogue gain supported\n> > > + * \\param[in] maxGain The maximum analogue gain supported\n> > Ah so this is only analogue ? but the steps are the total gain ?\n>\n>\n> Correct\n>\n> >\n> > > + *\n> > > + * This function configures the shutter and analogue gain limits that need to be\n> > > + * adhered to as the helper divides up exposure. Note that these function *must*\n> > > + * be called whenever those limits change and before splitExposure() is used.\n> > > + *\n> > > + * If the algorithm using the helpers needs to indicate that either shutter,\n> > > + * analogue gain or both should be fixed it can do so by setting both the minima\n> > > + * and maxima to the same value.\n> > Are minima and maxima intentional ? Isn't this minimum and maximum ?\n>\n>\n> Minima/maxima is the plural, meaning to refer to both \"minShutter\" and \"minGain\" for example\n>\n> >\n> > > + */\n> > > +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,\n> > Or just setLimits()\n> >\n> > > +\t\t\t\t\t utils::Duration maxShutter,\n> > > +\t\t\t\t\t double minGain,\n> > > +\t\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) const\n> > > +{\n> > > +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n> > > +}\n> > > +\n> > > +double ExposureModeHelper::clampGain(double gain) const\n> > > +{\n> > > +\treturn std::clamp(gain, minGain_, maxGain_);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Split exposure time into shutter and gain\n> > > + * \\param[in] exposure Exposure time\n> > > + *\n> > > + * This function divides a given exposure value into shutter time, analogue and\n> > exposure 'value' or exposure 'time' as in the function's brief ?\n>\n>\n> It's Exposure time rather than value, if by Exposure value you mean:\n> https://en.wikipedia.org/wiki/Exposure_value\n>\n\nThe 'Exposure' here is a combination of the shutter time multiplied by\na gain and to me it represents an absolute value ? I just find\nconfusing that an \"Exposure time\" is divided in \"shutter time\" and\n\"gain\". But indeed it might be only me, maybe check what others think ?\n\nMy main point however was: in \\brief it is reported as \"exposure time\"\nand in the extended description it's reported as \"exposure value\". I\nwould try to use one of the two consistently throughout the\ndocumentation.\n\n\n>\n>\n> >\n> > > + * digital gain by iterating through sets of shutter and gain limits. At each\n> > does this iterate 'limits' or the stages populated at init() time ?\n>\n>\n> The limits parsed from tuning data\n>\n> >\n> > > + * stage the current stage's shutter limit is multiplied by the previous stage's\n> > > + * gain limit (or 1.0 initially) to see if the combination of the two can meet\n> > > + * the required exposure value. If they cannot then splitExpothe current stage's shutter\n> > Exposure definitely seems to be a value then (and this matches my\n> > understanding)\n>\n>\n> I just mean the value of the exposure parameter; the input is the exposure\n> time needed to attain the target\n>\n> >\n> > 'splitExposthe' ?\n> >\n> >\n> > > + * limit is multiplied by the same stage's gain limit to see if that combination\n> > > + * can meet the required exposure value. If they cannot then the function moves\n> > > + * to consider the next stage.\n> > I think something went wrong with formatting here\n> >\n> > > + *\n> > > + * When a combination of shutter and gain _stage_ limits are found that are\n> > why '_stage_' ? and why limits ?\n> >\n> > Can't it be simply:\n> >\n> > * When a combination of shutter time and gain are found to be sufficient\n>\n>\n> I was trying to make it clear that it's working on the limits from the sets\n> parsed from tuning data rather than the runtime limits set through\n> setShutterGainLimits.\n>\n> >\n> > > + * sufficient to meet the required exposure value, the function attempts to\n> > > + * reduce shutter time as much as possible whilst fixing gain and still meeting\n> > > + * the exposure value. If a _runtime_ limit prevents shutter time from being\n> > Again, why _runtime_ ?\n>\n>\n> And here trying to emphasis that it's the limits from setShutterGainLimits\n> rather than those parsed from Yaml\n>\n\n_this_ gets rendered as italic in Doxygen, so it's fine if you want to\nemphasis it\n\n\n> >\n> > > + * lowered enough to meet the exposure value with gain fixed at the stage limit,\n> > > + * gain is also lowered to compensate.\n> > > + *\n> > > + * Once the shutter time and gain values are ascertained, gain is assigned as\n> > > + * analogue gain as much as possible, with digital gain only in use if the\n> > > + * maximum analogue gain runtime limit is unable to accomodate the exposure\n> > > + * value.\n> > > + *\n> > > + * If no combination of shutter and gain limits is found that meets the required\n> > > + * exposure value, the helper falls-back to simply maximising the shutter time\n> > > + * first, followed by analogue gain, followed by digital gain.\n> > > + *\n> > > + * @return Tuple of shutter time, analogue gain, and digital gain\n> > > + */\n> > > +std::tuple\n> > > +ExposureModeHelper::splitExposure(utils::Duration exposure) const\n> > > +{\n> > > +\tASSERT(maxShutter_);\n> > > +\tASSERT(maxGain_);\n> > > +\n> > > +\tbool gainFixed = minGain_ == maxGain_;\n> > > +\tbool shutterFixed = minShutter_ == maxShutter_;\n> > > +\n> > > +\t/*\n> > > +\t * There's no point entering the loop if we cannot change either gain\n> > > +\t * nor shutter anyway.\n> > > +\t */\n> > > +\tif (shutterFixed && gainFixed)\n> > > +\t\treturn { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };\n> > > +\n> > > +\tutils::Duration shutter;\n> > > +\tdouble stageGain;\n> > > +\tdouble gain;\n> > > +\n> > > +\tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n> > > +\t\tdouble lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);\n> > > +\t\tutils::Duration stageShutter = clampShutter(shutters_[stage]);\n> > > +\t\tstageGain = clampGain(gains_[stage]);\n> > > +\n> > > +\t\t/*\n> > > +\t\t * We perform the clamping on both shutter and gain in case the\n> > > +\t\t * helper has had limits set that prevent those values being\n> > > +\t\t * lowered beyond a certain minimum...this can happen at runtime\n> > > +\t\t * for various reasons and so would not be known when the stage\n> > > +\t\t * limits are initialised.\n> > > +\t\t */\n> > > +\n> > > +\t\tif (stageShutter * lastStageGain >= exposure) {\n> > > +\t\t\tshutter = clampShutter(exposure / clampGain(lastStageGain));\n> > > +\t\t\tgain = clampGain(exposure / shutter);\n> > > +\n> > > +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> > > +\t\t}\n> > > +\n> > > +\t\tif (stageShutter * stageGain >= exposure) {\n> > > +\t\t\tshutter = clampShutter(exposure / clampGain(stageGain));\n> > > +\t\t\tgain = clampGain(exposure / shutter);\n> > > +\n> > > +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * From here on all we can do is max out the shutter, followed by the\n> > > +\t * analogue gain. If we still haven't achieved the target we send the\n> > > +\t * rest of the exposure time to digital gain. If we were given no stages\n> > > +\t * to use then set stageGain to 1.0 so that shutter is maxed before gain\n> > > +\t * touched at all.\n> > > +\t */\n> > > +\tif (gains_.empty())\n> > > +\t\tstageGain = 1.0;\n> > > +\n> > > +\tshutter = clampShutter(exposure / clampGain(stageGain));\n> > > +\tgain = clampGain(exposure / shutter);\n> > > +\n> > > +\treturn { shutter, gain, exposure / (shutter * gain) };\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::minShutter()\n> > > + * \\brief Retrieve the configured minimum shutter time limit set through\n> > > + * setShutterGainLimits()\n> > No alignement in briefs\n> >\n> > > + * \\return The minShutter_ value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::maxShutter()\n> > > + * \\brief Retrieve the configured maximum shutter time set through\n> > > + * setShutterGainLimits()\n> > ditto\n> >\n> > > + * \\return The maxShutter_ value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::minGain()\n> > > + * \\brief Retrieve the configured minimum gain set through\n> > > + * setShutterGainLimits()\n> > > + * \\return The minGain_ value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn ExposureModeHelper::maxGain()\n> > > + * \\brief Retrieve the configured maximum gain set through\n> > > + * setShutterGainLimits()\n> > > + * \\return The maxGain_ value\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..1f672135\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > > @@ -0,0 +1,53 @@\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> > Missing \n> >\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() = default;\n> > > +\n> > > +\tvoid init(const std::vector> stages);\n> > > +\tvoid setShutterGainLimits(utils::Duration minShutter,\n> > > +\t\t\t\t utils::Duration maxShutter,\n> > > +\t\t\t\t double minGain, double maxGain);\n> > > +\n> > > +\tstd::tuple\n> > > +\tsplitExposure(utils::Duration exposure) const;\n> > > +\n> > > +\tutils::Duration minShutter() const { return minShutter_; }\n> > > +\tutils::Duration maxShutter() const { return maxShutter_; }\n> > > +\tdouble minGain() const { return minGain_; }\n> > > +\tdouble maxGain() const { return maxGain_; }\n> > > +\n> > > +private:\n> > > +\tutils::Duration clampShutter(utils::Duration shutter) const;\n> > > +\tdouble clampGain(double gain) const;\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 E311AC3200\n\tfor ;\n\tTue, 23 Apr 2024 10:20:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 707A861B3D;\n\tTue, 23 Apr 2024 12:19:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 36AF961AC9\n\tfor ;\n\tTue, 23 Apr 2024 12:19:58 +0200 (CEST)","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 173CC65D;\n\tTue, 23 Apr 2024 12:19:07 +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=\"AbwrCEPN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713867547;\n\tbh=qSHVgmL0bNRR9ESNy+I6JOW9KAJKPkLxID/1kH7FCTo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AbwrCEPNsNKDKvVLIy6iSJiOXw4FUJxcMdQi+G37QErmJbWfcyTdpyY8rcCSRXHnq\n\tG5xKAHvxtmsgxiX0PCjJb9bojBQbf6NXH+26nhegrfLIz9yhm6k5ACb62Y/Ci9FWt1\n\t8xIqVoOpSwhFEFtKakThasOGCMLGl6extYHpshY4=","Date":"Tue, 23 Apr 2024 12:19:55 +0200","From":"Jacopo Mondi ","To":"Dan Scally ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29319,"web_url":"https://patchwork.libcamera.org/comment/29319/","msgid":"<4e675606-24b2-48e6-ba52-5ce3be7f34a3@ideasonboard.com>","date":"2024-04-23T14:09:33","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 23/04/2024 11:19, Jacopo Mondi wrote:\n> Hi Dan\n>\n> On Tue, Apr 23, 2024 at 10:16:31AM +0100, Dan Scally wrote:\n>> Hi Jacopo\n>>\n>> On 19/04/2024 15:10, Jacopo Mondi wrote:\n>>> Hi Dan\n>>>\n>>> On Wed, Apr 17, 2024 at 02:15:31PM +0100, 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>>>> Changes in v2:\n>>>>\n>>>> \t- Expanded the documentation\n>>>> \t- Dropped the overloads for fixed shutter / gain - the same\n>>>> \t functionality is instead done by setting min and max shutter and gain\n>>>> \t to the same value\n>>>> \t- Changed ::init() to consume a vector of pairs instead of two separate\n>>>> \t vectors\n>>>> \t- Reworked splitExposure()\n>>>>\n>>>> src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n>>>> src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n>>>> src/ipa/libipa/meson.build | 2 +\n>>>> 3 files changed, 312 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..6463de18\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n>>>> @@ -0,0 +1,257 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2024, Paul Elder \n>>>> + *\n>>>> + * exposure_mode_helper.cpp - 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>>>> + * AEGC algorithms have a need to split exposure between shutter and gain.\n>>> s/shutter/shutter time ?\n>>>\n>>> This and in the other occurences\n>>>\n>>> also, \"gain\" which gain ? Analogue, digital or a combination of the\n>>> two ?\n>>\n>> Both\n>>\n> Maybe specify it then ? :)\n\n\nWill do!\n\n>\n>>>> + * Multiple implementations do so based on paired sets of shutter and gain\n>>>> + * limits; provide a helper to avoid duplicating the code.\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 into shutter and gain\n>>>> + *\n>>>> + * The ExposureModeHelper class provides a standard interface through which an\n>>>> + * AEGC algorithm can divide exposure between shutter and gain. It is configured\n>>>> + * with a set of shutter and gain pairs and works by initially fixing gain at\n>>>> + * 1.0 and increasing shutter up to the shutter value from the first pair in the\n>>>> + * set in an attempt to meet the required exposure value.\n>>>> + *\n>>>> + * If the required exposure is not achievable by the first shutter value alone\n>>>> + * it ramps gain up to the value from the first pair in the set. If the required\n>>>> + * exposure is still not met it then allows shutter to ramp up to the shutter\n>>>> + * value from the second pair in the set, and continues in this vein until\n>>>> + * either the required exposure value is met, or else the hardware's shutter or\n>>>> + * gain limits are reached.\n>>>> + *\n>>>> + * This method allows users to strike a balance between a well-exposed image and\n>>>> + * an acceptable frame-rate, as opposed to simply maximising shutter followed by\n>>>> + * gain. The same helpers can be used to perform the latter operation if needed\n>>>> + * by passing an empty set of pairs to the initialisation function.\n>>>> + *\n>>>> + * The gain values may exceed a camera sensor's analogue gain limits if either\n>>>> + * it or the IPA is also capable of digital gain. The configure() function must\n>>>> + * be called with the hardware's limits to inform the helper of those\n>>>> + * constraints. Any gain that is needed will be applied as analogue gain first\n>>>> + * until the hardware's limit is reached, following which digital gain will be\n>>>> + * used.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Construct an ExposureModeHelper instance\n>>>> + */\n>>>> +ExposureModeHelper::ExposureModeHelper()\n>>>> +\t: minShutter_(0), maxShutter_(0), minGain_(0), maxGain_(0)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Initialize an ExposureModeHelper instance\n>>>> + * \\param[in] stages The vector of paired shutter time and gain limits\n>>>> + *\n>>>> + * This function is expected to be called a single time once the algorithm that\n>>>> + * is using these helpers has built the necessary list of shutter and gain pairs\n>>>> + * to use (archetypically by parsing them from a tuning file during the\n>>> Is archetypically intentional ? Isn't \"typically\" enough ?\n>>\n>> It's intentional yes; \"archetypically\" is like \"in the first implementation\"\n>> whereas \"typically\" is like \"in most implementations\"...but I can switch to\n>> typically so it's less confusing, either works.\n>>\n> as long as it's intentional and makes sense to a native speaker, I'm\n> fine!\n>\n>>>> + * algorithm's .init() call).\n>>>> + *\n>>>> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n>>>> + * analogue and digital gain.\n>>>> + *\n>>> Shutter -time- ?\n>>>\n>>> And yes, now gain is better explained.\n>>>\n>>>> + * The vector of stages may be empty. In that case, the helper will simply use\n>>>> + * the runtime limits set through setShutterGainLimits() instead.\n>>>> + */\n>>>> +void ExposureModeHelper::init(const std::vector> stages)\n>>>> +{\n>>>> +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n>>>> +\tif (!shutters_.empty()) {\n>>>> +\t\tLOG(ExposureModeHelper, Warning)\n>>>> +\t\t\t<< \"Initialization attempted multiple times\";\n>>>> +\t\treturn;\n>>> As we do this in a function instead of a constructor, we can return a\n>>> value. Is this useful for users of this class ?\n>>\n>> I don't think there's any need to return anything in this function if that's what you mean\n>>\n> But this is an error condition which means the class user is not\n> respecting the intended API usage. Can we make the above Fatal ?\n\n\nYes if that makes sense to do; I'm never sure when Fatal is the right choice.\n\n>\n>>>> +\t}\n>>>> +\n>>>> +\tfor (auto stage : stages) {\n>>> auto const &\n>>>\n>>>> +\t\tshutters_.push_back(stage.first);\n>>>> +\t\tgains_.push_back(stage.second);\n>>>> +\t}\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Set the shutter and gain limits\n>>>> + * \\param[in] minShutter The minimum shutter time supported\n>>>> + * \\param[in] maxShutter The maximum shutter time supported\n>>>> + * \\param[in] minGain The minimum analogue gain supported\n>>>> + * \\param[in] maxGain The maximum analogue gain supported\n>>> Ah so this is only analogue ? but the steps are the total gain ?\n>>\n>> Correct\n>>\n>>>> + *\n>>>> + * This function configures the shutter and analogue gain limits that need to be\n>>>> + * adhered to as the helper divides up exposure. Note that these function *must*\n>>>> + * be called whenever those limits change and before splitExposure() is used.\n>>>> + *\n>>>> + * If the algorithm using the helpers needs to indicate that either shutter,\n>>>> + * analogue gain or both should be fixed it can do so by setting both the minima\n>>>> + * and maxima to the same value.\n>>> Are minima and maxima intentional ? Isn't this minimum and maximum ?\n>>\n>> Minima/maxima is the plural, meaning to refer to both \"minShutter\" and \"minGain\" for example\n>>\n>>>> + */\n>>>> +void ExposureModeHelper::setShutterGainLimits(utils::Duration minShutter,\n>>> Or just setLimits()\n>>>\n>>>> +\t\t\t\t\t utils::Duration maxShutter,\n>>>> +\t\t\t\t\t double minGain,\n>>>> +\t\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) const\n>>>> +{\n>>>> +\treturn std::clamp(shutter, minShutter_, maxShutter_);\n>>>> +}\n>>>> +\n>>>> +double ExposureModeHelper::clampGain(double gain) const\n>>>> +{\n>>>> +\treturn std::clamp(gain, minGain_, maxGain_);\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Split exposure time into shutter and gain\n>>>> + * \\param[in] exposure Exposure time\n>>>> + *\n>>>> + * This function divides a given exposure value into shutter time, analogue and\n>>> exposure 'value' or exposure 'time' as in the function's brief ?\n>>\n>> It's Exposure time rather than value, if by Exposure value you mean:\n>> https://en.wikipedia.org/wiki/Exposure_value\n>>\n> The 'Exposure' here is a combination of the shutter time multiplied by\n> a gain and to me it represents an absolute value ? I just find\n> confusing that an \"Exposure time\" is divided in \"shutter time\" and\n> \"gain\". But indeed it might be only me, maybe check what others think ?\n\n\nPerhaps the terminology is wrong...in the derived classes I think the combination of shutter time \nand gain that was applied for a specific frame is called the \"effectiveExposureValue\" [1] - perhaps \nwe should use that phrase to refer to the shutter time modified by gain? Or \"effective exposure time\"?\n\n\n[1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/algorithms/agc.cpp#n267\n\n>\n> My main point however was: in \\brief it is reported as \"exposure time\"\n> and in the extended description it's reported as \"exposure value\". I\n> would try to use one of the two consistently throughout the\n> documentation.\n>\n>\nFair enough; thanks.\n>>\n>>>> + * digital gain by iterating through sets of shutter and gain limits. At each\n>>> does this iterate 'limits' or the stages populated at init() time ?\n>>\n>> The limits parsed from tuning data\n>>\n>>>> + * stage the current stage's shutter limit is multiplied by the previous stage's\n>>>> + * gain limit (or 1.0 initially) to see if the combination of the two can meet\n>>>> + * the required exposure value. If they cannot then splitExpothe current stage's shutter\n>>> Exposure definitely seems to be a value then (and this matches my\n>>> understanding)\n>>\n>> I just mean the value of the exposure parameter; the input is the exposure\n>> time needed to attain the target\n>>\n>>> 'splitExposthe' ?\n>>>\n>>>\n>>>> + * limit is multiplied by the same stage's gain limit to see if that combination\n>>>> + * can meet the required exposure value. If they cannot then the function moves\n>>>> + * to consider the next stage.\n>>> I think something went wrong with formatting here\n>>>\n>>>> + *\n>>>> + * When a combination of shutter and gain _stage_ limits are found that are\n>>> why '_stage_' ? and why limits ?\n>>>\n>>> Can't it be simply:\n>>>\n>>> * When a combination of shutter time and gain are found to be sufficient\n>>\n>> I was trying to make it clear that it's working on the limits from the sets\n>> parsed from tuning data rather than the runtime limits set through\n>> setShutterGainLimits.\n>>\n>>>> + * sufficient to meet the required exposure value, the function attempts to\n>>>> + * reduce shutter time as much as possible whilst fixing gain and still meeting\n>>>> + * the exposure value. If a _runtime_ limit prevents shutter time from being\n>>> Again, why _runtime_ ?\n>>\n>> And here trying to emphasis that it's the limits from setShutterGainLimits\n>> rather than those parsed from Yaml\n>>\n> _this_ gets rendered as italic in Doxygen, so it's fine if you want to\n> emphasis it\n\n\nYep that was the aim\n\n>\n>\n>>>> + * lowered enough to meet the exposure value with gain fixed at the stage limit,\n>>>> + * gain is also lowered to compensate.\n>>>> + *\n>>>> + * Once the shutter time and gain values are ascertained, gain is assigned as\n>>>> + * analogue gain as much as possible, with digital gain only in use if the\n>>>> + * maximum analogue gain runtime limit is unable to accomodate the exposure\n>>>> + * value.\n>>>> + *\n>>>> + * If no combination of shutter and gain limits is found that meets the required\n>>>> + * exposure value, the helper falls-back to simply maximising the shutter time\n>>>> + * first, followed by analogue gain, followed by digital gain.\n>>>> + *\n>>>> + * @return Tuple of shutter time, analogue gain, and digital gain\n>>>> + */\n>>>> +std::tuple\n>>>> +ExposureModeHelper::splitExposure(utils::Duration exposure) const\n>>>> +{\n>>>> +\tASSERT(maxShutter_);\n>>>> +\tASSERT(maxGain_);\n>>>> +\n>>>> +\tbool gainFixed = minGain_ == maxGain_;\n>>>> +\tbool shutterFixed = minShutter_ == maxShutter_;\n>>>> +\n>>>> +\t/*\n>>>> +\t * There's no point entering the loop if we cannot change either gain\n>>>> +\t * nor shutter anyway.\n>>>> +\t */\n>>>> +\tif (shutterFixed && gainFixed)\n>>>> +\t\treturn { minShutter_, minGain_, exposure / (minShutter_ * minGain_) };\n>>>> +\n>>>> +\tutils::Duration shutter;\n>>>> +\tdouble stageGain;\n>>>> +\tdouble gain;\n>>>> +\n>>>> +\tfor (unsigned int stage = 0; stage < gains_.size(); stage++) {\n>>>> +\t\tdouble lastStageGain = stage == 0 ? 1.0 : clampGain(gains_[stage - 1]);\n>>>> +\t\tutils::Duration stageShutter = clampShutter(shutters_[stage]);\n>>>> +\t\tstageGain = clampGain(gains_[stage]);\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t * We perform the clamping on both shutter and gain in case the\n>>>> +\t\t * helper has had limits set that prevent those values being\n>>>> +\t\t * lowered beyond a certain minimum...this can happen at runtime\n>>>> +\t\t * for various reasons and so would not be known when the stage\n>>>> +\t\t * limits are initialised.\n>>>> +\t\t */\n>>>> +\n>>>> +\t\tif (stageShutter * lastStageGain >= exposure) {\n>>>> +\t\t\tshutter = clampShutter(exposure / clampGain(lastStageGain));\n>>>> +\t\t\tgain = clampGain(exposure / shutter);\n>>>> +\n>>>> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tif (stageShutter * stageGain >= exposure) {\n>>>> +\t\t\tshutter = clampShutter(exposure / clampGain(stageGain));\n>>>> +\t\t\tgain = clampGain(exposure / shutter);\n>>>> +\n>>>> +\t\t\treturn { shutter, gain, exposure / (shutter * gain) };\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * From here on all we can do is max out the shutter, followed by the\n>>>> +\t * analogue gain. If we still haven't achieved the target we send the\n>>>> +\t * rest of the exposure time to digital gain. If we were given no stages\n>>>> +\t * to use then set stageGain to 1.0 so that shutter is maxed before gain\n>>>> +\t * touched at all.\n>>>> +\t */\n>>>> +\tif (gains_.empty())\n>>>> +\t\tstageGain = 1.0;\n>>>> +\n>>>> +\tshutter = clampShutter(exposure / clampGain(stageGain));\n>>>> +\tgain = clampGain(exposure / shutter);\n>>>> +\n>>>> +\treturn { shutter, gain, exposure / (shutter * gain) };\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\fn ExposureModeHelper::minShutter()\n>>>> + * \\brief Retrieve the configured minimum shutter time limit set through\n>>>> + * setShutterGainLimits()\n>>> No alignement in briefs\n>>>\n>>>> + * \\return The minShutter_ value\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ExposureModeHelper::maxShutter()\n>>>> + * \\brief Retrieve the configured maximum shutter time set through\n>>>> + * setShutterGainLimits()\n>>> ditto\n>>>\n>>>> + * \\return The maxShutter_ value\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ExposureModeHelper::minGain()\n>>>> + * \\brief Retrieve the configured minimum gain set through\n>>>> + * setShutterGainLimits()\n>>>> + * \\return The minGain_ value\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn ExposureModeHelper::maxGain()\n>>>> + * \\brief Retrieve the configured maximum gain set through\n>>>> + * setShutterGainLimits()\n>>>> + * \\return The maxGain_ value\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..1f672135\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/exposure_mode_helper.h\n>>>> @@ -0,0 +1,53 @@\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>>> Missing \n>>>\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() = default;\n>>>> +\n>>>> +\tvoid init(const std::vector> stages);\n>>>> +\tvoid setShutterGainLimits(utils::Duration minShutter,\n>>>> +\t\t\t\t utils::Duration maxShutter,\n>>>> +\t\t\t\t double minGain, double maxGain);\n>>>> +\n>>>> +\tstd::tuple\n>>>> +\tsplitExposure(utils::Duration exposure) const;\n>>>> +\n>>>> +\tutils::Duration minShutter() const { return minShutter_; }\n>>>> +\tutils::Duration maxShutter() const { return maxShutter_; }\n>>>> +\tdouble minGain() const { return minGain_; }\n>>>> +\tdouble maxGain() const { return maxGain_; }\n>>>> +\n>>>> +private:\n>>>> +\tutils::Duration clampShutter(utils::Duration shutter) const;\n>>>> +\tdouble clampGain(double gain) const;\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 9C6A4BE08B\n\tfor ;\n\tTue, 23 Apr 2024 14:09:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7907A61B24;\n\tTue, 23 Apr 2024 16:09:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3535061AC1\n\tfor ;\n\tTue, 23 Apr 2024 16:09:37 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A56A71A2;\n\tTue, 23 Apr 2024 16:08:45 +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=\"lWOdysom\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713881325;\n\tbh=bOSMj3CUxmybBqASrxJOJ2giAAWPfTiS25LlIQdz+V0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=lWOdysomoZHa7RfrVhN7kCTHkzW2l6chjIaqwZaiU+1g7kmOw/7jC3c7vH31LZPma\n\tsAN4nk+i88KE1OI3GTtqqJrfnDSKnU4f+yIZhv/tRsN78PCb1G6otNdVs2ktnRb7qF\n\t6PKjFvoab9V9d9PZO0rmN0MIrk3QaaUpThAxU6qg=","Message-ID":"<4e675606-24b2-48e6-ba52-5ce3be7f34a3@ideasonboard.com>","Date":"Tue, 23 Apr 2024 15:09:33 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t\n\t\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":29322,"web_url":"https://patchwork.libcamera.org/comment/29322/","msgid":"","date":"2024-04-23T14:29:32","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. április 17., szerda 15:15 keltezéssel, Daniel Scally írta:\n\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> Changes in v2:\n> \n> \t- Expanded the documentation\n> \t- Dropped the overloads for fixed shutter / gain - the same\n> \t functionality is instead done by setting min and max shutter and gain\n> \t to the same value\n> \t- Changed ::init() to consume a vector of pairs instead of two separate\n> \t vectors\n> \t- Reworked splitExposure()\n> \n> src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n> src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 312 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> [...]\n> +/**\n> + * \\brief Initialize an ExposureModeHelper instance\n> + * \\param[in] stages The vector of paired shutter time and gain limits\n> + *\n> + * This function is expected to be called a single time once the algorithm that\n> + * is using these helpers has built the necessary list of shutter and gain pairs\n> + * to use (archetypically by parsing them from a tuning file during the\n> + * algorithm's .init() call).\n> + *\n> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> + * analogue and digital gain.\n> + *\n> + * The vector of stages may be empty. In that case, the helper will simply use\n> + * the runtime limits set through setShutterGainLimits() instead.\n> + */\n> +void ExposureModeHelper::init(const std::vector> stages)\n\nAny reason for not using span here?\n\n\n> +{\n> +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n> +\tif (!shutters_.empty()) {\n\nAny reason for not doing what init() does in the constructor?\n\n\n> +\t\tLOG(ExposureModeHelper, Warning)\n> +\t\t\t<< \"Initialization attempted multiple times\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tfor (auto stage : stages) {\n\nYou can use something like `const auto &[s, g] : stages`.\n\n\n> +\t\tshutters_.push_back(stage.first);\n> +\t\tgains_.push_back(stage.second);\n> +\t}\n> +}\n> [...]\n\n\nRegards,\nBarnabás Pőcze","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 ADEADC3200\n\tfor ;\n\tTue, 23 Apr 2024 14:29:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A56C161B28;\n\tTue, 23 Apr 2024 16:29:39 +0200 (CEST)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1964361AC1\n\tfor ;\n\tTue, 23 Apr 2024 16:29:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"OmgTkonh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1713882577; x=1714141777;\n\tbh=5jK2FMwYgZOkEt3vMeu9Yu5n3LOQE0iV5tBv+7HVz2k=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=OmgTkonhx+iGPBAokAz5UQeS1yq0IVbfVVfXOVE2wIjEnJfMnQODZAHYZ5Deme1U7\n\tkxYg/ZPckJHMEnUkFpcvb16WhPvwHfgsplO5qCBGhCG/nBBpvnNOHIJVZzB/tLP1U5\n\tv1OHIwEPVnI9zxj5rJrB2+ron/RNICcVGLg9PxBsJqTXvvUqvhEJWzXJC/rF3K7WU8\n\tzESTlw+XPpc9N9yUUJ1V4im3+YgDfvjTp4hWrCHW3i0yTC+I4CM1wws8WdOMC36cmd\n\tpDrg4LKSPY096ixco+aRmpO7VD4+5esoh2KDn1UqglwvSTuLlCV8o3vx/kqypcc5Bp\n\tr5WQYchYzj5wA==","Date":"Tue, 23 Apr 2024 14:29:32 +0000","To":"Daniel Scally ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"","In-Reply-To":"<20240417131536.484129-4-dan.scally@ideasonboard.com>","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"6133d6b0c48bcf51d32e0e7b2f9b7c5ca828195e","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":29323,"web_url":"https://patchwork.libcamera.org/comment/29323/","msgid":"<68f927ca-2beb-4029-8a0c-d36513a198ae@ideasonboard.com>","date":"2024-04-23T14:41:44","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Barnabas\n\nOn 23/04/2024 15:29, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2024. április 17., szerda 15:15 keltezéssel, Daniel Scally írta:\n>\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>> Changes in v2:\n>>\n>> \t- Expanded the documentation\n>> \t- Dropped the overloads for fixed shutter / gain - the same\n>> \t functionality is instead done by setting min and max shutter and gain\n>> \t to the same value\n>> \t- Changed ::init() to consume a vector of pairs instead of two separate\n>> \t vectors\n>> \t- Reworked splitExposure()\n>>\n>> src/ipa/libipa/exposure_mode_helper.cpp | 257 ++++++++++++++++++++++++\n>> src/ipa/libipa/exposure_mode_helper.h | 53 +++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 312 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>> [...]\n>> +/**\n>> + * \\brief Initialize an ExposureModeHelper instance\n>> + * \\param[in] stages The vector of paired shutter time and gain limits\n>> + *\n>> + * This function is expected to be called a single time once the algorithm that\n>> + * is using these helpers has built the necessary list of shutter and gain pairs\n>> + * to use (archetypically by parsing them from a tuning file during the\n>> + * algorithm's .init() call).\n>> + *\n>> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n>> + * analogue and digital gain.\n>> + *\n>> + * The vector of stages may be empty. In that case, the helper will simply use\n>> + * the runtime limits set through setShutterGainLimits() instead.\n>> + */\n>> +void ExposureModeHelper::init(const std::vector> stages)\n> Any reason for not using span here?\n\n\nThe shutter time and gain vectors are initially separate, and need combining like so:\n\n\n+ std::vector> stages; + for (unsigned int i = 0; i < \nshutters.size(); i++) { + stages.push_back({ + std::chrono::microseconds(shutters[i]), + gains[i] + \n}); + } + + std::shared_ptr helper = + std::make_shared(); + \nhelper->init(stages); So given I have to build a new vector with the paired stages anyway I couldn't \nsee value in then using a Span for the init() instead of just passing the vector...but quite \npossibly I am just not understanding why it's the better choice here.\n\n>\n>\n>> +{\n>> +\t/* We only need to check shutters_, as gains_ is filled alongside it */\n>> +\tif (!shutters_.empty()) {\n> Any reason for not doing what init() does in the constructor?\n\n\nNo; v3 does that.\n\n>\n>\n>> +\t\tLOG(ExposureModeHelper, Warning)\n>> +\t\t\t<< \"Initialization attempted multiple times\";\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tfor (auto stage : stages) {\n> You can use something like `const auto &[s, g] : stages`.\n\n\nthanks\n\n>\n>\n>> +\t\tshutters_.push_back(stage.first);\n>> +\t\tgains_.push_back(stage.second);\n>> +\t}\n>> +}\n>> [...]\n>\n> Regards,\n> Barnabás Pőcze","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 D4265BE08B\n\tfor ;\n\tTue, 23 Apr 2024 14:41:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED64961B24;\n\tTue, 23 Apr 2024 16:41:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1DAB961AC1\n\tfor ;\n\tTue, 23 Apr 2024 16:41:48 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9AF561A2;\n\tTue, 23 Apr 2024 16:40:56 +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=\"Q8DokQqb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713883256;\n\tbh=btmhXoGS6r16tNBegO761HXcnDvU8dPzQj46nqzpFA0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Q8DokQqbPgX6LrkHkQZyfq2kt2V+JtzszDrrZHcm9ixcf80EfPm9viVXLDRxmVUDW\n\tfLstBzpgyBjz7Wyw5qhykzS/0EEy4P2b5Ua7oiM59IEZFlrsBnUSx/ZPu0V6NrN5h+\n\trzyw9EbOrtfKEd2y14jdYbe5JcNdjZDB1TcF+RfE=","Message-ID":"<68f927ca-2beb-4029-8a0c-d36513a198ae@ideasonboard.com>","Date":"Tue, 23 Apr 2024 15:41:44 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":29324,"web_url":"https://patchwork.libcamera.org/comment/29324/","msgid":"","date":"2024-04-23T15:01:34","subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. április 23., kedd 16:41 keltezéssel, Dan Scally írta:\n\n> [...]\n> >> [...]\n> >> +/**\n> >> + * \\brief Initialize an ExposureModeHelper instance\n> >> + * \\param[in] stages The vector of paired shutter time and gain limits\n> >> + *\n> >> + * This function is expected to be called a single time once the algorithm that\n> >> + * is using these helpers has built the necessary list of shutter and gain pairs\n> >> + * to use (archetypically by parsing them from a tuning file during the\n> >> + * algorithm's .init() call).\n> >> + *\n> >> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> >> + * analogue and digital gain.\n> >> + *\n> >> + * The vector of stages may be empty. In that case, the helper will simply use\n> >> + * the runtime limits set through setShutterGainLimits() instead.\n> >> + */\n> >> +void ExposureModeHelper::init(const std::vector> stages)\n> > Any reason for not using span here?\n> \n> \n> The shutter time and gain vectors are initially separate, and need combining like so:\n> \n> \n> std::vector> stages;\n> for (unsigned int i = 0; i < shutters.size(); i++) {\n> stages.push_back({\n> std::chrono::microseconds(shutters[i]),\n> gains[i]\n> });\n> }\n> \n> std::shared_ptr helper =\n> std::make_shared();\n> helper->init(stages);\n> \n> So given I have to build a new vector with the paired stages anyway I couldn't\n> see value in then using a Span for the init() instead of just passing the vector...but quite\n> possibly I am just not understanding why it's the better choice here.\n\nNote that in the code above, there is a copy when calling init(), using a span avoids that.\n\n\n> [...]\n\n\nRegards,\nBarnabás Pőcze","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 6FAEEBE08B\n\tfor ;\n\tTue, 23 Apr 2024 15:01:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76D2A61AC9;\n\tTue, 23 Apr 2024 17:01:45 +0200 (CEST)","from mail-40133.protonmail.ch (mail-40133.protonmail.ch\n\t[185.70.40.133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C18F061AC1\n\tfor ;\n\tTue, 23 Apr 2024 17:01:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"fZ8e+Zpn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1713884502; x=1714143702;\n\tbh=1Ar5D6W66CdZUtSWWUo4vT2MuVfJrj51UIO0t5xZ9h8=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=fZ8e+ZpnJa/30F2U56wr1/UYJOZA0t6XfUksh3gPsPmmqvYh4wvJfZqUPmmT2YLtQ\n\tM4pI6UEyIKFnn+Izg28zWPu3ZPAGg+d8msFCwS+lWK+q3BU8wWsY0JL1YCRJD6hhXl\n\tJZLnnkHH4emJ4jywGY1zbF3utsQanDdPkNkTQCWk1TiLs7pdCEg8CmIXkgdufgotGX\n\te5Uis2xq6rD5jvxdmOshZv1BmndvanwqSvtj38cMPteje5SGJNNo0GprJAVv0qEuQ+\n\tfIxHSZdgmrg81ruzIeAHXD42Rep9KNIpQV/eORYd7yuK9RWgNmkAt/JOvqG1syrT8T\n\tDtQ9ylSFVFUQg==","Date":"Tue, 23 Apr 2024 15:01:34 +0000","To":"Dan Scally ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"","In-Reply-To":"<68f927ca-2beb-4029-8a0c-d36513a198ae@ideasonboard.com>","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t\n\t<68f927ca-2beb-4029-8a0c-d36513a198ae@ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"6acd6b211853fd1072c78e3ea8fad627d17fd486","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":29335,"web_url":"https://patchwork.libcamera.org/comment/29335/","msgid":"","date":"2024-04-24T16:59:24","subject":"Re: [PATCH v2 3/8] 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\n\nOn Tue, Apr 23, 2024 at 03:01:34PM +0000, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2024. április 23., kedd 16:41 keltezéssel, Dan Scally írta:\n>\n> > [...]\n> > >> [...]\n> > >> +/**\n> > >> + * \\brief Initialize an ExposureModeHelper instance\n> > >> + * \\param[in] stages The vector of paired shutter time and gain limits\n> > >> + *\n> > >> + * This function is expected to be called a single time once the algorithm that\n> > >> + * is using these helpers has built the necessary list of shutter and gain pairs\n> > >> + * to use (archetypically by parsing them from a tuning file during the\n> > >> + * algorithm's .init() call).\n> > >> + *\n> > >> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> > >> + * analogue and digital gain.\n> > >> + *\n> > >> + * The vector of stages may be empty. In that case, the helper will simply use\n> > >> + * the runtime limits set through setShutterGainLimits() instead.\n> > >> + */\n> > >> +void ExposureModeHelper::init(const std::vector> stages)\n> > > Any reason for not using span here?\n> >\n> >\n> > The shutter time and gain vectors are initially separate, and need combining like so:\n> >\n> >\n> > std::vector> stages;\n> > for (unsigned int i = 0; i < shutters.size(); i++) {\n> > stages.push_back({\n> > std::chrono::microseconds(shutters[i]),\n> > gains[i]\n> > });\n> > }\n> >\n> > std::shared_ptr helper =\n> > std::make_shared();\n> > helper->init(stages);\n> >\n> > So given I have to build a new vector with the paired stages anyway I couldn't\n> > see value in then using a Span for the init() instead of just passing the vector...but quite\n> > possibly I am just not understanding why it's the better choice here.\n>\n> Note that in the code above, there is a copy when calling init(), using a span avoids that.\n>\n\nOr just by passing the vector by reference and not value.\nThe pairs' items will be copied inside the helper anyway.\n\n>\n> > [...]\n>\n>\n> Regards,\n> Barnabás Pőcze","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 A8E9EBE08B\n\tfor ;\n\tWed, 24 Apr 2024 16:59:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A315A633F9;\n\tWed, 24 Apr 2024 18:59:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1DD67633EB\n\tfor ;\n\tWed, 24 Apr 2024 18:59:28 +0200 (CEST)","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 E1757674;\n\tWed, 24 Apr 2024 18:58:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vqH/SV60\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713977916;\n\tbh=DyBn9YgMvc+WfCsbfa24vmGqmn3bZCnGJYFB3ISNzBo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vqH/SV608YkSOP6JroLBLGrsNRPNjZ/HIT9oNGGqOyhnHJrUorDn23gEy/7DmvrmQ\n\tYJnk/yRSAIQDUjP32WboWl6SReAHz5MuE4lI5IkzxeJl9ZaAFzmSMF4+mN3dNuebju\n\t4kgIgeS8g6l0dpcPDXesTrK7xMjLh9gUigZvC2eY=","Date":"Wed, 24 Apr 2024 18:59:24 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Dan Scally , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t\n\t<68f927ca-2beb-4029-8a0c-d36513a198ae@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29339,"web_url":"https://patchwork.libcamera.org/comment/29339/","msgid":"<20240424185759.GB6282@pendragon.ideasonboard.com>","date":"2024-04-24T18:57:59","subject":"Re: [PATCH v2 3/8] 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 Wed, Apr 24, 2024 at 06:59:24PM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 23, 2024 at 03:01:34PM +0000, Barnabás Pőcze wrote:\n> > 2024. április 23., kedd 16:41 keltezéssel, Dan Scally írta:\n> >\n> > > [...]\n> > > >> [...]\n> > > >> +/**\n> > > >> + * \\brief Initialize an ExposureModeHelper instance\n> > > >> + * \\param[in] stages The vector of paired shutter time and gain limits\n> > > >> + *\n> > > >> + * This function is expected to be called a single time once the algorithm that\n> > > >> + * is using these helpers has built the necessary list of shutter and gain pairs\n> > > >> + * to use (archetypically by parsing them from a tuning file during the\n> > > >> + * algorithm's .init() call).\n> > > >> + *\n> > > >> + * The input steps are shutter and _total_ gain pairs; the gain encompasses both\n> > > >> + * analogue and digital gain.\n> > > >> + *\n> > > >> + * The vector of stages may be empty. In that case, the helper will simply use\n> > > >> + * the runtime limits set through setShutterGainLimits() instead.\n> > > >> + */\n> > > >> +void ExposureModeHelper::init(const std::vector> stages)\n> > > > Any reason for not using span here?\n> > >\n> > >\n> > > The shutter time and gain vectors are initially separate, and need combining like so:\n> > >\n> > >\n> > > std::vector> stages;\n> > > for (unsigned int i = 0; i < shutters.size(); i++) {\n> > > stages.push_back({\n> > > std::chrono::microseconds(shutters[i]),\n> > > gains[i]\n> > > });\n> > > }\n> > >\n> > > std::shared_ptr helper =\n> > > std::make_shared();\n> > > helper->init(stages);\n> > >\n> > > So given I have to build a new vector with the paired stages anyway I couldn't\n> > > see value in then using a Span for the init() instead of just passing the vector...but quite\n> > > possibly I am just not understanding why it's the better choice here.\n> >\n> > Note that in the code above, there is a copy when calling init(), using a span avoids that.\n> \n> Or just by passing the vector by reference and not value.\n> The pairs' items will be copied inside the helper anyway.\n\nIf we foresee that the caller won't need to retain access to the vector,\npassing an rvalue reference would be more efficient and avoid a copy.\n\n> > > [...]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F0587C3200\n\tfor ;\n\tWed, 24 Apr 2024 18:58:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBC9B633F9;\n\tWed, 24 Apr 2024 20:58:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E2E8633EB\n\tfor ;\n\tWed, 24 Apr 2024 20:58:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6BFABB1;\n\tWed, 24 Apr 2024 20:57: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=\"LecnNdk9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713985039;\n\tbh=nlwM8IePc0+YOTI91V7a7JzCyt3tIKDSzWbQPvZhppI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LecnNdk9puOQfqq8FFgBuvwpK3iHmis+w5Cz6OL/xDsFKdbV64n26qY4iCVAaUTAT\n\tNBCnKlTRqVgBq4PU/XrlYw/SxcoSFOa+klQjViP483E646jTAlv7bdkOZ5D7qUam6n\n\tUafr1wXN7WJltQRh/V2FZHpSd2ktY8S9hHwHjHco=","Date":"Wed, 24 Apr 2024 21:57:59 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tDan Scally ,\n\tlibcamera-devel@lists.libcamera.org, Paul Elder\n\t","Subject":"Re: [PATCH v2 3/8] ipa: libipa: Add ExposureModeHelper","Message-ID":"<20240424185759.GB6282@pendragon.ideasonboard.com>","References":"<20240417131536.484129-1-dan.scally@ideasonboard.com>\n\t<20240417131536.484129-4-dan.scally@ideasonboard.com>\n\t\n\t<68f927ca-2beb-4029-8a0c-d36513a198ae@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","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\" "}}]