[{"id":20957,"web_url":"https://patchwork.libcamera.org/comment/20957/","msgid":"","date":"2021-11-15T15:36:11","subject":"Re: [libcamera-devel] [PATCH v5 03/14] ipa: ipu3: Use sensor\n\tcontrols to update frameContext","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 Sat, Nov 13, 2021 at 09:49:36AM +0100, Jean-Michel Hautbois wrote:\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> Reviewed-by: Kieran Bingham \n> Reviewed-by: Umang Jain \n> Reviewed-by: Paul Elder \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 | 3 +++\n> 5 files changed, 30 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index b5d736c1..5723f6f3 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> \t/* Algorithm initialization should wait for first valid frames */\n> \t/* \\todo - have a number of frames given by DelayedControls ?\n> @@ -189,6 +188,10 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> \t\treturn;\n> \t}\n> \n> +\t/* Get the effective exposure and gain applied on the sensor. */\n> +\tuint32_t exposure = frameContext.sensor.exposure;\n> +\tdouble analogueGain = frameContext.sensor.gain;\n> +\n> \t/* Estimate the gain needed to have the proportion wanted */\n> \tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> \n> @@ -233,8 +236,9 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> \t\t\t << shutterTime << \" and \"\n> \t\t\t << stepGain;\n> \n> -\texposure = shutterTime / lineDuration_;\n> -\tanalogueGain = stepGain;\n> +\t/* Update the estimated exposure and gain. */\n> +\tframeContext.agc.exposure = shutterTime / lineDuration_;\n> +\tframeContext.agc.gain = stepGain;\n> \n> \t/*\n> \t * 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> -\t/* Get the latest exposure and gain applied */\n> -\tuint32_t &exposure = context.frameContext.agc.exposure;\n> -\tdouble &analogueGain = context.frameContext.agc.gain;\n> \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n> -\tcomputeExposure(exposure, analogueGain);\n> +\tcomputeExposure(context.frameContext);\n> \tframeCount_++;\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> \tvoid measureBrightness(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> \tvoid filterExposure();\n> -\tvoid computeExposure(uint32_t &exposure, double &gain);\n> +\tvoid computeExposure(IPAFrameContext &frameContext);\n> \n> \tuint64_t frameCount_;\n> \tuint64_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\nI'd expand this a bit\n\n * \\brief The sensor control values that were used to capture this frame\n\n(in a follow-up patch as the series has been merged)\n\nAlthough, looking at the code, this structure now stores both the\ncontrol values used to capture the frame (before calling\ncomputeExposure()), and the values for the next frame (after calling\ncomputeExposure()). It could be nice to split it in two, to make the\ncode and documentation clearer.\n\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> \t\t} gains;\n> \t} awb;\n> \n> +\tstruct {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t} sensor;\n> +\n> \tstruct {\n> \t\tdouble gamma;\n> \t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index bcc3863b..38e86e58 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -549,6 +549,9 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> \t\tconst ipu3_uapi_stats_3a *stats =\n> \t\t\treinterpret_cast(mem.data());\n> \n> +\t\tcontext_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> +\t\tcontext_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n\nCould you shorten the lines ? The second one is above our hard limit.\n\n> +\n> \t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> \t\tbreak;\n> \t}","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 4045EBF415\n\tfor ;\n\tMon, 15 Nov 2021 15:36:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 855626038A;\n\tMon, 15 Nov 2021 16:36:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A84F60233\n\tfor ;\n\tMon, 15 Nov 2021 16:36:34 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B77E39CA;\n\tMon, 15 Nov 2021 16:36:33 +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=\"mNRFzn5j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636990593;\n\tbh=TSEQo4CKNWMf+cv8YdgGPpSCWrODGrMEjTn3l0eF13g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mNRFzn5jTMtTd7emrojTp3pK17D2uteX7nWIR4W9GweVzRNWuCas6JEGWMZT0r5ga\n\tHlp9Qh+EJbtscoC8fEQTLfQ++DZpUMMWFtKFCTKnvAM1VrA1JMFrWq8bIaxwwevUJ4\n\to+tQlTjaCrVpJJBR5uL9Vv0y+GBiF6o3uLqyOJks=","Date":"Mon, 15 Nov 2021 17:36:11 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211113084947.21892-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211113084947.21892-4-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211113084947.21892-4-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20961,"web_url":"https://patchwork.libcamera.org/comment/20961/","msgid":"<3eb76452-d64a-7046-df8f-42198fd6d5c0@ideasonboard.com>","date":"2021-11-16T07:52:02","subject":"Re: [libcamera-devel] [PATCH v5 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":"Hi Laurent,\n\nOn 15/11/2021 16:36, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Sat, Nov 13, 2021 at 09:49:36AM +0100, Jean-Michel Hautbois wrote:\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>> Reviewed-by: Kieran Bingham \n>> Reviewed-by: Umang Jain \n>> Reviewed-by: Paul Elder \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 | 3 +++\n>> 5 files changed, 30 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n>> index b5d736c1..5723f6f3 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>> \t/* Algorithm initialization should wait for first valid frames */\n>> \t/* \\todo - have a number of frames given by DelayedControls ?\n>> @@ -189,6 +188,10 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> \t\treturn;\n>> \t}\n>> \n>> +\t/* Get the effective exposure and gain applied on the sensor. */\n>> +\tuint32_t exposure = frameContext.sensor.exposure;\n>> +\tdouble analogueGain = frameContext.sensor.gain;\n>> +\n>> \t/* Estimate the gain needed to have the proportion wanted */\n>> \tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n>> \n>> @@ -233,8 +236,9 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n>> \t\t\t << shutterTime << \" and \"\n>> \t\t\t << stepGain;\n>> \n>> -\texposure = shutterTime / lineDuration_;\n>> -\tanalogueGain = stepGain;\n>> +\t/* Update the estimated exposure and gain. */\n>> +\tframeContext.agc.exposure = shutterTime / lineDuration_;\n>> +\tframeContext.agc.gain = stepGain;\n>> \n>> \t/*\n>> \t * 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>> -\t/* Get the latest exposure and gain applied */\n>> -\tuint32_t &exposure = context.frameContext.agc.exposure;\n>> -\tdouble &analogueGain = context.frameContext.agc.gain;\n>> \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n>> -\tcomputeExposure(exposure, analogueGain);\n>> +\tcomputeExposure(context.frameContext);\n>> \tframeCount_++;\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>> \tvoid measureBrightness(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t const ipu3_uapi_grid_config &grid);\n>> \tvoid filterExposure();\n>> -\tvoid computeExposure(uint32_t &exposure, double &gain);\n>> +\tvoid computeExposure(IPAFrameContext &frameContext);\n>> \n>> \tuint64_t frameCount_;\n>> \tuint64_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> I'd expand this a bit\n> \n> * \\brief The sensor control values that were used to capture this frame\n> \n> (in a follow-up patch as the series has been merged)\n> \n> Although, looking at the code, this structure now stores both the\n> control values used to capture the frame (before calling\n> computeExposure()), and the values for the next frame (after calling\n> computeExposure()). It could be nice to split it in two, to make the\n> code and documentation clearer.\n\nHaving a IPACurrentFrameContext and IPANextFrameContext ? Isn't having \nIPAFrameContext.sensor for input and IPAFrameContext.agc for output \nenough to split ? Or did I misunderstand you :-) ?\n\n> \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>> \t\t} gains;\n>> \t} awb;\n>> \n>> +\tstruct {\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t} sensor;\n>> +\n>> \tstruct {\n>> \t\tdouble gamma;\n>> \t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index bcc3863b..38e86e58 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -549,6 +549,9 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>> \t\tconst ipu3_uapi_stats_3a *stats =\n>> \t\t\treinterpret_cast(mem.data());\n>> \n>> +\t\tcontext_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n>> +\t\tcontext_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> \n> Could you shorten the lines ? The second one is above our hard limit.\n\nI can but checkstyle isnt happy (I did, tbh)\n\n> \n>> +\n>> \t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n>> \t\tbreak;\n>> \t}\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 CAD33BDB1C\n\tfor ;\n\tTue, 16 Nov 2021 07:52:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 212B860230;\n\tTue, 16 Nov 2021 08:52:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28A7D60128\n\tfor ;\n\tTue, 16 Nov 2021 08:52:05 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:c966:1d53:a935:70a0] (unknown\n\t[IPv6:2a01:e0a:169:7140:c966:1d53:a935:70a0])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C18E693;\n\tTue, 16 Nov 2021 08:52:04 +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=\"f5vUSr4v\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637049124;\n\tbh=DW+vMjbRhP65JmAsI4bBsZKVWSXExvRBLp9WM49V6Zw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=f5vUSr4vfBmxDVbhcNtlUUwcGqX06fbHSagALmZbXUKf8MUd1FLytkOXGMk26ZNtC\n\tB+Mo63KJsWa2AOrCwfRBm0fDROfdxLkaQ0t1LDuiOoR6CdPePVMaIfHeWOZWD2X9LM\n\tdXI1XTJXg8x5Yv7WYyRLsyhCw4OHfvHmlfj1p0U8=","Message-ID":"<3eb76452-d64a-7046-df8f-42198fd6d5c0@ideasonboard.com>","Date":"Tue, 16 Nov 2021 08:52:02 +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":"Laurent Pinchart ","References":"<20211113084947.21892-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211113084947.21892-4-jeanmichel.hautbois@ideasonboard.com>\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 v5 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20984,"web_url":"https://patchwork.libcamera.org/comment/20984/","msgid":"","date":"2021-11-17T13:59:07","subject":"Re: [libcamera-devel] [PATCH v5 03/14] ipa: ipu3: Use sensor\n\tcontrols to update frameContext","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Tue, Nov 16, 2021 at 08:52:02AM +0100, Jean-Michel Hautbois wrote:\n> On 15/11/2021 16:36, Laurent Pinchart wrote:\n> > On Sat, Nov 13, 2021 at 09:49:36AM +0100, Jean-Michel Hautbois wrote:\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> >> Reviewed-by: Kieran Bingham \n> >> Reviewed-by: Umang Jain \n> >> Reviewed-by: Paul Elder \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 | 3 +++\n> >> 5 files changed, 30 insertions(+), 10 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> >> index b5d736c1..5723f6f3 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> >> \t/* Algorithm initialization should wait for first valid frames */\n> >> \t/* \\todo - have a number of frames given by DelayedControls ?\n> >> @@ -189,6 +188,10 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> >> \t\treturn;\n> >> \t}\n> >> \n> >> +\t/* Get the effective exposure and gain applied on the sensor. */\n> >> +\tuint32_t exposure = frameContext.sensor.exposure;\n> >> +\tdouble analogueGain = frameContext.sensor.gain;\n> >> +\n> >> \t/* Estimate the gain needed to have the proportion wanted */\n> >> \tdouble evGain = kEvGainTarget * knumHistogramBins / iqMean_;\n> >> \n> >> @@ -233,8 +236,9 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)\n> >> \t\t\t << shutterTime << \" and \"\n> >> \t\t\t << stepGain;\n> >> \n> >> -\texposure = shutterTime / lineDuration_;\n> >> -\tanalogueGain = stepGain;\n> >> +\t/* Update the estimated exposure and gain. */\n> >> +\tframeContext.agc.exposure = shutterTime / lineDuration_;\n> >> +\tframeContext.agc.gain = stepGain;\n> >> \n> >> \t/*\n> >> \t * 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> >> -\t/* Get the latest exposure and gain applied */\n> >> -\tuint32_t &exposure = context.frameContext.agc.exposure;\n> >> -\tdouble &analogueGain = context.frameContext.agc.gain;\n> >> \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n> >> -\tcomputeExposure(exposure, analogueGain);\n> >> +\tcomputeExposure(context.frameContext);\n> >> \tframeCount_++;\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> >> \tvoid measureBrightness(const ipu3_uapi_stats_3a *stats,\n> >> \t\t\t const ipu3_uapi_grid_config &grid);\n> >> \tvoid filterExposure();\n> >> -\tvoid computeExposure(uint32_t &exposure, double &gain);\n> >> +\tvoid computeExposure(IPAFrameContext &frameContext);\n> >> \n> >> \tuint64_t frameCount_;\n> >> \tuint64_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> > I'd expand this a bit\n> > \n> > * \\brief The sensor control values that were used to capture this frame\n> > \n> > (in a follow-up patch as the series has been merged)\n> > \n> > Although, looking at the code, this structure now stores both the\n> > control values used to capture the frame (before calling\n> > computeExposure()), and the values for the next frame (after calling\n> > computeExposure()). It could be nice to split it in two, to make the\n> > code and documentation clearer.\n> \n> Having a IPACurrentFrameContext and IPANextFrameContext ? Isn't having \n> IPAFrameContext.sensor for input and IPAFrameContext.agc for output \n> enough to split ? Or did I misunderstand you :-) ?\n\nI've missed that the have two separate fields (agc and sensor) in the\nframe context. Please ignore the second part of the comment. The\nproposal to expand the brief still holds.\n\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> >> \t\t} gains;\n> >> \t} awb;\n> >> \n> >> +\tstruct {\n> >> +\t\tuint32_t exposure;\n> >> +\t\tdouble gain;\n> >> +\t} sensor;\n> >> +\n> >> \tstruct {\n> >> \t\tdouble gamma;\n> >> \t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index bcc3863b..38e86e58 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -549,6 +549,9 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> >> \t\tconst ipu3_uapi_stats_3a *stats =\n> >> \t\t\treinterpret_cast(mem.data());\n> >> \n> >> +\t\tcontext_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >> +\t\tcontext_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > \n> > Could you shorten the lines ? The second one is above our hard limit.\n> \n> I can but checkstyle isnt happy (I did, tbh)\n> \n> >> +\n> >> \t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> >> \t\tbreak;\n> >> \t}","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 7D031BDB1C\n\tfor ;\n\tWed, 17 Nov 2021 13:59:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92F3A60376;\n\tWed, 17 Nov 2021 14:59:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6607D60121\n\tfor ;\n\tWed, 17 Nov 2021 14:59:34 +0100 (CET)","from pendragon.ideasonboard.com (85-76-82-213-nat.elisa-mobile.fi\n\t[85.76.82.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7ACAA2CF;\n\tWed, 17 Nov 2021 14:59:33 +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=\"v9KrzjC4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637157574;\n\tbh=mfr+lWfyZU4p6CcMcMFah+Oz8EEEy7qZqytAhJAdHAo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v9KrzjC4/Rwom7eMjUPSW4dE56BTDslEW83DDoFOD4t4lP9OmCc/MOnqYEXVLTtZ0\n\tTrAyb1oDWd5eO+8GLWwUPIhHyRBMof62kO4uqm23nKg/ASJ9ykQyaXS+smzlGE4IGr\n\tAeTQIuj4H7NFVuyxNeWh+t0QRNIjI2Sk8HPUGS9o=","Date":"Wed, 17 Nov 2021 15:59:07 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211113084947.21892-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211113084947.21892-4-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<3eb76452-d64a-7046-df8f-42198fd6d5c0@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<3eb76452-d64a-7046-df8f-42198fd6d5c0@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]