[{"id":25047,"web_url":"https://patchwork.libcamera.org/comment/25047/","msgid":"<166371884168.18961.15851239113055930923@Monstersaurus>","date":"2022-09-21T00:07:21","subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)\n> Rework the algorithm's usage of the active state to store the value of\n> controls for the last queued request in the queueRequest() function, and\n> store a copy of the values in the corresponding frame context.\n> \n> The frame context is used in the prepare() function to populate the ISP\n> parameters with values corresponding to the right frame.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----\n> src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> src/ipa/rkisp1/rkisp1.cpp | 9 ++++---\n> 5 files changed, 55 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 4124c3272bc7..e7198169d11b 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> /**\n> * \\brief Estimate the new exposure and gain values\n> * \\param[inout] context The shared IPA Context\n> + * \\param[in] frameContext The FrameContext for this frame\n> * \\param[in] yGain The gain calculated on the current brightness level\n> * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> */\n> -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> + double yGain, double iqMeanGain)\n> {\n> IPASessionConfiguration &configuration = context.configuration;\n> IPAActiveState &activeState = context.activeState;\n> \n> /* Get the effective exposure and gain applied on the sensor. */\n> - uint32_t exposure = activeState.sensor.exposure;\n> - double analogueGain = activeState.sensor.gain;\n> + uint32_t exposure = frameContext.sensor.exposure;\n> + double analogueGain = frameContext.sensor.gain;\n> \n> /* Use the highest of the two gain estimates. */\n> double evGain = std::max(yGain, iqMeanGain);\n> @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> * new exposure and gain for the scene.\n> */\n> void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> + RkISP1FrameContext &frameContext,\n> const rkisp1_stat_buffer *stats)\n> {\n> const rkisp1_cif_isp_stat *params = &stats->params;\n> @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> break;\n> }\n> \n> - computeExposure(context, yGain, iqMeanGain);\n> + computeExposure(context, frameContext, yGain, iqMeanGain);\n\nIt may be useful to compare somewhere here the values for gain/exposure\nthat we expected to be set on the sensor, as stored in our frame\ncontext, with the values that could be retrieved from the\nDelayedControls implementation to make sure they are\nconsistent/coherrent.\n\n\n> frameCount_++;\n> }\n> \n> @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> - rkisp1_params_cfg *params)\n> + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> {\n> + frameContext.agc.exposure = context.activeState.agc.exposure;\n> + frameContext.agc.gain = context.activeState.agc.gain;\n> +\n\nThis may be a little dubious ... due to the delays inherrent with\nsetting a gain and exposure...\n\nIf they are not known to be exactly the values used by the sensor for\nthis frame, but we still need to store them here, then I'd add a todo or\na warning note...\n\n\nIs prepare() called before the sensor starts in the inline-ISP case with\nRKISP?\n\n\n> if (frame > 0)\n> return;\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> index be8932040c8e..d6c6fb130fc1 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.h\n> +++ b/src/ipa/rkisp1/algorithms/agc.h\n> @@ -34,7 +34,8 @@ public:\n> const rkisp1_stat_buffer *stats) override;\n> \n> private:\n> - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> + double yGain, double iqMeanGain);\n> utils::Duration filterExposure(utils::Duration exposureValue);\n> double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 211428717838..88e0631c8d39 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAActiveState::agc\n> * \\brief State for the Automatic Gain Control algorithm\n> *\n> - * The exposure and gain determined are expected to be applied to the sensor\n> - * at the earliest opportunity.\n> + * The exposure and gain are the latest values computed by the AGC algorithm.\n> *\n> * \\var IPAActiveState::agc.exposure\n> * \\brief Exposure time expressed as a number of lines\n> *\n> * \\var IPAActiveState::agc.gain\n> * \\brief Analogue gain multiplier\n> - *\n> - * The gain should be adapted to the sensor specific gain code before applying.\n> */\n> \n> /**\n> @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> * \\brief Indicates if ISP parameters need to be updated\n> */\n> \n> -/**\n> - * \\var IPAActiveState::sensor\n> - * \\brief Effective sensor values\n> - *\n> - * \\var IPAActiveState::sensor.exposure\n> - * \\brief Exposure time expressed as a number of lines\n> - *\n> - * \\var IPAActiveState::sensor.gain\n> - * \\brief Analogue gain multiplier\n> - */\n> -\n> /**\n> * \\struct RkISP1FrameContext\n> * \\brief Per-frame context for algorithms\n> @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> * \\todo Populate the frame context for all algorithms\n> */\n> \n> +/**\n> + * \\var RkISP1FrameContext::agc\n> + * \\brief Automatic Gain Control parameters for this frame\n> + *\n> + * The exposure and gain are provided by the AGC algorithm, and are to be\n> + * applied to the sensor in order to take effect for this frame.\n> + *\n> + * \\var RkISP1FrameContext::agc.exposure\n> + * \\brief Exposure time expressed as a number of lines\n> + *\n> + * \\var RkISP1FrameContext::agc.gain\n> + * \\brief Analogue gain multiplier\n> + *\n> + * The gain should be adapted to the sensor specific gain code before applying.\n> + */\n> +\n> +/**\n> + * \\var RkISP1FrameContext::sensor\n> + * \\brief Sensor configuration that used been used for this frame\n> + *\n> + * \\var RkISP1FrameContext::sensor.exposure\n> + * \\brief Exposure time expressed as a number of lines\n> + *\n> + * \\var RkISP1FrameContext::sensor.gain\n> + * \\brief Analogue gain multiplier\n> + */\n> +\n> /**\n> * \\struct IPAContext\n> * \\brief Global IPA context data shared between all algorithms\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 4e38ef40ed3c..ecf993cd22d7 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -82,14 +82,18 @@ struct IPAActiveState {\n> uint8_t sharpness;\n> bool updateParams;\n> } filter;\n> -\n> - struct {\n> - uint32_t exposure;\n> - double gain;\n> - } sensor;\n> };\n> \n> struct RkISP1FrameContext : public FrameContext {\n> + struct {\n> + uint32_t exposure;\n> + double gain;\n> + } agc;\n> +\n> + struct {\n> + uint32_t exposure;\n> + double gain;\n> + } sensor;\n> };\n> \n> struct IPAContext {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 4f6e0b0f87b9..a23cfc7fb24d 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> reinterpret_cast(\n> mappedBuffers_.at(bufferId).planes()[0].data());\n> \n> - context_.activeState.sensor.exposure =\n> + frameContext.sensor.exposure =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> - context_.activeState.sensor.gain =\n> + frameContext.sensor.gain =\n> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n\nAha - good - so here we're getting the 'effective' values that were set\nat the sensor, before we do any processing with them.\n\nSo this is the point that would be interesting to compare 'what we asked\nfor' ... 'vs what we actually got'.\n\n\n> \n> unsigned int aeState = 0;\n> @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> \n> void IPARkISP1::setControls(unsigned int frame)\n> {\n> - uint32_t exposure = context_.activeState.agc.exposure;\n> - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> + uint32_t exposure = frameContext.agc.exposure;\n> + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n\nSo this is the part where we need to figure out more regarding PFC ...\nbut that's a separate topic for the moment.\n\nSo while this patch won't meet our PFC requriements - I think that's\nfine and this helps build what we need to get there.\n\n\nI'm a bit more hesitant here (hence leaving this to come back to) ...\nbut I don't think this is 'worse' than where we already are - and\nprogresses the frame context conversions.\n\nPerhaps with some more notes / a todo on the fact that we need to do\nmore about handling of setting exposure / gains for a specific frame...\n\nReviewed-by: Kieran Bingham \n\n> \n> ControlList ctrls(ctrls_);\n> ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 B9A65C0DA4\n\tfor ;\n\tWed, 21 Sep 2022 00:07:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8A9B621DD;\n\tWed, 21 Sep 2022 02:07:27 +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 65CEC61F7D\n\tfor ;\n\tWed, 21 Sep 2022 02:07:25 +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 D59DC415;\n\tWed, 21 Sep 2022 02:07:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663718848;\n\tbh=HV9Wjgh/Xn0BbfWsMU1ma2pS7ICekA5z1rH+ktKW4+A=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=ri2QqcCxYeEcXOPwEofePPfio2VGPOsxvLXuN9l3Z/g3Xd0EwOdYmjvSqithecf0p\n\t+XQDKOpgUYdhT8XmGzzSEIGx66Qz7jB7TCrmt4LBsNjIKV8scua8jLfqVvEXyl/TfT\n\tkKvbfYp0mJae7vkkyLTDbFZorqcO18C9zVE7hE3w5X874w6oH2MvtdK53LBjB4+OI5\n\t2mXEnILHaCG4mSYZc+nenLOwxtwQEmIe3QsA11tXalk6uKqRIjAviCMSteEIC5n10n\n\t5xFjFGEdd755mzxRuYT0zE3lMjdapG7wJFGQw7lFDRpSEk/Jguw/jjooA2dap7ryHq\n\tMD3zgcmupTBFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663718845;\n\tbh=HV9Wjgh/Xn0BbfWsMU1ma2pS7ICekA5z1rH+ktKW4+A=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=FcITClUBthhxT7f4rTjeThpzp2ePVNUwXhgt2Du2jqqcitNCF1wEJfp5NZK7GQiId\n\txEplHH67M7jr9CWHoeaueOYisD9e7TP3W9/q4eU2NLrsx56VK9ljQToo73lRVbnwdB\n\t8q2fseDZydK34EvPrTDS17Y9J6PfeonxqnkppP8k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FcITClUB\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 21 Sep 2022 01:07:21 +0100","Message-ID":"<166371884168.18961.15851239113055930923@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25065,"web_url":"https://patchwork.libcamera.org/comment/25065/","msgid":"<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>","date":"2022-09-21T20:13:53","subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Kieran,\n\nOn Wed, Sep 21, 2022 at 01:07:21AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)\n> > Rework the algorithm's usage of the active state to store the value of\n> > controls for the last queued request in the queueRequest() function, and\n> > store a copy of the values in the corresponding frame context.\n> >\n> > The frame context is used in the prepare() function to populate the ISP\n> > parameters with values corresponding to the right frame.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----\n> > src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> > src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> > src/ipa/rkisp1/rkisp1.cpp | 9 ++++---\n> > 5 files changed, 55 insertions(+), 32 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 4124c3272bc7..e7198169d11b 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> > /**\n> > * \\brief Estimate the new exposure and gain values\n> > * \\param[inout] context The shared IPA Context\n> > + * \\param[in] frameContext The FrameContext for this frame\n> > * \\param[in] yGain The gain calculated on the current brightness level\n> > * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> > */\n> > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> > +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> > + double yGain, double iqMeanGain)\n> > {\n> > IPASessionConfiguration &configuration = context.configuration;\n> > IPAActiveState &activeState = context.activeState;\n> >\n> > /* Get the effective exposure and gain applied on the sensor. */\n> > - uint32_t exposure = activeState.sensor.exposure;\n> > - double analogueGain = activeState.sensor.gain;\n> > + uint32_t exposure = frameContext.sensor.exposure;\n> > + double analogueGain = frameContext.sensor.gain;\n> >\n> > /* Use the highest of the two gain estimates. */\n> > double evGain = std::max(yGain, iqMeanGain);\n> > @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> > * new exposure and gain for the scene.\n> > */\n> > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > + RkISP1FrameContext &frameContext,\n> > const rkisp1_stat_buffer *stats)\n> > {\n> > const rkisp1_cif_isp_stat *params = &stats->params;\n> > @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > break;\n> > }\n> >\n> > - computeExposure(context, yGain, iqMeanGain);\n> > + computeExposure(context, frameContext, yGain, iqMeanGain);\n>\n> It may be useful to compare somewhere here the values for gain/exposure\n> that we expected to be set on the sensor, as stored in our frame\n> context, with the values that could be retrieved from the\n> DelayedControls implementation to make sure they are\n> consistent/coherrent.\n>\n>\n> > frameCount_++;\n> > }\n> >\n> > @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > */\n> > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > - rkisp1_params_cfg *params)\n> > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > {\n> > + frameContext.agc.exposure = context.activeState.agc.exposure;\n> > + frameContext.agc.gain = context.activeState.agc.gain;\n> > +\n>\n> This may be a little dubious ... due to the delays inherrent with\n> setting a gain and exposure...\n>\n> If they are not known to be exactly the values used by the sensor for\n> this frame, but we still need to store them here, then I'd add a todo or\n> a warning note...\n\nDo I read the code and the comment here right, and this mean we report\nas frameContext.agc (== the agc values computed for this frame) the\nlast values computed by the AGC algorithm overall, which might be\ndifferent than the ones computed for the last frame, if we have two\nconsecutive calls to process() (which calls computeExposure()).\n\n\nAm I wrong thinking that prepare() and process() are synched to events\nwith different clocks ? prepare() is called in the \"frame ready\" call\npath, which process() in the ISP \"stats ready\" call path. Do we have\nguarantees that the two are in sync ?\n>\n>\n> Is prepare() called before the sensor starts in the inline-ISP case with\n> RKISP?\n>\n>\n> > if (frame > 0)\n> > return;\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > index be8932040c8e..d6c6fb130fc1 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > @@ -34,7 +34,8 @@ public:\n> > const rkisp1_stat_buffer *stats) override;\n> >\n> > private:\n> > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> > + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> > + double yGain, double iqMeanGain);\n> > utils::Duration filterExposure(utils::Duration exposureValue);\n> > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 211428717838..88e0631c8d39 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\var IPAActiveState::agc\n> > * \\brief State for the Automatic Gain Control algorithm\n> > *\n> > - * The exposure and gain determined are expected to be applied to the sensor\n> > - * at the earliest opportunity.\n> > + * The exposure and gain are the latest values computed by the AGC algorithm.\n> > *\n> > * \\var IPAActiveState::agc.exposure\n> > * \\brief Exposure time expressed as a number of lines\n> > *\n> > * \\var IPAActiveState::agc.gain\n> > * \\brief Analogue gain multiplier\n> > - *\n> > - * The gain should be adapted to the sensor specific gain code before applying.\n> > */\n> >\n> > /**\n> > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\brief Indicates if ISP parameters need to be updated\n> > */\n> >\n> > -/**\n> > - * \\var IPAActiveState::sensor\n> > - * \\brief Effective sensor values\n> > - *\n> > - * \\var IPAActiveState::sensor.exposure\n> > - * \\brief Exposure time expressed as a number of lines\n> > - *\n> > - * \\var IPAActiveState::sensor.gain\n> > - * \\brief Analogue gain multiplier\n> > - */\n> > -\n> > /**\n> > * \\struct RkISP1FrameContext\n> > * \\brief Per-frame context for algorithms\n> > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\todo Populate the frame context for all algorithms\n> > */\n> >\n> > +/**\n> > + * \\var RkISP1FrameContext::agc\n> > + * \\brief Automatic Gain Control parameters for this frame\n> > + *\n> > + * The exposure and gain are provided by the AGC algorithm, and are to be\n> > + * applied to the sensor in order to take effect for this frame.\n> > + *\n> > + * \\var RkISP1FrameContext::agc.exposure\n> > + * \\brief Exposure time expressed as a number of lines\n> > + *\n> > + * \\var RkISP1FrameContext::agc.gain\n> > + * \\brief Analogue gain multiplier\n> > + *\n> > + * The gain should be adapted to the sensor specific gain code before applying.\n> > + */\n> > +\n> > +/**\n> > + * \\var RkISP1FrameContext::sensor\n> > + * \\brief Sensor configuration that used been used for this frame\n> > + *\n> > + * \\var RkISP1FrameContext::sensor.exposure\n> > + * \\brief Exposure time expressed as a number of lines\n> > + *\n> > + * \\var RkISP1FrameContext::sensor.gain\n> > + * \\brief Analogue gain multiplier\n> > + */\n> > +\n> > /**\n> > * \\struct IPAContext\n> > * \\brief Global IPA context data shared between all algorithms\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 4e38ef40ed3c..ecf993cd22d7 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -82,14 +82,18 @@ struct IPAActiveState {\n> > uint8_t sharpness;\n> > bool updateParams;\n> > } filter;\n> > -\n> > - struct {\n> > - uint32_t exposure;\n> > - double gain;\n> > - } sensor;\n> > };\n> >\n> > struct RkISP1FrameContext : public FrameContext {\n> > + struct {\n> > + uint32_t exposure;\n> > + double gain;\n> > + } agc;\n> > +\n> > + struct {\n> > + uint32_t exposure;\n> > + double gain;\n> > + } sensor;\n> > };\n> >\n> > struct IPAContext {\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 4f6e0b0f87b9..a23cfc7fb24d 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > reinterpret_cast(\n> > mappedBuffers_.at(bufferId).planes()[0].data());\n> >\n> > - context_.activeState.sensor.exposure =\n> > + frameContext.sensor.exposure =\n> > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > - context_.activeState.sensor.gain =\n> > + frameContext.sensor.gain =\n> > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n>\n> Aha - good - so here we're getting the 'effective' values that were set\n> at the sensor, before we do any processing with them.\n>\n> So this is the point that would be interesting to compare 'what we asked\n> for' ... 'vs what we actually got'.\n>\n>\n> >\n> > unsigned int aeState = 0;\n> > @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> >\n> > void IPARkISP1::setControls(unsigned int frame)\n> > {\n> > - uint32_t exposure = context_.activeState.agc.exposure;\n> > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> > + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> > + uint32_t exposure = frameContext.agc.exposure;\n> > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n>\n> So this is the part where we need to figure out more regarding PFC ...\n> but that's a separate topic for the moment.\n>\n\nI might have missed why ? :)\n\n> So while this patch won't meet our PFC requriements - I think that's\n> fine and this helps build what we need to get there.\n>\n>\n> I'm a bit more hesitant here (hence leaving this to come back to) ...\n> but I don't think this is 'worse' than where we already are - and\n> progresses the frame context conversions.\n>\n> Perhaps with some more notes / a todo on the fact that we need to do\n> more about handling of setting exposure / gains for a specific frame...\n>\n> Reviewed-by: Kieran Bingham \n>\n> >\n> > ControlList ctrls(ctrls_);\n> > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 C998BC0DA4\n\tfor ;\n\tWed, 21 Sep 2022 20:13:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75BF1621F6;\n\tWed, 21 Sep 2022 22:13:58 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71C1D600AA\n\tfor ;\n\tWed, 21 Sep 2022 22:13:56 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 7C41B10000A;\n\tWed, 21 Sep 2022 20:13:55 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663791238;\n\tbh=aVs08bL+arvwh6DfzibAtANhQ0OEUmN8NoxF5lMKOhw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=GAmxj+LypecCWDGI194xgE+m+FVNgm9RL9TrMljk5yQaX9omYvNr7kLnSzHqvmE6g\n\ttJM1BMeAJhIphwIJr14wFe+Cf9DW1fk1W1Ijpae0m37qCdF1CZoeEtJD/WqxU2NYOI\n\tAd/Mjoy64+q4vounwSTiruenJTk8u1ZZrPC1b4lXLRVh+rNncQdDhTHF2uMDZgeQN4\n\tRC49TKMwoN5hc7M8i0ttmJ3hvR6JBjNLdlgzvDkiNzL1mcAjvZvG2WYc0uc18n78uK\n\t0Hm9eLF+f8I55IZqzQyf4axVC3D5B4Dw5W0Z2k0NG8k4+5Bd+1EKXKshbGRo1qDLgE\n\t17CZHptxr4QTA==","Date":"Wed, 21 Sep 2022 22:13:53 +0200","To":"Kieran Bingham ","Message-ID":"<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>\n\t<166371884168.18961.15851239113055930923@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166371884168.18961.15851239113055930923@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25071,"web_url":"https://patchwork.libcamera.org/comment/25071/","msgid":"<166383716310.56880.14265919686115696723@Monstersaurus>","date":"2022-09-22T08:59:23","subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-09-21 21:13:53)\n> Hi Laurent, Kieran,\n> \n> On Wed, Sep 21, 2022 at 01:07:21AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)\n> > > Rework the algorithm's usage of the active state to store the value of\n> > > controls for the last queued request in the queueRequest() function, and\n> > > store a copy of the values in the corresponding frame context.\n> > >\n> > > The frame context is used in the prepare() function to populate the ISP\n> > > parameters with values corresponding to the right frame.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----\n> > > src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> > > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> > > src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> > > src/ipa/rkisp1/rkisp1.cpp | 9 ++++---\n> > > 5 files changed, 55 insertions(+), 32 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 4124c3272bc7..e7198169d11b 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> > > /**\n> > > * \\brief Estimate the new exposure and gain values\n> > > * \\param[inout] context The shared IPA Context\n> > > + * \\param[in] frameContext The FrameContext for this frame\n> > > * \\param[in] yGain The gain calculated on the current brightness level\n> > > * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> > > */\n> > > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> > > +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> > > + double yGain, double iqMeanGain)\n> > > {\n> > > IPASessionConfiguration &configuration = context.configuration;\n> > > IPAActiveState &activeState = context.activeState;\n> > >\n> > > /* Get the effective exposure and gain applied on the sensor. */\n> > > - uint32_t exposure = activeState.sensor.exposure;\n> > > - double analogueGain = activeState.sensor.gain;\n> > > + uint32_t exposure = frameContext.sensor.exposure;\n> > > + double analogueGain = frameContext.sensor.gain;\n> > >\n> > > /* Use the highest of the two gain estimates. */\n> > > double evGain = std::max(yGain, iqMeanGain);\n> > > @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> > > * new exposure and gain for the scene.\n> > > */\n> > > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > + RkISP1FrameContext &frameContext,\n> > > const rkisp1_stat_buffer *stats)\n> > > {\n> > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > break;\n> > > }\n> > >\n> > > - computeExposure(context, yGain, iqMeanGain);\n> > > + computeExposure(context, frameContext, yGain, iqMeanGain);\n> >\n> > It may be useful to compare somewhere here the values for gain/exposure\n> > that we expected to be set on the sensor, as stored in our frame\n> > context, with the values that could be retrieved from the\n> > DelayedControls implementation to make sure they are\n> > consistent/coherrent.\n> >\n> >\n> > > frameCount_++;\n> > > }\n> > >\n> > > @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > */\n> > > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > - rkisp1_params_cfg *params)\n> > > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > > {\n> > > + frameContext.agc.exposure = context.activeState.agc.exposure;\n> > > + frameContext.agc.gain = context.activeState.agc.gain;\n> > > +\n> >\n> > This may be a little dubious ... due to the delays inherrent with\n> > setting a gain and exposure...\n> >\n> > If they are not known to be exactly the values used by the sensor for\n> > this frame, but we still need to store them here, then I'd add a todo or\n> > a warning note...\n> \n> Do I read the code and the comment here right, and this mean we report\n> as frameContext.agc (== the agc values computed for this frame) the\n> last values computed by the AGC algorithm overall, which might be\n> different than the ones computed for the last frame, if we have two\n> consecutive calls to process() (which calls computeExposure()).\n\nWhat are you distinguising between 'the last frame' and the 'last value\ncomputed by AGC?'.\n\nprocess() is currently always the last step where the AGC actually runs,\nso the ActiveState reports the 'best'/'most-up-to-date' desired values\nthat the AGC would like to use for automatic mode.\n\nHowever - the issue I have here is that for Sensor controls - this point\nin the time line is too late to apply those values.\n\nWe can make adjustments to the gains on the ISP here - but not the\nsensor.\n\nWe (already know that we) need to identify a way to ensure we\nsyncrhonise DelayedControls actions both here on the IPA side along with\nthe Pipeline Handler side.\n\nThat could be either communicating with DelayedControls, or changing the\ncurrent implementation (I've thought about 'scheduled controls' in the\npast, but it's all still WIP).\n\n \n> Am I wrong thinking that prepare() and process() are synched to events\n> with different clocks ? prepare() is called in the \"frame ready\" call\n> path, which process() in the ISP \"stats ready\" call path. Do we have\n> guarantees that the two are in sync ?\n\n\nprepare() is to generate configuration for the ISP.\nprocess() reports the statistics of the ISP after completion.\n\nFor any given frame, prepare can only ever be called before that frame,\nand for the same frame process will be after.\n\n\nI don't believe we enforce any requirement for them to be called in\nlockstep, and at the overall design level - we shouldn't enforce that.\n\nYou could have (not specifically on RKISP)\n\n Frame 1 Frame 2 Frame 3 Frame 4 \n\n prepare(1)\n prepare(2)\n process(1)\n\t prepare(3)\n process(2)\n\t\t process(3)\n\t\t prepare(4)\n process(4)\n\n\n\n\n\n> >\n> >\n> > Is prepare() called before the sensor starts in the inline-ISP case with\n> > RKISP?\n\nThat part I don't know yet. But even if prepare() is called just before\nthe sensor starts for a given frame - it's absolutely not currently guaranteed that\nthe values applied to the sensor match the ActiveState values. So\nputting those values in the frameContext is misleading.\n\nIn theory we 'could' identify what is applied on the sensor and make up\nthe difference with an ISP digital gain to 'speed' up the convergence ?\nBut either way - we 'must' track what the sensor *actually* gets\nconfigured with here in the FrameContext.\n\nI think all that can be on top of these patches though - as long as we\nhighlight the issue here.\n\n> >\n> >\n> > > if (frame > 0)\n> > > return;\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > index be8932040c8e..d6c6fb130fc1 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > @@ -34,7 +34,8 @@ public:\n> > > const rkisp1_stat_buffer *stats) override;\n> > >\n> > > private:\n> > > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> > > + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> > > + double yGain, double iqMeanGain);\n> > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> > > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > index 211428717838..88e0631c8d39 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\var IPAActiveState::agc\n> > > * \\brief State for the Automatic Gain Control algorithm\n> > > *\n> > > - * The exposure and gain determined are expected to be applied to the sensor\n> > > - * at the earliest opportunity.\n> > > + * The exposure and gain are the latest values computed by the AGC algorithm.\n> > > *\n> > > * \\var IPAActiveState::agc.exposure\n> > > * \\brief Exposure time expressed as a number of lines\n> > > *\n> > > * \\var IPAActiveState::agc.gain\n> > > * \\brief Analogue gain multiplier\n> > > - *\n> > > - * The gain should be adapted to the sensor specific gain code before applying.\n> > > */\n> > >\n> > > /**\n> > > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\brief Indicates if ISP parameters need to be updated\n> > > */\n> > >\n> > > -/**\n> > > - * \\var IPAActiveState::sensor\n> > > - * \\brief Effective sensor values\n> > > - *\n> > > - * \\var IPAActiveState::sensor.exposure\n> > > - * \\brief Exposure time expressed as a number of lines\n> > > - *\n> > > - * \\var IPAActiveState::sensor.gain\n> > > - * \\brief Analogue gain multiplier\n> > > - */\n> > > -\n> > > /**\n> > > * \\struct RkISP1FrameContext\n> > > * \\brief Per-frame context for algorithms\n> > > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> > > * \\todo Populate the frame context for all algorithms\n> > > */\n> > >\n> > > +/**\n> > > + * \\var RkISP1FrameContext::agc\n> > > + * \\brief Automatic Gain Control parameters for this frame\n> > > + *\n> > > + * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > + * applied to the sensor in order to take effect for this frame.\n> > > + *\n> > > + * \\var RkISP1FrameContext::agc.exposure\n> > > + * \\brief Exposure time expressed as a number of lines\n> > > + *\n> > > + * \\var RkISP1FrameContext::agc.gain\n> > > + * \\brief Analogue gain multiplier\n> > > + *\n> > > + * The gain should be adapted to the sensor specific gain code before applying.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var RkISP1FrameContext::sensor\n> > > + * \\brief Sensor configuration that used been used for this frame\n> > > + *\n> > > + * \\var RkISP1FrameContext::sensor.exposure\n> > > + * \\brief Exposure time expressed as a number of lines\n> > > + *\n> > > + * \\var RkISP1FrameContext::sensor.gain\n> > > + * \\brief Analogue gain multiplier\n> > > + */\n> > > +\n> > > /**\n> > > * \\struct IPAContext\n> > > * \\brief Global IPA context data shared between all algorithms\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 4e38ef40ed3c..ecf993cd22d7 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -82,14 +82,18 @@ struct IPAActiveState {\n> > > uint8_t sharpness;\n> > > bool updateParams;\n> > > } filter;\n> > > -\n> > > - struct {\n> > > - uint32_t exposure;\n> > > - double gain;\n> > > - } sensor;\n> > > };\n> > >\n> > > struct RkISP1FrameContext : public FrameContext {\n> > > + struct {\n> > > + uint32_t exposure;\n> > > + double gain;\n> > > + } agc;\n> > > +\n> > > + struct {\n> > > + uint32_t exposure;\n> > > + double gain;\n> > > + } sensor;\n> > > };\n> > >\n> > > struct IPAContext {\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index 4f6e0b0f87b9..a23cfc7fb24d 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > reinterpret_cast(\n> > > mappedBuffers_.at(bufferId).planes()[0].data());\n> > >\n> > > - context_.activeState.sensor.exposure =\n> > > + frameContext.sensor.exposure =\n> > > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > - context_.activeState.sensor.gain =\n> > > + frameContext.sensor.gain =\n> > > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> >\n> > Aha - good - so here we're getting the 'effective' values that were set\n> > at the sensor, before we do any processing with them.\n> >\n> > So this is the point that would be interesting to compare 'what we asked\n> > for' ... 'vs what we actually got'.\n> >\n> >\n> > >\n> > > unsigned int aeState = 0;\n> > > @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > >\n> > > void IPARkISP1::setControls(unsigned int frame)\n> > > {\n> > > - uint32_t exposure = context_.activeState.agc.exposure;\n> > > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> > > + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> > > + uint32_t exposure = frameContext.agc.exposure;\n> > > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n> >\n> > So this is the part where we need to figure out more regarding PFC ...\n> > but that's a separate topic for the moment.\n> >\n> \n> I might have missed why ? :)\n\nBecause the current implementations as I understand it mean that we are\ncalling setControls for a given frame, and those controls will then be\n'applied' in 2 or 3 frames time. So ... not actaully the frame\nreferenced by the FrameContext.\n\n\n> > So while this patch won't meet our PFC requriements - I think that's\n> > fine and this helps build what we need to get there.\n> >\n> >\n> > I'm a bit more hesitant here (hence leaving this to come back to) ...\n> > but I don't think this is 'worse' than where we already are - and\n> > progresses the frame context conversions.\n> >\n> > Perhaps with some more notes / a todo on the fact that we need to do\n> > more about handling of setting exposure / gains for a specific frame...\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > >\n> > > ControlList ctrls(ctrls_);\n> > > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\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 30C59C0DA4\n\tfor ;\n\tThu, 22 Sep 2022 08:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FB9D62200;\n\tThu, 22 Sep 2022 10:59:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4E00B6219B\n\tfor ;\n\tThu, 22 Sep 2022 10:59:26 +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 A2FC06BB;\n\tThu, 22 Sep 2022 10:59:25 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663837167;\n\tbh=5vwn159R3/Tf4yvmCWIjjQYXFKRoekW4eoHjmk2qxhM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=0w34UCFcckwaA6jISlUJfYCr16221UizgWv2qNzZjPk3zfl6AEP6AbAwhZp5AWgcQ\n\tpvPu0WVRS/hy+6Q25aImgTH+2DlXxATFdJlHnKzgDBtwj1pJqd/6vYRabuj/NHQeOO\n\twhmY2wi67nM0jsOqJRZZKi+Zak7vVilVSOz0/uZKxGYWhNzt2bK/LEgRlYxsoD2JPD\n\tJMkmgJrX+oyJG+QAAbFA7tg1/GKVAkNvyqetINQdL08dW5l82wf3/hejy2I/kfRm/D\n\tyOQC8e554twRUigIUU5rUx9P1FouqgTScmL/PPtQrSIh2XC0olaBzi24YOtLXO2BCJ\n\tLFOpu7v481FLw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663837165;\n\tbh=5vwn159R3/Tf4yvmCWIjjQYXFKRoekW4eoHjmk2qxhM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=FOyp+T4K0GRVlhombOvRAEupVB9w2/cdFKefjHZ4fVfo+qy2JK9b1PY+1IAb5CPyK\n\trd4ZHbnQzek59SR/Xz2YxHEtc6B7hVGBHwEbNI0fAFEGBoXxWl5JxnA0ci0P9PFyoh\n\tarl6AVsc9v8X191xYxlZmoC8RD9S/hKqvWZmholE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FOyp+T4K\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>\n\t<166371884168.18961.15851239113055930923@Monstersaurus>\n\t<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>","To":"Jacopo Mondi ","Date":"Thu, 22 Sep 2022 09:59:23 +0100","Message-ID":"<166383716310.56880.14265919686115696723@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25074,"web_url":"https://patchwork.libcamera.org/comment/25074/","msgid":"<20220922101702.i6hsqbdj3yfqrbv6@uno.localdomain>","date":"2022-09-22T10:17:02","subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Thu, Sep 22, 2022 at 09:59:23AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2022-09-21 21:13:53)\n> > Hi Laurent, Kieran,\n> >\n> > On Wed, Sep 21, 2022 at 01:07:21AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)\n> > > > Rework the algorithm's usage of the active state to store the value of\n> > > > controls for the last queued request in the queueRequest() function, and\n> > > > store a copy of the values in the corresponding frame context.\n> > > >\n> > > > The frame context is used in the prepare() function to populate the ISP\n> > > > parameters with values corresponding to the right frame.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----\n> > > > src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> > > > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> > > > src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> > > > src/ipa/rkisp1/rkisp1.cpp | 9 ++++---\n> > > > 5 files changed, 55 insertions(+), 32 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index 4124c3272bc7..e7198169d11b 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> > > > /**\n> > > > * \\brief Estimate the new exposure and gain values\n> > > > * \\param[inout] context The shared IPA Context\n> > > > + * \\param[in] frameContext The FrameContext for this frame\n> > > > * \\param[in] yGain The gain calculated on the current brightness level\n> > > > * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> > > > */\n> > > > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> > > > +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> > > > + double yGain, double iqMeanGain)\n> > > > {\n> > > > IPASessionConfiguration &configuration = context.configuration;\n> > > > IPAActiveState &activeState = context.activeState;\n> > > >\n> > > > /* Get the effective exposure and gain applied on the sensor. */\n> > > > - uint32_t exposure = activeState.sensor.exposure;\n> > > > - double analogueGain = activeState.sensor.gain;\n> > > > + uint32_t exposure = frameContext.sensor.exposure;\n> > > > + double analogueGain = frameContext.sensor.gain;\n> > > >\n> > > > /* Use the highest of the two gain estimates. */\n> > > > double evGain = std::max(yGain, iqMeanGain);\n> > > > @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> > > > * new exposure and gain for the scene.\n> > > > */\n> > > > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > + RkISP1FrameContext &frameContext,\n> > > > const rkisp1_stat_buffer *stats)\n> > > > {\n> > > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > > @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > break;\n> > > > }\n> > > >\n> > > > - computeExposure(context, yGain, iqMeanGain);\n> > > > + computeExposure(context, frameContext, yGain, iqMeanGain);\n> > >\n> > > It may be useful to compare somewhere here the values for gain/exposure\n> > > that we expected to be set on the sensor, as stored in our frame\n> > > context, with the values that could be retrieved from the\n> > > DelayedControls implementation to make sure they are\n> > > consistent/coherrent.\n> > >\n> > >\n> > > > frameCount_++;\n> > > > }\n> > > >\n> > > > @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > */\n> > > > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > - rkisp1_params_cfg *params)\n> > > > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > > > {\n> > > > + frameContext.agc.exposure = context.activeState.agc.exposure;\n> > > > + frameContext.agc.gain = context.activeState.agc.gain;\n> > > > +\n> > >\n> > > This may be a little dubious ... due to the delays inherrent with\n> > > setting a gain and exposure...\n> > >\n> > > If they are not known to be exactly the values used by the sensor for\n> > > this frame, but we still need to store them here, then I'd add a todo or\n> > > a warning note...\n> >\n> > Do I read the code and the comment here right, and this mean we report\n> > as frameContext.agc (== the agc values computed for this frame) the\n> > last values computed by the AGC algorithm overall, which might be\n> > different than the ones computed for the last frame, if we have two\n> > consecutive calls to process() (which calls computeExposure()).\n>\n> What are you distinguising between 'the last frame' and the 'last value\n> computed by AGC?'.\n>\n> process() is currently always the last step where the AGC actually runs,\n> so the ActiveState reports the 'best'/'most-up-to-date' desired values\n> that the AGC would like to use for automatic mode.\n\nThat's my point, if process() has been called two consecutive times\n(n, n + 1) we store in the FrameContext 'n' the values computed for\nframe 'n + 1'\n\nI'm not even sure that's an issue, just making sure this is known..\n\n>\n> However - the issue I have here is that for Sensor controls - this point\n> in the time line is too late to apply those values.\n>\n\nOn this point I'm not concerned, but I might be missing something.\nWe'll make sure we process frames early enough, and the values we want\nto have applied at frame X will be computed here and correctly applied\nin advance, so what we report here should be what will actually be\nrealized for that frame ?\n\n> We can make adjustments to the gains on the ISP here - but not the\n> sensor.\n>\n> We (already know that we) need to identify a way to ensure we\n> syncrhonise DelayedControls actions both here on the IPA side along with\n> the Pipeline Handler side.\n>\n> That could be either communicating with DelayedControls, or changing the\n> current implementation (I've thought about 'scheduled controls' in the\n> past, but it's all still WIP).\n>\n>\n> > Am I wrong thinking that prepare() and process() are synched to events\n> > with different clocks ? prepare() is called in the \"frame ready\" call\n> > path, which process() in the ISP \"stats ready\" call path. Do we have\n> > guarantees that the two are in sync ?\n>\n>\n> prepare() is to generate configuration for the ISP.\n> process() reports the statistics of the ISP after completion.\n>\n> For any given frame, prepare can only ever be called before that frame,\n> and for the same frame process will be after.\n>\n\nI was using IPU3 as a refernce as there fillParmsBuffer() (which calls\nalgo->prepare()) is called on the CIO2 buffer ready event, so the\nassumption that prepare() is called before that frame is produced does\nnot hold there ?\n\n>\n> I don't believe we enforce any requirement for them to be called in\n> lockstep, and at the overall design level - we shouldn't enforce that.\n>\n> You could have (not specifically on RKISP)\n>\n> Frame 1 Frame 2 Frame 3 Frame 4\n>\n> prepare(1)\n> prepare(2)\n> process(1)\n> \t prepare(3)\n\nLooking at the RkISP1 AWB:\n\nExactly, here in prepare(3) we store in the FrameContext(3) the automatic gains\ncomputed on process(1). I guess that's ok but\n\n> process(2)\n> \t\t process(3)\n> \t\t prepare(4)\n\nhere in prepare(4) we'll use the ones computed in process(3)\n\nSo there's no actually guarantee of which frames the auto gains we\nstore in the FrameContext() have been computed on. I guess that's not\nmuch relevant for automatic modes ?\n\n> process(4)\n>\n>\n>\n>\n>\n> > >\n> > >\n> > > Is prepare() called before the sensor starts in the inline-ISP case with\n> > > RKISP?\n>\n> That part I don't know yet. But even if prepare() is called just before\n> the sensor starts for a given frame - it's absolutely not currently guaranteed that\n> the values applied to the sensor match the ActiveState values. So\n> putting those values in the frameContext is misleading.\n>\n> In theory we 'could' identify what is applied on the sensor and make up\n> the difference with an ISP digital gain to 'speed' up the convergence ?\n> But either way - we 'must' track what the sensor *actually* gets\n> configured with here in the FrameContext.\n>\n> I think all that can be on top of these patches though - as long as we\n> highlight the issue here.\n>\n\nAbsolutely, nothing blocking the series here, it's already a move in\nthe right direction and if we have to improve we should be doing that\non top.\n\n\n> > >\n> > >\n> > > > if (frame > 0)\n> > > > return;\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > > index be8932040c8e..d6c6fb130fc1 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > > @@ -34,7 +34,8 @@ public:\n> > > > const rkisp1_stat_buffer *stats) override;\n> > > >\n> > > > private:\n> > > > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> > > > + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> > > > + double yGain, double iqMeanGain);\n> > > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> > > > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > index 211428717838..88e0631c8d39 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> > > > * \\var IPAActiveState::agc\n> > > > * \\brief State for the Automatic Gain Control algorithm\n> > > > *\n> > > > - * The exposure and gain determined are expected to be applied to the sensor\n> > > > - * at the earliest opportunity.\n> > > > + * The exposure and gain are the latest values computed by the AGC algorithm.\n> > > > *\n> > > > * \\var IPAActiveState::agc.exposure\n> > > > * \\brief Exposure time expressed as a number of lines\n> > > > *\n> > > > * \\var IPAActiveState::agc.gain\n> > > > * \\brief Analogue gain multiplier\n> > > > - *\n> > > > - * The gain should be adapted to the sensor specific gain code before applying.\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> > > > * \\brief Indicates if ISP parameters need to be updated\n> > > > */\n> > > >\n> > > > -/**\n> > > > - * \\var IPAActiveState::sensor\n> > > > - * \\brief Effective sensor values\n> > > > - *\n> > > > - * \\var IPAActiveState::sensor.exposure\n> > > > - * \\brief Exposure time expressed as a number of lines\n> > > > - *\n> > > > - * \\var IPAActiveState::sensor.gain\n> > > > - * \\brief Analogue gain multiplier\n> > > > - */\n> > > > -\n> > > > /**\n> > > > * \\struct RkISP1FrameContext\n> > > > * \\brief Per-frame context for algorithms\n> > > > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> > > > * \\todo Populate the frame context for all algorithms\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\var RkISP1FrameContext::agc\n> > > > + * \\brief Automatic Gain Control parameters for this frame\n> > > > + *\n> > > > + * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > > + * applied to the sensor in order to take effect for this frame.\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::agc.exposure\n> > > > + * \\brief Exposure time expressed as a number of lines\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::agc.gain\n> > > > + * \\brief Analogue gain multiplier\n> > > > + *\n> > > > + * The gain should be adapted to the sensor specific gain code before applying.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var RkISP1FrameContext::sensor\n> > > > + * \\brief Sensor configuration that used been used for this frame\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::sensor.exposure\n> > > > + * \\brief Exposure time expressed as a number of lines\n> > > > + *\n> > > > + * \\var RkISP1FrameContext::sensor.gain\n> > > > + * \\brief Analogue gain multiplier\n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\struct IPAContext\n> > > > * \\brief Global IPA context data shared between all algorithms\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 4e38ef40ed3c..ecf993cd22d7 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -82,14 +82,18 @@ struct IPAActiveState {\n> > > > uint8_t sharpness;\n> > > > bool updateParams;\n> > > > } filter;\n> > > > -\n> > > > - struct {\n> > > > - uint32_t exposure;\n> > > > - double gain;\n> > > > - } sensor;\n> > > > };\n> > > >\n> > > > struct RkISP1FrameContext : public FrameContext {\n> > > > + struct {\n> > > > + uint32_t exposure;\n> > > > + double gain;\n> > > > + } agc;\n> > > > +\n> > > > + struct {\n> > > > + uint32_t exposure;\n> > > > + double gain;\n> > > > + } sensor;\n> > > > };\n> > > >\n> > > > struct IPAContext {\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index 4f6e0b0f87b9..a23cfc7fb24d 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > reinterpret_cast(\n> > > > mappedBuffers_.at(bufferId).planes()[0].data());\n> > > >\n> > > > - context_.activeState.sensor.exposure =\n> > > > + frameContext.sensor.exposure =\n> > > > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > - context_.activeState.sensor.gain =\n> > > > + frameContext.sensor.gain =\n> > > > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > >\n> > > Aha - good - so here we're getting the 'effective' values that were set\n> > > at the sensor, before we do any processing with them.\n> > >\n> > > So this is the point that would be interesting to compare 'what we asked\n> > > for' ... 'vs what we actually got'.\n> > >\n> > >\n> > > >\n> > > > unsigned int aeState = 0;\n> > > > @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > >\n> > > > void IPARkISP1::setControls(unsigned int frame)\n> > > > {\n> > > > - uint32_t exposure = context_.activeState.agc.exposure;\n> > > > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> > > > + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > + uint32_t exposure = frameContext.agc.exposure;\n> > > > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n> > >\n> > > So this is the part where we need to figure out more regarding PFC ...\n> > > but that's a separate topic for the moment.\n> > >\n> >\n> > I might have missed why ? :)\n>\n> Because the current implementations as I understand it mean that we are\n> calling setControls for a given frame, and those controls will then be\n> 'applied' in 2 or 3 frames time. So ... not actaully the frame\n> referenced by the FrameContext.\n\nHow so ? Don't we assume we operate early enough to have the here\ncomputed values are applied in time to have them realized at the right\nframe ?\n\n>\n>\n> > > So while this patch won't meet our PFC requriements - I think that's\n> > > fine and this helps build what we need to get there.\n> > >\n> > >\n> > > I'm a bit more hesitant here (hence leaving this to come back to) ...\n> > > but I don't think this is 'worse' than where we already are - and\n> > > progresses the frame context conversions.\n> > >\n> > > Perhaps with some more notes / a todo on the fact that we need to do\n> > > more about handling of setting exposure / gains for a specific frame...\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > >\n> > > > ControlList ctrls(ctrls_);\n> > > > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> > > > --\n> > > > Regards,\n> > > >\n> > > > Laurent Pinchart\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 AAE7FC3272\n\tfor ;\n\tThu, 22 Sep 2022 10:17:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2ACB62202;\n\tThu, 22 Sep 2022 12:17:06 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::221])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 263CD6219A\n\tfor ;\n\tThu, 22 Sep 2022 12:17:06 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A30B5240007;\n\tThu, 22 Sep 2022 10:17:04 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663841826;\n\tbh=SVCnR+BjFFNwKcC4CHDiy3YQ1GjKbyJjetwS1fBlcbI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=fztBIEvcCUrYdcIkKw5e8Ai83IGFHVuUT5kFyBE16/luYx6jashUdM3UY65m7Yg2x\n\tb85sQEraTRFopO6Qkxsrs39IECHIlIYnK4KUtfTHyQ9ZkfXUDaiYgk8PML/zP0Bpqu\n\tjiDwYZSmwaVprYIoUGScvg7Gn6rMVI9I0qbpEREw3/QBEInMPvRM/Bh2B6XJvGwWi7\n\tOUzYdvFQ0kh/d+KWJyE70QgZsc3jmAeIk0nyMYg/TXz6Stl3TpUSrLtUnoHCmYW4xn\n\tU19QvTbqbHsuxcOvNP/FoeZGZXKVPFW9Evxh1Zi2DvsYZ5RzW7ZWnY/2VjuohMG5PX\n\tQvZPc0GdQO4qw==","Date":"Thu, 22 Sep 2022 12:17:02 +0200","To":"Kieran Bingham ","Message-ID":"<20220922101702.i6hsqbdj3yfqrbv6@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>\n\t<166371884168.18961.15851239113055930923@Monstersaurus>\n\t<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>\n\t<166383716310.56880.14265919686115696723@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166383716310.56880.14265919686115696723@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25085,"web_url":"https://patchwork.libcamera.org/comment/25085/","msgid":"<166384861879.56880.6604830110425767187@Monstersaurus>","date":"2022-09-22T12:10:18","subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2022-09-22 11:17:02)\n> Hi Kieran\n> \n> On Thu, Sep 22, 2022 at 09:59:23AM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2022-09-21 21:13:53)\n> > > Hi Laurent, Kieran,\n> > >\n> > > On Wed, Sep 21, 2022 at 01:07:21AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)\n> > > > > Rework the algorithm's usage of the active state to store the value of\n> > > > > controls for the last queued request in the queueRequest() function, and\n> > > > > store a copy of the values in the corresponding frame context.\n> > > > >\n> > > > > The frame context is used in the prepare() function to populate the ISP\n> > > > > parameters with values corresponding to the right frame.\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----\n> > > > > src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> > > > > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> > > > > src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> > > > > src/ipa/rkisp1/rkisp1.cpp | 9 ++++---\n> > > > > 5 files changed, 55 insertions(+), 32 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > index 4124c3272bc7..e7198169d11b 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> > > > > /**\n> > > > > * \\brief Estimate the new exposure and gain values\n> > > > > * \\param[inout] context The shared IPA Context\n> > > > > + * \\param[in] frameContext The FrameContext for this frame\n> > > > > * \\param[in] yGain The gain calculated on the current brightness level\n> > > > > * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> > > > > */\n> > > > > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> > > > > +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> > > > > + double yGain, double iqMeanGain)\n> > > > > {\n> > > > > IPASessionConfiguration &configuration = context.configuration;\n> > > > > IPAActiveState &activeState = context.activeState;\n> > > > >\n> > > > > /* Get the effective exposure and gain applied on the sensor. */\n> > > > > - uint32_t exposure = activeState.sensor.exposure;\n> > > > > - double analogueGain = activeState.sensor.gain;\n> > > > > + uint32_t exposure = frameContext.sensor.exposure;\n> > > > > + double analogueGain = frameContext.sensor.gain;\n> > > > >\n> > > > > /* Use the highest of the two gain estimates. */\n> > > > > double evGain = std::max(yGain, iqMeanGain);\n> > > > > @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> > > > > * new exposure and gain for the scene.\n> > > > > */\n> > > > > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > > + RkISP1FrameContext &frameContext,\n> > > > > const rkisp1_stat_buffer *stats)\n> > > > > {\n> > > > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > > > @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > break;\n> > > > > }\n> > > > >\n> > > > > - computeExposure(context, yGain, iqMeanGain);\n> > > > > + computeExposure(context, frameContext, yGain, iqMeanGain);\n> > > >\n> > > > It may be useful to compare somewhere here the values for gain/exposure\n> > > > that we expected to be set on the sensor, as stored in our frame\n> > > > context, with the values that could be retrieved from the\n> > > > DelayedControls implementation to make sure they are\n> > > > consistent/coherrent.\n> > > >\n> > > >\n> > > > > frameCount_++;\n> > > > > }\n> > > > >\n> > > > > @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > */\n> > > > > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > > - rkisp1_params_cfg *params)\n> > > > > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > > > > {\n> > > > > + frameContext.agc.exposure = context.activeState.agc.exposure;\n> > > > > + frameContext.agc.gain = context.activeState.agc.gain;\n\nSo perhaps what this should be setting is\n'frameContext.agc.desiredExposure'\n\nIn otherwords, at the time of processing this image, the AGC ran and\ndetermined it would prefer to have an exposure of X\n\nBut currently this reads to me as...\n\nThis frame's Exposure is X\n\nWhich is completely wrong.\n\n\n> > > > > +\n> > > >\n> > > > This may be a little dubious ... due to the delays inherrent with\n> > > > setting a gain and exposure...\n> > > >\n> > > > If they are not known to be exactly the values used by the sensor for\n> > > > this frame, but we still need to store them here, then I'd add a todo or\n> > > > a warning note...\n> > >\n> > > Do I read the code and the comment here right, and this mean we report\n> > > as frameContext.agc (== the agc values computed for this frame) the\n> > > last values computed by the AGC algorithm overall, which might be\n> > > different than the ones computed for the last frame, if we have two\n> > > consecutive calls to process() (which calls computeExposure()).\n> >\n> > What are you distinguising between 'the last frame' and the 'last value\n> > computed by AGC?'.\n> >\n> > process() is currently always the last step where the AGC actually runs,\n> > so the ActiveState reports the 'best'/'most-up-to-date' desired values\n> > that the AGC would like to use for automatic mode.\n> \n> That's my point, if process() has been called two consecutive times\n> (n, n + 1) we store in the FrameContext 'n' the values computed for\n> frame 'n + 1'\n> \n> I'm not even sure that's an issue, just making sure this is known..\n\nIf we end up prepareing two frames for the ISP at 'the same time' ...\nthen yes, I would expect them to have the same values, as that is the\n'most up to date information we have at that time'.\n\n> > However - the issue I have here is that for Sensor controls - this point\n> > in the time line is too late to apply those values.\n> >\n> \n> On this point I'm not concerned, but I might be missing something.\n> We'll make sure we process frames early enough, and the values we want\n> to have applied at frame X will be computed here and correctly applied\n> in advance, so what we report here should be what will actually be\n> realized for that frame ?\n\n\"We will \" ... do this is fine, but I don't think we do right now. That\nwas my only highlight. I think this is fine for us to keep building on\ntop of though.\n\n\n> > We can make adjustments to the gains on the ISP here - but not the\n> > sensor.\n> >\n> > We (already know that we) need to identify a way to ensure we\n> > syncrhonise DelayedControls actions both here on the IPA side along with\n> > the Pipeline Handler side.\n> >\n> > That could be either communicating with DelayedControls, or changing the\n> > current implementation (I've thought about 'scheduled controls' in the\n> > past, but it's all still WIP).\n> >\n> >\n> > > Am I wrong thinking that prepare() and process() are synched to events\n> > > with different clocks ? prepare() is called in the \"frame ready\" call\n> > > path, which process() in the ISP \"stats ready\" call path. Do we have\n> > > guarantees that the two are in sync ?\n> >\n> >\n> > prepare() is to generate configuration for the ISP.\n> > process() reports the statistics of the ISP after completion.\n> >\n> > For any given frame, prepare can only ever be called before that frame,\n> > and for the same frame process will be after.\n> >\n> \n> I was using IPU3 as a refernce as there fillParmsBuffer() (which calls\n> algo->prepare()) is called on the CIO2 buffer ready event, so the\n> assumption that prepare() is called before that frame is produced does\n> not hold there ?\n\nAha, I meant 'prepare' is called before the frame is processed by the\nISP, and process() is called after the frame is processed by the ISP...\n\n\n> > I don't believe we enforce any requirement for them to be called in\n> > lockstep, and at the overall design level - we shouldn't enforce that.\n> >\n> > You could have (not specifically on RKISP)\n> >\n> > Frame 1 Frame 2 Frame 3 Frame 4\n> >\n> > prepare(1)\n> > prepare(2)\n> > process(1)\n> > prepare(3)\n> \n> Looking at the RkISP1 AWB:\n> \n> Exactly, here in prepare(3) we store in the FrameContext(3) the automatic gains\n> computed on process(1). I guess that's ok but\n> \n> > process(2)\n> > process(3)\n> > prepare(4)\n> \n> here in prepare(4) we'll use the ones computed in process(3)\n> \n> So there's no actually guarantee of which frames the auto gains we\n> store in the FrameContext() have been computed on. I guess that's not\n> much relevant for automatic modes ?\n\nFor automatic - no, it shouldn't matter. As far as I'm aware, we go with\n'the most up to date information we have at the time we are preparing\nthat frame for the ISP to process' ... i.e. the current ActiveState.\n\n> > process(4)\n> >\n> >\n> > > >\n> > > >\n> > > > Is prepare() called before the sensor starts in the inline-ISP case with\n> > > > RKISP?\n> >\n> > That part I don't know yet. But even if prepare() is called just before\n> > the sensor starts for a given frame - it's absolutely not currently guaranteed that\n> > the values applied to the sensor match the ActiveState values. So\n> > putting those values in the frameContext is misleading.\n> >\n> > In theory we 'could' identify what is applied on the sensor and make up\n> > the difference with an ISP digital gain to 'speed' up the convergence ?\n> > But either way - we 'must' track what the sensor *actually* gets\n> > configured with here in the FrameContext.\n> >\n> > I think all that can be on top of these patches though - as long as we\n> > highlight the issue here.\n> >\n> \n> Absolutely, nothing blocking the series here, it's already a move in\n> the right direction and if we have to improve we should be doing that\n> on top.\n\nAck.\n\n> > > >\n> > > > > if (frame > 0)\n> > > > > return;\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > > > index be8932040c8e..d6c6fb130fc1 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > > > @@ -34,7 +34,8 @@ public:\n> > > > > const rkisp1_stat_buffer *stats) override;\n> > > > >\n> > > > > private:\n> > > > > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> > > > > + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> > > > > + double yGain, double iqMeanGain);\n> > > > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > > > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> > > > > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > index 211428717838..88e0631c8d39 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > * \\var IPAActiveState::agc\n> > > > > * \\brief State for the Automatic Gain Control algorithm\n> > > > > *\n> > > > > - * The exposure and gain determined are expected to be applied to the sensor\n> > > > > - * at the earliest opportunity.\n> > > > > + * The exposure and gain are the latest values computed by the AGC algorithm.\n> > > > > *\n> > > > > * \\var IPAActiveState::agc.exposure\n> > > > > * \\brief Exposure time expressed as a number of lines\n> > > > > *\n> > > > > * \\var IPAActiveState::agc.gain\n> > > > > * \\brief Analogue gain multiplier\n> > > > > - *\n> > > > > - * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > * \\brief Indicates if ISP parameters need to be updated\n> > > > > */\n> > > > >\n> > > > > -/**\n> > > > > - * \\var IPAActiveState::sensor\n> > > > > - * \\brief Effective sensor values\n> > > > > - *\n> > > > > - * \\var IPAActiveState::sensor.exposure\n> > > > > - * \\brief Exposure time expressed as a number of lines\n> > > > > - *\n> > > > > - * \\var IPAActiveState::sensor.gain\n> > > > > - * \\brief Analogue gain multiplier\n> > > > > - */\n> > > > > -\n> > > > > /**\n> > > > > * \\struct RkISP1FrameContext\n> > > > > * \\brief Per-frame context for algorithms\n> > > > > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > * \\todo Populate the frame context for all algorithms\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\var RkISP1FrameContext::agc\n> > > > > + * \\brief Automatic Gain Control parameters for this frame\n> > > > > + *\n> > > > > + * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > > > + * applied to the sensor in order to take effect for this frame.\n> > > > > + *\n> > > > > + * \\var RkISP1FrameContext::agc.exposure\n> > > > > + * \\brief Exposure time expressed as a number of lines\n> > > > > + *\n> > > > > + * \\var RkISP1FrameContext::agc.gain\n> > > > > + * \\brief Analogue gain multiplier\n> > > > > + *\n> > > > > + * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\var RkISP1FrameContext::sensor\n> > > > > + * \\brief Sensor configuration that used been used for this frame\n> > > > > + *\n> > > > > + * \\var RkISP1FrameContext::sensor.exposure\n> > > > > + * \\brief Exposure time expressed as a number of lines\n> > > > > + *\n> > > > > + * \\var RkISP1FrameContext::sensor.gain\n> > > > > + * \\brief Analogue gain multiplier\n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\struct IPAContext\n> > > > > * \\brief Global IPA context data shared between all algorithms\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index 4e38ef40ed3c..ecf993cd22d7 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > @@ -82,14 +82,18 @@ struct IPAActiveState {\n> > > > > uint8_t sharpness;\n> > > > > bool updateParams;\n> > > > > } filter;\n> > > > > -\n> > > > > - struct {\n> > > > > - uint32_t exposure;\n> > > > > - double gain;\n> > > > > - } sensor;\n> > > > > };\n> > > > >\n> > > > > struct RkISP1FrameContext : public FrameContext {\n> > > > > + struct {\n> > > > > + uint32_t exposure;\n> > > > > + double gain;\n> > > > > + } agc;\n> > > > > +\n> > > > > + struct {\n> > > > > + uint32_t exposure;\n> > > > > + double gain;\n> > > > > + } sensor;\n> > > > > };\n> > > > >\n> > > > > struct IPAContext {\n> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > index 4f6e0b0f87b9..a23cfc7fb24d 100644\n> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > > reinterpret_cast(\n> > > > > mappedBuffers_.at(bufferId).planes()[0].data());\n> > > > >\n> > > > > - context_.activeState.sensor.exposure =\n> > > > > + frameContext.sensor.exposure =\n> > > > > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > > - context_.activeState.sensor.gain =\n> > > > > + frameContext.sensor.gain =\n> > > > > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > > >\n> > > > Aha - good - so here we're getting the 'effective' values that were set\n> > > > at the sensor, before we do any processing with them.\n> > > >\n> > > > So this is the point that would be interesting to compare 'what we asked\n> > > > for' ... 'vs what we actually got'.\n> > > >\n> > > >\n> > > > >\n> > > > > unsigned int aeState = 0;\n> > > > > @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > >\n> > > > > void IPARkISP1::setControls(unsigned int frame)\n> > > > > {\n> > > > > - uint32_t exposure = context_.activeState.agc.exposure;\n> > > > > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> > > > > + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > + uint32_t exposure = frameContext.agc.exposure;\n> > > > > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n> > > >\n> > > > So this is the part where we need to figure out more regarding PFC ...\n> > > > but that's a separate topic for the moment.\n> > > >\n> > >\n> > > I might have missed why ? :)\n> >\n> > Because the current implementations as I understand it mean that we are\n> > calling setControls for a given frame, and those controls will then be\n> > 'applied' in 2 or 3 frames time. So ... not actaully the frame\n> > referenced by the FrameContext.\n> \n> How so ? Don't we assume we operate early enough to have the here\n> computed values are applied in time to have them realized at the right\n> frame ?\n\nNo ... I don't think we do right now.\n\nOhh - it's worse. It's all currently wrong ;-)\n\nIPARkISP1::processStatsBuffer(frame) {\n\t...\n\tsetControls(frame);\n\t...\n}\n\n\nSo - we're calling 'setControls' which then gets the FrameContext\n'frame'.\n\nThat is setting 'what the AGC' would like the sensor to be as soon as it\ncan be.\n\nBut it shouldn't be parsing \n FrameContext.agc.exposure\n\nTo me - that is 'what the exposure *is* of the frame related to\nFramecontext.\n\nIf we extend this with a new entry (referenced at the top of this mail)\nsuch as :\n\n FrameContext.agc.desiredExposure\n\nOr such - then I'd be happier with this in the short term. But I really\ndon't want to mix up \"What the AGC would like\" and \"What the Frame\nexposure is\" in a single field of the frame context.\n\n\n\n\n> > > > So while this patch won't meet our PFC requriements - I think that's\n> > > > fine and this helps build what we need to get there.\n> > > >\n> > > >\n> > > > I'm a bit more hesitant here (hence leaving this to come back to) ...\n> > > > but I don't think this is 'worse' than where we already are - and\n> > > > progresses the frame context conversions.\n> > > >\n> > > > Perhaps with some more notes / a todo on the fact that we need to do\n> > > > more about handling of setting exposure / gains for a specific frame...\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > >\n> > > > > ControlList ctrls(ctrls_);\n> > > > > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> > > > > --\n> > > > > Regards,\n> > > > >\n> > > > > Laurent Pinchart\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 8C746C0DA4\n\tfor ;\n\tThu, 22 Sep 2022 12:10:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D56B26220C;\n\tThu, 22 Sep 2022 14:10:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 532246219A\n\tfor ;\n\tThu, 22 Sep 2022 14:10:22 +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 AA600107;\n\tThu, 22 Sep 2022 14:10:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663848623;\n\tbh=YKDu8pQk+KE7HrjlFX6LaayjGTAH2lWY894n6KHV5Wg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=spv4U+wJnFopPwCHs67oY/YeneyOX97A6yQLBprc7yOtA1xpvrq8enJJ377crwLOt\n\thWRDsRW1va31SJmmISzA3KB7iB6b+41/qIGiErAxktKXqBs3kCiXel+6dp+Q0y6Jli\n\tedjmlQNZ9ImCEiU3fwpZfiNDeDJ2s+HAxzZ+bCH1yXkXOwRdGK4IYCwFh4NDL2NNwR\n\trwUkd4GoQZcRi+dRpE+k7dFwA+uijDyW08fS1VN48zqW0BTzHTxscsHoQSlA4pzj7N\n\ttK09S9xS1KXNC+3XFYBvM2fogf1pATq42NDiH4otoUO/2iLTbBYlwqqDH9eVDZUxRn\n\tBKGJGc5vp3+Ow==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663848621;\n\tbh=YKDu8pQk+KE7HrjlFX6LaayjGTAH2lWY894n6KHV5Wg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hPigdIJ9ph3FSEkP+cjInaqGWvO6QywvnSXFbBQJg6vZUYz6NFg9PjQrWIUS3bH48\n\tI59TkEFwIZf4JYbi/aD77EdDxcrlaj2q6Z0rmG8dY47t8uIokjhAmw/UK0T0gZR1v9\n\tWPgUq/8fjNeVPxCKFNcVk/XXYdogDsAOidLDWY9k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hPigdIJ9\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220922101702.i6hsqbdj3yfqrbv6@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>\n\t<166371884168.18961.15851239113055930923@Monstersaurus>\n\t<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>\n\t<166383716310.56880.14265919686115696723@Monstersaurus>\n\t<20220922101702.i6hsqbdj3yfqrbv6@uno.localdomain>","To":"Jacopo Mondi ","Date":"Thu, 22 Sep 2022 13:10:18 +0100","Message-ID":"<166384861879.56880.6604830110425767187@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25098,"web_url":"https://patchwork.libcamera.org/comment/25098/","msgid":"","date":"2022-09-22T22:07:45","subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Sep 22, 2022 at 01:10:18PM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2022-09-22 11:17:02)\n> > On Thu, Sep 22, 2022 at 09:59:23AM +0100, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi (2022-09-21 21:13:53)\n> > > > On Wed, Sep 21, 2022 at 01:07:21AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)\n> > > > > > Rework the algorithm's usage of the active state to store the value of\n> > > > > > controls for the last queued request in the queueRequest() function, and\n> > > > > > store a copy of the values in the corresponding frame context.\n> > > > > >\n> > > > > > The frame context is used in the prepare() function to populate the ISP\n> > > > > > parameters with values corresponding to the right frame.\n> > > > > >\n> > > > > > Signed-off-by: Laurent Pinchart \n> > > > > > ---\n> > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----\n> > > > > > src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> > > > > > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> > > > > > src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> > > > > > src/ipa/rkisp1/rkisp1.cpp | 9 ++++---\n> > > > > > 5 files changed, 55 insertions(+), 32 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > index 4124c3272bc7..e7198169d11b 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> > > > > > /**\n> > > > > > * \\brief Estimate the new exposure and gain values\n> > > > > > * \\param[inout] context The shared IPA Context\n> > > > > > + * \\param[in] frameContext The FrameContext for this frame\n> > > > > > * \\param[in] yGain The gain calculated on the current brightness level\n> > > > > > * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> > > > > > */\n> > > > > > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> > > > > > +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> > > > > > + double yGain, double iqMeanGain)\n> > > > > > {\n> > > > > > IPASessionConfiguration &configuration = context.configuration;\n> > > > > > IPAActiveState &activeState = context.activeState;\n> > > > > >\n> > > > > > /* Get the effective exposure and gain applied on the sensor. */\n> > > > > > - uint32_t exposure = activeState.sensor.exposure;\n> > > > > > - double analogueGain = activeState.sensor.gain;\n> > > > > > + uint32_t exposure = frameContext.sensor.exposure;\n> > > > > > + double analogueGain = frameContext.sensor.gain;\n> > > > > >\n> > > > > > /* Use the highest of the two gain estimates. */\n> > > > > > double evGain = std::max(yGain, iqMeanGain);\n> > > > > > @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> > > > > > * new exposure and gain for the scene.\n> > > > > > */\n> > > > > > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > > > + RkISP1FrameContext &frameContext,\n> > > > > > const rkisp1_stat_buffer *stats)\n> > > > > > {\n> > > > > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > > > > @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > break;\n> > > > > > }\n> > > > > >\n> > > > > > - computeExposure(context, yGain, iqMeanGain);\n> > > > > > + computeExposure(context, frameContext, yGain, iqMeanGain);\n> > > > >\n> > > > > It may be useful to compare somewhere here the values for gain/exposure\n> > > > > that we expected to be set on the sensor, as stored in our frame\n> > > > > context, with the values that could be retrieved from the\n> > > > > DelayedControls implementation to make sure they are\n> > > > > consistent/coherrent.\n\nI'll add a todo comment. Note that it doesn't matter that much if we\nreceive what we've asked, as long as the values we get correspond to\nwhat has been applied to the frame. That's the important part to ensure\nthe algorithm is stable. Synchronizing the values computed by the\nalgorithm with a particular frame is only important in manual mode.\n\n> > > > > > frameCount_++;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > > */\n> > > > > > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > > > - rkisp1_params_cfg *params)\n> > > > > > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > > > > > {\n> > > > > > + frameContext.agc.exposure = context.activeState.agc.exposure;\n> > > > > > + frameContext.agc.gain = context.activeState.agc.gain;\n> \n> So perhaps what this should be setting is\n> 'frameContext.agc.desiredExposure'\n> \n> In otherwords, at the time of processing this image, the AGC ran and\n> determined it would prefer to have an exposure of X\n> \n> But currently this reads to me as...\n> \n> This frame's Exposure is X\n> \n> Which is completely wrong.\n\nThe exposure and gain members are documented as\n\n * The exposure and gain are the latest values computed by the AGC algorithm.\n\nI don't want to call them desiredExposure and desiredGain, as we would\nthen have to prefix pretty much everything with either \"desired\" or\n\"computed\".\n\n> > > > > > +\n> > > > >\n> > > > > This may be a little dubious ... due to the delays inherrent with\n> > > > > setting a gain and exposure...\n> > > > >\n> > > > > If they are not known to be exactly the values used by the sensor for\n> > > > > this frame, but we still need to store them here, then I'd add a todo or\n> > > > > a warning note...\n> > > >\n> > > > Do I read the code and the comment here right, and this mean we report\n> > > > as frameContext.agc (== the agc values computed for this frame) the\n> > > > last values computed by the AGC algorithm overall, which might be\n> > > > different than the ones computed for the last frame, if we have two\n> > > > consecutive calls to process() (which calls computeExposure()).\n> > >\n> > > What are you distinguising between 'the last frame' and the 'last value\n> > > computed by AGC?'.\n> > >\n> > > process() is currently always the last step where the AGC actually runs,\n> > > so the ActiveState reports the 'best'/'most-up-to-date' desired values\n> > > that the AGC would like to use for automatic mode.\n> > \n> > That's my point, if process() has been called two consecutive times\n> > (n, n + 1) we store in the FrameContext 'n' the values computed for\n> > frame 'n + 1'\n> > \n> > I'm not even sure that's an issue, just making sure this is known..\n> \n> If we end up prepareing two frames for the ISP at 'the same time' ...\n> then yes, I would expect them to have the same values, as that is the\n> 'most up to date information we have at that time'.\n\nThat matches my understanding.\n\n> > > However - the issue I have here is that for Sensor controls - this point\n> > > in the time line is too late to apply those values.\n> > \n> > On this point I'm not concerned, but I might be missing something.\n> > We'll make sure we process frames early enough, and the values we want\n> > to have applied at frame X will be computed here and correctly applied\n> > in advance, so what we report here should be what will actually be\n> > realized for that frame ?\n> \n> \"We will \" ... do this is fine, but I don't think we do right now. That\n> was my only highlight. I think this is fine for us to keep building on\n> top of though.\n\nThat may well be the case. I think this patch brings us to a state where\nwe can start testing the interaction with DelayedControls. I do expect\nit to hurt, but that's a required step in the healing process :-)\n\n> > > We can make adjustments to the gains on the ISP here - but not the\n> > > sensor.\n> > >\n> > > We (already know that we) need to identify a way to ensure we\n> > > syncrhonise DelayedControls actions both here on the IPA side along with\n> > > the Pipeline Handler side.\n> > >\n> > > That could be either communicating with DelayedControls, or changing the\n> > > current implementation (I've thought about 'scheduled controls' in the\n> > > past, but it's all still WIP).\n> > >\n> > > > Am I wrong thinking that prepare() and process() are synched to events\n> > > > with different clocks ? prepare() is called in the \"frame ready\" call\n> > > > path, which process() in the ISP \"stats ready\" call path. Do we have\n> > > > guarantees that the two are in sync ?\n> > >\n> > > prepare() is to generate configuration for the ISP.\n> > > process() reports the statistics of the ISP after completion.\n> > >\n> > > For any given frame, prepare can only ever be called before that frame,\n> > > and for the same frame process will be after.\n> > \n> > I was using IPU3 as a refernce as there fillParmsBuffer() (which calls\n> > algo->prepare()) is called on the CIO2 buffer ready event, so the\n> > assumption that prepare() is called before that frame is produced does\n> > not hold there ?\n> \n> Aha, I meant 'prepare' is called before the frame is processed by the\n> ISP, and process() is called after the frame is processed by the ISP...\n> \n> > > I don't believe we enforce any requirement for them to be called in\n> > > lockstep, and at the overall design level - we shouldn't enforce that.\n> > >\n> > > You could have (not specifically on RKISP)\n> > >\n> > > Frame 1 Frame 2 Frame 3 Frame 4\n> > >\n> > > prepare(1)\n> > > prepare(2)\n> > > process(1)\n> > > prepare(3)\n> > \n> > Looking at the RkISP1 AWB:\n> > \n> > Exactly, here in prepare(3) we store in the FrameContext(3) the automatic gains\n> > computed on process(1). I guess that's ok but\n> > \n> > > process(2)\n> > > process(3)\n> > > prepare(4)\n> > \n> > here in prepare(4) we'll use the ones computed in process(3)\n> > \n> > So there's no actually guarantee of which frames the auto gains we\n> > store in the FrameContext() have been computed on. I guess that's not\n> > much relevant for automatic modes ?\n> \n> For automatic - no, it shouldn't matter. As far as I'm aware, we go with\n> 'the most up to date information we have at the time we are preparing\n> that frame for the ISP to process' ... i.e. the current ActiveState.\n\nThat's right, the latest values we have are the best, as they correspond\nto the latest statistics, and thus the newest available information\nabout the scene.\n\n> > > process(4)\n> > >\n> > > > > Is prepare() called before the sensor starts in the inline-ISP case with\n> > > > > RKISP?\n> > >\n> > > That part I don't know yet. But even if prepare() is called just before\n> > > the sensor starts for a given frame - it's absolutely not currently guaranteed that\n> > > the values applied to the sensor match the ActiveState values. So\n> > > putting those values in the frameContext is misleading.\n> > >\n> > > In theory we 'could' identify what is applied on the sensor and make up\n> > > the difference with an ISP digital gain to 'speed' up the convergence ?\n> > > But either way - we 'must' track what the sensor *actually* gets\n> > > configured with here in the FrameContext.\n> > >\n> > > I think all that can be on top of these patches though - as long as we\n> > > highlight the issue here.\n> > \n> > Absolutely, nothing blocking the series here, it's already a move in\n> > the right direction and if we have to improve we should be doing that\n> > on top.\n> \n> Ack.\n> \n> > > > > > if (frame > 0)\n> > > > > > return;\n> > > > > >\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > > > > index be8932040c8e..d6c6fb130fc1 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > > > > @@ -34,7 +34,8 @@ public:\n> > > > > > const rkisp1_stat_buffer *stats) override;\n> > > > > >\n> > > > > > private:\n> > > > > > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> > > > > > + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> > > > > > + double yGain, double iqMeanGain);\n> > > > > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > > > > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> > > > > > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > index 211428717838..88e0631c8d39 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > * \\var IPAActiveState::agc\n> > > > > > * \\brief State for the Automatic Gain Control algorithm\n> > > > > > *\n> > > > > > - * The exposure and gain determined are expected to be applied to the sensor\n> > > > > > - * at the earliest opportunity.\n> > > > > > + * The exposure and gain are the latest values computed by the AGC algorithm.\n> > > > > > *\n> > > > > > * \\var IPAActiveState::agc.exposure\n> > > > > > * \\brief Exposure time expressed as a number of lines\n> > > > > > *\n> > > > > > * \\var IPAActiveState::agc.gain\n> > > > > > * \\brief Analogue gain multiplier\n> > > > > > - *\n> > > > > > - * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > > */\n> > > > > >\n> > > > > > /**\n> > > > > > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > * \\brief Indicates if ISP parameters need to be updated\n> > > > > > */\n> > > > > >\n> > > > > > -/**\n> > > > > > - * \\var IPAActiveState::sensor\n> > > > > > - * \\brief Effective sensor values\n> > > > > > - *\n> > > > > > - * \\var IPAActiveState::sensor.exposure\n> > > > > > - * \\brief Exposure time expressed as a number of lines\n> > > > > > - *\n> > > > > > - * \\var IPAActiveState::sensor.gain\n> > > > > > - * \\brief Analogue gain multiplier\n> > > > > > - */\n> > > > > > -\n> > > > > > /**\n> > > > > > * \\struct RkISP1FrameContext\n> > > > > > * \\brief Per-frame context for algorithms\n> > > > > > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > * \\todo Populate the frame context for all algorithms\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\var RkISP1FrameContext::agc\n> > > > > > + * \\brief Automatic Gain Control parameters for this frame\n> > > > > > + *\n> > > > > > + * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > > > > + * applied to the sensor in order to take effect for this frame.\n> > > > > > + *\n> > > > > > + * \\var RkISP1FrameContext::agc.exposure\n> > > > > > + * \\brief Exposure time expressed as a number of lines\n> > > > > > + *\n> > > > > > + * \\var RkISP1FrameContext::agc.gain\n> > > > > > + * \\brief Analogue gain multiplier\n> > > > > > + *\n> > > > > > + * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\var RkISP1FrameContext::sensor\n> > > > > > + * \\brief Sensor configuration that used been used for this frame\n> > > > > > + *\n> > > > > > + * \\var RkISP1FrameContext::sensor.exposure\n> > > > > > + * \\brief Exposure time expressed as a number of lines\n> > > > > > + *\n> > > > > > + * \\var RkISP1FrameContext::sensor.gain\n> > > > > > + * \\brief Analogue gain multiplier\n> > > > > > + */\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\struct IPAContext\n> > > > > > * \\brief Global IPA context data shared between all algorithms\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > index 4e38ef40ed3c..ecf993cd22d7 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > @@ -82,14 +82,18 @@ struct IPAActiveState {\n> > > > > > uint8_t sharpness;\n> > > > > > bool updateParams;\n> > > > > > } filter;\n> > > > > > -\n> > > > > > - struct {\n> > > > > > - uint32_t exposure;\n> > > > > > - double gain;\n> > > > > > - } sensor;\n> > > > > > };\n> > > > > >\n> > > > > > struct RkISP1FrameContext : public FrameContext {\n> > > > > > + struct {\n> > > > > > + uint32_t exposure;\n> > > > > > + double gain;\n> > > > > > + } agc;\n> > > > > > +\n> > > > > > + struct {\n> > > > > > + uint32_t exposure;\n> > > > > > + double gain;\n> > > > > > + } sensor;\n> > > > > > };\n> > > > > >\n> > > > > > struct IPAContext {\n> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > index 4f6e0b0f87b9..a23cfc7fb24d 100644\n> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > > > reinterpret_cast(\n> > > > > > mappedBuffers_.at(bufferId).planes()[0].data());\n> > > > > >\n> > > > > > - context_.activeState.sensor.exposure =\n> > > > > > + frameContext.sensor.exposure =\n> > > > > > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > > > - context_.activeState.sensor.gain =\n> > > > > > + frameContext.sensor.gain =\n> > > > > > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > > > >\n> > > > > Aha - good - so here we're getting the 'effective' values that were set\n> > > > > at the sensor, before we do any processing with them.\n> > > > >\n> > > > > So this is the point that would be interesting to compare 'what we asked\n> > > > > for' ... 'vs what we actually got'.\n\nOr rather in the process() function of the AGC, which is called just\nbelow.\n\n> > > > > >\n> > > > > > unsigned int aeState = 0;\n> > > > > > @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > > >\n> > > > > > void IPARkISP1::setControls(unsigned int frame)\n> > > > > > {\n> > > > > > - uint32_t exposure = context_.activeState.agc.exposure;\n> > > > > > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> > > > > > + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > > + uint32_t exposure = frameContext.agc.exposure;\n> > > > > > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n> > > > >\n> > > > > So this is the part where we need to figure out more regarding PFC ...\n> > > > > but that's a separate topic for the moment.\n> > > >\n> > > > I might have missed why ? :)\n> > >\n> > > Because the current implementations as I understand it mean that we are\n> > > calling setControls for a given frame, and those controls will then be\n> > > 'applied' in 2 or 3 frames time. So ... not actaully the frame\n> > > referenced by the FrameContext.\n> > \n> > How so ? Don't we assume we operate early enough to have the here\n> > computed values are applied in time to have them realized at the right\n> > frame ?\n> \n> No ... I don't think we do right now.\n> \n> Ohh - it's worse. It's all currently wrong ;-)\n> \n> IPARkISP1::processStatsBuffer(frame) {\n> \t...\n> \tsetControls(frame);\n> \t...\n> }\n> \n> \n> So - we're calling 'setControls' which then gets the FrameContext\n> 'frame'.\n> \n> That is setting 'what the AGC' would like the sensor to be as soon as it\n> can be.\n> \n> But it shouldn't be parsing \n> FrameContext.agc.exposure\n> \n> To me - that is 'what the exposure *is* of the frame related to\n> Framecontext.\n\nThat's stored in sensor.exposure (and sensor.gain).\n\n> If we extend this with a new entry (referenced at the top of this mail)\n> such as :\n> \n> FrameContext.agc.desiredExposure\n> \n> Or such - then I'd be happier with this in the short term. But I really\n> don't want to mix up \"What the AGC would like\" and \"What the Frame\n> exposure is\" in a single field of the frame context.\n\nThat we agree on, but those are already stored in two separate fields\n:-)\n\n> > > > > So while this patch won't meet our PFC requriements - I think that's\n> > > > > fine and this helps build what we need to get there.\n> > > > >\n> > > > > I'm a bit more hesitant here (hence leaving this to come back to) ...\n> > > > > but I don't think this is 'worse' than where we already are - and\n> > > > > progresses the frame context conversions.\n> > > > >\n> > > > > Perhaps with some more notes / a todo on the fact that we need to do\n> > > > > more about handling of setting exposure / gains for a specific frame...\n> > > > >\n> > > > > Reviewed-by: Kieran Bingham \n> > > > >\n> > > > > > ControlList ctrls(ctrls_);\n> > > > > > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));","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 731ADC0DA4\n\tfor ;\n\tThu, 22 Sep 2022 22:08:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DD7B6221F;\n\tFri, 23 Sep 2022 00:08:02 +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 A7418621F8\n\tfor ;\n\tFri, 23 Sep 2022 00:08:00 +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 D34C64F7;\n\tFri, 23 Sep 2022 00:07:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663884482;\n\tbh=4eQjRaZR/aJTKpriccdqH3heCZ5TGuxqc07M+EkHc3s=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=APVWwY/nZ+Tm/wYBvP15IzKl5MYaTNj6BjdNZt9uzrsuvqRd+1gvUts8nzzQpEg7E\n\twy3Gkq7ReQUaXHQJ2yu+Nx/2lojTsVrkkwbPs1/u+bUKYjIe6zPzcWdCpQZajntrr4\n\tYOdw8DfrBcM6mlB0vBACb475rschsvJKkF1wImikw0hj/sSncbunVAc2Hlpm29watB\n\tUQOMMa6Z5w7oZqp5vTPX5lwDc+E4JP40Ek2G4a2xuo1VRzK2ZFIcVnNd9hCpXdnu1u\n\th/zr0H8CS2ZArmLvGRiAJGlKJ8Z5J+tw6Vv2JfiiR6iR34ryhZ/cV9X30c1OtzFXYj\n\tErRYX+8iQPbxw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663884480;\n\tbh=4eQjRaZR/aJTKpriccdqH3heCZ5TGuxqc07M+EkHc3s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=usLhiq7rYpOLotBjvWoQ1EFLqxDaWZnUu55LHfZWSPlwmlJphkOUVo9rVOvPMF6n0\n\tVL4LBg0dmHAXYBW3FdaR3cYmmen3iwjcdkHyOZ08+/o9X+gUwAEciBYG8kEwhA67Pq\n\tbqAVMgJkiXRZqFIT87My/l2PVHq0VeUKT6wXm0s0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"usLhiq7r\"; dkim-atps=neutral","Date":"Fri, 23 Sep 2022 01:07:45 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>\n\t<166371884168.18961.15851239113055930923@Monstersaurus>\n\t<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>\n\t<166383716310.56880.14265919686115696723@Monstersaurus>\n\t<20220922101702.i6hsqbdj3yfqrbv6@uno.localdomain>\n\t<166384861879.56880.6604830110425767187@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166384861879.56880.6604830110425767187@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25099,"web_url":"https://patchwork.libcamera.org/comment/25099/","msgid":"<166388508948.1632173.8793752945655220678@Monstersaurus>","date":"2022-09-22T22:18:09","subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-09-22 23:07:45)\n> On Thu, Sep 22, 2022 at 01:10:18PM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2022-09-22 11:17:02)\n> > > On Thu, Sep 22, 2022 at 09:59:23AM +0100, Kieran Bingham wrote:\n> > > > Quoting Jacopo Mondi (2022-09-21 21:13:53)\n> > > > > On Wed, Sep 21, 2022 at 01:07:21AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > > > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)\n> > > > > > > Rework the algorithm's usage of the active state to store the value of\n> > > > > > > controls for the last queued request in the queueRequest() function, and\n> > > > > > > store a copy of the values in the corresponding frame context.\n> > > > > > >\n> > > > > > > The frame context is used in the prepare() function to populate the ISP\n> > > > > > > parameters with values corresponding to the right frame.\n> > > > > > >\n> > > > > > > Signed-off-by: Laurent Pinchart \n> > > > > > > ---\n> > > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----\n> > > > > > > src/ipa/rkisp1/algorithms/agc.h | 3 ++-\n> > > > > > > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------\n> > > > > > > src/ipa/rkisp1/ipa_context.h | 14 ++++++----\n> > > > > > > src/ipa/rkisp1/rkisp1.cpp | 9 ++++---\n> > > > > > > 5 files changed, 55 insertions(+), 32 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > > index 4124c3272bc7..e7198169d11b 100644\n> > > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > > > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)\n> > > > > > > /**\n> > > > > > > * \\brief Estimate the new exposure and gain values\n> > > > > > > * \\param[inout] context The shared IPA Context\n> > > > > > > + * \\param[in] frameContext The FrameContext for this frame\n> > > > > > > * \\param[in] yGain The gain calculated on the current brightness level\n> > > > > > > * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n> > > > > > > */\n> > > > > > > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n> > > > > > > +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,\n> > > > > > > + double yGain, double iqMeanGain)\n> > > > > > > {\n> > > > > > > IPASessionConfiguration &configuration = context.configuration;\n> > > > > > > IPAActiveState &activeState = context.activeState;\n> > > > > > >\n> > > > > > > /* Get the effective exposure and gain applied on the sensor. */\n> > > > > > > - uint32_t exposure = activeState.sensor.exposure;\n> > > > > > > - double analogueGain = activeState.sensor.gain;\n> > > > > > > + uint32_t exposure = frameContext.sensor.exposure;\n> > > > > > > + double analogueGain = frameContext.sensor.gain;\n> > > > > > >\n> > > > > > > /* Use the highest of the two gain estimates. */\n> > > > > > > double evGain = std::max(yGain, iqMeanGain);\n> > > > > > > @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const\n> > > > > > > * new exposure and gain for the scene.\n> > > > > > > */\n> > > > > > > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > > > > + RkISP1FrameContext &frameContext,\n> > > > > > > const rkisp1_stat_buffer *stats)\n> > > > > > > {\n> > > > > > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > > > > > @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > > break;\n> > > > > > > }\n> > > > > > >\n> > > > > > > - computeExposure(context, yGain, iqMeanGain);\n> > > > > > > + computeExposure(context, frameContext, yGain, iqMeanGain);\n> > > > > >\n> > > > > > It may be useful to compare somewhere here the values for gain/exposure\n> > > > > > that we expected to be set on the sensor, as stored in our frame\n> > > > > > context, with the values that could be retrieved from the\n> > > > > > DelayedControls implementation to make sure they are\n> > > > > > consistent/coherrent.\n> \n> I'll add a todo comment. Note that it doesn't matter that much if we\n> receive what we've asked, as long as the values we get correspond to\n> what has been applied to the frame. That's the important part to ensure\n> the algorithm is stable. Synchronizing the values computed by the\n> algorithm with a particular frame is only important in manual mode.\n> \n> > > > > > > frameCount_++;\n> > > > > > > }\n> > > > > > >\n> > > > > > > @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > > > */\n> > > > > > > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > > > > > - [[maybe_unused]] RkISP1FrameContext &frameContext,\n> > > > > > > - rkisp1_params_cfg *params)\n> > > > > > > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)\n> > > > > > > {\n> > > > > > > + frameContext.agc.exposure = context.activeState.agc.exposure;\n> > > > > > > + frameContext.agc.gain = context.activeState.agc.gain;\n> > \n> > So perhaps what this should be setting is\n> > 'frameContext.agc.desiredExposure'\n> > \n> > In otherwords, at the time of processing this image, the AGC ran and\n> > determined it would prefer to have an exposure of X\n> > \n> > But currently this reads to me as...\n> > \n> > This frame's Exposure is X\n> > \n> > Which is completely wrong.\n> \n> The exposure and gain members are documented as\n> \n> * The exposure and gain are the latest values computed by the AGC algorithm.\n> \n> I don't want to call them desiredExposure and desiredGain, as we would\n> then have to prefix pretty much everything with either \"desired\" or\n> \"computed\".\n> \n> > > > > > > +\n> > > > > >\n> > > > > > This may be a little dubious ... due to the delays inherrent with\n> > > > > > setting a gain and exposure...\n> > > > > >\n> > > > > > If they are not known to be exactly the values used by the sensor for\n> > > > > > this frame, but we still need to store them here, then I'd add a todo or\n> > > > > > a warning note...\n> > > > >\n> > > > > Do I read the code and the comment here right, and this mean we report\n> > > > > as frameContext.agc (== the agc values computed for this frame) the\n> > > > > last values computed by the AGC algorithm overall, which might be\n> > > > > different than the ones computed for the last frame, if we have two\n> > > > > consecutive calls to process() (which calls computeExposure()).\n> > > >\n> > > > What are you distinguising between 'the last frame' and the 'last value\n> > > > computed by AGC?'.\n> > > >\n> > > > process() is currently always the last step where the AGC actually runs,\n> > > > so the ActiveState reports the 'best'/'most-up-to-date' desired values\n> > > > that the AGC would like to use for automatic mode.\n> > > \n> > > That's my point, if process() has been called two consecutive times\n> > > (n, n + 1) we store in the FrameContext 'n' the values computed for\n> > > frame 'n + 1'\n> > > \n> > > I'm not even sure that's an issue, just making sure this is known..\n> > \n> > If we end up prepareing two frames for the ISP at 'the same time' ...\n> > then yes, I would expect them to have the same values, as that is the\n> > 'most up to date information we have at that time'.\n> \n> That matches my understanding.\n> \n> > > > However - the issue I have here is that for Sensor controls - this point\n> > > > in the time line is too late to apply those values.\n> > > \n> > > On this point I'm not concerned, but I might be missing something.\n> > > We'll make sure we process frames early enough, and the values we want\n> > > to have applied at frame X will be computed here and correctly applied\n> > > in advance, so what we report here should be what will actually be\n> > > realized for that frame ?\n> > \n> > \"We will \" ... do this is fine, but I don't think we do right now. That\n> > was my only highlight. I think this is fine for us to keep building on\n> > top of though.\n> \n> That may well be the case. I think this patch brings us to a state where\n> we can start testing the interaction with DelayedControls. I do expect\n> it to hurt, but that's a required step in the healing process :-)\n> \n> > > > We can make adjustments to the gains on the ISP here - but not the\n> > > > sensor.\n> > > >\n> > > > We (already know that we) need to identify a way to ensure we\n> > > > syncrhonise DelayedControls actions both here on the IPA side along with\n> > > > the Pipeline Handler side.\n> > > >\n> > > > That could be either communicating with DelayedControls, or changing the\n> > > > current implementation (I've thought about 'scheduled controls' in the\n> > > > past, but it's all still WIP).\n> > > >\n> > > > > Am I wrong thinking that prepare() and process() are synched to events\n> > > > > with different clocks ? prepare() is called in the \"frame ready\" call\n> > > > > path, which process() in the ISP \"stats ready\" call path. Do we have\n> > > > > guarantees that the two are in sync ?\n> > > >\n> > > > prepare() is to generate configuration for the ISP.\n> > > > process() reports the statistics of the ISP after completion.\n> > > >\n> > > > For any given frame, prepare can only ever be called before that frame,\n> > > > and for the same frame process will be after.\n> > > \n> > > I was using IPU3 as a refernce as there fillParmsBuffer() (which calls\n> > > algo->prepare()) is called on the CIO2 buffer ready event, so the\n> > > assumption that prepare() is called before that frame is produced does\n> > > not hold there ?\n> > \n> > Aha, I meant 'prepare' is called before the frame is processed by the\n> > ISP, and process() is called after the frame is processed by the ISP...\n> > \n> > > > I don't believe we enforce any requirement for them to be called in\n> > > > lockstep, and at the overall design level - we shouldn't enforce that.\n> > > >\n> > > > You could have (not specifically on RKISP)\n> > > >\n> > > > Frame 1 Frame 2 Frame 3 Frame 4\n> > > >\n> > > > prepare(1)\n> > > > prepare(2)\n> > > > process(1)\n> > > > prepare(3)\n> > > \n> > > Looking at the RkISP1 AWB:\n> > > \n> > > Exactly, here in prepare(3) we store in the FrameContext(3) the automatic gains\n> > > computed on process(1). I guess that's ok but\n> > > \n> > > > process(2)\n> > > > process(3)\n> > > > prepare(4)\n> > > \n> > > here in prepare(4) we'll use the ones computed in process(3)\n> > > \n> > > So there's no actually guarantee of which frames the auto gains we\n> > > store in the FrameContext() have been computed on. I guess that's not\n> > > much relevant for automatic modes ?\n> > \n> > For automatic - no, it shouldn't matter. As far as I'm aware, we go with\n> > 'the most up to date information we have at the time we are preparing\n> > that frame for the ISP to process' ... i.e. the current ActiveState.\n> \n> That's right, the latest values we have are the best, as they correspond\n> to the latest statistics, and thus the newest available information\n> about the scene.\n> \n> > > > process(4)\n> > > >\n> > > > > > Is prepare() called before the sensor starts in the inline-ISP case with\n> > > > > > RKISP?\n> > > >\n> > > > That part I don't know yet. But even if prepare() is called just before\n> > > > the sensor starts for a given frame - it's absolutely not currently guaranteed that\n> > > > the values applied to the sensor match the ActiveState values. So\n> > > > putting those values in the frameContext is misleading.\n> > > >\n> > > > In theory we 'could' identify what is applied on the sensor and make up\n> > > > the difference with an ISP digital gain to 'speed' up the convergence ?\n> > > > But either way - we 'must' track what the sensor *actually* gets\n> > > > configured with here in the FrameContext.\n> > > >\n> > > > I think all that can be on top of these patches though - as long as we\n> > > > highlight the issue here.\n> > > \n> > > Absolutely, nothing blocking the series here, it's already a move in\n> > > the right direction and if we have to improve we should be doing that\n> > > on top.\n> > \n> > Ack.\n> > \n> > > > > > > if (frame > 0)\n> > > > > > > return;\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h\n> > > > > > > index be8932040c8e..d6c6fb130fc1 100644\n> > > > > > > --- a/src/ipa/rkisp1/algorithms/agc.h\n> > > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.h\n> > > > > > > @@ -34,7 +34,8 @@ public:\n> > > > > > > const rkisp1_stat_buffer *stats) override;\n> > > > > > >\n> > > > > > > private:\n> > > > > > > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);\n> > > > > > > + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,\n> > > > > > > + double yGain, double iqMeanGain);\n> > > > > > > utils::Duration filterExposure(utils::Duration exposureValue);\n> > > > > > > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);\n> > > > > > > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;\n> > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > > index 211428717838..88e0631c8d39 100644\n> > > > > > > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > > > > > > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > > * \\var IPAActiveState::agc\n> > > > > > > * \\brief State for the Automatic Gain Control algorithm\n> > > > > > > *\n> > > > > > > - * The exposure and gain determined are expected to be applied to the sensor\n> > > > > > > - * at the earliest opportunity.\n> > > > > > > + * The exposure and gain are the latest values computed by the AGC algorithm.\n> > > > > > > *\n> > > > > > > * \\var IPAActiveState::agc.exposure\n> > > > > > > * \\brief Exposure time expressed as a number of lines\n> > > > > > > *\n> > > > > > > * \\var IPAActiveState::agc.gain\n> > > > > > > * \\brief Analogue gain multiplier\n> > > > > > > - *\n> > > > > > > - * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > > > */\n> > > > > > >\n> > > > > > > /**\n> > > > > > > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > > * \\brief Indicates if ISP parameters need to be updated\n> > > > > > > */\n> > > > > > >\n> > > > > > > -/**\n> > > > > > > - * \\var IPAActiveState::sensor\n> > > > > > > - * \\brief Effective sensor values\n> > > > > > > - *\n> > > > > > > - * \\var IPAActiveState::sensor.exposure\n> > > > > > > - * \\brief Exposure time expressed as a number of lines\n> > > > > > > - *\n> > > > > > > - * \\var IPAActiveState::sensor.gain\n> > > > > > > - * \\brief Analogue gain multiplier\n> > > > > > > - */\n> > > > > > > -\n> > > > > > > /**\n> > > > > > > * \\struct RkISP1FrameContext\n> > > > > > > * \\brief Per-frame context for algorithms\n> > > > > > > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {\n> > > > > > > * \\todo Populate the frame context for all algorithms\n> > > > > > > */\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\var RkISP1FrameContext::agc\n> > > > > > > + * \\brief Automatic Gain Control parameters for this frame\n> > > > > > > + *\n> > > > > > > + * The exposure and gain are provided by the AGC algorithm, and are to be\n> > > > > > > + * applied to the sensor in order to take effect for this frame.\n> > > > > > > + *\n> > > > > > > + * \\var RkISP1FrameContext::agc.exposure\n> > > > > > > + * \\brief Exposure time expressed as a number of lines\n> > > > > > > + *\n> > > > > > > + * \\var RkISP1FrameContext::agc.gain\n> > > > > > > + * \\brief Analogue gain multiplier\n> > > > > > > + *\n> > > > > > > + * The gain should be adapted to the sensor specific gain code before applying.\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\var RkISP1FrameContext::sensor\n> > > > > > > + * \\brief Sensor configuration that used been used for this frame\n> > > > > > > + *\n> > > > > > > + * \\var RkISP1FrameContext::sensor.exposure\n> > > > > > > + * \\brief Exposure time expressed as a number of lines\n> > > > > > > + *\n> > > > > > > + * \\var RkISP1FrameContext::sensor.gain\n> > > > > > > + * \\brief Analogue gain multiplier\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > /**\n> > > > > > > * \\struct IPAContext\n> > > > > > > * \\brief Global IPA context data shared between all algorithms\n> > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > index 4e38ef40ed3c..ecf993cd22d7 100644\n> > > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > @@ -82,14 +82,18 @@ struct IPAActiveState {\n> > > > > > > uint8_t sharpness;\n> > > > > > > bool updateParams;\n> > > > > > > } filter;\n> > > > > > > -\n> > > > > > > - struct {\n> > > > > > > - uint32_t exposure;\n> > > > > > > - double gain;\n> > > > > > > - } sensor;\n> > > > > > > };\n> > > > > > >\n> > > > > > > struct RkISP1FrameContext : public FrameContext {\n> > > > > > > + struct {\n> > > > > > > + uint32_t exposure;\n> > > > > > > + double gain;\n> > > > > > > + } agc;\n> > > > > > > +\n> > > > > > > + struct {\n> > > > > > > + uint32_t exposure;\n> > > > > > > + double gain;\n> > > > > > > + } sensor;\n> > > > > > > };\n> > > > > > >\n> > > > > > > struct IPAContext {\n> > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > index 4f6e0b0f87b9..a23cfc7fb24d 100644\n> > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > > > > @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > > > > reinterpret_cast(\n> > > > > > > mappedBuffers_.at(bufferId).planes()[0].data());\n> > > > > > >\n> > > > > > > - context_.activeState.sensor.exposure =\n> > > > > > > + frameContext.sensor.exposure =\n> > > > > > > sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > > > > > > - context_.activeState.sensor.gain =\n> > > > > > > + frameContext.sensor.gain =\n> > > > > > > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > > > > >\n> > > > > > Aha - good - so here we're getting the 'effective' values that were set\n> > > > > > at the sensor, before we do any processing with them.\n> > > > > >\n> > > > > > So this is the point that would be interesting to compare 'what we asked\n> > > > > > for' ... 'vs what we actually got'.\n> \n> Or rather in the process() function of the AGC, which is called just\n> below.\n> \n> > > > > > >\n> > > > > > > unsigned int aeState = 0;\n> > > > > > > @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > > > > >\n> > > > > > > void IPARkISP1::setControls(unsigned int frame)\n> > > > > > > {\n> > > > > > > - uint32_t exposure = context_.activeState.agc.exposure;\n> > > > > > > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);\n> > > > > > > + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);\n> > > > > > > + uint32_t exposure = frameContext.agc.exposure;\n> > > > > > > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);\n> > > > > >\n> > > > > > So this is the part where we need to figure out more regarding PFC ...\n> > > > > > but that's a separate topic for the moment.\n> > > > >\n> > > > > I might have missed why ? :)\n> > > >\n> > > > Because the current implementations as I understand it mean that we are\n> > > > calling setControls for a given frame, and those controls will then be\n> > > > 'applied' in 2 or 3 frames time. So ... not actaully the frame\n> > > > referenced by the FrameContext.\n> > > \n> > > How so ? Don't we assume we operate early enough to have the here\n> > > computed values are applied in time to have them realized at the right\n> > > frame ?\n> > \n> > No ... I don't think we do right now.\n> > \n> > Ohh - it's worse. It's all currently wrong ;-)\n> > \n> > IPARkISP1::processStatsBuffer(frame) {\n> > ...\n> > setControls(frame);\n> > ...\n> > }\n> > \n> > \n> > So - we're calling 'setControls' which then gets the FrameContext\n> > 'frame'.\n> > \n> > That is setting 'what the AGC' would like the sensor to be as soon as it\n> > can be.\n> > \n> > But it shouldn't be parsing \n> > FrameContext.agc.exposure\n> > \n> > To me - that is 'what the exposure *is* of the frame related to\n> > Framecontext.\n> \n> That's stored in sensor.exposure (and sensor.gain).\n> \n> > If we extend this with a new entry (referenced at the top of this mail)\n> > such as :\n> > \n> > FrameContext.agc.desiredExposure\n> > \n> > Or such - then I'd be happier with this in the short term. But I really\n> > don't want to mix up \"What the AGC would like\" and \"What the Frame\n> > exposure is\" in a single field of the frame context.\n> \n> That we agree on, but those are already stored in two separate fields\n> :-)\n\nAha - perfect, I'd missed / forgotten about the sensor structure. Ok\nthen - move along, nothing to see here ... ;-)\n--\nKieran\n\n> \n> > > > > > So while this patch won't meet our PFC requriements - I think that's\n> > > > > > fine and this helps build what we need to get there.\n> > > > > >\n> > > > > > I'm a bit more hesitant here (hence leaving this to come back to) ...\n> > > > > > but I don't think this is 'worse' than where we already are - and\n> > > > > > progresses the frame context conversions.\n> > > > > >\n> > > > > > Perhaps with some more notes / a todo on the fact that we need to do\n> > > > > > more about handling of setting exposure / gains for a specific frame...\n> > > > > >\n> > > > > > Reviewed-by: Kieran Bingham \n> > > > > >\n> > > > > > > ControlList ctrls(ctrls_);\n> > > > > > > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure));\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 E797EBD16B\n\tfor ;\n\tThu, 22 Sep 2022 22:18:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42CC362220;\n\tFri, 23 Sep 2022 00:18:14 +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 E5598621F8\n\tfor ;\n\tFri, 23 Sep 2022 00:18:11 +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 54D144F7;\n\tFri, 23 Sep 2022 00:18:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663885094;\n\tbh=TfJuHm7LZrXNt8jbRmAikyxR14tINeovEQvYqV9CpNo=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=dRKdtQgI5n5XecxMXbOs37T69M/vhSG+5tjCRqFcf6iD/e71qu21wUF5T1Yuly1uU\n\tEQHZ0t9gLIJjH7I0dV/cvWM3J5/HUBm4aTnH1ULLvnNMThv+JmRKuTrGeO01tI3G5b\n\tf6jowOnI0G8rZWoHJmR6mjaV6njkoLYo/IUgsfeoWCW/GhPHtGuWj1qGYAeikl8rnI\n\tuwZVVOTvSOF1Ybix3WBxJ/L51//NFP1a8FKYNEz1abRm33wVlLNYcvompKVBXrAPyM\n\tFamba2x+4NvCpStZKDMBK8M3On+EXtfxEKpdevcPWsGZxmo4Krg/X77nGcvUCIlxYx\n\tSdBa66/2rQx+Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663885091;\n\tbh=TfJuHm7LZrXNt8jbRmAikyxR14tINeovEQvYqV9CpNo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=tahsHTlpnZl8pVIC2Vq5dx85WtEfjx/LDjemC9kqn1WlcSoFssA6EfLh2Bqgdyl6B\n\tKV/1yJSy7JfpMoH/p5TxAvA+mBYGBmkmevTxjNbQeEy5EEMfvv7Kllx38NUYwQ2a5J\n\tuG3hadlgPtzp+xKn0GZV2bLpl1+VdJW6wiVNkTvk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tahsHTlp\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-20-laurent.pinchart@ideasonboard.com>\n\t<166371884168.18961.15851239113055930923@Monstersaurus>\n\t<20220921201353.cns7gogdsq6wtv4n@uno.localdomain>\n\t<166383716310.56880.14265919686115696723@Monstersaurus>\n\t<20220922101702.i6hsqbdj3yfqrbv6@uno.localdomain>\n\t<166384861879.56880.6604830110425767187@Monstersaurus>\n\t","To":"Laurent Pinchart ","Date":"Thu, 22 Sep 2022 23:18:09 +0100","Message-ID":"<166388508948.1632173.8793752945655220678@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store\n\tper-frame information in frame context","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]