[{"id":22189,"web_url":"https://patchwork.libcamera.org/comment/22189/","msgid":"","date":"2022-02-23T17:30:45","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: rkisp1: agc: Introduce\n\thistogram calculation","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 Wed, Feb 23, 2022 at 11:48:23AM +0100, Jean-Michel Hautbois wrote:\n> As for the IPU3, we can estimate the histogram of the luminance. The\n> RkISP1 can estimate multiple ones, the R, G and B ones, the Y only one\n> and a combination of RGB. The one we are interested by in AGC is the Y\n> histogram.\n\nWhen reading the commit message (including the subject) I thought you\nwere only calculating a histogram, possibly to be used by AWB in patch\n4/4. Then I realized the main purpose of the patch is to introduce a\nsecond gain calculation method. The commit message should explain this.\n\n> Use the hardware revision to determine the number of bins of the\n> produced histogram, and use it to populate a vector passed down to the\n> libipa::Histogram class.\n\nThe second part will need to be updated, see below.\n\n> As the sensor deGamma and AWB are applied, we also need to get back to\n> the relative luminance value of 0.16, as for the IPU3.\n\nThey're not yet in this patch, and degamma is actually dropped from this\nseries compared to v1. Does AWB result in a need for the relative\nluminance target to go from 0.4 to 0.16 ? That's a big change. I'm not\nsaying the new value is wrong, but I'm not sure we understand the\nreason.\n\n> Signed-off-by: Jean-Michel Hautbois \n> Tested-by: Peter Griffin \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 97 ++++++++++++++++++++++++++-----\n> src/ipa/rkisp1/algorithms/agc.h | 4 +-\n> src/ipa/rkisp1/ipa_context.cpp | 3 +\n> src/ipa/rkisp1/ipa_context.h | 1 +\n> 4 files changed, 89 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 50980835..0311ecf4 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -16,6 +16,8 @@\n> \n> #include \n> \n> +#include \"libipa/histogram.h\"\n> +\n> /**\n> * \\file agc.h\n> */\n> @@ -43,6 +45,9 @@ static constexpr utils::Duration kMaxShutterSpeed = 60ms;\n> /* Number of frames to wait before calculating stats on minimum exposure */\n> static constexpr uint32_t kNumStartupFrames = 10;\n> \n> +/* Target value to reach for the top 2% of the histogram */\n> +static constexpr double kEvGainTarget = 0.5;\n> +\n> /*\n> * Relative luminance target.\n> *\n> @@ -51,10 +56,10 @@ static constexpr uint32_t kNumStartupFrames = 10;\n> *\n> * \\todo Why is the value different between IPU3 and RkISP1 ?\n> */\n> -static constexpr double kRelativeLuminanceTarget = 0.4;\n> +static constexpr double kRelativeLuminanceTarget = 0.16;\n> \n> Agc::Agc()\n> -\t: frameCount_(0), numCells_(0), filteredExposure_(0s)\n> +\t: frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s)\n> {\n> }\n> \n> @@ -65,8 +70,7 @@ Agc::Agc()\n> *\n> * \\return 0\n> */\n> -int Agc::configure(IPAContext &context,\n> -\t\t [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> +int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> {\n> \t/* Configure the default exposure and gain. */\n> \tcontext.frameContext.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);\n> @@ -77,10 +81,19 @@ int Agc::configure(IPAContext &context,\n> \t * - versions < V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,\n> \t * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.\n> \t */\n> -\tif (context.configuration.hw.revision < RKISP1_V12)\n> +\tif (context.configuration.hw.revision < RKISP1_V12) {\n> \t\tnumCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;\n> -\telse\n> +\t\tnumHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;\n> +\t} else {\n> \t\tnumCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;\n> +\t\tnumHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;\n> +\t}\n> +\n> +\t/* Define the measurement window for AGC. */\n\nLet's expand this:\n\n\t/*\n\t * Define the measurement window for AGC as a centered rectangle\n\t * covering 3/4 of the image width and height.\n\t */\n\n(assuming I understand correctly what this does :-))\n\n> +\tcontext.configuration.agc.measureWindow.h_offs = configInfo.outputSize.width / 8;\n> +\tcontext.configuration.agc.measureWindow.v_offs = configInfo.outputSize.height / 8;\n> +\tcontext.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;\n> +\tcontext.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;\n> \n> \treturn 0;\n> }\n> @@ -125,7 +138,7 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> * \\param[inout] frameContext The shared IPA frame Context\n> * \\param[in] yGain The gain calculated on the current brightness level\n\nMissing documentation for iqMeanGain. You can copy it from the IPU3 IPA.\n\n> */\n> -void Agc::computeExposure(IPAContext &context, double yGain)\n> +void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> {\n> \tIPASessionConfiguration &configuration = context.configuration;\n> \tIPAFrameContext &frameContext = context.frameContext;\n> @@ -134,6 +147,9 @@ void Agc::computeExposure(IPAContext &context, double yGain)\n> \tuint32_t exposure = frameContext.sensor.exposure;\n> \tdouble analogueGain = frameContext.sensor.gain;\n> \n> +\t/* Use the highest of the two gain estimates. */\n> +\tdouble evGain = std::max(yGain, iqMeanGain);\n> +\n> \tutils::Duration minShutterSpeed = configuration.agc.minShutterSpeed;\n> \tutils::Duration maxShutterSpeed = std::min(configuration.agc.maxShutterSpeed,\n> \t\t\t\t\t\t kMaxShutterSpeed);\n> @@ -144,7 +160,7 @@ void Agc::computeExposure(IPAContext &context, double yGain)\n> \t\t\t\t\t kMaxAnalogueGain);\n> \n> \t/* Consider within 1% of the target as correctly exposed. */\n> -\tif (utils::abs_diff(yGain, 1.0) < 0.01)\n> +\tif (utils::abs_diff(evGain, 1.0) < 0.01)\n> \t\treturn;\n> \n> \t/* extracted from Rpi::Agc::computeTargetExposure. */\n> @@ -161,13 +177,13 @@ void Agc::computeExposure(IPAContext &context, double yGain)\n> \tLOG(RkISP1Agc, 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 \" << yGain;\n> +\t\t\t << \" Needed ev gain \" << evGain;\n> \n> \t/*\n> \t * Calculate the current exposure value for the scene as the latest\n> \t * exposure value applied multiplied by the new estimated gain.\n> \t */\n> -\tutils::Duration exposureValue = effectiveExposureValue * yGain;\n> +\tutils::Duration exposureValue = effectiveExposureValue * evGain;\n> \n> \t/* Clamp the exposure value to the min and max authorized. */\n> \tutils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain;\n> @@ -238,6 +254,23 @@ double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,\n> \treturn ySum / numCells_ / 255;\n> }\n> \n> +/**\n> + * \\brief Estimate the mean value of the top 2% of the histogram\n> + * \\param[in] hist The histogram statistics computed by the ImgU\n> + * \\return The mean value of the top 2% of the histogram\n> + */\n> +double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> +{\n> +\tstd::vector measHist(numHistBins_);\n> +\n> +\t/* Initialise the histogram array using the maximum available size */\n> +\tfor (unsigned int histBin = 0; histBin < numHistBins_; histBin++)\n> +\t\tmeasHist.push_back(hist->hist_bins[histBin]);\n\nhist->hist_bins is a __u32 array, I think you can skip the copy and do\n\n\t/* Estimate the quantile mean of the top 2% of the histogram. */\n\tHistogram histogram{ Span(hist->hist_bins, numHistBins_) };\n\treturn histogram.interQuantileMean(0.98, 1.0);\n\n> +\n> +\t/* Estimate the quantile mean of the top 2% of the histogram. */\n> +\treturn Histogram(Span(measHist)).interQuantileMean(0.98, 1.0);\n> +}\n> +\n> /**\n> * \\brief Process RkISP1 statistics, and run AGC operations\n> * \\param[in] context The shared IPA context\n> @@ -252,9 +285,13 @@ void Agc::process(IPAContext &context, const rkisp1_stat_buffer *stats)\n> \tASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);\n> \n> \tconst rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> +\tconst rkisp1_cif_isp_hist_stat *hist = ¶ms->hist;\n> \n> \tframeCount_ = context.frameContext.frameId;\n> \n> +\tdouble iqMean = measureBrightness(hist);\n> +\tdouble iqMeanGain = kEvGainTarget * numHistBins_ / iqMean;\n> +\n> \t/*\n> \t * Estimate the gain needed to achieve a relative luminance target. To\n> \t * account for non-linearity caused by saturation, the value needs to be\n> @@ -277,14 +314,44 @@ void Agc::process(IPAContext &context, const rkisp1_stat_buffer *stats)\n> \t\t\tbreak;\n> \t}\n> \n> -\tcomputeExposure(context, yGain);\n> +\tcomputeExposure(context, yGain, iqMeanGain);\n> }\n> \n> -void Agc::prepare([[maybe_unused]] IPAContext &context,\n> -\t\t rkisp1_params_cfg *params)\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Agc::prepare(IPAContext &context, rkisp1_params_cfg *params)\n> {\n> -\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_AEC;\n> -\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_AEC;\n> +\t/* Configure the measurement window. */\n> +\tparams->meas.aec_config.meas_window = context.configuration.agc.measureWindow;\n> +\t/* Use a continuous methode for measure. */\n\ns/methode/method/\n\n> +\tparams->meas.aec_config.autostop = RKISP1_CIF_ISP_EXP_CTRL_AUTOSTOP_0;\n> +\t/* Estimate Y as (R + G + B) x (85/256). */\n> +\tparams->meas.aec_config.mode = RKISP1_CIF_ISP_EXP_MEASURING_MODE_1;\n> +\n> +\tif (context.frameContext.frameId == 0) {\n\nAs for the BLC, I think you can have\n\n\tif (context.frameContext.frameId > 0)\n\t\treturn;\n\nat the beginning of the function instead.\n\n> +\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AEC;\n> +\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_AEC;\n> +\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_AEC;\n> +\t}\n> +\n> +\t/* Configure histogram. */\n> +\tparams->meas.hst_config.meas_window = context.configuration.agc.measureWindow;\n> +\t/* Produce the luminance histogram. */\n> +\tparams->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;\n> +\t/* Set an average weighted histogram. */\n> +\tfor (unsigned int histBin = 0; histBin < numHistBins_; histBin++)\n> +\t\tparams->meas.hst_config.hist_weight[histBin] = 1;\n> +\t/* Step size can't be less than 3. */\n> +\tparams->meas.hst_config.histogram_predivider = 4;\n> +\n> +\tif (context.frameContext.frameId == 0) {\n> +\t\t/* Update the configuration for histogram. */\n> +\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_HST;\n> +\t\t/* Enable the histogram measure unit. */\n> +\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_HST;\n> +\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_HST;\n> +\t}\n> }\n> \n> } /* namespace ipa::rkisp1::algorithms */\n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index 942c9d7a..872776d0 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -32,13 +32,15 @@ public:\n> \tvoid process(IPAContext &context, const rkisp1_stat_buffer *stats) override;\n> \n> private:\n> -\tvoid computeExposure(IPAContext &Context, double yGain);\n> +\tvoid computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> \tutils::Duration filterExposure(utils::Duration exposureValue);\n> \tdouble estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> +\tdouble measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> \n> \tuint64_t frameCount_;\n> \n> \tuint32_t numCells_;\n> +\tuint32_t numHistBins_;\n> \n> \tutils::Duration filteredExposure_;\n> };\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 992c9225..39acef47 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -71,6 +71,9 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPASessionConfiguration::agc.maxAnalogueGain\n> * \\brief Maximum analogue gain supported with the configured sensor\n> *\n> + * \\var IPASessionConfiguration::agc.measureWindow\n> + * \\brief AGC measure window\n> + *\n> * \\var IPASessionConfiguration::hw\n> * \\brief RkISP1-specific hardware information\n> *\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index c447369f..9ac3b40c 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -22,6 +22,7 @@ struct IPASessionConfiguration {\n> \t\tutils::Duration maxShutterSpeed;\n> \t\tdouble minAnalogueGain;\n> \t\tdouble maxAnalogueGain;\n> +\t\tstruct rkisp1_cif_isp_window measureWindow;\n> \t} agc;\n> \n> \tstruct {","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 EE089BF415\n\tfor ;\n\tWed, 23 Feb 2022 17:30:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C88C610B3;\n\tWed, 23 Feb 2022 18:30:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 463E5601FF\n\tfor ;\n\tWed, 23 Feb 2022 18:30:56 +0100 (CET)","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 A0418DD;\n\tWed, 23 Feb 2022 18:30:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"a1mUbjjp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1645637455;\n\tbh=BHun2Mw7r07VpOToig+9h6uBuyLcT2T6l1/gwOesBLc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a1mUbjjpw+whxRiKKm0hPxAiQr/VFpx0XRPko911RvOSbOYDV2LZlytPfW9aaYGrB\n\tZgYnSP0RKZNoBgdioZNdBylXzs/bkGYM3Nl0w+MhbAt64MHBg7H5Fk2XTfDiysYx/r\n\tmI69Xyl7iUyBooE8l9ZW2gXImTO462P96+rl/U6c=","Date":"Wed, 23 Feb 2022 19:30:45 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20220223104824.25723-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20220223104824.25723-4-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220223104824.25723-4-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: rkisp1: agc: Introduce\n\thistogram calculation","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\" "}}]