[{"id":20462,"web_url":"https://patchwork.libcamera.org/comment/20462/","msgid":"","date":"2021-10-25T21:20:53","subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Fri, Oct 22, 2021 at 05:12:06PM +0200, Jean-Michel Hautbois wrote:\n> The AGC class was not documented while developing. Extend that to\n> reference the origins of the implementation, and improve the\n> descriptions on how the algorithm operates internally.\n> \n> While at it, rename the functions which have bad names.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> v3:\n> - rename processBrightness to measureBrightness\n> - rename lockExposureGain to computeExposure\n> - reword some comments\n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---\n> src/ipa/ipu3/algorithms/agc.h | 6 +--\n> 2 files changed, 75 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 6c151232..b145e522 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -2,7 +2,7 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * ipu3_agc.cpp - AGC/AEC control algorithm\n> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm\n> */\n> \n> #include \"agc.h\"\n> @@ -17,17 +17,37 @@\n> \n> #include \"libipa/histogram.h\"\n> \n> +/**\n> + * \\file agc.h\n> + */\n> +\n> namespace libcamera {\n> \n> using namespace std::literals::chrono_literals;\n> \n> namespace ipa::ipu3::algorithms {\n> \n> +/**\n> + * \\class Agc\n> + * \\brief A mean-based auto-exposure algorithm\n> + *\n> + * This algorithm calculates a shutter time and an analogue gain so that the\n> + * average value of the green channel of the brightest 2% of pixels approaches\n> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same\n> + * weight, like an average-metering case. In this metering mode, the camera uses\n> + * light information from the entire scene and creates an average for the final\n> + * exposure setting, giving no weighting to any particular portion of the\n> + * metered area.\n> + *\n> + * Reference: Battiato, Messina & Castorina. (2008). Exposure\n> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.\n> + */\n> +\n> LOG_DEFINE_CATEGORY(IPU3Agc)\n> \n> /* Number of frames to wait before calculating stats on minimum exposure */\n> static constexpr uint32_t kInitialFrameMinAECount = 4;\n> -/* Number of frames to wait between new gain/exposure estimations */\n> +/* Number of frames to wait between new gain/shutter time estimations */\n> static constexpr uint32_t kFrameSkipCount = 6;\n> \n> /* Limits for analogue gain values */\n> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;\n> \n> /* Histogram constants */\n> static constexpr uint32_t knumHistogramBins = 256;\n> +/* Target value to reach for the top 2% of the histogram */\n> static constexpr double kEvGainTarget = 0.5;\n> \n> Agc::Agc()\n> @@ -45,10 +66,18 @@ Agc::Agc()\n> {\n> }\n> \n> +/**\n> + * \\brief Configure the AGC given a configInfo\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] configInfo The IPA configuration data\n> + *\n> + * \\return 0\n> + */\n> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> {\n> \tstride_ = context.configuration.grid.stride;\n> \n> +\t/* \\todo use the IPAContext to provide the limits */\n> \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> \t\t / configInfo.sensorInfo.pixelRate;\n> \n> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> \treturn 0;\n> }\n> \n> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> +/**\n> + * \\brief Estimate the mean value of the top 2% of the histogram\n> + * \\param[in] stats The statistics computed by the ImgU\n> + * \\param[in] grid The grid used to store the statistics in the IPU3\n> + */\n> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid)\n> {\n> +\t/* Initialise the histogram array */\n> \tuint32_t hist[knumHistogramBins] = { 0 };\n> \n> \tfor (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\tif (cell->sat_ratio == 0) {\n> \t\t\t\tuint8_t gr = cell->Gr_avg;\n> \t\t\t\tuint8_t gb = cell->Gb_avg;\n> +\t\t\t\t/*\n> +\t\t\t\t * Store the average green value to estimate the\n> +\t\t\t\t * brightness. Even the over exposed pixels are\n\ns/over exposed/overexposed/\n\n> +\t\t\t\t * taken into account.\n> +\t\t\t\t */\n> \t\t\t\thist[(gr + gb) / 2]++;\n> \t\t\t}\n> \t\t}\n> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \tiqMean_ = Histogram(Span(hist)).interQuantileMean(0.98, 1.0);\n> }\n> \n> +/**\n> + * \\brief Apply a filter on the exposure value to limit the speed of changes\n> + */\n> void Agc::filterExposure()\n> {\n> \tdouble speed = 0.2;\n> @@ -106,7 +149,7 @@ void Agc::filterExposure()\n> \t\t/*\n> \t\t * If we are close to the desired result, go faster to avoid making\n> \t\t * multiple micro-adjustments.\n> -\t\t * \\ todo: Make this customisable?\n> +\t\t * \\todo: Make this customisable?\n\ns/://\n\n> \t\t */\n> \t\tif (filteredExposure_ < 1.2 * currentExposure_ &&\n> \t\t filteredExposure_ > 0.8 * currentExposure_)\n> @@ -119,7 +162,12 @@ void Agc::filterExposure()\n> \tLOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << filteredExposure_;\n> }\n> \n> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> +/**\n> + * \\brief Estimate the new exposure and gain values\n> + * \\param[inout] exposure The exposure value reference as a number of lines\n> + * \\param[inout] gain The gain reference to be updated\n> + */\n> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> {\n> \t/* Algorithm initialization should wait for first valid frames */\n> \t/* \\todo - have a number of frames given by DelayedControls ?\n> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> \t\treturn;\n> \t}\n> \n> +\t/* Estimate the gain needed to have the proportion wanted */\n> \tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> \t/* extracted from Rpi::Agc::computeTargetExposure */\n> +\t/* Calculate the shutter time in seconds */\n> \tutils::Duration currentShutter = exposure * lineDuration_;\n> \tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentShutter * analogueGain\n> \t\t\t << \" Shutter speed \" << currentShutter\n> \t\t\t << \" Gain \" << analogueGain\n> \t\t\t << \" Needed ev gain \" << evGain;\n> \n> +\t/*\n> +\t * Estimate the current exposure value for the scene as the latest\n> +\t * exposure value applied multiplied by the new estimated gain\n\ns/gain/gain./\n\nReviewed-by: Laurent Pinchart \n\n> +\t */\n> \tcurrentExposure_ = prevExposureValue_ * evGain;\n> \tutils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;\n> \tutils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;\n> \n> +\t/* Clamp the exposure value to the min and max authorized */\n> \tutils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;\n> \tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> \tLOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> \t/* \\todo: estimate if we need to desaturate */\n> \tfilterExposure();\n> \n> +\t/* Divide the exposure value as new exposure and gain values */\n> \tutils::Duration exposureValue = filteredExposure_;\n> \tutils::Duration shutterTime = minShutterSpeed;\n> \n> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> \tprevExposureValue_ = shutterTime * analogueGain;\n> }\n> \n> +/**\n> + * \\brief Process IPU3 statistics, and run AGC operations\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] stats The IPU3 statistics and ISP results\n> + *\n> + * Identify the current image brightness, and use that to estimate the optimal\n> + * new exposure and gain for the scene.\n> + */\n> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> +\t/* Get the latest exposure and gain applied */\n> \tuint32_t &exposure = context.frameContext.agc.exposure;\n> \tdouble &analogueGain = context.frameContext.agc.gain;\n> -\tprocessBrightness(stats, context.configuration.grid.bdsGrid);\n> -\tlockExposureGain(exposure, analogueGain);\n> +\tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n> +\tcomputeExposure(exposure, analogueGain);\n> \tframeCount_++;\n> }\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 1840205b..69e0b831 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -2,7 +2,7 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * agc.h - IPU3 AGC/AEC control algorithm\n> + * agc.h - IPU3 AGC/AEC mean-based control algorithm\n> */\n> #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n> #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n> @@ -31,10 +31,10 @@ public:\n> \tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> \n> private:\n> -\tvoid processBrightness(const ipu3_uapi_stats_3a *stats,\n> +\tvoid measureBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> \tvoid filterExposure();\n> -\tvoid lockExposureGain(uint32_t &exposure, double &gain);\n> +\tvoid computeExposure(uint32_t &exposure, double &gain);\n> \n> \tuint64_t frameCount_;\n> \tuint64_t lastFrame_;","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 845B4BF415\n\tfor ;\n\tMon, 25 Oct 2021 21:21:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0C8D60125;\n\tMon, 25 Oct 2021 23:21:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D913260125\n\tfor ;\n\tMon, 25 Oct 2021 23:21:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F8B8E0A;\n\tMon, 25 Oct 2021 23:21:16 +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=\"wXy9IBHH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635196876;\n\tbh=Qs+wjviN0wbDW14JKjoLk6zcQVYOE3pLJpugCT/Kg5Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wXy9IBHH+UisQOmI2xx6nabx3pxglxCyia+zlaivMwjptgR8Cl56AS7szc/Ph+ELt\n\tS7Sx3fHSWddMq+WWcHBg5dc9bfuAU1C6J43t4zXz6keFdZjNNdC1DMXRJZpus9Ts5E\n\tR86eCHkP8bhLaVXZ+Eib7xyY0BqtJ0eQw/zhcmrI=","Date":"Tue, 26 Oct 2021 00:20:53 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211022151218.111966-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20465,"web_url":"https://patchwork.libcamera.org/comment/20465/","msgid":"<163519717249.1095920.5254802987281640587@Monstersaurus>","date":"2021-10-25T21:26:12","subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)\n> The AGC class was not documented while developing. Extend that to\n> reference the origins of the implementation, and improve the\n> descriptions on how the algorithm operates internally.\n> \n> While at it, rename the functions which have bad names.\n\nAha, these are just renames. It threw me off thinking there were bigger\nchanges in here, but a rename is fine.\n\n> Signed-off-by: Jean-Michel Hautbois \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> v3:\n> - rename processBrightness to measureBrightness\n> - rename lockExposureGain to computeExposure\n> - reword some comments\n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---\n> src/ipa/ipu3/algorithms/agc.h | 6 +--\n> 2 files changed, 75 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 6c151232..b145e522 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -2,7 +2,7 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * ipu3_agc.cpp - AGC/AEC control algorithm\n> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm\n> */\n> \n> #include \"agc.h\"\n> @@ -17,17 +17,37 @@\n> \n> #include \"libipa/histogram.h\"\n> \n> +/**\n> + * \\file agc.h\n> + */\n> +\n> namespace libcamera {\n> \n> using namespace std::literals::chrono_literals;\n> \n> namespace ipa::ipu3::algorithms {\n> \n> +/**\n> + * \\class Agc\n> + * \\brief A mean-based auto-exposure algorithm\n> + *\n> + * This algorithm calculates a shutter time and an analogue gain so that the\n\nIs it an analogue gain? or just a gain? Does the algorithm care if it is\nanalogue?\n\nPerhaps it does, as it's targetted to the Sensor controls.\n\n> + * average value of the green channel of the brightest 2% of pixels approaches\n> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same\n> + * weight, like an average-metering case. In this metering mode, the camera uses\n> + * light information from the entire scene and creates an average for the final\n> + * exposure setting, giving no weighting to any particular portion of the\n> + * metered area.\n> + *\n> + * Reference: Battiato, Messina & Castorina. (2008). Exposure\n> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.\n> + */\n> +\n> LOG_DEFINE_CATEGORY(IPU3Agc)\n> \n> /* Number of frames to wait before calculating stats on minimum exposure */\n> static constexpr uint32_t kInitialFrameMinAECount = 4;\n> -/* Number of frames to wait between new gain/exposure estimations */\n> +/* Number of frames to wait between new gain/shutter time estimations */\n> static constexpr uint32_t kFrameSkipCount = 6;\n> \n> /* Limits for analogue gain values */\n> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;\n> \n> /* Histogram constants */\n> static constexpr uint32_t knumHistogramBins = 256;\n\nI'd have a new line here.\n\n> +/* Target value to reach for the top 2% of the histogram */\n\nIs this something that is expected to be tuned? or is 0.5 used to mean\nquite literally 'the middle' between 1 and 0?\n\n> static constexpr double kEvGainTarget = 0.5;\n> \n> Agc::Agc()\n> @@ -45,10 +66,18 @@ Agc::Agc()\n> {\n> }\n> \n> +/**\n> + * \\brief Configure the AGC given a configInfo\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] configInfo The IPA configuration data\n> + *\n> + * \\return 0\n> + */\n> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> {\n> stride_ = context.configuration.grid.stride;\n> \n> + /* \\todo use the IPAContext to provide the limits */\n> lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> / configInfo.sensorInfo.pixelRate;\n> \n> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> return 0;\n> }\n> \n> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> +/**\n> + * \\brief Estimate the mean value of the top 2% of the histogram\n> + * \\param[in] stats The statistics computed by the ImgU\n> + * \\param[in] grid The grid used to store the statistics in the IPU3\n> + */\n> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n> const ipu3_uapi_grid_config &grid)\n> {\n> + /* Initialise the histogram array */\n> uint32_t hist[knumHistogramBins] = { 0 };\n> \n> for (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> if (cell->sat_ratio == 0) {\n> uint8_t gr = cell->Gr_avg;\n> uint8_t gb = cell->Gb_avg;\n> + /*\n> + * Store the average green value to estimate the\n> + * brightness. Even the over exposed pixels are\n> + * taken into account.\n> + */\n> hist[(gr + gb) / 2]++;\n> }\n> }\n> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> iqMean_ = Histogram(Span(hist)).interQuantileMean(0.98, 1.0);\n> }\n> \n> +/**\n> + * \\brief Apply a filter on the exposure value to limit the speed of changes\n> + */\n> void Agc::filterExposure()\n> {\n> double speed = 0.2;\n> @@ -106,7 +149,7 @@ void Agc::filterExposure()\n> /*\n> * If we are close to the desired result, go faster to avoid making\n> * multiple micro-adjustments.\n> - * \\ todo: Make this customisable?\n> + * \\todo: Make this customisable?\n> */\n> if (filteredExposure_ < 1.2 * currentExposure_ &&\n> filteredExposure_ > 0.8 * currentExposure_)\n> @@ -119,7 +162,12 @@ void Agc::filterExposure()\n> LOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << filteredExposure_;\n> }\n> \n> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> +/**\n> + * \\brief Estimate the new exposure and gain values\n> + * \\param[inout] exposure The exposure value reference as a number of lines\n> + * \\param[inout] gain The gain reference to be updated\n> + */\n> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> {\n> /* Algorithm initialization should wait for first valid frames */\n> /* \\todo - have a number of frames given by DelayedControls ?\n> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> return;\n> }\n> \n> + /* Estimate the gain needed to have the proportion wanted */\n> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> /* extracted from Rpi::Agc::computeTargetExposure */\n> + /* Calculate the shutter time in seconds */\n> utils::Duration currentShutter = exposure * lineDuration_;\n> LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentShutter * analogueGain\n> << \" Shutter speed \" << currentShutter\n> << \" Gain \" << analogueGain\n> << \" Needed ev gain \" << evGain;\n> \n> + /*\n> + * Estimate the current exposure value for the scene as the latest\n> + * exposure value applied multiplied by the new estimated gain\n> + */\n\nDoes this estimate what the sensor is currently configured to expose?\nOr does it 'calculate' what is desired for the next control to set?\n\nEstimate here implies that the algorithm is guessing what the sensor\nmight be configured as (which I would hope we can 'know' or determine?)\n\nOk, the brief of this function clears up my question, as it says\n'Estimate the new'.\n\nIs 'estimate' intentional? I would interpret that we would 'calculate'\nthe new values. Or in fact, given the function name, we might be\n'computing' the new vaules...\n\n\n> currentExposure_ = prevExposureValue_ * evGain;\n> utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;\n> utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;\n> \n> + /* Clamp the exposure value to the min and max authorized */\n> utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;\n> currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> /* \\todo: estimate if we need to desaturate */\n> filterExposure();\n> \n> + /* Divide the exposure value as new exposure and gain values */\n> utils::Duration exposureValue = filteredExposure_;\n> utils::Duration shutterTime = minShutterSpeed;\n> \n> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> prevExposureValue_ = shutterTime * analogueGain;\n> }\n> \n> +/**\n> + * \\brief Process IPU3 statistics, and run AGC operations\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] stats The IPU3 statistics and ISP results\n> + *\n> + * Identify the current image brightness, and use that to estimate the optimal\n> + * new exposure and gain for the scene.\n\nEstimate or calculate? Estimate might be correct (here and above) if we\ndon't know in a single iteration what values to choose, as I suspect\nit's somewhat of an adaptive algorithm that takes a couple of iterations\nto get to a desired goal.\n\nIf estimate is correct - then I think that covers my main concerns, or\nwith an update to 'calculate' if that's more correct:\n\nReviewed-by: Kieran Bingham \n\n> + */\n> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> + /* Get the latest exposure and gain applied */\n> uint32_t &exposure = context.frameContext.agc.exposure;\n> double &analogueGain = context.frameContext.agc.gain;\n> - processBrightness(stats, context.configuration.grid.bdsGrid);\n> - lockExposureGain(exposure, analogueGain);\n> + measureBrightness(stats, context.configuration.grid.bdsGrid);\n> + computeExposure(exposure, analogueGain);\n> frameCount_++;\n> }\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 1840205b..69e0b831 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -2,7 +2,7 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * agc.h - IPU3 AGC/AEC control algorithm\n> + * agc.h - IPU3 AGC/AEC mean-based control algorithm\n> */\n> #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n> #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n> @@ -31,10 +31,10 @@ public:\n> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> \n> private:\n> - void processBrightness(const ipu3_uapi_stats_3a *stats,\n> + void measureBrightness(const ipu3_uapi_stats_3a *stats,\n> const ipu3_uapi_grid_config &grid);\n> void filterExposure();\n> - void lockExposureGain(uint32_t &exposure, double &gain);\n> + void computeExposure(uint32_t &exposure, double &gain);\n> \n> uint64_t frameCount_;\n> uint64_t lastFrame_;\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 879F9BDB1C\n\tfor ;\n\tMon, 25 Oct 2021 21:26:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD8706487A;\n\tMon, 25 Oct 2021 23:26:16 +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 5641960125\n\tfor ;\n\tMon, 25 Oct 2021 23:26:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E7885E0A;\n\tMon, 25 Oct 2021 23:26:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZzDZIf4A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635197175;\n\tbh=HvrAGeT2R6LAHsGpG+N0qd4PfzEui1ySZL/LsgtVQIA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZzDZIf4AglE9TpbvwonwRvWEliMMJahij1ZyM4AsoII1YTw5Vlm7Zn74MMfBAL1oF\n\tfN0wO/OX30mET34XNq4F5cCpkTazPbgRwS/+y3eMfsO7mVlTxh/q0kUG42/UuG2Lsz\n\tXBVhPuahAF6Lvep9WL+7dQ1ZWlF4BNH3vjrDAExs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>","References":"<20211022151218.111966-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 25 Oct 2021 22:26:12 +0100","Message-ID":"<163519717249.1095920.5254802987281640587@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","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":20496,"web_url":"https://patchwork.libcamera.org/comment/20496/","msgid":"<9189a20c-e6d8-d359-9e63-acb836905131@ideasonboard.com>","date":"2021-10-26T06:13:17","subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 25/10/2021 23:26, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)\n>> The AGC class was not documented while developing. Extend that to\n>> reference the origins of the implementation, and improve the\n>> descriptions on how the algorithm operates internally.\n>>\n>> While at it, rename the functions which have bad names.\n> \n> Aha, these are just renames. It threw me off thinking there were bigger\n> changes in here, but a rename is fine.\n> \n>> Signed-off-by: Jean-Michel Hautbois \n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> v3:\n>> - rename processBrightness to measureBrightness\n>> - rename lockExposureGain to computeExposure\n>> - reword some comments\n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---\n>> src/ipa/ipu3/algorithms/agc.h | 6 +--\n>> 2 files changed, 75 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 6c151232..b145e522 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -2,7 +2,7 @@\n>> /*\n>> * Copyright (C) 2021, Ideas On Board\n>> *\n>> - * ipu3_agc.cpp - AGC/AEC control algorithm\n>> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm\n>> */\n>> \n>> #include \"agc.h\"\n>> @@ -17,17 +17,37 @@\n>> \n>> #include \"libipa/histogram.h\"\n>> \n>> +/**\n>> + * \\file agc.h\n>> + */\n>> +\n>> namespace libcamera {\n>> \n>> using namespace std::literals::chrono_literals;\n>> \n>> namespace ipa::ipu3::algorithms {\n>> \n>> +/**\n>> + * \\class Agc\n>> + * \\brief A mean-based auto-exposure algorithm\n>> + *\n>> + * This algorithm calculates a shutter time and an analogue gain so that the\n> \n> Is it an analogue gain? or just a gain? Does the algorithm care if it is\n> analogue?\n> \n> Perhaps it does, as it's targetted to the Sensor controls.\n\nQuoting you and Laurent in v1:\n\nKieran:\nSurely the algorithm itself doesn't care if this is an analogue gain or \na digital gain.\n\nLaurent:\nAnalogue gain is applied in the sensor, before the ADC, while digital\ngain can be applied anywhere after the ADC. Most sensors will have a\ndigital gain, which is usually unused, and ISPs also have digital gains.\nIt's important to split gain between analogue and digital correctly as\nthey have different implications (in terms of noise for instance), so\nthe algorithm needs to care.\n\nKieran:\nThe IPU3 will apply it as an analogue gain - but that's independent \nisn't it ?\n\nLaurent:\nIf you meant the IPU3 IPA, yes, it applies the gain as analogue gain. If\nyou meant the IPU3 hardware, no, it has no analogue gain.\n\nKieran:\nPresumably if we weren't able to apply it fully as analogue - the \nremainder could be applied digitally to meet the desired gain requested \n by the algorithm?\n\nLaurent:\nThat's usually what a basic implementation will do, there could be\nreason to do something more complex.\n\nMe:\nWe apply analogue gains right now and should take care of digital gain \nlater :-).\n\n> \n>> + * average value of the green channel of the brightest 2% of pixels approaches\n>> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same\n>> + * weight, like an average-metering case. In this metering mode, the camera uses\n>> + * light information from the entire scene and creates an average for the final\n>> + * exposure setting, giving no weighting to any particular portion of the\n>> + * metered area.\n>> + *\n>> + * Reference: Battiato, Messina & Castorina. (2008). Exposure\n>> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.\n>> + */\n>> +\n>> LOG_DEFINE_CATEGORY(IPU3Agc)\n>> \n>> /* Number of frames to wait before calculating stats on minimum exposure */\n>> static constexpr uint32_t kInitialFrameMinAECount = 4;\n>> -/* Number of frames to wait between new gain/exposure estimations */\n>> +/* Number of frames to wait between new gain/shutter time estimations */\n>> static constexpr uint32_t kFrameSkipCount = 6;\n>> \n>> /* Limits for analogue gain values */\n>> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;\n>> \n>> /* Histogram constants */\n>> static constexpr uint32_t knumHistogramBins = 256;\n> \n> I'd have a new line here.\n> \n>> +/* Target value to reach for the top 2% of the histogram */\n> \n> Is this something that is expected to be tuned? or is 0.5 used to mean\n> quite literally 'the middle' between 1 and 0?\n> \n>> static constexpr double kEvGainTarget = 0.5;\n>> \n>> Agc::Agc()\n>> @@ -45,10 +66,18 @@ Agc::Agc()\n>> {\n>> }\n>> \n>> +/**\n>> + * \\brief Configure the AGC given a configInfo\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] configInfo The IPA configuration data\n>> + *\n>> + * \\return 0\n>> + */\n>> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>> {\n>> stride_ = context.configuration.grid.stride;\n>> \n>> + /* \\todo use the IPAContext to provide the limits */\n>> lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n>> / configInfo.sensorInfo.pixelRate;\n>> \n>> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>> return 0;\n>> }\n>> \n>> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> +/**\n>> + * \\brief Estimate the mean value of the top 2% of the histogram\n>> + * \\param[in] stats The statistics computed by the ImgU\n>> + * \\param[in] grid The grid used to store the statistics in the IPU3\n>> + */\n>> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n>> const ipu3_uapi_grid_config &grid)\n>> {\n>> + /* Initialise the histogram array */\n>> uint32_t hist[knumHistogramBins] = { 0 };\n>> \n>> for (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n>> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> if (cell->sat_ratio == 0) {\n>> uint8_t gr = cell->Gr_avg;\n>> uint8_t gb = cell->Gb_avg;\n>> + /*\n>> + * Store the average green value to estimate the\n>> + * brightness. Even the over exposed pixels are\n>> + * taken into account.\n>> + */\n>> hist[(gr + gb) / 2]++;\n>> }\n>> }\n>> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> iqMean_ = Histogram(Span(hist)).interQuantileMean(0.98, 1.0);\n>> }\n>> \n>> +/**\n>> + * \\brief Apply a filter on the exposure value to limit the speed of changes\n>> + */\n>> void Agc::filterExposure()\n>> {\n>> double speed = 0.2;\n>> @@ -106,7 +149,7 @@ void Agc::filterExposure()\n>> /*\n>> * If we are close to the desired result, go faster to avoid making\n>> * multiple micro-adjustments.\n>> - * \\ todo: Make this customisable?\n>> + * \\todo: Make this customisable?\n>> */\n>> if (filteredExposure_ < 1.2 * currentExposure_ &&\n>> filteredExposure_ > 0.8 * currentExposure_)\n>> @@ -119,7 +162,12 @@ void Agc::filterExposure()\n>> LOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << filteredExposure_;\n>> }\n>> \n>> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> +/**\n>> + * \\brief Estimate the new exposure and gain values\n>> + * \\param[inout] exposure The exposure value reference as a number of lines\n>> + * \\param[inout] gain The gain reference to be updated\n>> + */\n>> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> {\n>> /* Algorithm initialization should wait for first valid frames */\n>> /* \\todo - have a number of frames given by DelayedControls ?\n>> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> return;\n>> }\n>> \n>> + /* Estimate the gain needed to have the proportion wanted */\n>> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n>> \n>> /* extracted from Rpi::Agc::computeTargetExposure */\n>> + /* Calculate the shutter time in seconds */\n>> utils::Duration currentShutter = exposure * lineDuration_;\n>> LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentShutter * analogueGain\n>> << \" Shutter speed \" << currentShutter\n>> << \" Gain \" << analogueGain\n>> << \" Needed ev gain \" << evGain;\n>> \n>> + /*\n>> + * Estimate the current exposure value for the scene as the latest\n>> + * exposure value applied multiplied by the new estimated gain\n>> + */\n> \n> Does this estimate what the sensor is currently configured to expose?\n> Or does it 'calculate' what is desired for the next control to set?\n> \n> Estimate here implies that the algorithm is guessing what the sensor\n> might be configured as (which I would hope we can 'know' or determine?)\n> \n> Ok, the brief of this function clears up my question, as it says\n> 'Estimate the new'.\n> \n> Is 'estimate' intentional? I would interpret that we would 'calculate'\n> the new values. Or in fact, given the function name, we might be\n> 'computing' the new vaules...\n> \n> \n>> currentExposure_ = prevExposureValue_ * evGain;\n>> utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;\n>> utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;\n>> \n>> + /* Clamp the exposure value to the min and max authorized */\n>> utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;\n>> currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n>> LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n>> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> /* \\todo: estimate if we need to desaturate */\n>> filterExposure();\n>> \n>> + /* Divide the exposure value as new exposure and gain values */\n>> utils::Duration exposureValue = filteredExposure_;\n>> utils::Duration shutterTime = minShutterSpeed;\n>> \n>> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> prevExposureValue_ = shutterTime * analogueGain;\n>> }\n>> \n>> +/**\n>> + * \\brief Process IPU3 statistics, and run AGC operations\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] stats The IPU3 statistics and ISP results\n>> + *\n>> + * Identify the current image brightness, and use that to estimate the optimal\n>> + * new exposure and gain for the scene.\n> \n> Estimate or calculate? Estimate might be correct (here and above) if we\n> don't know in a single iteration what values to choose, as I suspect\n> it's somewhat of an adaptive algorithm that takes a couple of iterations\n> to get to a desired goal.\n> \n> If estimate is correct - then I think that covers my main concerns, or\n> with an update to 'calculate' if that's more correct:\n> \n> Reviewed-by: Kieran Bingham \n> \n>> + */\n>> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>> {\n>> + /* Get the latest exposure and gain applied */\n>> uint32_t &exposure = context.frameContext.agc.exposure;\n>> double &analogueGain = context.frameContext.agc.gain;\n>> - processBrightness(stats, context.configuration.grid.bdsGrid);\n>> - lockExposureGain(exposure, analogueGain);\n>> + measureBrightness(stats, context.configuration.grid.bdsGrid);\n>> + computeExposure(exposure, analogueGain);\n>> frameCount_++;\n>> }\n>> \n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index 1840205b..69e0b831 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -2,7 +2,7 @@\n>> /*\n>> * Copyright (C) 2021, Ideas On Board\n>> *\n>> - * agc.h - IPU3 AGC/AEC control algorithm\n>> + * agc.h - IPU3 AGC/AEC mean-based control algorithm\n>> */\n>> #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n>> #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n>> @@ -31,10 +31,10 @@ public:\n>> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n>> \n>> private:\n>> - void processBrightness(const ipu3_uapi_stats_3a *stats,\n>> + void measureBrightness(const ipu3_uapi_stats_3a *stats,\n>> const ipu3_uapi_grid_config &grid);\n>> void filterExposure();\n>> - void lockExposureGain(uint32_t &exposure, double &gain);\n>> + void computeExposure(uint32_t &exposure, double &gain);\n>> \n>> uint64_t frameCount_;\n>> uint64_t lastFrame_;\n>> -- \n>> 2.32.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 51660BF415\n\tfor ;\n\tTue, 26 Oct 2021 06:13:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3E2E64878;\n\tTue, 26 Oct 2021 08:13:21 +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 414B66486B\n\tfor ;\n\tTue, 26 Oct 2021 08:13:20 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:dce3:eb54:18d7:6f3d] (unknown\n\t[IPv6:2a01:e0a:169:7140:dce3:eb54:18d7:6f3d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CEE393F0;\n\tTue, 26 Oct 2021 08:13:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sVShoSLx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635228799;\n\tbh=cP0AiWHAcTfMo3gHhQCNt0YNDWuwbZQnv2dL/JbQVA4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=sVShoSLx9+tpEYBSnFmhEeZzy+3p9RLr4CfzeVAjVaw9GM7hQDN2v453CrglemY8S\n\tsQ9ncu6DN4s8c03rO1xEvwCXdsaiGmq+nFOCSuNHp5Tm79G7b0Rabz22fQqmVcWe08\n\tEEzKXH19ydw4rOC8Jvk01H7YWQ/bitrFu2EdoHqQ=","Message-ID":"<9189a20c-e6d8-d359-9e63-acb836905131@ideasonboard.com>","Date":"Tue, 26 Oct 2021 08:13:17 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.1.2","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211022151218.111966-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>\n\t<163519717249.1095920.5254802987281640587@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163519717249.1095920.5254802987281640587@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","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":20497,"web_url":"https://patchwork.libcamera.org/comment/20497/","msgid":"<248ee72b-beac-7268-31d3-90745c018253@ideasonboard.com>","date":"2021-10-26T06:23:45","subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 25/10/2021 23:26, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)\n>> The AGC class was not documented while developing. Extend that to\n>> reference the origins of the implementation, and improve the\n>> descriptions on how the algorithm operates internally.\n>>\n>> While at it, rename the functions which have bad names.\n> \n> Aha, these are just renames. It threw me off thinking there were bigger\n> changes in here, but a rename is fine.\n> \n>> Signed-off-by: Jean-Michel Hautbois \n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> v3:\n>> - rename processBrightness to measureBrightness\n>> - rename lockExposureGain to computeExposure\n>> - reword some comments\n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---\n>> src/ipa/ipu3/algorithms/agc.h | 6 +--\n>> 2 files changed, 75 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 6c151232..b145e522 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -2,7 +2,7 @@\n>> /*\n>> * Copyright (C) 2021, Ideas On Board\n>> *\n>> - * ipu3_agc.cpp - AGC/AEC control algorithm\n>> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm\n>> */\n>> \n>> #include \"agc.h\"\n>> @@ -17,17 +17,37 @@\n>> \n>> #include \"libipa/histogram.h\"\n>> \n>> +/**\n>> + * \\file agc.h\n>> + */\n>> +\n>> namespace libcamera {\n>> \n>> using namespace std::literals::chrono_literals;\n>> \n>> namespace ipa::ipu3::algorithms {\n>> \n>> +/**\n>> + * \\class Agc\n>> + * \\brief A mean-based auto-exposure algorithm\n>> + *\n>> + * This algorithm calculates a shutter time and an analogue gain so that the\n> \n> Is it an analogue gain? or just a gain? Does the algorithm care if it is\n> analogue?\n> \n> Perhaps it does, as it's targetted to the Sensor controls.\n> \n>> + * average value of the green channel of the brightest 2% of pixels approaches\n>> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same\n>> + * weight, like an average-metering case. In this metering mode, the camera uses\n>> + * light information from the entire scene and creates an average for the final\n>> + * exposure setting, giving no weighting to any particular portion of the\n>> + * metered area.\n>> + *\n>> + * Reference: Battiato, Messina & Castorina. (2008). Exposure\n>> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.\n>> + */\n>> +\n>> LOG_DEFINE_CATEGORY(IPU3Agc)\n>> \n>> /* Number of frames to wait before calculating stats on minimum exposure */\n>> static constexpr uint32_t kInitialFrameMinAECount = 4;\n>> -/* Number of frames to wait between new gain/exposure estimations */\n>> +/* Number of frames to wait between new gain/shutter time estimations */\n>> static constexpr uint32_t kFrameSkipCount = 6;\n>> \n>> /* Limits for analogue gain values */\n>> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;\n>> \n>> /* Histogram constants */\n>> static constexpr uint32_t knumHistogramBins = 256;\n> \n> I'd have a new line here.\n> \n>> +/* Target value to reach for the top 2% of the histogram */\n> \n> Is this something that is expected to be tuned? or is 0.5 used to mean\n> quite literally 'the middle' between 1 and 0?\n\nIt can be tuned later if we want to have low and high bounds. As we work \nwith cumulative histograms, 0.5 is... half of the counted pixels.\n\nQuoting \"Raspberry Pi Camera Algorithm and Tuning Guide\"\n(https://datasheets.raspberrypi.com/camera/raspberry-pi-camera-guide.pdf):\n\n\"I(0.98, 1) = 0.5, lower bound. Here we are saying that the top 2% of \nthe histogram must lie at or above 0.5 in the pixel range (metering all \nhappens before gamma, so this is a moderately bright value). Or in \nshort, we are requiring “some of the pixels to be reasonably bright”. \nThis is a good strategy for snowy or beach scenes, also for images of \ndocuments. It actually raises the exposure of the whole scene; without \nit, the entire image would look dull grey which is undesirable in such \ncircumstances. On the other hand when, as is usually the case, there is \nan abundance of bright and dark pixels, this constraint has no effect, \nso it is often reasonable to apply this constraint all the time.\"\n\n> \n>> static constexpr double kEvGainTarget = 0.5;\n>> \n>> Agc::Agc()\n>> @@ -45,10 +66,18 @@ Agc::Agc()\n>> {\n>> }\n>> \n>> +/**\n>> + * \\brief Configure the AGC given a configInfo\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] configInfo The IPA configuration data\n>> + *\n>> + * \\return 0\n>> + */\n>> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>> {\n>> stride_ = context.configuration.grid.stride;\n>> \n>> + /* \\todo use the IPAContext to provide the limits */\n>> lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n>> / configInfo.sensorInfo.pixelRate;\n>> \n>> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>> return 0;\n>> }\n>> \n>> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> +/**\n>> + * \\brief Estimate the mean value of the top 2% of the histogram\n>> + * \\param[in] stats The statistics computed by the ImgU\n>> + * \\param[in] grid The grid used to store the statistics in the IPU3\n>> + */\n>> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n>> const ipu3_uapi_grid_config &grid)\n>> {\n>> + /* Initialise the histogram array */\n>> uint32_t hist[knumHistogramBins] = { 0 };\n>> \n>> for (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n>> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> if (cell->sat_ratio == 0) {\n>> uint8_t gr = cell->Gr_avg;\n>> uint8_t gb = cell->Gb_avg;\n>> + /*\n>> + * Store the average green value to estimate the\n>> + * brightness. Even the over exposed pixels are\n>> + * taken into account.\n>> + */\n>> hist[(gr + gb) / 2]++;\n>> }\n>> }\n>> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> iqMean_ = Histogram(Span(hist)).interQuantileMean(0.98, 1.0);\n>> }\n>> \n>> +/**\n>> + * \\brief Apply a filter on the exposure value to limit the speed of changes\n>> + */\n>> void Agc::filterExposure()\n>> {\n>> double speed = 0.2;\n>> @@ -106,7 +149,7 @@ void Agc::filterExposure()\n>> /*\n>> * If we are close to the desired result, go faster to avoid making\n>> * multiple micro-adjustments.\n>> - * \\ todo: Make this customisable?\n>> + * \\todo: Make this customisable?\n>> */\n>> if (filteredExposure_ < 1.2 * currentExposure_ &&\n>> filteredExposure_ > 0.8 * currentExposure_)\n>> @@ -119,7 +162,12 @@ void Agc::filterExposure()\n>> LOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << filteredExposure_;\n>> }\n>> \n>> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> +/**\n>> + * \\brief Estimate the new exposure and gain values\n>> + * \\param[inout] exposure The exposure value reference as a number of lines\n>> + * \\param[inout] gain The gain reference to be updated\n>> + */\n>> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> {\n>> /* Algorithm initialization should wait for first valid frames */\n>> /* \\todo - have a number of frames given by DelayedControls ?\n>> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> return;\n>> }\n>> \n>> + /* Estimate the gain needed to have the proportion wanted */\n>> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n>> \n>> /* extracted from Rpi::Agc::computeTargetExposure */\n>> + /* Calculate the shutter time in seconds */\n>> utils::Duration currentShutter = exposure * lineDuration_;\n>> LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentShutter * analogueGain\n>> << \" Shutter speed \" << currentShutter\n>> << \" Gain \" << analogueGain\n>> << \" Needed ev gain \" << evGain;\n>> \n>> + /*\n>> + * Estimate the current exposure value for the scene as the latest\n>> + * exposure value applied multiplied by the new estimated gain\n>> + */\n> \n> Does this estimate what the sensor is currently configured to expose?\n> Or does it 'calculate' what is desired for the next control to set?\n> \n> Estimate here implies that the algorithm is guessing what the sensor\n> might be configured as (which I would hope we can 'know' or determine?)\n> \n> Ok, the brief of this function clears up my question, as it says\n> 'Estimate the new'.\n> \n> Is 'estimate' intentional? I would interpret that we would 'calculate'\n> the new values. Or in fact, given the function name, we might be\n> 'computing' the new vaules...\n\nIt might be calculate or compute then ;-).\n\n> \n> \n>> currentExposure_ = prevExposureValue_ * evGain;\n>> utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;\n>> utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;\n>> \n>> + /* Clamp the exposure value to the min and max authorized */\n>> utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;\n>> currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n>> LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n>> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> /* \\todo: estimate if we need to desaturate */\n>> filterExposure();\n>> \n>> + /* Divide the exposure value as new exposure and gain values */\n>> utils::Duration exposureValue = filteredExposure_;\n>> utils::Duration shutterTime = minShutterSpeed;\n>> \n>> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n>> prevExposureValue_ = shutterTime * analogueGain;\n>> }\n>> \n>> +/**\n>> + * \\brief Process IPU3 statistics, and run AGC operations\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] stats The IPU3 statistics and ISP results\n>> + *\n>> + * Identify the current image brightness, and use that to estimate the optimal\n>> + * new exposure and gain for the scene.\n> \n> Estimate or calculate? Estimate might be correct (here and above) if we\n> don't know in a single iteration what values to choose, as I suspect\n> it's somewhat of an adaptive algorithm that takes a couple of iterations\n> to get to a desired goal.\n\nIn this case, it is more an estimation, I think, as it will apply new \nvalues, which will take some frames to have an effect, and will \nre-estimate the exposure/gain...\n\nWhen we will have the IPAFrameContext implemented with a queue and the \n\"real\" exposure and gain applied on the frame we are looking at, then it \nwill be more a calculation, I guess :-).\n\n> \n> If estimate is correct - then I think that covers my main concerns, or\n> with an update to 'calculate' if that's more correct:\n> \n> Reviewed-by: Kieran Bingham \n> \n>> + */\n>> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>> {\n>> + /* Get the latest exposure and gain applied */\n>> uint32_t &exposure = context.frameContext.agc.exposure;\n>> double &analogueGain = context.frameContext.agc.gain;\n>> - processBrightness(stats, context.configuration.grid.bdsGrid);\n>> - lockExposureGain(exposure, analogueGain);\n>> + measureBrightness(stats, context.configuration.grid.bdsGrid);\n>> + computeExposure(exposure, analogueGain);\n>> frameCount_++;\n>> }\n>> \n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index 1840205b..69e0b831 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -2,7 +2,7 @@\n>> /*\n>> * Copyright (C) 2021, Ideas On Board\n>> *\n>> - * agc.h - IPU3 AGC/AEC control algorithm\n>> + * agc.h - IPU3 AGC/AEC mean-based control algorithm\n>> */\n>> #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n>> #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n>> @@ -31,10 +31,10 @@ public:\n>> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n>> \n>> private:\n>> - void processBrightness(const ipu3_uapi_stats_3a *stats,\n>> + void measureBrightness(const ipu3_uapi_stats_3a *stats,\n>> const ipu3_uapi_grid_config &grid);\n>> void filterExposure();\n>> - void lockExposureGain(uint32_t &exposure, double &gain);\n>> + void computeExposure(uint32_t &exposure, double &gain);\n>> \n>> uint64_t frameCount_;\n>> uint64_t lastFrame_;\n>> -- \n>> 2.32.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D5668BDB1C\n\tfor ;\n\tTue, 26 Oct 2021 06:23:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41FB96487A;\n\tTue, 26 Oct 2021 08:23: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 3F5276486B\n\tfor ;\n\tTue, 26 Oct 2021 08:23:48 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:dce3:eb54:18d7:6f3d] (unknown\n\t[IPv6:2a01:e0a:169:7140:dce3:eb54:18d7:6f3d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CAE09DBF;\n\tTue, 26 Oct 2021 08:23:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mV0qY22R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635229427;\n\tbh=jBqkpEU3O0ebd96NqxaiwjZfomPGekultPFUL158tco=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=mV0qY22RwaHYig+69Awtvxg0cbOr8uRlPvhoZ4FcR6L/bsqw49KfG+FxMmwBtkc1A\n\ttkOO/u6LKHSu3v/IWfBEH6onSLBCWHScTnzwmXygVoGGi3/lf6utkNb5sJRuViTccR\n\tX5/Dw6Q6jvl8p138gmyoQgYjkdJ9S2rYgtcj3TbE=","Message-ID":"<248ee72b-beac-7268-31d3-90745c018253@ideasonboard.com>","Date":"Tue, 26 Oct 2021 08:23:45 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.1.2","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211022151218.111966-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>\n\t<163519717249.1095920.5254802987281640587@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163519717249.1095920.5254802987281640587@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","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":20519,"web_url":"https://patchwork.libcamera.org/comment/20519/","msgid":"<163523947806.1142945.17045045880402965033@Monstersaurus>","date":"2021-10-26T09:11:18","subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-10-26 07:23:45)\n> \n> \n> On 25/10/2021 23:26, Kieran Bingham wrote:\n> > Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)\n> >> The AGC class was not documented while developing. Extend that to\n> >> reference the origins of the implementation, and improve the\n> >> descriptions on how the algorithm operates internally.\n> >>\n> >> While at it, rename the functions which have bad names.\n> > \n> > Aha, these are just renames. It threw me off thinking there were bigger\n> > changes in here, but a rename is fine.\n> > \n> >> Signed-off-by: Jean-Michel Hautbois \n> >> Signed-off-by: Kieran Bingham \n> >>\n> >> ---\n> >> v3:\n> >> - rename processBrightness to measureBrightness\n> >> - rename lockExposureGain to computeExposure\n> >> - reword some comments\n> >> ---\n> >> src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---\n> >> src/ipa/ipu3/algorithms/agc.h | 6 +--\n> >> 2 files changed, 75 insertions(+), 10 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> >> index 6c151232..b145e522 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> >> @@ -2,7 +2,7 @@\n> >> /*\n> >> * Copyright (C) 2021, Ideas On Board\n> >> *\n> >> - * ipu3_agc.cpp - AGC/AEC control algorithm\n> >> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm\n> >> */\n> >> \n> >> #include \"agc.h\"\n> >> @@ -17,17 +17,37 @@\n> >> \n> >> #include \"libipa/histogram.h\"\n> >> \n> >> +/**\n> >> + * \\file agc.h\n> >> + */\n> >> +\n> >> namespace libcamera {\n> >> \n> >> using namespace std::literals::chrono_literals;\n> >> \n> >> namespace ipa::ipu3::algorithms {\n> >> \n> >> +/**\n> >> + * \\class Agc\n> >> + * \\brief A mean-based auto-exposure algorithm\n> >> + *\n> >> + * This algorithm calculates a shutter time and an analogue gain so that the\n> > \n> > Is it an analogue gain? or just a gain? Does the algorithm care if it is\n> > analogue?\n> > \n> > Perhaps it does, as it's targetted to the Sensor controls.\n> > \n> >> + * average value of the green channel of the brightest 2% of pixels approaches\n> >> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same\n> >> + * weight, like an average-metering case. In this metering mode, the camera uses\n> >> + * light information from the entire scene and creates an average for the final\n> >> + * exposure setting, giving no weighting to any particular portion of the\n> >> + * metered area.\n> >> + *\n> >> + * Reference: Battiato, Messina & Castorina. (2008). Exposure\n> >> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.\n> >> + */\n> >> +\n> >> LOG_DEFINE_CATEGORY(IPU3Agc)\n> >> \n> >> /* Number of frames to wait before calculating stats on minimum exposure */\n> >> static constexpr uint32_t kInitialFrameMinAECount = 4;\n> >> -/* Number of frames to wait between new gain/exposure estimations */\n> >> +/* Number of frames to wait between new gain/shutter time estimations */\n> >> static constexpr uint32_t kFrameSkipCount = 6;\n> >> \n> >> /* Limits for analogue gain values */\n> >> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;\n> >> \n> >> /* Histogram constants */\n> >> static constexpr uint32_t knumHistogramBins = 256;\n> > \n> > I'd have a new line here.\n> > \n> >> +/* Target value to reach for the top 2% of the histogram */\n> > \n> > Is this something that is expected to be tuned? or is 0.5 used to mean\n> > quite literally 'the middle' between 1 and 0?\n> \n> It can be tuned later if we want to have low and high bounds. As we work \n> with cumulative histograms, 0.5 is... half of the counted pixels.\n> \n> Quoting \"Raspberry Pi Camera Algorithm and Tuning Guide\"\n> (https://datasheets.raspberrypi.com/camera/raspberry-pi-camera-guide.pdf):\n> \n> \"I(0.98, 1) = 0.5, lower bound. Here we are saying that the top 2% of \n> the histogram must lie at or above 0.5 in the pixel range (metering all \n> happens before gamma, so this is a moderately bright value). Or in \n> short, we are requiring “some of the pixels to be reasonably bright”. \n> This is a good strategy for snowy or beach scenes, also for images of \n> documents. It actually raises the exposure of the whole scene; without \n> it, the entire image would look dull grey which is undesirable in such \n> circumstances. On the other hand when, as is usually the case, there is \n> an abundance of bright and dark pixels, this constraint has no effect, \n> so it is often reasonable to apply this constraint all the time.\"\n\nThat sounds like important information to convey along with this\nvariable, but should probably be sumarised a bit.\n\nI don't mind if this is handled later though on top of this series as\nit's already quite large, and nearing integration.\n\nCould you make a note to revisit this later please?\n\n\n> >> static constexpr double kEvGainTarget = 0.5;\n> >> \n> >> Agc::Agc()\n> >> @@ -45,10 +66,18 @@ Agc::Agc()\n> >> {\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Configure the AGC given a configInfo\n> >> + * \\param[in] context The shared IPA context\n> >> + * \\param[in] configInfo The IPA configuration data\n> >> + *\n> >> + * \\return 0\n> >> + */\n> >> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> >> {\n> >> stride_ = context.configuration.grid.stride;\n> >> \n> >> + /* \\todo use the IPAContext to provide the limits */\n> >> lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> >> / configInfo.sensorInfo.pixelRate;\n> >> \n> >> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n> >> return 0;\n> >> }\n> >> \n> >> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> >> +/**\n> >> + * \\brief Estimate the mean value of the top 2% of the histogram\n> >> + * \\param[in] stats The statistics computed by the ImgU\n> >> + * \\param[in] grid The grid used to store the statistics in the IPU3\n> >> + */\n> >> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,\n> >> const ipu3_uapi_grid_config &grid)\n> >> {\n> >> + /* Initialise the histogram array */\n> >> uint32_t hist[knumHistogramBins] = { 0 };\n> >> \n> >> for (unsigned int cellY = 0; cellY < grid.height; cellY++) {\n> >> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> >> if (cell->sat_ratio == 0) {\n> >> uint8_t gr = cell->Gr_avg;\n> >> uint8_t gb = cell->Gb_avg;\n> >> + /*\n> >> + * Store the average green value to estimate the\n> >> + * brightness. Even the over exposed pixels are\n> >> + * taken into account.\n> >> + */\n> >> hist[(gr + gb) / 2]++;\n> >> }\n> >> }\n> >> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> >> iqMean_ = Histogram(Span(hist)).interQuantileMean(0.98, 1.0);\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Apply a filter on the exposure value to limit the speed of changes\n> >> + */\n> >> void Agc::filterExposure()\n> >> {\n> >> double speed = 0.2;\n> >> @@ -106,7 +149,7 @@ void Agc::filterExposure()\n> >> /*\n> >> * If we are close to the desired result, go faster to avoid making\n> >> * multiple micro-adjustments.\n> >> - * \\ todo: Make this customisable?\n> >> + * \\todo: Make this customisable?\n> >> */\n> >> if (filteredExposure_ < 1.2 * currentExposure_ &&\n> >> filteredExposure_ > 0.8 * currentExposure_)\n> >> @@ -119,7 +162,12 @@ void Agc::filterExposure()\n> >> LOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << filteredExposure_;\n> >> }\n> >> \n> >> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> >> +/**\n> >> + * \\brief Estimate the new exposure and gain values\n> >> + * \\param[inout] exposure The exposure value reference as a number of lines\n> >> + * \\param[inout] gain The gain reference to be updated\n> >> + */\n> >> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> >> {\n> >> /* Algorithm initialization should wait for first valid frames */\n> >> /* \\todo - have a number of frames given by DelayedControls ?\n> >> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> >> return;\n> >> }\n> >> \n> >> + /* Estimate the gain needed to have the proportion wanted */\n> >> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> >> \n> >> /* extracted from Rpi::Agc::computeTargetExposure */\n> >> + /* Calculate the shutter time in seconds */\n> >> utils::Duration currentShutter = exposure * lineDuration_;\n> >> LOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentShutter * analogueGain\n> >> << \" Shutter speed \" << currentShutter\n> >> << \" Gain \" << analogueGain\n> >> << \" Needed ev gain \" << evGain;\n> >> \n> >> + /*\n> >> + * Estimate the current exposure value for the scene as the latest\n> >> + * exposure value applied multiplied by the new estimated gain\n> >> + */\n> > \n> > Does this estimate what the sensor is currently configured to expose?\n> > Or does it 'calculate' what is desired for the next control to set?\n> > \n> > Estimate here implies that the algorithm is guessing what the sensor\n> > might be configured as (which I would hope we can 'know' or determine?)\n> > \n> > Ok, the brief of this function clears up my question, as it says\n> > 'Estimate the new'.\n> > \n> > Is 'estimate' intentional? I would interpret that we would 'calculate'\n> > the new values. Or in fact, given the function name, we might be\n> > 'computing' the new vaules...\n> \n> It might be calculate or compute then ;-).\n> \n> > \n> > \n> >> currentExposure_ = prevExposureValue_ * evGain;\n> >> utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;\n> >> utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;\n> >> \n> >> + /* Clamp the exposure value to the min and max authorized */\n> >> utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;\n> >> currentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> >> LOG(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_\n> >> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> >> /* \\todo: estimate if we need to desaturate */\n> >> filterExposure();\n> >> \n> >> + /* Divide the exposure value as new exposure and gain values */\n> >> utils::Duration exposureValue = filteredExposure_;\n> >> utils::Duration shutterTime = minShutterSpeed;\n> >> \n> >> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)\n> >> prevExposureValue_ = shutterTime * analogueGain;\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Process IPU3 statistics, and run AGC operations\n> >> + * \\param[in] context The shared IPA context\n> >> + * \\param[in] stats The IPU3 statistics and ISP results\n> >> + *\n> >> + * Identify the current image brightness, and use that to estimate the optimal\n> >> + * new exposure and gain for the scene.\n> > \n> > Estimate or calculate? Estimate might be correct (here and above) if we\n> > don't know in a single iteration what values to choose, as I suspect\n> > it's somewhat of an adaptive algorithm that takes a couple of iterations\n> > to get to a desired goal.\n> \n> In this case, it is more an estimation, I think, as it will apply new \n> values, which will take some frames to have an effect, and will \n> re-estimate the exposure/gain...\n> \n> When we will have the IPAFrameContext implemented with a queue and the \n> \"real\" exposure and gain applied on the frame we are looking at, then it \n> will be more a calculation, I guess :-).\n\nOk - I think it sounds like we're shooting ourselves in the foot by not\nkeeping a queue of FrameContexts that we've sent out. That should not be\ndifficult to implement, so lets tackle that after integrating this\nseries.\n\n> > If estimate is correct - then I think that covers my main concerns, or\n> > with an update to 'calculate' if that's more correct:\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> >> + */\n> >> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> >> {\n> >> + /* Get the latest exposure and gain applied */\n> >> uint32_t &exposure = context.frameContext.agc.exposure;\n> >> double &analogueGain = context.frameContext.agc.gain;\n> >> - processBrightness(stats, context.configuration.grid.bdsGrid);\n> >> - lockExposureGain(exposure, analogueGain);\n> >> + measureBrightness(stats, context.configuration.grid.bdsGrid);\n> >> + computeExposure(exposure, analogueGain);\n> >> frameCount_++;\n> >> }\n> >> \n> >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> >> index 1840205b..69e0b831 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.h\n> >> +++ b/src/ipa/ipu3/algorithms/agc.h\n> >> @@ -2,7 +2,7 @@\n> >> /*\n> >> * Copyright (C) 2021, Ideas On Board\n> >> *\n> >> - * agc.h - IPU3 AGC/AEC control algorithm\n> >> + * agc.h - IPU3 AGC/AEC mean-based control algorithm\n> >> */\n> >> #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n> >> #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n> >> @@ -31,10 +31,10 @@ public:\n> >> void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> >> \n> >> private:\n> >> - void processBrightness(const ipu3_uapi_stats_3a *stats,\n> >> + void measureBrightness(const ipu3_uapi_stats_3a *stats,\n> >> const ipu3_uapi_grid_config &grid);\n> >> void filterExposure();\n> >> - void lockExposureGain(uint32_t &exposure, double &gain);\n> >> + void computeExposure(uint32_t &exposure, double &gain);\n> >> \n> >> uint64_t frameCount_;\n> >> uint64_t lastFrame_;\n> >> -- \n> >> 2.32.0\n> >>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5B5C1BDB1C\n\tfor ;\n\tTue, 26 Oct 2021 09:11:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1351164878;\n\tTue, 26 Oct 2021 11:11:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEE6264878\n\tfor ;\n\tTue, 26 Oct 2021 11:11:20 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A35183F0;\n\tTue, 26 Oct 2021 11:11:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pvyuTaPa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635239480;\n\tbh=fQKtDBMI1qjwIKhZGQf0Cth+T93YgZ5t1gIHCYiR8gw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=pvyuTaPaGY+GcwVG77/BzFSME6pN3XaZUR/tQ1gLp1LmBbbliIuJgf/pvmUGCXa8F\n\tclUzdLmMwxzLBWPBag2FfMxPkS2mL6EzEeoVRksB7rHU5pwI57KEtk5eDBNG7+JR4Q\n\tzvjNxIpQkuTZTFMUGP7zDK5RcV7+OFvo14n4bBAg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<248ee72b-beac-7268-31d3-90745c018253@ideasonboard.com>","References":"<20211022151218.111966-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>\n\t<163519717249.1095920.5254802987281640587@Monstersaurus>\n\t<248ee72b-beac-7268-31d3-90745c018253@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 26 Oct 2021 10:11:18 +0100","Message-ID":"<163523947806.1142945.17045045880402965033@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","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":20520,"web_url":"https://patchwork.libcamera.org/comment/20520/","msgid":"<163523959868.1142945.6860628488647272234@Monstersaurus>","date":"2021-10-26T09:13:18","subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-10-26 07:13:17)\n> Hi Kieran,\n> \n> On 25/10/2021 23:26, Kieran Bingham wrote:\n> > Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)\n> >> The AGC class was not documented while developing. Extend that to\n> >> reference the origins of the implementation, and improve the\n> >> descriptions on how the algorithm operates internally.\n> >>\n> >> While at it, rename the functions which have bad names.\n> > \n> > Aha, these are just renames. It threw me off thinking there were bigger\n> > changes in here, but a rename is fine.\n> > \n> >> Signed-off-by: Jean-Michel Hautbois \n> >> Signed-off-by: Kieran Bingham \n> >>\n> >> ---\n> >> v3:\n> >> - rename processBrightness to measureBrightness\n> >> - rename lockExposureGain to computeExposure\n> >> - reword some comments\n> >> ---\n> >> src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---\n> >> src/ipa/ipu3/algorithms/agc.h | 6 +--\n> >> 2 files changed, 75 insertions(+), 10 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> >> index 6c151232..b145e522 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> >> @@ -2,7 +2,7 @@\n> >> /*\n> >> * Copyright (C) 2021, Ideas On Board\n> >> *\n> >> - * ipu3_agc.cpp - AGC/AEC control algorithm\n> >> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm\n> >> */\n> >> \n> >> #include \"agc.h\"\n> >> @@ -17,17 +17,37 @@\n> >> \n> >> #include \"libipa/histogram.h\"\n> >> \n> >> +/**\n> >> + * \\file agc.h\n> >> + */\n> >> +\n> >> namespace libcamera {\n> >> \n> >> using namespace std::literals::chrono_literals;\n> >> \n> >> namespace ipa::ipu3::algorithms {\n> >> \n> >> +/**\n> >> + * \\class Agc\n> >> + * \\brief A mean-based auto-exposure algorithm\n> >> + *\n> >> + * This algorithm calculates a shutter time and an analogue gain so that the\n> > \n> > Is it an analogue gain? or just a gain? Does the algorithm care if it is\n> > analogue?\n> > \n> > Perhaps it does, as it's targetted to the Sensor controls.\n> \n> Quoting you and Laurent in v1:\n> \n> Kieran:\n> Surely the algorithm itself doesn't care if this is an analogue gain or \n> a digital gain.\n> \n> Laurent:\n> Analogue gain is applied in the sensor, before the ADC, while digital\n> gain can be applied anywhere after the ADC. Most sensors will have a\n> digital gain, which is usually unused, and ISPs also have digital gains.\n> It's important to split gain between analogue and digital correctly as\n> they have different implications (in terms of noise for instance), so\n> the algorithm needs to care.\n> \n> Kieran:\n> The IPU3 will apply it as an analogue gain - but that's independent \n> isn't it ?\n> \n> Laurent:\n> If you meant the IPU3 IPA, yes, it applies the gain as analogue gain. If\n> you meant the IPU3 hardware, no, it has no analogue gain.\n> \n> Kieran:\n> Presumably if we weren't able to apply it fully as analogue - the \n> remainder could be applied digitally to meet the desired gain requested \n> by the algorithm?\n> \n> Laurent:\n> That's usually what a basic implementation will do, there could be\n> reason to do something more complex.\n> \n> Me:\n> We apply analogue gains right now and should take care of digital gain \n> later :-).\n\nOk, lets leave this all as is then. Thanks for the refresh.\n\n(perhaps in the future we'll have a way to maintain discussion history\nalongside progressive patch versions...)\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 82DE6BDB1C\n\tfor ;\n\tTue, 26 Oct 2021 09:13:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6BB864878;\n\tTue, 26 Oct 2021 11:13:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D128260123\n\tfor ;\n\tTue, 26 Oct 2021 11:13:21 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7483C3F0;\n\tTue, 26 Oct 2021 11:13:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XhxPIJun\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635239601;\n\tbh=XRD2Mm+lRNdUNb/whotqdDEHS4LdWJLRauDAhQCK0W4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XhxPIJunhHA8aOj1z7ECGYueIsBr9Ad8ONI/d4brsaQaBNDvsXXw/m9Cw5UFASCt9\n\tmDVtIkS42623X8D3ro3O/HZqUHVitH5ahlmFaozQmeD7KFgRW3NNMn98S+bKVN7ZWb\n\t7y95pimCW3vluGt/GlIVPDh7AOJNOFMWVSI6Uo1o=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<9189a20c-e6d8-d359-9e63-acb836905131@ideasonboard.com>","References":"<20211022151218.111966-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211022151218.111966-8-jeanmichel.hautbois@ideasonboard.com>\n\t<163519717249.1095920.5254802987281640587@Monstersaurus>\n\t<9189a20c-e6d8-d359-9e63-acb836905131@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 26 Oct 2021 10:13:18 +0100","Message-ID":"<163523959868.1142945.6860628488647272234@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v3 07/19] ipa: ipu3: agc: Document AGC\n\tmean-based algorithm","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\" "}}]