[{"id":20713,"web_url":"https://patchwork.libcamera.org/comment/20713/","msgid":"<20211108141043.tgy2w2n2vdqomsbt@uno.localdomain>","date":"2021-11-08T14:10:43","subject":"Re: [libcamera-devel] [PATCH 17/22] ipa: ipu3: Use a\n\tIPAFrameContext pointer from the per-frame queue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Jean-Michel\n\nOn Mon, Nov 08, 2021 at 02:13:45PM +0100, Jean-Michel Hautbois wrote:\n> We have one frame context shared between the algorithms thanks to a\n> local cached context_ variable in IPAIPU3. Now that we can store the\n> frame contexts in a queue, implement all the needed functions for that\n> and convert the frame context to a pointer.\n>\n> The algorithm are now using the values applied on the frame they are\n> processing, and not the latest one.\n>\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 12 ++++----\n> src/ipa/ipu3/algorithms/agc.h | 2 +-\n> src/ipa/ipu3/algorithms/awb.cpp | 14 ++++-----\n> src/ipa/ipu3/algorithms/tone_mapping.cpp | 8 ++---\n> src/ipa/ipu3/ipa_context.h | 2 +-\n> src/ipa/ipu3/ipu3.cpp | 38 +++++++++++++++++++-----\n> 6 files changed, 49 insertions(+), 27 deletions(-)\n>\n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 52cf2753..2eee5b6b 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -245,7 +245,7 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\n> * \\param[in] stats The IPU3 statistics and ISP results\n> * \\param[in] currentYGain The gain calculated on the current brightness level\n> */\n> -double Agc::computeInitialY(IPAFrameContext &frameContext,\n> +double Agc::computeInitialY(IPAFrameContext *frameContext,\n> \t\t\t const ipu3_uapi_grid_config &grid,\n> \t\t\t const ipu3_uapi_stats_3a *stats,\n> \t\t\t double currentYGain)\n> @@ -271,9 +271,9 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> \t * Estimate the sum of the brightness values, weighted with the gains\n> \t * applied on the channels in AWB.\n> \t */\n> -\tdouble Y_sum = redSum * frameContext.awb.gains.red * .299 +\n> -\t\t greenSum * frameContext.awb.gains.green * .587 +\n> -\t\t blueSum * frameContext.awb.gains.blue * .114;\n> +\tdouble Y_sum = redSum * frameContext->awb.gains.red * .299 +\n> +\t\t greenSum * frameContext->awb.gains.green * .587 +\n> +\t\t blueSum * frameContext->awb.gains.blue * .114;\n>\n> \t/* And return the average brightness */\n> \treturn Y_sum / (grid.height * grid.width);\n> @@ -291,8 +291,8 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> \t/* Get the latest exposure and gain applied */\n> -\tuint32_t &exposure = context.frameContext.agc.exposure;\n> -\tdouble &analogueGain = context.frameContext.agc.gain;\n> +\tuint32_t &exposure = context.frameContext->agc.exposure;\n> +\tdouble &analogueGain = context.frameContext->agc.gain;\n> \tmeasureBrightness(stats, context.configuration.grid.bdsGrid);\n>\n> \tdouble currentYGain = 1.0;\n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 6f5d71e0..5d6bef9d 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -35,7 +35,7 @@ private:\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> \tvoid filterExposure();\n> \tvoid computeExposure(uint32_t &exposure, double &gain, double currentYGain);\n> -\tdouble computeInitialY(IPAFrameContext &frameContext,\n> +\tdouble computeInitialY(IPAFrameContext *frameContext,\n> \t\t\t const ipu3_uapi_grid_config &grid,\n> \t\t\t const ipu3_uapi_stats_3a *stats,\n> \t\t\t double currentYGain);\n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index a4114659..bd55d377 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -382,9 +382,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> \t * The results are cached, so if no results were calculated, we set the\n> \t * cached values from asyncResults_ here.\n> \t */\n> -\tcontext.frameContext.awb.gains.blue = asyncResults_.blueGain;\n> -\tcontext.frameContext.awb.gains.green = asyncResults_.greenGain;\n> -\tcontext.frameContext.awb.gains.red = asyncResults_.redGain;\n> +\tcontext.frameContext->awb.gains.blue = asyncResults_.blueGain;\n> +\tcontext.frameContext->awb.gains.green = asyncResults_.greenGain;\n> +\tcontext.frameContext->awb.gains.red = asyncResults_.redGain;\n> }\n>\n> constexpr uint16_t Awb::threshold(float value)\n> @@ -432,10 +432,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> \tparams->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset\n> \t\t\t\t\t\t\t* params->acc_param.bnr.opt_center.y_reset;\n> \t/* Convert to u3.13 fixed point values */\n> -\tparams->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green;\n> -\tparams->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red;\n> -\tparams->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue;\n> -\tparams->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green;\n> +\tparams->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext->awb.gains.green;\n> +\tparams->acc_param.bnr.wb_gains.r = 8192 * context.frameContext->awb.gains.red;\n> +\tparams->acc_param.bnr.wb_gains.b = 8192 * context.frameContext->awb.gains.blue;\n> +\tparams->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext->awb.gains.green;\n>\n> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n>\n> diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> index 5d74c552..50498f41 100644\n> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> @@ -57,7 +57,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,\n> {\n> \t/* Copy the calculated LUT into the parameters buffer. */\n> \tmemcpy(params->acc_param.gamma.gc_lut.lut,\n> -\t context.frameContext.toneMapping.gammaCorrection.lut,\n> +\t context.frameContext->toneMapping.gammaCorrection.lut,\n> \t IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n> \t sizeof(params->acc_param.gamma.gc_lut.lut[0]));\n>\n> @@ -84,11 +84,11 @@ void ToneMapping::process(IPAContext &context,\n> \t */\n> \tgamma_ = 1.1;\n>\n> -\tif (context.frameContext.toneMapping.gamma == gamma_)\n> +\tif (context.frameContext->toneMapping.gamma == gamma_)\n> \t\treturn;\n>\n> \tstruct ipu3_uapi_gamma_corr_lut &lut =\n> -\t\tcontext.frameContext.toneMapping.gammaCorrection;\n> +\t\tcontext.frameContext->toneMapping.gammaCorrection;\n>\n> \tfor (uint32_t i = 0; i < std::size(lut.lut); i++) {\n> \t\tdouble j = static_cast(i) / (std::size(lut.lut) - 1);\n> @@ -98,7 +98,7 @@ void ToneMapping::process(IPAContext &context,\n> \t\tlut.lut[i] = gamma * 8191;\n> \t}\n>\n> -\tcontext.frameContext.toneMapping.gamma = gamma_;\n> +\tcontext.frameContext->toneMapping.gamma = gamma_;\n> }\n>\n> } /* namespace ipa::ipu3::algorithms */\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index ee8f7b55..69780915 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -57,7 +57,7 @@ struct IPAFrameContext {\n>\n> struct IPAContext {\n> \tIPASessionConfiguration configuration;\n> -\tIPAFrameContext frameContext;\n> +\tIPAFrameContext *frameContext;\n> };\n>\n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 177c5c2f..b60ab7e7 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -522,10 +522,26 @@ void IPAIPU3::unmapBuffers(const std::vector &ids)\n>\n> void IPAIPU3::frameStarted([[maybe_unused]] unsigned int frame)\n> {\n> +\t/*\n> +\t * Instantiate a new IPAFrameContext to push to the queue. The lifetime\n> +\t * of this pointer is indirectly controlled by the\n> +\t * ActionSetSensorControls event, as this is where we will delete it.\n> +\t */\n> +\tstruct IPAFrameContext *frameContext = new IPAFrameContext;\n> +\tframeContext->frameId = frame;\n> +\tframeContextQueue.push(frameContext);\n\nMaybe a detail:\n\nWouldn't it be safer to store unique_ptr in the queue\ninstead of manually allocate/free ?\n\n> }\n>\n> void IPAIPU3::frameCompleted([[maybe_unused]] unsigned int frame)\n> {\n> +\t/*\n> +\t * Remove the pointer from the queue, it should not be accessed\n> +\t * anymore and delete it.\n> +\t */\n> +\tstruct IPAFrameContext *frameContext = frameContextQueue.front();\n> +\tASSERT(frameContext->frameId == frame);\n> +\tframeContextQueue.pop();\n> +\tdelete frameContext;\n> }\n>\n> /**\n> @@ -579,10 +595,6 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> \t\tconst ipu3_uapi_stats_3a *stats =\n> \t\t\treinterpret_cast(mem.data());\n>\n> -\t\t/* \\todo move those into processControls */\n> -\t\tcontext_.frameContext.agc.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> -\t\tcontext_.frameContext.agc.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> -\n> \t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> \t\t/*\n> \t\t* To save incurring extra IPC calls, we do not send explicit events\n> @@ -610,9 +622,12 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> */\n> void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \t\t\t [[maybe_unused]] const ControlList &controls,\n> -\t\t\t [[maybe_unused]] const ControlList &sensorCtrls)\n> +\t\t\t const ControlList &sensorCtrls)\n> {\n> -\t/* \\todo Start processing for 'frame' based on 'controls'. */\n> +\tstruct IPAFrameContext *frameContext = frameContextQueue.back();\n> +\tASSERT(frameContext->frameId == frame);\n> +\tframeContext->agc.exposure = sensorCtrls.get(V4L2_CID_EXPOSURE).get();\n> +\tframeContext->agc.gain = camHelper_->gain(sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> }\n>\n> /**\n> @@ -636,6 +651,9 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> \t */\n> \tparams->use = {};\n>\n> +\t/* Set the reference to the current frame context */\n> +\tcontext_.frameContext = frameContextQueue.front();\n> +\n> \tfor (auto const &algo : algorithms_)\n> \t\talgo->prepare(context_, params);\n>\n> @@ -661,6 +679,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> {\n> \tControlList ctrls(controls::controls);\n>\n> +\t/* Set the reference to the current frame context */\n> +\tcontext_.frameContext = frameContextQueue.front();\n> +\n> \tfor (auto const &algo : algorithms_)\n> \t\talgo->process(context_, stats);\n>\n> @@ -690,8 +711,9 @@ void IPAIPU3::setControls(unsigned int frame)\n> \tIPU3Action op;\n> \top.op = ActionSetSensorControls;\n>\n> -\texposure_ = context_.frameContext.agc.exposure;\n> -\tgain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n> +\t/* Apply the exposure and gain updated values */\n> +\texposure_ = context_.frameContext->agc.exposure;\n> +\tgain_ = camHelper_->gainCode(context_.frameContext->agc.gain);\n>\n> \tControlList ctrls(ctrls_);\n> \tctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> --\n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C46FEBDB1C\n\tfor ;\n\tMon, 8 Nov 2021 14:09:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2EA646035D;\n\tMon, 8 Nov 2021 15:09:53 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4CEC86032C\n\tfor ;\n\tMon, 8 Nov 2021 15:09:51 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 956E0240017;\n\tMon, 8 Nov 2021 14:09:50 +0000 (UTC)"],"Date":"Mon, 8 Nov 2021 15:10:43 +0100","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20211108141043.tgy2w2n2vdqomsbt@uno.localdomain>","References":"<20211108131350.130665-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211108131350.130665-18-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211108131350.130665-18-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 17/22] ipa: ipu3: Use a\n\tIPAFrameContext pointer from the per-frame queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20728,"web_url":"https://patchwork.libcamera.org/comment/20728/","msgid":"<163638777308.275423.16181466655875962303@Monstersaurus>","date":"2021-11-08T16:09:33","subject":"Re: [libcamera-devel] [PATCH 17/22] ipa: ipu3: Use a\n\tIPAFrameContext pointer from the per-frame queue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-11-08 14:10:43)\n> Hi Jean-Michel\n> \n> On Mon, Nov 08, 2021 at 02:13:45PM +0100, Jean-Michel Hautbois wrote:\n> > We have one frame context shared between the algorithms thanks to a\n> > local cached context_ variable in IPAIPU3. Now that we can store the\n> > frame contexts in a queue, implement all the needed functions for that\n> > and convert the frame context to a pointer.\n> >\n> > The algorithm are now using the values applied on the frame they are\n> > processing, and not the latest one.\n> >\n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 12 ++++----\n> > src/ipa/ipu3/algorithms/agc.h | 2 +-\n> > src/ipa/ipu3/algorithms/awb.cpp | 14 ++++-----\n> > src/ipa/ipu3/algorithms/tone_mapping.cpp | 8 ++---\n> > src/ipa/ipu3/ipa_context.h | 2 +-\n> > src/ipa/ipu3/ipu3.cpp | 38 +++++++++++++++++++-----\n> > 6 files changed, 49 insertions(+), 27 deletions(-)\n> >\n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index 52cf2753..2eee5b6b 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -245,7 +245,7 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain, double curre\n> > * \\param[in] stats The IPU3 statistics and ISP results\n> > * \\param[in] currentYGain The gain calculated on the current brightness level\n> > */\n> > -double Agc::computeInitialY(IPAFrameContext &frameContext,\n> > +double Agc::computeInitialY(IPAFrameContext *frameContext,\n> > const ipu3_uapi_grid_config &grid,\n> > const ipu3_uapi_stats_3a *stats,\n> > double currentYGain)\n> > @@ -271,9 +271,9 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> > * Estimate the sum of the brightness values, weighted with the gains\n> > * applied on the channels in AWB.\n> > */\n> > - double Y_sum = redSum * frameContext.awb.gains.red * .299 +\n> > - greenSum * frameContext.awb.gains.green * .587 +\n> > - blueSum * frameContext.awb.gains.blue * .114;\n> > + double Y_sum = redSum * frameContext->awb.gains.red * .299 +\n> > + greenSum * frameContext->awb.gains.green * .587 +\n> > + blueSum * frameContext->awb.gains.blue * .114;\n> >\n> > /* And return the average brightness */\n> > return Y_sum / (grid.height * grid.width);\n> > @@ -291,8 +291,8 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,\n> > void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> > {\n> > /* Get the latest exposure and gain applied */\n> > - uint32_t &exposure = context.frameContext.agc.exposure;\n> > - double &analogueGain = context.frameContext.agc.gain;\n> > + uint32_t &exposure = context.frameContext->agc.exposure;\n> > + double &analogueGain = context.frameContext->agc.gain;\n> > measureBrightness(stats, context.configuration.grid.bdsGrid);\n> >\n> > double currentYGain = 1.0;\n> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> > index 6f5d71e0..5d6bef9d 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.h\n> > +++ b/src/ipa/ipu3/algorithms/agc.h\n> > @@ -35,7 +35,7 @@ private:\n> > const ipu3_uapi_grid_config &grid);\n> > void filterExposure();\n> > void computeExposure(uint32_t &exposure, double &gain, double currentYGain);\n> > - double computeInitialY(IPAFrameContext &frameContext,\n> > + double computeInitialY(IPAFrameContext *frameContext,\n> > const ipu3_uapi_grid_config &grid,\n> > const ipu3_uapi_stats_3a *stats,\n> > double currentYGain);\n> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> > index a4114659..bd55d377 100644\n> > --- a/src/ipa/ipu3/algorithms/awb.cpp\n> > +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> > @@ -382,9 +382,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> > * The results are cached, so if no results were calculated, we set the\n> > * cached values from asyncResults_ here.\n> > */\n> > - context.frameContext.awb.gains.blue = asyncResults_.blueGain;\n> > - context.frameContext.awb.gains.green = asyncResults_.greenGain;\n> > - context.frameContext.awb.gains.red = asyncResults_.redGain;\n> > + context.frameContext->awb.gains.blue = asyncResults_.blueGain;\n> > + context.frameContext->awb.gains.green = asyncResults_.greenGain;\n> > + context.frameContext->awb.gains.red = asyncResults_.redGain;\n> > }\n> >\n> > constexpr uint16_t Awb::threshold(float value)\n> > @@ -432,10 +432,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> > params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset\n> > * params->acc_param.bnr.opt_center.y_reset;\n> > /* Convert to u3.13 fixed point values */\n> > - params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green;\n> > - params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red;\n> > - params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue;\n> > - params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green;\n> > + params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext->awb.gains.green;\n> > + params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext->awb.gains.red;\n> > + params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext->awb.gains.blue;\n> > + params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext->awb.gains.green;\n> >\n> > LOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n> >\n> > diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> > index 5d74c552..50498f41 100644\n> > --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> > +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> > @@ -57,7 +57,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,\n> > {\n> > /* Copy the calculated LUT into the parameters buffer. */\n> > memcpy(params->acc_param.gamma.gc_lut.lut,\n> > - context.frameContext.toneMapping.gammaCorrection.lut,\n> > + context.frameContext->toneMapping.gammaCorrection.lut,\n> > IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n> > sizeof(params->acc_param.gamma.gc_lut.lut[0]));\n> >\n> > @@ -84,11 +84,11 @@ void ToneMapping::process(IPAContext &context,\n> > */\n> > gamma_ = 1.1;\n> >\n> > - if (context.frameContext.toneMapping.gamma == gamma_)\n> > + if (context.frameContext->toneMapping.gamma == gamma_)\n> > return;\n> >\n> > struct ipu3_uapi_gamma_corr_lut &lut =\n> > - context.frameContext.toneMapping.gammaCorrection;\n> > + context.frameContext->toneMapping.gammaCorrection;\n> >\n> > for (uint32_t i = 0; i < std::size(lut.lut); i++) {\n> > double j = static_cast(i) / (std::size(lut.lut) - 1);\n> > @@ -98,7 +98,7 @@ void ToneMapping::process(IPAContext &context,\n> > lut.lut[i] = gamma * 8191;\n> > }\n> >\n> > - context.frameContext.toneMapping.gamma = gamma_;\n> > + context.frameContext->toneMapping.gamma = gamma_;\n> > }\n> >\n> > } /* namespace ipa::ipu3::algorithms */\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index ee8f7b55..69780915 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -57,7 +57,7 @@ struct IPAFrameContext {\n> >\n> > struct IPAContext {\n> > IPASessionConfiguration configuration;\n> > - IPAFrameContext frameContext;\n> > + IPAFrameContext *frameContext;\n> > };\n> >\n> > } /* namespace ipa::ipu3 */\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 177c5c2f..b60ab7e7 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -522,10 +522,26 @@ void IPAIPU3::unmapBuffers(const std::vector &ids)\n> >\n> > void IPAIPU3::frameStarted([[maybe_unused]] unsigned int frame)\n> > {\n> > + /*\n> > + * Instantiate a new IPAFrameContext to push to the queue. The lifetime\n> > + * of this pointer is indirectly controlled by the\n> > + * ActionSetSensorControls event, as this is where we will delete it.\n> > + */\n> > + struct IPAFrameContext *frameContext = new IPAFrameContext;\n> > + frameContext->frameId = frame;\n> > + frameContextQueue.push(frameContext);\n> \n> Maybe a detail:\n> \n> Wouldn't it be safer to store unique_ptr in the queue\n> instead of manually allocate/free ?\n\nI'm curious if we should even be storing pointers at all, and not put a\nfull IPAFrameContext on the queue instead.\n\nReading https://en.cppreference.com/w/cpp/container/deque (which is the\ndefault queue implementation I believe) sounds like the implementation\nwill be able to reuse existing allocations then, rather than performing\na new/delete for every IPAFrameContext instance.\n\nIn essence, I think it can be a cheap-ish scalable ring buffer, which is\nwhat we actually want here.\n\nI think \"[libcamera-devel] [PATCH 14/22] ipa: ipu3: Introduce a queue of\nIPAFrameContext\" should be squashed into the content of this patch\nthough. There's no point separating the implementation of the queue from\nthe addition of it to the IPU3. I think it's clearer together.\n\n\n> > }\n> >\n> > void IPAIPU3::frameCompleted([[maybe_unused]] unsigned int frame)\n> > {\n> > + /*\n> > + * Remove the pointer from the queue, it should not be accessed\n> > + * anymore and delete it.\n> > + */\n> > + struct IPAFrameContext *frameContext = frameContextQueue.front();\n\nAlthough - It's worth checking to see if .front() gives a reference...\n\nhttps://www.cplusplus.com/reference/deque/deque/front/\n\n.front() returns a reference, so we can put the whole IPAFrameContext on\nthere without making copies at points like this. (Had to check, as I was\nconcerned if this would be a reason to not put the whole structure on\nthe queue).\n\n\n\n\n> > + ASSERT(frameContext->frameId == frame);\n> > + frameContextQueue.pop();\n> > + delete frameContext;\n> > }\n> >\n> > /**\n> > @@ -579,10 +595,6 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> > const ipu3_uapi_stats_3a *stats =\n> > reinterpret_cast(mem.data());\n> >\n> > - /* \\todo move those into processControls */\n\nOk, I see why there was a todo now.\n\n> > - context_.frameContext.agc.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > - context_.frameContext.agc.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get());\n> > -\n> > parseStatistics(event.frame, event.frameTimestamp, stats);\n> > /*\n> > * To save incurring extra IPC calls, we do not send explicit events\n> > @@ -610,9 +622,12 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> > */\n> > void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> > [[maybe_unused]] const ControlList &controls,\n> > - [[maybe_unused]] const ControlList &sensorCtrls)\n> > + const ControlList &sensorCtrls)\n> > {\n> > - /* \\todo Start processing for 'frame' based on 'controls'. */\n> > + struct IPAFrameContext *frameContext = frameContextQueue.back();\n> > + ASSERT(frameContext->frameId == frame);\n> > + frameContext->agc.exposure = sensorCtrls.get(V4L2_CID_EXPOSURE).get();\n> > + frameContext->agc.gain = camHelper_->gain(sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN).get());\n\nI'm ... still concerned about how a sequencing diagram would show the\nusage of these values.\n\nProcessControls is the 'first' event that occurs. I don't think I see\nhow the IPA should consider the exposure and gain of the sensor at that\npoint.\n\nThe IPA should be in charge of 'setting' the exposure and gain on frames\nas they go out, and I think it should only care about what values were\nset on the frame associated with the statistics that it processes.\n\n> > }\n> >\n> > /**\n> > @@ -636,6 +651,9 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > */\n> > params->use = {};\n> >\n> > + /* Set the reference to the current frame context */\n> > + context_.frameContext = frameContextQueue.front();\n> > +\n> > for (auto const &algo : algorithms_)\n> > algo->prepare(context_, params);\n> >\n> > @@ -661,6 +679,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > {\n> > ControlList ctrls(controls::controls);\n> >\n> > + /* Set the reference to the current frame context */\n> > + context_.frameContext = frameContextQueue.front();\n> > +\n\nSeeing this set the reference in both fillParams and parseStatistics\nfeels risky/racy. It might not be right now - but it feels like it could\nbe in the future when those two events might have potential to happen in\nparallel. Or maybe we would prevent that from happening.\n\n\n\n> > for (auto const &algo : algorithms_)\n> > algo->process(context_, stats);\n> >\n> > @@ -690,8 +711,9 @@ void IPAIPU3::setControls(unsigned int frame)\n> > IPU3Action op;\n> > op.op = ActionSetSensorControls;\n> >\n> > - exposure_ = context_.frameContext.agc.exposure;\n> > - gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n> > + /* Apply the exposure and gain updated values */\n> > + exposure_ = context_.frameContext->agc.exposure;\n> > + gain_ = camHelper_->gainCode(context_.frameContext->agc.gain);\n\nIf exposure_ and gain_ get set immediately here, do they need to be\nstored as class variables? Can they be local? or do they get used\nsomewhere else? (I'd expect any other usages to be specific to a\nFrameContext reference...)\n\n> >\n> > ControlList ctrls(ctrls_);\n> > ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > --\n> > 2.32.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DE66FBDB1C\n\tfor ;\n\tMon, 8 Nov 2021 16:09:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 591AA6034A;\n\tMon, 8 Nov 2021 17:09:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C6BE6032C\n\tfor ;\n\tMon, 8 Nov 2021 17:09:35 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E75A98BB;\n\tMon, 8 Nov 2021 17:09:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CM3oX0I6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636387775;\n\tbh=Opkij2L0HQ2tscxsiibSacU2yWJadzgr08IZYhXDUk0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=CM3oX0I6U94RAK6R7GlgVe9ALm9kh1vhcycq2Ig+YjPWzE5wIxD4ymdcJl+wF71TL\n\tZypbP0kNX5aeasEFw6pujiD+YLY2d9fa0cQsHmDtU00WY5CxKC4mXfQcQUW+76Aon0\n\tI7WFnHcoOllLZk0USgAgPGAP/kTJ+CxP1D44OVrY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211108141043.tgy2w2n2vdqomsbt@uno.localdomain>","References":"<20211108131350.130665-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211108131350.130665-18-jeanmichel.hautbois@ideasonboard.com>\n\t<20211108141043.tgy2w2n2vdqomsbt@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ,\n\tJean-Michel Hautbois ","Date":"Mon, 08 Nov 2021 16:09:33 +0000","Message-ID":"<163638777308.275423.16181466655875962303@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 17/22] ipa: ipu3: Use a\n\tIPAFrameContext pointer from the per-frame queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]