[{"id":20855,"web_url":"https://patchwork.libcamera.org/comment/20855/","msgid":"<163663640847.2258438.6805928188842185548@Monstersaurus>","date":"2021-11-11T13:13:28","subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: Use sensor\n\tcontrols to update frameContext","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-11-11 11:05:54)\n> The pipeline handler populates the new sensorControls ControlList, to\n> have the effective exposure and gain values for the current frame. This\n> is done when a statistics buffer is received.\n> \n> Make those values the frameContext::sensor values for the frame when the\n> EventStatReady event is received.\n> \n> AGC also needs to use frameContext.sensor as its input values and\n> frameContext.agc as its output values. Modify computeExposure by passing\n> it the frameContext instead of individual exposure and gain values.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 19 ++++++++++---------\n> src/ipa/ipu3/algorithms/agc.h | 2 +-\n> src/ipa/ipu3/ipa_context.cpp | 11 +++++++++++\n> src/ipa/ipu3/ipa_context.h | 5 +++++\n> src/ipa/ipu3/ipu3.cpp | 4 ++++\n> 5 files changed, 31 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index b5d736c1..825b35d4 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -169,10 +169,9 @@ void Agc::filterExposure()\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> + * \\param[inout] frameContext The shared IPA frame Context\n> */\n> -void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> +void Agc::computeExposure(IPAFrameContext &frameContext)\n> {\n> /* Algorithm initialization should wait for first valid frames */\n> /* \\todo - have a number of frames given by DelayedControls ?\n> @@ -189,6 +188,10 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> return;\n> }\n> \n> + /* Get the effective exposure and gain applied on the sensor. */\n> + uint32_t &exposure = frameContext.sensor.exposure;\n> + double &analogueGain = frameContext.sensor.gain;\n\nI don't think these need to be references now, but it doesn't\nparticularly matter.\n\nPerhaps removing the & would make it clear that the\nframeContext.sensor.exposure and frameContext.sensor.gain isn't going to\nbe modified in this function (which the & could otherwise imply).\n\n\n> +\n> /* Estimate the gain needed to have the proportion wanted */\n> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> @@ -233,8 +236,9 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> << shutterTime << \" and \"\n> << stepGain;\n> \n> - exposure = shutterTime / lineDuration_;\n> - analogueGain = stepGain;\n> + /* Update the estimated exposure and gain. */\n> + frameContext.agc.exposure = shutterTime / lineDuration_;\n> + frameContext.agc.gain = stepGain;\n> \n> /*\n> * Update the exposure value for the next process call.\n> @@ -257,11 +261,8 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> */\n> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> - /* Get the latest exposure and gain applied */\n> - uint32_t &exposure = context.frameContext.agc.exposure;\n> - double &analogueGain = context.frameContext.agc.gain;\n> measureBrightness(stats, context.configuration.grid.bdsGrid);\n> - computeExposure(exposure, analogueGain);\n> + computeExposure(context.frameContext);\n\nThat's a lot cleaner...\n\n> frameCount_++;\n> }\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 69e0b831..f0db25ee 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -34,7 +34,7 @@ private:\n> void measureBrightness(const ipu3_uapi_stats_3a *stats,\n> const ipu3_uapi_grid_config &grid);\n> void filterExposure();\n> - void computeExposure(uint32_t &exposure, double &gain);\n> + void computeExposure(IPAFrameContext &frameContext);\n> \n> uint64_t frameCount_;\n> uint64_t lastFrame_;\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 2355a9c7..a7ff957d 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -119,6 +119,17 @@ namespace libcamera::ipa::ipu3 {\n> * \\brief White balance gain for B channel\n> */\n> \n> +/**\n> + * \\var IPAFrameContext::sensor\n> + * \\brief Effective sensor values\n> + *\n> + * \\var IPAFrameContext::sensor.exposure\n> + * \\brief Exposure time expressed as a number of lines\n> + *\n> + * \\var IPAFrameContext::sensor.gain\n> + * \\brief Analogue gain multiplier\n> + */\n> +\n> /**\n> * \\var IPAFrameContext::toneMapping\n> * \\brief Context for ToneMapping and Gamma control\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 1e46c61a..a5a19800 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -47,6 +47,11 @@ struct IPAFrameContext {\n> } gains;\n> } awb;\n> \n> + struct {\n> + uint32_t exposure;\n> + double gain;\n> + } sensor;\n> +\n> struct {\n> double gamma;\n> struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index bcc3863b..dee002a5 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -549,6 +549,10 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> const ipu3_uapi_stats_3a *stats =\n> reinterpret_cast(mem.data());\n> \n> + /* \\todo move those into processControls ? */\n\nI still haven't understood why we want to move these into\nprocessControls.\n\nI think that is supposed to deal with processing controls from the\nRequest?\n\nFor everything else\n\n\nReviewed-by: Kieran Bingham \n\n> + context_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> + context_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> +\n> parseStatistics(event.frame, event.frameTimestamp, stats);\n> break;\n> }\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 51103BF415\n\tfor ;\n\tThu, 11 Nov 2021 13:13:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9689B6035D;\n\tThu, 11 Nov 2021 14:13:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FBF760345\n\tfor ;\n\tThu, 11 Nov 2021 14:13:31 +0100 (CET)","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 9DA9A4A0;\n\tThu, 11 Nov 2021 14:13:30 +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=\"vTKFppJv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636636410;\n\tbh=pVAys5hQyc503u2ETk4o1d8Ygv2gG0RnagXiXKHvElQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=vTKFppJvt1OxAa+EyIM2Bidezf++aaQ1YxKL3tXCzrMaE3AML5qljNxAug7yM/NHW\n\tWzIgPptTl+IpApby1k6ntV7hNS+pkLeDFruOEbhXEhTXRXqKy1GfKi94l1AUXx4Y4K\n\tZGh2LOHzLSa8MiIvoY0lXla7tH+xVUY+NAjYtVrE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211111110605.105202-4-jeanmichel.hautbois@ideasonboard.com>","References":"<20211111110605.105202-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111110605.105202-4-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 11 Nov 2021 13:13:28 +0000","Message-ID":"<163663640847.2258438.6805928188842185548@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: Use sensor\n\tcontrols to update frameContext","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20859,"web_url":"https://patchwork.libcamera.org/comment/20859/","msgid":"","date":"2021-11-11T13:51:19","subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: Use sensor\n\tcontrols to update frameContext","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 11/11/2021 14:13, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-11-11 11:05:54)\n>> The pipeline handler populates the new sensorControls ControlList, to\n>> have the effective exposure and gain values for the current frame. This\n>> is done when a statistics buffer is received.\n>>\n>> Make those values the frameContext::sensor values for the frame when the\n>> EventStatReady event is received.\n>>\n>> AGC also needs to use frameContext.sensor as its input values and\n>> frameContext.agc as its output values. Modify computeExposure by passing\n>> it the frameContext instead of individual exposure and gain values.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/algorithms/agc.cpp | 19 ++++++++++---------\n>> src/ipa/ipu3/algorithms/agc.h | 2 +-\n>> src/ipa/ipu3/ipa_context.cpp | 11 +++++++++++\n>> src/ipa/ipu3/ipa_context.h | 5 +++++\n>> src/ipa/ipu3/ipu3.cpp | 4 ++++\n>> 5 files changed, 31 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index b5d736c1..825b35d4 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -169,10 +169,9 @@ void Agc::filterExposure()\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>> + * \\param[inout] frameContext The shared IPA frame Context\n>> */\n>> -void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> +void Agc::computeExposure(IPAFrameContext &frameContext)\n>> {\n>> /* Algorithm initialization should wait for first valid frames */\n>> /* \\todo - have a number of frames given by DelayedControls ?\n>> @@ -189,6 +188,10 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> return;\n>> }\n>> \n>> + /* Get the effective exposure and gain applied on the sensor. */\n>> + uint32_t &exposure = frameContext.sensor.exposure;\n>> + double &analogueGain = frameContext.sensor.gain;\n> \n> I don't think these need to be references now, but it doesn't\n> particularly matter.\n> \n> Perhaps removing the & would make it clear that the\n> frameContext.sensor.exposure and frameContext.sensor.gain isn't going to\n> be modified in this function (which the & could otherwise imply).\n> \n> \n>> +\n>> /* Estimate the gain needed to have the proportion wanted */\n>> double evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n>> \n>> @@ -233,8 +236,9 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> << shutterTime << \" and \"\n>> << stepGain;\n>> \n>> - exposure = shutterTime / lineDuration_;\n>> - analogueGain = stepGain;\n>> + /* Update the estimated exposure and gain. */\n>> + frameContext.agc.exposure = shutterTime / lineDuration_;\n>> + frameContext.agc.gain = stepGain;\n>> \n>> /*\n>> * Update the exposure value for the next process call.\n>> @@ -257,11 +261,8 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> */\n>> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>> {\n>> - /* Get the latest exposure and gain applied */\n>> - uint32_t &exposure = context.frameContext.agc.exposure;\n>> - double &analogueGain = context.frameContext.agc.gain;\n>> measureBrightness(stats, context.configuration.grid.bdsGrid);\n>> - computeExposure(exposure, analogueGain);\n>> + computeExposure(context.frameContext);\n> \n> That's a lot cleaner...\n> \n>> frameCount_++;\n>> }\n>> \n>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n>> index 69e0b831..f0db25ee 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -34,7 +34,7 @@ private:\n>> void measureBrightness(const ipu3_uapi_stats_3a *stats,\n>> const ipu3_uapi_grid_config &grid);\n>> void filterExposure();\n>> - void computeExposure(uint32_t &exposure, double &gain);\n>> + void computeExposure(IPAFrameContext &frameContext);\n>> \n>> uint64_t frameCount_;\n>> uint64_t lastFrame_;\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index 2355a9c7..a7ff957d 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -119,6 +119,17 @@ namespace libcamera::ipa::ipu3 {\n>> * \\brief White balance gain for B channel\n>> */\n>> \n>> +/**\n>> + * \\var IPAFrameContext::sensor\n>> + * \\brief Effective sensor values\n>> + *\n>> + * \\var IPAFrameContext::sensor.exposure\n>> + * \\brief Exposure time expressed as a number of lines\n>> + *\n>> + * \\var IPAFrameContext::sensor.gain\n>> + * \\brief Analogue gain multiplier\n>> + */\n>> +\n>> /**\n>> * \\var IPAFrameContext::toneMapping\n>> * \\brief Context for ToneMapping and Gamma control\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index 1e46c61a..a5a19800 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -47,6 +47,11 @@ struct IPAFrameContext {\n>> } gains;\n>> } awb;\n>> \n>> + struct {\n>> + uint32_t exposure;\n>> + double gain;\n>> + } sensor;\n>> +\n>> struct {\n>> double gamma;\n>> struct ipu3_uapi_gamma_corr_lut gammaCorrection;\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index bcc3863b..dee002a5 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -549,6 +549,10 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>> const ipu3_uapi_stats_3a *stats =\n>> reinterpret_cast(mem.data());\n>> \n>> + /* \\todo move those into processControls ? */\n> \n> I still haven't understood why we want to move these into\n> processControls.\n> \n> I think that is supposed to deal with processing controls from the\n> Request?\n> \n\nThat is more a question, I will remove it as a comment for the series, \nand if it appears later we need to move those then it will answer the \nquestion :-).\n\n> For everything else\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n>> + context_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> + context_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>> +\n>> parseStatistics(event.frame, event.frameTimestamp, stats);\n>> break;\n>> }\n>> -- \n>> 2.32.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CEAA4BF415\n\tfor ;\n\tThu, 11 Nov 2021 13:51:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A8316035A;\n\tThu, 11 Nov 2021 14:51:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D2C8600B5\n\tfor ;\n\tThu, 11 Nov 2021 14:51:22 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:e627:8337:a781:d98] (unknown\n\t[IPv6:2a01:e0a:169:7140:e627:8337:a781:d98])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C84235D;\n\tThu, 11 Nov 2021 14:51:21 +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=\"fkOuw95n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636638681;\n\tbh=U/rD54rdm4/3HxPtmte+R5pBhFGfbvUiGLqnKMmbm78=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=fkOuw95n1YReOK8OCOycwqelJ2a5NCsYcaYJLd6eo6dO1GDdYnElmv3y33ytJDbNQ\n\tOkXxDTdsofUhas/xBvdcWIzYSSDNQfjI3uwJeUocvI3J3escG0gNbaFMG5i2cOaGiG\n\tdWcReZaVmOqIQ9uNU67oec/SJ8EE5HIlIMkzMKO4=","Message-ID":"","Date":"Thu, 11 Nov 2021 14:51:19 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211111110605.105202-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211111110605.105202-4-jeanmichel.hautbois@ideasonboard.com>\n\t<163663640847.2258438.6805928188842185548@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163663640847.2258438.6805928188842185548@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: Use sensor\n\tcontrols to update frameContext","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]