[{"id":19665,"web_url":"https://patchwork.libcamera.org/comment/19665/","msgid":"","date":"2021-09-14T05:21:44","subject":"Re: [libcamera-devel] [PATCH 09/11] 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 Mon, Sep 13, 2021 at 04:58:08PM +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> Signed-off-by: Jean-Michel Hautbois \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 116 +++++++++++++++++++++++++++-----\n> src/ipa/ipu3/algorithms/agc.h | 2 +-\n> 2 files changed, 101 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 2ef998b7..9a55bb72 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,27 +17,49 @@\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 a gain so that the average value\n\ns/a gain/an analogue gain/\n\n> + * of the green channel of the brightest 2% of pixels approaches 0.5. The AWB\n> + * gains are not used here, and all cells in the grid have the same weight, like\n> + * an average-metering case. In this metering mode, the camera uses light\n> + * 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> -/* Maximum ISO value for analogue gain */\n> -static constexpr uint32_t kMinISO = 100;\n> -static constexpr uint32_t kMaxISO = 1500;\n> -\n> -/* Maximum analogue gain value\n> - * \\todo grab it from a camera helper */\n> -static constexpr uint32_t kMinGain = kMinISO / 100;\n> -static constexpr uint32_t kMaxGain = kMaxISO / 100;\n> +/*\n> + * Minimum analogue gain value\n> + * \\todo grab it from a camera helper\n> + */\n> +static constexpr uint32_t kMinGain = 1;\n> +/*\n> + * Maximum analogue gain value\n> + * \\todo grab it from a camera helper\n> + */\n> +static constexpr uint32_t kMaxGain = 15;\n\nI was about to just comment that these two values should be\nfloating-point numbers, but then wondered how and why they were used as\nintegers in the code. I recommend trying the same exercise, it's\n\"interesting\" (and a bug fix is clearly needed).\n\n> /* \\todo use calculated value based on sensor */\n> static constexpr uint32_t kMinExposure = 1;\n> @@ -45,6 +67,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,9 +80,17 @@ 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([[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\nDid you mean s/store/provide/ ?\n\n> \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> \t\t / configInfo.sensorInfo.pixelRate;\n> \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> @@ -67,9 +98,22 @@ int Agc::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\ns/mean quantile/inter-quantile mean/\n\nIt's the mean value of samples between two given quantiles. But you\ncould equally say \"Estimate the mean value of the top 2% of the\nhistogram\", which would be easier to understand.\n\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::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid)\n\nMaybe the function should also be renamed accordingly ?\ncalculateBrightness() would already be a bit more descriptive, or\nperhaps measureBrightness(). calculateTop2PercentBrightness() seems a\nbit overkill.\n\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\nDon't we do so already ?\n\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 +129,7 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> \tuint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;\n> \tuint32_t i, j;\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,13 +137,18 @@ void Agc::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\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + j * grid.width];\n> \t\t\tif (currentCell->sat_ratio == 0) {\n> \t\t\t\tuint8_t Gr = currentCell->Gr_avg;\n> \t\t\t\tuint8_t Gb = currentCell->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> +\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> @@ -108,18 +158,24 @@ 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> \tif (prevExposure_ == 0s) {\n> -\t\t/* DG stands for digital gain.*/\n> +\t\t/*\n> +\t\t * DG stands for digital gain, which is always 1.0 for now as it\n> +\t\t * is not implemented right now.\n> +\t\t */\n> \t\tprevExposure_ = currentExposure_;\n> \t\tprevExposureNoDg_ = currentExposureNoDg_;\n> \t} else {\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 (prevExposure_ < 1.2 * currentExposure_ &&\n> \t\t prevExposure_ > 0.8 * currentExposure_)\n> @@ -130,10 +186,12 @@ void Agc::filterExposure()\n> \t\tprevExposureNoDg_ = speed * currentExposureNoDg_ +\n> \t\t\t\tprevExposureNoDg_ * (1.0 - speed);\n> \t}\n> +\n> \t/*\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 implement digital gain setting\n\ns/implement/Implement/\n\n> \t */\n> \tdouble fastReduceThreshold = 0.4;\n> \tif (prevExposureNoDg_ <\n> @@ -142,6 +200,11 @@ void Agc::filterExposure()\n> \tLOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << prevExposure_;\n> }\n> \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::lockExposureGain(uint32_t &exposure, double &gain)\n\nHere too a better name for the function would be nice.\n\n> {\n> \t/* Algorithm initialization should wait for first valid frames */\n> @@ -154,22 +217,33 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> \t\tLOG(IPU3Agc, 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> +\n> +\t\t/*\n> +\t\t * Estimate the current exposure value for the scene as shutter\n> +\t\t * time multiplicated by the analogue gain.\n\ns/multiplicated/multiplied/\n\n> +\t\t */\n> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> \t\t\t\t << \" Shutter speed \" << currentShutter\n> \t\t\t\t << \" Gain \" << gain;\n> +\n> +\t\t/* Apply the gain calculated to the current exposure value */\n> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n> +\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(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n> \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,10 +259,20 @@ void Agc::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> + * 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 &gain = context.frameContext.agc.gain;\n> +\n> \tprocessBrightness(stats, context.configuration.grid.bdsGrid);\n> \tlockExposureGain(exposure, gain);\n\nWhile at it I would have written\n\n \tlockExposureGain(context.frameContext.agc.exposure,\n\t\t\t context.frameContext.agc.gain);\n\nI'm not a big fan of passing those by reference, maybe passing a pointer\nto the context could be better, but that's for a separate patch.\n\n> \tframeCount_++;\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index e36797d3..9449ba48 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__","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 E380ABDB1D\n\tfor ;\n\tTue, 14 Sep 2021 05:22:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C6A769187;\n\tTue, 14 Sep 2021 07:22:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 592B66916F\n\tfor ;\n\tTue, 14 Sep 2021 07:22:09 +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 C3F992A5;\n\tTue, 14 Sep 2021 07:22:08 +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=\"IHuX4Hy8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631596929;\n\tbh=kAPvg1yIi8mT2Bcs2brNB1RfrjwBofkrzq6ci294TtM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IHuX4Hy8JFi9e7iMYNCtNtluqV9OSlYJwaX3yKWtskl2F/TdiVqpabXEK0eIaXSdM\n\tyAaBBxbrUBjX7eN/EtIqWhRDSBjChd85ATisLNWjsYjVTvL8hHZKh/oSaOLCC/e6DV\n\tQ2WCoafmU6o27ZtwFPUTxy622jRBO98p0u2ew8us=","Date":"Tue, 14 Sep 2021 08:21:44 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-10-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210913145810.66515-10-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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":19679,"web_url":"https://patchwork.libcamera.org/comment/19679/","msgid":"<1d6d21dd-7ea1-e6e9-830e-ab5c34de3708@ideasonboard.com>","date":"2021-09-14T11:20:14","subject":"Re: [libcamera-devel] [PATCH 09/11] 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":"On 14/09/2021 06:21, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Mon, Sep 13, 2021 at 04:58:08PM +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>> Signed-off-by: Jean-Michel Hautbois \n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 116 +++++++++++++++++++++++++++-----\n>> src/ipa/ipu3/algorithms/agc.h | 2 +-\n>> 2 files changed, 101 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 2ef998b7..9a55bb72 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,27 +17,49 @@\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 a gain so that the average value\n> \n> s/a gain/an analogue gain/\n\nSurely the algorithm itself doesn't care if this is an analogue gain or\na digital gain.\n\nThe IPU3 will apply it as an analogue gain - but that's independent\nisn't it ?\n\nPresumably if we weren't able to apply it fully as analogue - the\nremainder could be applied digitally to meet the desired gain requested\nby the algorithm?\n\n\n\n>> + * of the green channel of the brightest 2% of pixels approaches 0.5. The AWB\n>> + * gains are not used here, and all cells in the grid have the same weight, like\n>> + * an average-metering case. In this metering mode, the camera uses light\n>> + * 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>> -/* Maximum ISO value for analogue gain */\n>> -static constexpr uint32_t kMinISO = 100;\n>> -static constexpr uint32_t kMaxISO = 1500;\n>> -\n>> -/* Maximum analogue gain value\n>> - * \\todo grab it from a camera helper */\n>> -static constexpr uint32_t kMinGain = kMinISO / 100;\n>> -static constexpr uint32_t kMaxGain = kMaxISO / 100;\n>> +/*\n>> + * Minimum analogue gain value\n>> + * \\todo grab it from a camera helper\n>> + */\n>> +static constexpr uint32_t kMinGain = 1;\n>> +/*\n>> + * Maximum analogue gain value\n>> + * \\todo grab it from a camera helper\n>> + */\n>> +static constexpr uint32_t kMaxGain = 15;\n> \n> I was about to just comment that these two values should be\n> floating-point numbers, but then wondered how and why they were used as\n> integers in the code. I recommend trying the same exercise, it's\n> \"interesting\" (and a bug fix is clearly needed).\n> \n>> /* \\todo use calculated value based on sensor */\n>> static constexpr uint32_t kMinExposure = 1;\n>> @@ -45,6 +67,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,9 +80,17 @@ 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([[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> \n> Did you mean s/store/provide/ ?\n> \n>> \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n>> \t\t / configInfo.sensorInfo.pixelRate;\n>> \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n>> @@ -67,9 +98,22 @@ int Agc::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> \n> s/mean quantile/inter-quantile mean/\n> \n> It's the mean value of samples between two given quantiles. But you\n> could equally say \"Estimate the mean value of the top 2% of the\n> histogram\", which would be easier to understand.\n> \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::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t const ipu3_uapi_grid_config &grid)\n> \n> Maybe the function should also be renamed accordingly ?\n> calculateBrightness() would already be a bit more descriptive, or\n> perhaps measureBrightness(). calculateTop2PercentBrightness() seems a\n> bit overkill.\n> \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> \n> Don't we do so already ?\n\nIs this relating to the line directly below which references the grid\nfrom the stats->stats_4a_config.awb_config.grid?\n\nCan we simply/directly reference our grid from our IPAContext?\n\nAnd if so - it might be better to do that immediately, rather than add\nthe \\todo.\n\nAnd if we do that we should mention that it is referenced from our grid,\nrather than the one provided in the statistics due to a kernel bug..\n\n\n> \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 +129,7 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n>> \tuint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;\n>> \tuint32_t i, j;\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,13 +137,18 @@ void Agc::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\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + j * grid.width];\n>> \t\t\tif (currentCell->sat_ratio == 0) {\n>> \t\t\t\tuint8_t Gr = currentCell->Gr_avg;\n>> \t\t\t\tuint8_t Gb = currentCell->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>> +\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>> @@ -108,18 +158,24 @@ 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>> \tif (prevExposure_ == 0s) {\n>> -\t\t/* DG stands for digital gain.*/\n>> +\t\t/*\n>> +\t\t * DG stands for digital gain, which is always 1.0 for now as it\n>> +\t\t * is not implemented right now.\n>> +\t\t */\n>> \t\tprevExposure_ = currentExposure_;\n>> \t\tprevExposureNoDg_ = currentExposureNoDg_;\n>> \t} else {\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> \n> s/://\n> \n>> \t\t */\n>> \t\tif (prevExposure_ < 1.2 * currentExposure_ &&\n>> \t\t prevExposure_ > 0.8 * currentExposure_)\n>> @@ -130,10 +186,12 @@ void Agc::filterExposure()\n>> \t\tprevExposureNoDg_ = speed * currentExposureNoDg_ +\n>> \t\t\t\tprevExposureNoDg_ * (1.0 - speed);\n>> \t}\n>> +\n>> \t/*\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 implement digital gain setting\n> \n> s/implement/Implement/\n> \n>> \t */\n>> \tdouble fastReduceThreshold = 0.4;\n>> \tif (prevExposureNoDg_ <\n>> @@ -142,6 +200,11 @@ void Agc::filterExposure()\n>> \tLOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << prevExposure_;\n>> }\n>> \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::lockExposureGain(uint32_t &exposure, double &gain)\n> \n> Here too a better name for the function would be nice.\n> \n>> {\n>> \t/* Algorithm initialization should wait for first valid frames */\n>> @@ -154,22 +217,33 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)\n>> \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n>> \t\tLOG(IPU3Agc, 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>> +\n>> +\t\t/*\n>> +\t\t * Estimate the current exposure value for the scene as shutter\n>> +\t\t * time multiplicated by the analogue gain.\n> \n> s/multiplicated/multiplied/\n> \n>> +\t\t */\n>> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n>> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n>> \t\t\t\t << \" Shutter speed \" << currentShutter\n>> \t\t\t\t << \" Gain \" << gain;\n>> +\n>> +\t\t/* Apply the gain calculated to the current exposure value */\n>> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n>> +\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(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n>> \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,10 +259,20 @@ void Agc::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>> + * 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 &gain = context.frameContext.agc.gain;\n>> +\n>> \tprocessBrightness(stats, context.configuration.grid.bdsGrid);\n>> \tlockExposureGain(exposure, gain);\n> \n> While at it I would have written\n> \n> \tlockExposureGain(context.frameContext.agc.exposure,\n> \t\t\t context.frameContext.agc.gain);\n> \n> I'm not a big fan of passing those by reference, maybe passing a pointer\n> to the context could be better, but that's for a separate patch.>\n>> \tframeCount_++;\n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index e36797d3..9449ba48 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>","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 A40F3BDC71\n\tfor ;\n\tTue, 14 Sep 2021 11:20:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98FC96918B;\n\tTue, 14 Sep 2021 13:20: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 BE43160248\n\tfor ;\n\tTue, 14 Sep 2021 13:20:19 +0200 (CEST)","from [192.168.0.20]\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 4DED72A5;\n\tTue, 14 Sep 2021 13:20:19 +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=\"sBBS2HPn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631618419;\n\tbh=mhH8JrDLXXMMGNXeTjE66gcUrXNpM2YUNfjTtoiIfxY=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=sBBS2HPnMnpXu89haq2jvS+V0fRqnlvX+phHJygD3Jwhp62oGNsV3FFhA9qOHMTyB\n\tY8NmwazxaQCimwZSr6N6n0K3Gp0On+XiD4rLmYwJc9oyRUzor3ZU6YqjuASK8gUFmk\n\t5iebxQq1UfMJVk7G08yTRvjEL8qhXkp1SrG2etLk=","To":"Laurent Pinchart ,\n\tJean-Michel Hautbois ","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-10-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Kieran Bingham ","Message-ID":"<1d6d21dd-7ea1-e6e9-830e-ab5c34de3708@ideasonboard.com>","Date":"Tue, 14 Sep 2021 12:20:14 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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":19680,"web_url":"https://patchwork.libcamera.org/comment/19680/","msgid":"","date":"2021-09-14T16:18:47","subject":"Re: [libcamera-devel] [PATCH 09/11] 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 Kieran,\n\nOn Tue, Sep 14, 2021 at 12:20:14PM +0100, Kieran Bingham wrote:\n> On 14/09/2021 06:21, Laurent Pinchart wrote:\n> > On Mon, Sep 13, 2021 at 04:58:08PM +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> >> Signed-off-by: Jean-Michel Hautbois \n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/ipa/ipu3/algorithms/agc.cpp | 116 +++++++++++++++++++++++++++-----\n> >> src/ipa/ipu3/algorithms/agc.h | 2 +-\n> >> 2 files changed, 101 insertions(+), 17 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> >> index 2ef998b7..9a55bb72 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,27 +17,49 @@\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 a gain so that the average value\n> > \n> > s/a gain/an analogue gain/\n> \n> Surely the algorithm itself doesn't care if this is an analogue gain or\n> a digital gain.\n\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\n> The IPU3 will apply it as an analogue gain - but that's independent\n> isn't it ?\n\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\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\nThat's usually what a basic implementation will do, there could be\nreason to do something more complex.\n\n> >> + * of the green channel of the brightest 2% of pixels approaches 0.5. The AWB\n> >> + * gains are not used here, and all cells in the grid have the same weight, like\n> >> + * an average-metering case. In this metering mode, the camera uses light\n> >> + * 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> >> -/* Maximum ISO value for analogue gain */\n> >> -static constexpr uint32_t kMinISO = 100;\n> >> -static constexpr uint32_t kMaxISO = 1500;\n> >> -\n> >> -/* Maximum analogue gain value\n> >> - * \\todo grab it from a camera helper */\n> >> -static constexpr uint32_t kMinGain = kMinISO / 100;\n> >> -static constexpr uint32_t kMaxGain = kMaxISO / 100;\n> >> +/*\n> >> + * Minimum analogue gain value\n> >> + * \\todo grab it from a camera helper\n> >> + */\n> >> +static constexpr uint32_t kMinGain = 1;\n> >> +/*\n> >> + * Maximum analogue gain value\n> >> + * \\todo grab it from a camera helper\n> >> + */\n> >> +static constexpr uint32_t kMaxGain = 15;\n> > \n> > I was about to just comment that these two values should be\n> > floating-point numbers, but then wondered how and why they were used as\n> > integers in the code. I recommend trying the same exercise, it's\n> > \"interesting\" (and a bug fix is clearly needed).\n> > \n> >> /* \\todo use calculated value based on sensor */\n> >> static constexpr uint32_t kMinExposure = 1;\n> >> @@ -45,6 +67,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,9 +80,17 @@ 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([[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> > \n> > Did you mean s/store/provide/ ?\n> > \n> >> \tlineDuration_ = configInfo.sensorInfo.lineLength * 1.0s\n> >> \t\t / configInfo.sensorInfo.pixelRate;\n> >> \tmaxExposureTime_ = kMaxExposure * lineDuration_;\n> >> @@ -67,9 +98,22 @@ int Agc::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> > \n> > s/mean quantile/inter-quantile mean/\n> > \n> > It's the mean value of samples between two given quantiles. But you\n> > could equally say \"Estimate the mean value of the top 2% of the\n> > histogram\", which would be easier to understand.\n> > \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::processBrightness(const ipu3_uapi_stats_3a *stats,\n> >> \t\t\t const ipu3_uapi_grid_config &grid)\n> > \n> > Maybe the function should also be renamed accordingly ?\n> > calculateBrightness() would already be a bit more descriptive, or\n> > perhaps measureBrightness(). calculateTop2PercentBrightness() seems a\n> > bit overkill.\n> > \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> > \n> > Don't we do so already ?\n> \n> Is this relating to the line directly below which references the grid\n> from the stats->stats_4a_config.awb_config.grid?\n> \n> Can we simply/directly reference our grid from our IPAContext?\n> \n> And if so - it might be better to do that immediately, rather than add\n> the \\todo.\n> \n> And if we do that we should mention that it is referenced from our grid,\n> rather than the one provided in the statistics due to a kernel bug..\n\nI meant that we use grid.width, with the following comment:\n\n * We observed a bit shift which makes the value 160 to\n * be 32 in the stats grid. Use the one from configure.\n\nIsn't this what the \\todo is about ? Or did I misundertand either the\ncode of the \\todo ?\n\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 +129,7 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,\n> >> \tuint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;\n> >> \tuint32_t i, j;\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,13 +137,18 @@ void Agc::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\tconst ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + j * grid.width];\n> >> \t\t\tif (currentCell->sat_ratio == 0) {\n> >> \t\t\t\tuint8_t Gr = currentCell->Gr_avg;\n> >> \t\t\t\tuint8_t Gb = currentCell->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> >> +\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> >> @@ -108,18 +158,24 @@ 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> >> \tif (prevExposure_ == 0s) {\n> >> -\t\t/* DG stands for digital gain.*/\n> >> +\t\t/*\n> >> +\t\t * DG stands for digital gain, which is always 1.0 for now as it\n> >> +\t\t * is not implemented right now.\n> >> +\t\t */\n> >> \t\tprevExposure_ = currentExposure_;\n> >> \t\tprevExposureNoDg_ = currentExposureNoDg_;\n> >> \t} else {\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> > \n> > s/://\n> > \n> >> \t\t */\n> >> \t\tif (prevExposure_ < 1.2 * currentExposure_ &&\n> >> \t\t prevExposure_ > 0.8 * currentExposure_)\n> >> @@ -130,10 +186,12 @@ void Agc::filterExposure()\n> >> \t\tprevExposureNoDg_ = speed * currentExposureNoDg_ +\n> >> \t\t\t\tprevExposureNoDg_ * (1.0 - speed);\n> >> \t}\n> >> +\n> >> \t/*\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 implement digital gain setting\n> > \n> > s/implement/Implement/\n> > \n> >> \t */\n> >> \tdouble fastReduceThreshold = 0.4;\n> >> \tif (prevExposureNoDg_ <\n> >> @@ -142,6 +200,11 @@ void Agc::filterExposure()\n> >> \tLOG(IPU3Agc, Debug) << \"After filtering, total_exposure \" << prevExposure_;\n> >> }\n> >> \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::lockExposureGain(uint32_t &exposure, double &gain)\n> > \n> > Here too a better name for the function would be nice.\n> > \n> >> {\n> >> \t/* Algorithm initialization should wait for first valid frames */\n> >> @@ -154,22 +217,33 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> >> \tif (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {\n> >> \t\tLOG(IPU3Agc, 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> >> +\n> >> +\t\t/*\n> >> +\t\t * Estimate the current exposure value for the scene as shutter\n> >> +\t\t * time multiplicated by the analogue gain.\n> > \n> > s/multiplicated/multiplied/\n> > \n> >> +\t\t */\n> >> \t\tcurrentExposureNoDg_ = currentShutter * gain;\n> >> \t\tLOG(IPU3Agc, Debug) << \"Actual total exposure \" << currentExposureNoDg_\n> >> \t\t\t\t << \" Shutter speed \" << currentShutter\n> >> \t\t\t\t << \" Gain \" << gain;\n> >> +\n> >> +\t\t/* Apply the gain calculated to the current exposure value */\n> >> \t\tcurrentExposure_ = currentExposureNoDg_ * newGain;\n> >> +\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(IPU3Agc, Debug) << \"Target total exposure \" << currentExposure_;\n> >> \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,10 +259,20 @@ void Agc::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> >> + * 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 &gain = context.frameContext.agc.gain;\n> >> +\n> >> \tprocessBrightness(stats, context.configuration.grid.bdsGrid);\n> >> \tlockExposureGain(exposure, gain);\n> > \n> > While at it I would have written\n> > \n> > \tlockExposureGain(context.frameContext.agc.exposure,\n> > \t\t\t context.frameContext.agc.gain);\n> > \n> > I'm not a big fan of passing those by reference, maybe passing a pointer\n> > to the context could be better, but that's for a separate patch.>\n> >> \tframeCount_++;\n> >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> >> index e36797d3..9449ba48 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__","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 C5360BDB1D\n\tfor ;\n\tTue, 14 Sep 2021 16:19:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D4536024A;\n\tTue, 14 Sep 2021 18:19:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A59760132\n\tfor ;\n\tTue, 14 Sep 2021 18:19:12 +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 BA155499;\n\tTue, 14 Sep 2021 18:19:11 +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=\"sYEaYBnt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631636351;\n\tbh=3D016FmNTc4UJw7e+oeLT3f5bZ50lMelccWhT84JklI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sYEaYBnt3QZKsYM+oyiFzj/l2/EhHEIymxLlBmPHT/flYCbLlcof4gHNdKz3I2Am3\n\tSi+fW68QZw5SFYlIA8TQ5ms9CaYxmunV4Kslrpy6aejXId9hk2lN1jwLlaNhZEice6\n\tei8XcelbkDvfZRPgQRxX0O/FJD3/REOKZGuCSkW8=","Date":"Tue, 14 Sep 2021 19:18:47 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-10-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<1d6d21dd-7ea1-e6e9-830e-ab5c34de3708@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1d6d21dd-7ea1-e6e9-830e-ab5c34de3708@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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":20380,"web_url":"https://patchwork.libcamera.org/comment/20380/","msgid":"<163489823652.4111934.14381155620880818805@Monstersaurus>","date":"2021-10-22T10:23:56","subject":"Re: [libcamera-devel] [PATCH 09/11] 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":"Hi,\n\nSeems I'm replying to quite an old mail, so I'm not sure if this is\nalready done/handled/relevant, but in case it is...\n\nQuoting Laurent Pinchart (2021-09-14 17:18:47)\n> Hi Kieran,\n> \n> On Tue, Sep 14, 2021 at 12:20:14PM +0100, Kieran Bingham wrote:\n> > On 14/09/2021 06:21, Laurent Pinchart wrote:\n> > >> {\n> > >> + /*\n> > >> + * Get the applied grid from the statistics buffer. When the kernel\n> > >> + * receives a grid from the parameters buffer, it will check and align\n> > >> + * all the values. For instance, it will automatically fill the x_end\n> > >> + * value based on x_start, grid width and log2 width.\n> > >> + * \\todo Use the grid calculated in configure as there is a bug in IPU3\n> > >> + * causing the width (maybe height) to be bit-shifted.\n> > > \n> > > Don't we do so already ?\n> > \n> > Is this relating to the line directly below which references the grid\n> > from the stats->stats_4a_config.awb_config.grid?\n> > \n> > Can we simply/directly reference our grid from our IPAContext?\n> > \n> > And if so - it might be better to do that immediately, rather than add\n> > the \\todo.\n> > \n> > And if we do that we should mention that it is referenced from our grid,\n> > rather than the one provided in the statistics due to a kernel bug..\n> \n> I meant that we use grid.width, with the following comment:\n> \n> * We observed a bit shift which makes the value 160 to\n> * be 32 in the stats grid. Use the one from configure.\n> \n> Isn't this what the \\todo is about ? Or did I misundertand either the\n> code of the \\todo ?\n\nMy statement was that the todo being added sounds so simple - it would\nbe better to add a patch to *do* the thing, rather than add a todo.\n\nIf you believe the action of the todo is already done, then that's even\nbetter.\n\nEither way, I don't think this should be added as a todo - and it should\nbe ... done ;-), and perhaps it already is. If it isn't then can we do\nit please? It sounds like a very short patch if it isn't done already.\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 4B68FBF415\n\tfor ;\n\tFri, 22 Oct 2021 10:24:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9952268F59;\n\tFri, 22 Oct 2021 12:24: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 50C846012A\n\tfor ;\n\tFri, 22 Oct 2021 12:23:59 +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 D998851D;\n\tFri, 22 Oct 2021 12:23:58 +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=\"W2xgABZr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634898238;\n\tbh=VvW13UkXn8bkfqOqSyOL4xHP8tEm1bHo/W0mLTXy7mQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=W2xgABZrKmooeaExdBjiVEd07DKzjyEqceA22hxFUrQNZlbkvD666z9a60oUjK1Jt\n\tn6AfV6uAo4YAFJSEK2R+cILdKwH4nSbP0I/qsu8vqVLIXg26m/UM59V9fIKtlZr1T/\n\tK9luiRfItjkRbymJlha+UqLsIhlP/4/EBdbgTLGw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-10-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<1d6d21dd-7ea1-e6e9-830e-ab5c34de3708@ideasonboard.com>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ","Date":"Fri, 22 Oct 2021 11:23:56 +0100","Message-ID":"<163489823652.4111934.14381155620880818805@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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":20386,"web_url":"https://patchwork.libcamera.org/comment/20386/","msgid":"","date":"2021-10-22T13:15:29","subject":"Re: [libcamera-devel] [PATCH 09/11] 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":"On Fri, Oct 22, 2021 at 11:23:56AM +0100, Kieran Bingham wrote:\n> Hi,\n> \n> Seems I'm replying to quite an old mail, so I'm not sure if this is\n> already done/handled/relevant, but in case it is...\n> \n> Quoting Laurent Pinchart (2021-09-14 17:18:47)\n> > Hi Kieran,\n> > \n> > On Tue, Sep 14, 2021 at 12:20:14PM +0100, Kieran Bingham wrote:\n> > > On 14/09/2021 06:21, Laurent Pinchart wrote:\n> > > >> {\n> > > >> + /*\n> > > >> + * Get the applied grid from the statistics buffer. When the kernel\n> > > >> + * receives a grid from the parameters buffer, it will check and align\n> > > >> + * all the values. For instance, it will automatically fill the x_end\n> > > >> + * value based on x_start, grid width and log2 width.\n> > > >> + * \\todo Use the grid calculated in configure as there is a bug in IPU3\n> > > >> + * causing the width (maybe height) to be bit-shifted.\n> > > > \n> > > > Don't we do so already ?\n> > > \n> > > Is this relating to the line directly below which references the grid\n> > > from the stats->stats_4a_config.awb_config.grid?\n> > > \n> > > Can we simply/directly reference our grid from our IPAContext?\n> > > \n> > > And if so - it might be better to do that immediately, rather than add\n> > > the \\todo.\n> > > \n> > > And if we do that we should mention that it is referenced from our grid,\n> > > rather than the one provided in the statistics due to a kernel bug..\n> > \n> > I meant that we use grid.width, with the following comment:\n> > \n> > * We observed a bit shift which makes the value 160 to\n> > * be 32 in the stats grid. Use the one from configure.\n> > \n> > Isn't this what the \\todo is about ? Or did I misundertand either the\n> > code of the \\todo ?\n> \n> My statement was that the todo being added sounds so simple - it would\n> be better to add a patch to *do* the thing, rather than add a todo.\n> \n> If you believe the action of the todo is already done, then that's even\n> better.\n> \n> Either way, I don't think this should be added as a todo - and it should\n> be ... done ;-), and perhaps it already is. If it isn't then can we do\n> it please? It sounds like a very short patch if it isn't done already.\n\nI understand it as being done already, but I could be wrong.","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 A10B8BF415\n\tfor ;\n\tFri, 22 Oct 2021 13:15:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E81768F5B;\n\tFri, 22 Oct 2021 15:15:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A8916023B\n\tfor ;\n\tFri, 22 Oct 2021 15:15:51 +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 963DE51D;\n\tFri, 22 Oct 2021 15:15:50 +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=\"SuzmtQBG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634908550;\n\tbh=MPr3L8pzV0TC7qSaJeK9Q5TIao4Eecj0CWUcr7nAEQ4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SuzmtQBGEIvj2zuDbbPssj2+g7ypMaDS/7cKe4BFQVbkJ7PdHhHxA6Aj4G4S8kYlg\n\tHmCqwbIQ52JVGaudb0+q/9Xn/rZxipROBOFdCmSe1M26P1E5FoBBmCKjQBIsAhzIoy\n\tbzwGphVJe0eTXdyuwEsIO8KJ28lF9RBvgXiSly7A=","Date":"Fri, 22 Oct 2021 16:15:29 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-10-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<1d6d21dd-7ea1-e6e9-830e-ab5c34de3708@ideasonboard.com>\n\t\n\t<163489823652.4111934.14381155620880818805@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163489823652.4111934.14381155620880818805@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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":20388,"web_url":"https://patchwork.libcamera.org/comment/20388/","msgid":"<20ba636f-5f4b-98be-7e03-497f252dbefa@ideasonboard.com>","date":"2021-10-22T13:19:08","subject":"Re: [libcamera-devel] [PATCH 09/11] 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 22/10/2021 15:15, Laurent Pinchart wrote:\n> On Fri, Oct 22, 2021 at 11:23:56AM +0100, Kieran Bingham wrote:\n>> Hi,\n>>\n>> Seems I'm replying to quite an old mail, so I'm not sure if this is\n>> already done/handled/relevant, but in case it is...\n>>\n>> Quoting Laurent Pinchart (2021-09-14 17:18:47)\n>>> Hi Kieran,\n>>>\n>>> On Tue, Sep 14, 2021 at 12:20:14PM +0100, Kieran Bingham wrote:\n>>>> On 14/09/2021 06:21, Laurent Pinchart wrote:\n>>>>>> {\n>>>>>> + /*\n>>>>>> + * Get the applied grid from the statistics buffer. When the kernel\n>>>>>> + * receives a grid from the parameters buffer, it will check and align\n>>>>>> + * all the values. For instance, it will automatically fill the x_end\n>>>>>> + * value based on x_start, grid width and log2 width.\n>>>>>> + * \\todo Use the grid calculated in configure as there is a bug in IPU3\n>>>>>> + * causing the width (maybe height) to be bit-shifted.\n>>>>>\n>>>>> Don't we do so already ?\n>>>>\n>>>> Is this relating to the line directly below which references the grid\n>>>> from the stats->stats_4a_config.awb_config.grid?\n>>>>\n>>>> Can we simply/directly reference our grid from our IPAContext?\n>>>>\n>>>> And if so - it might be better to do that immediately, rather than add\n>>>> the \\todo.\n>>>>\n>>>> And if we do that we should mention that it is referenced from our grid,\n>>>> rather than the one provided in the statistics due to a kernel bug..\n>>>\n>>> I meant that we use grid.width, with the following comment:\n>>>\n>>> * We observed a bit shift which makes the value 160 to\n>>> * be 32 in the stats grid. Use the one from configure.\n>>>\n>>> Isn't this what the \\todo is about ? Or did I misundertand either the\n>>> code of the \\todo ?\n>>\n>> My statement was that the todo being added sounds so simple - it would\n>> be better to add a patch to *do* the thing, rather than add a todo.\n>>\n>> If you believe the action of the todo is already done, then that's even\n>> better.\n>>\n>> Either way, I don't think this should be added as a todo - and it should\n>> be ... done ;-), and perhaps it already is. If it isn't then can we do\n>> it please? It sounds like a very short patch if it isn't done already.\n> \n> I understand it as being done already, but I could be wrong.\n> \n\nI think it has been done... I don't have that patch in the \"Document \nIPAIPU3\" v3 series ;-)","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 C9968BDB1C\n\tfor ;\n\tFri, 22 Oct 2021 13:19:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86B0368F5B;\n\tFri, 22 Oct 2021 15:19:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7D976012A\n\tfor ;\n\tFri, 22 Oct 2021 15:19:10 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:22cc:3af6:5ccb:8367] (unknown\n\t[IPv6:2a01:e0a:169:7140:22cc:3af6:5ccb:8367])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6104B51D;\n\tFri, 22 Oct 2021 15:19:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PrExD787\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634908750;\n\tbh=dhD/9CodQLmnk8x3LxVfQXQKOmSnDAMwVWBGxJd/BXE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=PrExD787KVBQzVwIylgekQ56+aOV9hwarXnGq8g3MEt4kbeWRKEizjoGbCuzZ0UzB\n\t4xksEcTCfVcUr7NUZxJ/QlbNWtySr4+5kL47gfoAtyxx+6ByKcfX5KDgkM7QMVlfQe\n\tZrMtmyJ8MCPHM3x4lSHwnG5ab1S7iXFdEWcAHVmg=","Message-ID":"<20ba636f-5f4b-98be-7e03-497f252dbefa@ideasonboard.com>","Date":"Fri, 22 Oct 2021 15:19:08 +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":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210913145810.66515-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913145810.66515-10-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<1d6d21dd-7ea1-e6e9-830e-ab5c34de3708@ideasonboard.com>\n\t\n\t<163489823652.4111934.14381155620880818805@Monstersaurus>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 09/11] 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\" "}}]