[{"id":19189,"web_url":"https://patchwork.libcamera.org/comment/19189/","msgid":"","date":"2021-08-30T23:43:43","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: ipu3: 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, Aug 27, 2021 at 10:02:26AM +0200, Jean-Michel Hautbois wrote:\n> The AGC mean-based algorithm is partially documented. Improve the\n> documentation to help understand how it works and mark some \\todo for\n> future improvements.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc_mean.cpp | 91 +++++++++++++++++++++++++---\n> src/ipa/ipu3/algorithms/agc_mean.h | 2 +-\n> 2 files changed, 84 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc_mean.cpp b/src/ipa/ipu3/algorithms/agc_mean.cpp\n> index 193f6e9a..b535e14b 100644\n> --- a/src/ipa/ipu3/algorithms/agc_mean.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc_mean.cpp\n> @@ -2,7 +2,7 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * agc_mean.cpp - AGC/AEC control algorithm\n> + * agc_mean.cpp - AGC/AEC mean-based control algorithm\n\nThis should go to the patch 2/4.\n\n> */\n> \n> #include \"agc_mean.h\"\n> @@ -17,12 +17,26 @@\n> \n> #include \"libipa/histogram.h\"\n> \n> +/**\n> + * \\file agc_mean.h\n> + */\n> +\n> namespace libcamera {\n> \n> using namespace std::literals::chrono_literals;\n> \n> namespace ipa::ipu3::algorithms {\n> \n> +/**\n> + * \\class AgMean\n> + * \\brief The class to use the mean-based auto-exposure algorithm\n> + *\n> + * The mean-based algorithm is calculating an exposure and gain value such as\n\ns/such as/such that/\n\n> + * a given quantity of pixels lie in the top 2% of the histogram. The AWB gains\n\nIs that right ? I could be wrong, but I understand it as setting\nexposure and gain such that the mean value of the 2% brightest pixels\n(the top 2% of the histogram) equals a fixed target.\n\n> + * are not used here, and all cells in the grid have the same weight, like an\n> + * average-metering case.\n> + */\n> +\n> LOG_DEFINE_CATEGORY(IPU3AgcMean)\n> \n> /* Number of frames to wait before calculating stats on minimum exposure */\n> @@ -30,13 +44,20 @@ static constexpr uint32_t kInitialFrameMinAECount = 4;\n> /* Number of frames to wait between new gain/exposure estimations */\n> static constexpr uint32_t kFrameSkipCount = 6;\n> \n> -/* Maximum ISO value for analogue gain */\n> +/* Minimum ISO value for analogue gain (no digital gain supported) */\n> static constexpr uint32_t kMinISO = 100;\n> +/* Maximum ISO value for analogue gain (no digital gain supported) */\n> static constexpr uint32_t kMaxISO = 1500;\n\nEven better, drop those (as they're no ISO values) and set kMinGain and\nkMaxGain to 1 and 15 respectively.\n\n> \n> -/* Maximum analogue gain value\n> - * \\todo grab it from a camera helper */\n> +/*\n> + * Minimum analogue gain value\n> + * \\todo grab it from a camera helper\n> + */\n> static constexpr uint32_t kMinGain = kMinISO / 100;\n> +/*\n> + * Maximum analogue gain value\n> + * \\todo grab it from a camera helper\n> + */\n> static constexpr uint32_t kMaxGain = kMaxISO / 100;\n> \n> /* \\todo use calculated value based on sensor */\n> @@ -45,6 +66,7 @@ static constexpr uint32_t kMaxExposure = 1976;\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> /* A cell is 8 bytes and contains averages for RGB values and saturation ratio */\n> @@ -57,8 +79,18 @@ AgcMean::AgcMean()\n> {\n> }\n> \n> +/**\n> + * \\brief Configure the AGC given a configInfo\n> + * \\param[in] context The shared IPA context (\\todo not used yet)\n\nDrop the todo, it's clear from the [[maybe_unused]]\n\n> + * \\param[in] configInfo The IPA configuration data, received from the pipeline\n> + * handler\n\nFrom the point of view of the algorithm it's irrelevant where the\nconfiguration data comes from, you can drop the second part of the\nsentence.\n\n> + *\n> + * \\return 0\n> + */\n> int AgcMean::configure([[maybe_unused]] IPAContext &context,\n> -\t\t const IPAConfigInfo &configInfo){\n> +\t\t const IPAConfigInfo &configInfo)\n> +{\n> +\t/* \\todo use the configInfo fields and IPAContext to store the limits */\n> \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> \t\t / configInfo.sensorInfo.pixelRate;\n> \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> @@ -66,9 +98,22 @@ int AgcMean::configure([[maybe_unused]] IPAContext &context,\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Estimate the mean quantile of the top 2% of the histogram\n> + * \\param[in] stats The statistics buffer coming from the pipeline handler\n\nSame here, where it comes from isn't relevant, you can write \"The\nstatistics computed by the ImgU\" for instance.\n\n> + * \\param[in] grid The grid used to store the statistics in the IPU3\n> + */\n> void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid)\n> {\n> +\t/*\n> +\t * Get the applied grid from the statistics buffer. When the kernel\n> +\t * receives a grid from the parameters buffer, it will check and align\n> +\t * all the values. For instance, it will automatically fill the x_end\n> +\t * value based on x_start, grid width and log2 width.\n> +\t * \\todo Use the grid calculated in configure as there is a bug in IPU3\n> +\t * causing the width (maybe height) to be bit-shifted.\n> +\t */\n> \tconst struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;\n> \tRectangle aeRegion = { statsAeGrid.x_start,\n> \t\t\t statsAeGrid.y_start,\n> @@ -85,6 +130,7 @@ void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \tuint32_t i, j;\n> \tuint32_t count = 0;\n> \n> +\t/* Initialise the histogram array */\n> \tuint32_t hist[knumHistogramBins] = { 0 };\n> \tfor (j = topleftY;\n> \t j < topleftY + (aeRegion.size().height >> grid.block_height_log2);\n> @@ -92,12 +138,18 @@ void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\tfor (i = startX + startY; i < endX + startY; i += kCellSize) {\n> \t\t\t/*\n> \t\t\t * The grid width (and maybe height) is not reliable.\n> -\t\t\t * We observed a bit shift which makes the value 160 to be 32 in the stats grid.\n> -\t\t\t * Use the one passed at init time.\n> +\t\t\t * We observed a bit shift which makes the value 160 to\n> +\t\t\t * be 32 in the stats grid. Use the one from configure.\n> \t\t\t */\n> \t\t\tif (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {\n> \t\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];\n> \t\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];\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> +\t\t\t\t * taken into account.\n> +\t\t\t\t * \\todo remove count which is not used.\n> +\t\t\t\t */\n> \t\t\t\thist[(Gr + Gb) / 2]++;\n> \t\t\t\tcount++;\n> \t\t\t}\n> @@ -108,11 +160,14 @@ void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,\n\nWhile at it, there's a comment here that states\n\n\t/* Estimate the quantile mean of the top 2% of the histogram */\n\nI think that's misleading, shouldn't it be\n\n\t/* Estimate the mean of the top 2% of the histogram */\n\n?\n\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 AgcMean::filterExposure()\n> {\n> \tdouble speed = 0.2;\n> \tif (prevExposure_ == 0s) {\n> -\t\t/* DG stands for digital gain.*/\n> +\t\t/* DG stands for digital gain, which is always 1.0 for now. */\n> \t\tprevExposure_ = currentExposure_;\n> \t\tprevExposureNoDg_ = currentExposureNoDg_;\n> \t} else {\n> @@ -134,6 +189,7 @@ void AgcMean::filterExposure()\n> \t * We can't let the no_dg exposure deviate too far below the\n> \t * total exposure, as there might not be enough digital gain available\n> \t * in the ISP to hide it (which will cause nasty oscillation).\n> +\t * \\todo add the digital gain usage\n> \t */\n> \tdouble fastReduceThreshold = 0.4;\n> \tif (prevExposureNoDg_ <\n> @@ -142,6 +198,11 @@ void AgcMean::filterExposure()\n> \tLOG(IPU3AgcMean, Debug) << \"After filtering, total_exposure \" << prevExposure_;\n> }\n> \n> +/**\n> + * \\brief Estimate the new exposure and gain values\n> + * \\param[in] exposure The exposure value reference as a number of lines\n> + * \\param[in] gain The gain reference to be updated\n> + */\n> void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)\n> {\n> \t/* Algorithm initialization should wait for first valid frames */\n> @@ -154,15 +215,20 @@ void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)\n> \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> \t\tLOG(IPU3AgcMean, Debug) << \"!!! Good exposure with iqMean = \" << iqMean_;\n> \t} else {\n> +\t\t/* Estimate the gain needed to have the proportion wanted */\n> \t\tdouble newGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> \t\t/* extracted from Rpi::Agc::computeTargetExposure */\n> +\t\t/* Calculate the shutter time in seconds */\n> \t\tlibcamera::utils::Duration currentShutter = exposure * lineDuration_;\n> +\t\t/* Ev = shutter_time * gain */\n> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n> \t\tLOG(IPU3AgcMean, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> \t\t\t\t << \" Shutter speed \" << currentShutter\n> \t\t\t\t << \" Gain \" << gain;\n> +\t\t/* Apply the gain calculated to the current exposure value */\n> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n> +\t\t/* Clamp the exposure value to the min and max authorized */\n> \t\tlibcamera::utils::Duration maxTotalExposure = maxExposureTime_ * kMaxGain;\n> \t\tcurrentExposure_ = std::min(currentExposure_, maxTotalExposure);\n> \t\tLOG(IPU3AgcMean, Debug) << \"Target total exposure \" << currentExposure_;\n> @@ -170,6 +236,7 @@ void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)\n> \t\t/* \\todo: estimate if we need to desaturate */\n> \t\tfilterExposure();\n> \n> +\t\t/* Divide the exposure value as new exposure and gain values */\n> \t\tlibcamera::utils::Duration newExposure = 0.0s;\n> \t\tif (currentShutter < maxExposureTime_) {\n> \t\t\texposure = std::clamp(static_cast(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);\n> @@ -185,11 +252,19 @@ void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)\n> \tlastFrame_ = frameCount_;\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> void AgcMean::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 &gain = context.frameContext.agc.gain;\n> +\t/* Calculate the current brightness */\n> \tprocessBrightness(stats, context.configuration.grid.bdsGrid);\n> +\t/* Update the exposure and gain values */\n> \tlockExposureGain(exposure, gain);\n> \tframeCount_++;\n> }\n> diff --git a/src/ipa/ipu3/algorithms/agc_mean.h b/src/ipa/ipu3/algorithms/agc_mean.h\n> index 97114121..6232597d 100644\n> --- a/src/ipa/ipu3/algorithms/agc_mean.h\n> +++ b/src/ipa/ipu3/algorithms/agc_mean.h\n> @@ -2,7 +2,7 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * agc_mean.h - IPU3 AGC/AEC control algorithm\n> + * agc_mean.h - AGC/AEC mean-based control algorithm\n\nThis should go to patch 2/4 too.\n\n> */\n> #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__\n> #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__","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 A4C98BD87D\n\tfor ;\n\tMon, 30 Aug 2021 23:44:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2DB66916C;\n\tTue, 31 Aug 2021 01:44:00 +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 B629A68891\n\tfor ;\n\tTue, 31 Aug 2021 01:43:57 +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 355495A7;\n\tTue, 31 Aug 2021 01:43:57 +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=\"WRgFykoD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630367037;\n\tbh=/RIU1c/laD6DuSF3miwsaSzsZ/dlYiiy1PwUA9vTVAA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WRgFykoD9jv5Rqq4D0DCLTXhDcmpZYjaAX5r18QryhZxhasAU8Zt41KXHIa4qmGdz\n\tBaUm37gO//Dt4/hIauRE6vo9CLnb6tGhFMK+PXMoCkA6KqN4S9dIWux84yEOk6VObc\n\trVyHaNh1PJU+OMKvH5Muekl6V7FDPx9/LX/UKVIo=","Date":"Tue, 31 Aug 2021 02:43:43 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210827080227.26370-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210827080227.26370-4-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210827080227.26370-4-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: ipu3: 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\" "}}]