[{"id":21817,"web_url":"https://patchwork.libcamera.org/comment/21817/","msgid":"<8bf65a87-14cf-f816-2313-30a7e0ff6130@ideasonboard.com>","date":"2021-12-17T09:03:42","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Add a IPAFrameContext\n\tqueue","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nThanks for the patch !\n\nOn 14/12/2021 19:43, Umang Jain wrote:\n> Having a single IPAFrameContext queue is limiting especially when\n> we need to preserve per-frame controls. Right now, we are not processing\n> any controls on the IPA side (processControls()) but sooner or later\n> we need to preserve the controls setting for the frames in a sequential\n> fashion. Since IPAIPU3::processControls() is executed on\n> IPU3CameraData::queuePendingRequests() code path, we need to store the\n> incoming control setting in a separate IPAFrameContext and push that\n> into the queue.\n> \n> The controls for the next frame to be processed by the IPA are retrieved\n> back from the queue in parseStatistics() and set via setControls().\n> Meanwhile, the last IPAFrameContext is preserved as prevFrameContext to\n> populate the metadata (for ActionMetadataReady).\n> \n\nIn order to ease the flow reading, maybe could we pick this patch ?\nhttps://patchwork.libcamera.org/patch/14484/\n\n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n> Changes in v2:\n> - Fix over-exposed image stream issue explained in v1.\n>    The issue was non-initialized data being used in the frameContext.\n>   \n> ---\n>   src/ipa/ipu3/algorithms/agc.cpp          | 18 ++++++-------\n>   src/ipa/ipu3/algorithms/agc.h            |  2 +-\n>   src/ipa/ipu3/algorithms/awb.cpp          | 18 +++++++------\n>   src/ipa/ipu3/algorithms/tone_mapping.cpp | 11 ++++----\n>   src/ipa/ipu3/ipa_context.cpp             | 32 +++++++++++++++++-----\n>   src/ipa/ipu3/ipa_context.h               | 11 +++++++-\n>   src/ipa/ipu3/ipu3.cpp                    | 34 +++++++++++++++++++-----\n>   7 files changed, 89 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 8d6f18f6..b57ca215 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -99,8 +99,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)\n>   \tmaxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);\n>   \n>   \t/* Configure the default exposure and gain. */\n> -\tcontext.frameContext.agc.gain = minAnalogueGain_;\n> -\tcontext.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;\n> +\tIPAFrameContext &frameContext = context.frameContextQueue.front();\n> +\tframeContext.agc.gain = minAnalogueGain_;\n> +\tframeContext.agc.exposure = minShutterSpeed_ / lineDuration_;\n>   \n>   \treturn 0;\n>   }\n> @@ -178,12 +179,12 @@ void Agc::filterExposure()\n>    * \\param[in] yGain The gain calculated based on the relative luminance target\n>    * \\param[in] iqMeanGain The gain calculated based on the relative luminance target\n>    */\n> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,\n> -\t\t\t  double iqMeanGain)\n> +void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)\n>   {\n\nYou need to change the associated doc ;-).\n\n>   \t/* Get the effective exposure and gain applied on the sensor. */\n> -\tuint32_t exposure = frameContext.sensor.exposure;\n> -\tdouble analogueGain = frameContext.sensor.gain;\n> +\tuint32_t exposure = context.prevFrameContext.sensor.exposure;\n> +\tdouble analogueGain = context.prevFrameContext.sensor.gain;\n> +\tIPAFrameContext &frameContext = context.frameContextQueue.front();\n\nI would move this line juste before the assignement at the end of the \nfunction ?\n\n>   \n>   \t/* Use the highest of the two gain estimates. */\n>   \tdouble evGain = std::max(yGain, iqMeanGain);\n> @@ -333,9 +334,8 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>   \t */\n>   \tdouble yGain = 1.0;\n>   \tdouble yTarget = kRelativeLuminanceTarget;\n> -\n>   \tfor (unsigned int i = 0; i < 8; i++) {\n> -\t\tdouble yValue = estimateLuminance(context.frameContext,\n> +\t\tdouble yValue = estimateLuminance(context.prevFrameContext,\n>   \t\t\t\t\t\t  context.configuration.grid.bdsGrid,\n>   \t\t\t\t\t\t  stats, yGain);\n>   \t\tdouble extraGain = std::min(10.0, yTarget / (yValue + .001));\n> @@ -348,7 +348,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>   \t\t\tbreak;\n>   \t}\n>   \n> -\tcomputeExposure(context.frameContext, yGain, iqMeanGain);\n> +\tcomputeExposure(context, yGain, iqMeanGain);\n>   \tframeCount_++;\n>   }\n>   \n> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h\n> index 96ec7005..1b444b12 100644\n> --- a/src/ipa/ipu3/algorithms/agc.h\n> +++ b/src/ipa/ipu3/algorithms/agc.h\n> @@ -34,7 +34,7 @@ private:\n>   \tdouble measureBrightness(const ipu3_uapi_stats_3a *stats,\n>   \t\t\t\t const ipu3_uapi_grid_config &grid) const;\n>   \tvoid filterExposure();\n> -\tvoid computeExposure(IPAFrameContext &frameContext, double yGain,\n> +\tvoid computeExposure(IPAContext &context, double yGain,\n>   \t\t\t     double iqMeanGain);\n>   \tdouble estimateLuminance(IPAFrameContext &frameContext,\n>   \t\t\t\t const ipu3_uapi_grid_config &grid,\n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index 1dc27fc9..3c004928 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -382,16 +382,17 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>   void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>   {\n>   \tcalculateWBGains(stats);\n> +\tIPAFrameContext &frameContext = context.frameContextQueue.front();\n>   \n>   \t/*\n>   \t * Gains are only recalculated if enough zones were detected.\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.temperatureK = asyncResults_.temperatureK;\n> +\tframeContext.awb.gains.blue = asyncResults_.blueGain;\n> +\tframeContext.awb.gains.green = asyncResults_.greenGain;\n> +\tframeContext.awb.gains.red = asyncResults_.redGain;\n> +\tframeContext.awb.temperatureK = asyncResults_.temperatureK;\n>   }\n>   \n>   constexpr uint16_t Awb::threshold(float value)\n> @@ -434,6 +435,7 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n>   \t */\n>   \tparams->acc_param.bnr = imguCssBnrDefaults;\n>   \tSize &bdsOutputSize = context.configuration.grid.bdsOutputSize;\n> +\tIPAFrameContext &frameContext = context.frameContextQueue.front();\n>   \tparams->acc_param.bnr.column_size = bdsOutputSize.width;\n>   \tparams->acc_param.bnr.opt_center.x_reset = grid.x_start - (bdsOutputSize.width / 2);\n>   \tparams->acc_param.bnr.opt_center.y_reset = grid.y_start - (bdsOutputSize.height / 2);\n> @@ -442,10 +444,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 * frameContext.awb.gains.green;\n> +\tparams->acc_param.bnr.wb_gains.r  = 8192 * frameContext.awb.gains.red;\n> +\tparams->acc_param.bnr.wb_gains.b  = 8192 * frameContext.awb.gains.blue;\n> +\tparams->acc_param.bnr.wb_gains.gb = 8192 * 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 2040eda5..bf710bd1 100644\n> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> @@ -42,7 +42,7 @@ int ToneMapping::configure(IPAContext &context,\n>   \t\t\t   [[maybe_unused]] const IPAConfigInfo &configInfo)\n>   {\n>   \t/* Initialise tone mapping gamma value. */\n> -\tcontext.frameContext.toneMapping.gamma = 0.0;\n> +\tcontext.frameContextQueue.front().toneMapping.gamma = 0.0;\n>   \n>   \treturn 0;\n>   }\n> @@ -60,7 +60,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.frameContextQueue.front().toneMapping.gammaCorrection.lut,\n>   \t       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n>   \t       sizeof(params->acc_param.gamma.gc_lut.lut[0]));\n>   \n> @@ -80,6 +80,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,\n>   void ToneMapping::process(IPAContext &context,\n>   \t\t\t  [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>   {\n> +\tIPAFrameContext &frameContext = context.frameContextQueue.front();\n>   \t/*\n>   \t * Hardcode gamma to 1.1 as a default for now.\n>   \t *\n> @@ -87,11 +88,11 @@ void ToneMapping::process(IPAContext &context,\n>   \t */\n>   \tgamma_ = 1.1;\n>   \n> -\tif (context.frameContext.toneMapping.gamma == gamma_)\n> +\tif (frameContext.toneMapping.gamma == gamma_)\n>   \t\treturn;\n>   \n>   \tstruct ipu3_uapi_gamma_corr_lut &lut =\n> -\t\tcontext.frameContext.toneMapping.gammaCorrection;\n> +\t\tframeContext.toneMapping.gammaCorrection;\n>   \n>   \tfor (uint32_t i = 0; i < std::size(lut.lut); i++) {\n>   \t\tdouble j = static_cast<double>(i) / (std::size(lut.lut) - 1);\n> @@ -101,7 +102,7 @@ void ToneMapping::process(IPAContext &context,\n>   \t\tlut.lut[i] = gamma * 8191;\n>   \t}\n>   \n> -\tcontext.frameContext.toneMapping.gamma = gamma_;\n> +\tframeContext.toneMapping.gamma = gamma_;\n>   }\n>   \n>   } /* namespace ipa::ipu3::algorithms */\n> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> index 86794ac1..f503b93d 100644\n> --- a/src/ipa/ipu3/ipa_context.cpp\n> +++ b/src/ipa/ipu3/ipa_context.cpp\n> @@ -39,6 +39,23 @@ namespace libcamera::ipa::ipu3 {\n>    * algorithm, but should only be written by its owner.\n>    */\n>   \n> +/**\n> + * \\brief Construct a IPAFrameContext instance\n> + */\n> +IPAFrameContext::IPAFrameContext() = default;\n> +\n> +/**\n> + * \\brief Move constructor for IPAFrameContext\n> + * \\param[in] other The other IPAFrameContext\n> + */\n> +IPAFrameContext::IPAFrameContext(IPAFrameContext &&other) = default;\n> +\n> +/**\n> + * \\brief Move assignment operator for IPAFrameContext\n> + * \\param[in] other The other IPAFrameContext\n> + */\n> +IPAFrameContext &IPAFrameContext::operator=(IPAFrameContext &&other) = default;\n> +\n>   /**\n>    * \\struct IPAContext\n>    * \\brief Global IPA context data shared between all algorithms\n> @@ -46,13 +63,11 @@ namespace libcamera::ipa::ipu3 {\n>    * \\var IPAContext::configuration\n>    * \\brief The IPA session configuration, immutable during the session\n>    *\n> - * \\var IPAContext::frameContext\n> - * \\brief The frame context for the frame being processed\n> + * \\var IPAContext::frameContextQueue\n> + * \\brief A queue of frame contexts to be processed by the IPA\n>    *\n> - * \\todo While the frame context is supposed to be per-frame, this\n> - * single frame context stores data related to both the current frame\n> - * and the previous frames, with fields being updated as the algorithms\n> - * are run. This needs to be turned into real per-frame data storage.\n> + * \\var IPAContext::prevFrameContext\n> + * \\brief The latest frame context which the IPA has finished processing\n>    */\n>   \n>   /**\n> @@ -86,6 +101,11 @@ namespace libcamera::ipa::ipu3 {\n>    * \\brief Maximum analogue gain supported with the configured sensor\n>    */\n>   \n> +/**\n> + * \\var IPAFrameContext::frame\n> + * \\brief Frame number of the corresponding frame context\n> + */\n> +\n>   /**\n>    * \\var IPAFrameContext::agc\n>    * \\brief Context for the Automatic Gain Control algorithm\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index c6dc0814..a5e298bd 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -14,6 +14,8 @@\n>   \n>   #include <libcamera/geometry.h>\n>   \n> +#include <queue>\n> +\n>   namespace libcamera {\n>   \n>   namespace ipa::ipu3 {\n> @@ -34,6 +36,12 @@ struct IPASessionConfiguration {\n>   };\n>   \n>   struct IPAFrameContext {\n> +\tuint32_t frame;\n> +\n> +\tIPAFrameContext();\n> +\tIPAFrameContext(IPAFrameContext &&other);\n> +\tIPAFrameContext &operator=(IPAFrameContext &&other);\n> +\n>   \tstruct {\n>   \t\tuint32_t exposure;\n>   \t\tdouble gain;\n> @@ -62,7 +70,8 @@ struct IPAFrameContext {\n>   \n>   struct IPAContext {\n>   \tIPASessionConfiguration configuration;\n> -\tIPAFrameContext frameContext;\n> +\tstd::queue<IPAFrameContext> frameContextQueue;\n> +\tIPAFrameContext prevFrameContext;\n>   };\n>   \n>   } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 3d307708..763dbd7d 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -324,6 +324,8 @@ int IPAIPU3::start()\n>    */\n>   void IPAIPU3::stop()\n>   {\n> +\twhile (!context_.frameContextQueue.empty())\n> +\t\tcontext_.frameContextQueue.pop();\n>   }\n>   \n>   /**\n> @@ -457,6 +459,14 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>   \t/* Clean context at configuration */\n>   \tcontext_ = {};\n>   \n> +\t/*\n> +\t * Insert a initial context into the queue to faciliate\n> +\t * algo->configure() below.\n> +\t */\n> +\tIPAFrameContext initContext;\n> +\tinitContext.frame = 0;\n> +\tcontext_.frameContextQueue.push(std::move(initContext));\n> +\n>   \tcalculateBdsGrid(configInfo.bdsOutputSize);\n>   \n>   \tlineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\n> @@ -547,8 +557,9 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>   \t\tconst ipu3_uapi_stats_3a *stats =\n>   \t\t\treinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n>   \n> -\t\tcontext_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> -\t\tcontext_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n> +\t\tIPAFrameContext &curFrameContext = context_.frameContextQueue.front();\n\nShould we test if frameContextQueue is empty ? As an assert probably as \nit should never happen ?\n\n> +\t\tcurFrameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> +\t\tcurFrameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n>   \n>   \t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n>   \t\tbreak;\n> @@ -571,6 +582,11 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>   \t\t\t      [[maybe_unused]] const ControlList &controls)\n>   {\n>   \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> +\n> +\tIPAFrameContext newContext;\n> +\tnewContext.frame = frame;\n> +\n> +\tcontext_.frameContextQueue.push(std::move(newContext));\n\nThat's why the patch proposed to mark the beginning and and of a frame \ncould make sense. You we create and push it with the frame number in \nframeStarted and pop it in frameCompleted ?\n\n>   }\n>   \n>   /**\n> @@ -619,6 +635,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>   {\n>   \tControlList ctrls(controls::controls);\n>   \n> +\tcontext_.prevFrameContext = std::move(context_.frameContextQueue.front());\n> +\tcontext_.frameContextQueue.pop();\n\nDo we want to pop it here or after the parseStatistics call ?\n> +\n>   \tfor (auto const &algo : algorithms_)\n>   \t\talgo->process(context_, stats);\n>   \n> @@ -628,11 +647,11 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>   \tint64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>();\n>   \tctrls.set(controls::FrameDuration, frameDuration);\n>   \n> -\tctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);\n> +\tctrls.set(controls::AnalogueGain, context_.prevFrameContext.sensor.gain);\n>   \n> -\tctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);\n> +\tctrls.set(controls::ColourTemperature, context_.prevFrameContext.awb.temperatureK);\n>   \n> -\tctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>());\n> +\tctrls.set(controls::ExposureTime, context_.prevFrameContext.sensor.exposure * lineDuration_.get<std::micro>());\n>   \n>   \t/*\n>   \t * \\todo The Metadata provides a path to getting extended data\n> @@ -661,8 +680,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> +\tIPAFrameContext &context = context_.frameContextQueue.front();\n> +\texposure_ = context.agc.exposure;\n> +\tgain_ = camHelper_->gainCode(context.agc.gain);\n>   \n>   \tControlList ctrls(ctrls_);\n>   \tctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A2FDABF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 17 Dec 2021 09:03:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDF5760592;\n\tFri, 17 Dec 2021 10:03:47 +0100 (CET)","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 45A7F60223\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 17 Dec 2021 10:03:46 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:7f55:44a0:7b3f:1443] (unknown\n\t[IPv6:2a01:e0a:169:7140:7f55:44a0:7b3f:1443])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB45192A;\n\tFri, 17 Dec 2021 10:03:45 +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=\"sO5X3vK3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639731825;\n\tbh=jFYD3AdL3lXmGLPQ2JhAEkpg1cnyYBtqA1nsE/iyd+M=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=sO5X3vK3cEo1GlBuA/QiXr/6zDhaetA0ysQgXf0a9jjPJa5rfZpXL+0OWlBxGdm14\n\twuRGzBGUupvddMkj0CoixcGDZ6GTZJ1GULcCxiPPsJ2RgRlb1MblxgWraB+e6QW1vA\n\t3QRoOUlgzIsafwDEgikV7+RV2IywUvjFvGsX5LMc=","Message-ID":"<8bf65a87-14cf-f816-2313-30a7e0ff6130@ideasonboard.com>","Date":"Fri, 17 Dec 2021 10:03:42 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211214184323.571771-1-umang.jain@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","In-Reply-To":"<20211214184323.571771-1-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Add a IPAFrameContext\n\tqueue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21818,"web_url":"https://patchwork.libcamera.org/comment/21818/","msgid":"<28f3d251-3c01-dbc9-5fc1-6ae99230e055@ideasonboard.com>","date":"2021-12-17T09:22:40","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Add a IPAFrameContext\n\tqueue","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM Thanks for the review\n\nOn 12/17/21 2:33 PM, Jean-Michel Hautbois wrote:\n> Hi Umang,\n>\n> Thanks for the patch !\n>\n> On 14/12/2021 19:43, Umang Jain wrote:\n>> Having a single IPAFrameContext queue is limiting especially when\n>> we need to preserve per-frame controls. Right now, we are not processing\n>> any controls on the IPA side (processControls()) but sooner or later\n>> we need to preserve the controls setting for the frames in a sequential\n>> fashion. Since IPAIPU3::processControls() is executed on\n>> IPU3CameraData::queuePendingRequests() code path, we need to store the\n>> incoming control setting in a separate IPAFrameContext and push that\n>> into the queue.\n>>\n>> The controls for the next frame to be processed by the IPA are retrieved\n>> back from the queue in parseStatistics() and set via setControls().\n>> Meanwhile, the last IPAFrameContext is preserved as prevFrameContext to\n>> populate the metadata (for ActionMetadataReady).\n>>\n>\n> In order to ease the flow reading, maybe could we pick this patch ?\n> https://patchwork.libcamera.org/patch/14484/\n\n\nOKay, I see it has one R-b tag so fine by me\n\nI think I should also port the IPU3Event/Actions to respective functions \nas we have for vimc IPA interface. As reported earlier, I have a WIP \nbranch on it already. So it makes sense to complete that first and then \nintroduce the frameStart() and frameCompleted() as per your patch.\n\n>\n>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>> ---\n>> Changes in v2:\n>> - Fix over-exposed image stream issue explained in v1.\n>>    The issue was non-initialized data being used in the frameContext.\n>>   ---\n>>   src/ipa/ipu3/algorithms/agc.cpp          | 18 ++++++-------\n>>   src/ipa/ipu3/algorithms/agc.h            |  2 +-\n>>   src/ipa/ipu3/algorithms/awb.cpp          | 18 +++++++------\n>>   src/ipa/ipu3/algorithms/tone_mapping.cpp | 11 ++++----\n>>   src/ipa/ipu3/ipa_context.cpp             | 32 +++++++++++++++++-----\n>>   src/ipa/ipu3/ipa_context.h               | 11 +++++++-\n>>   src/ipa/ipu3/ipu3.cpp                    | 34 +++++++++++++++++++-----\n>>   7 files changed, 89 insertions(+), 37 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp \n>> b/src/ipa/ipu3/algorithms/agc.cpp\n>> index 8d6f18f6..b57ca215 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.cpp\n>> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n>> @@ -99,8 +99,9 @@ int Agc::configure(IPAContext &context, const \n>> IPAConfigInfo &configInfo)\n>>       maxAnalogueGain_ = \n>> std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);\n>>         /* Configure the default exposure and gain. */\n>> -    context.frameContext.agc.gain = minAnalogueGain_;\n>> -    context.frameContext.agc.exposure = minShutterSpeed_ / \n>> lineDuration_;\n>> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n>> +    frameContext.agc.gain = minAnalogueGain_;\n>> +    frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;\n>>         return 0;\n>>   }\n>> @@ -178,12 +179,12 @@ void Agc::filterExposure()\n>>    * \\param[in] yGain The gain calculated based on the relative \n>> luminance target\n>>    * \\param[in] iqMeanGain The gain calculated based on the relative \n>> luminance target\n>>    */\n>> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,\n>> -              double iqMeanGain)\n>> +void Agc::computeExposure(IPAContext &context, double yGain, double \n>> iqMeanGain)\n>>   {\n>\n> You need to change the associated doc ;-).\n\n\nops, yeah\n\n>\n>>       /* Get the effective exposure and gain applied on the sensor. */\n>> -    uint32_t exposure = frameContext.sensor.exposure;\n>> -    double analogueGain = frameContext.sensor.gain;\n>> +    uint32_t exposure = context.prevFrameContext.sensor.exposure;\n>> +    double analogueGain = context.prevFrameContext.sensor.gain;\n>> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n>\n> I would move this line juste before the assignement at the end of the \n> function ?\n>\n>>         /* Use the highest of the two gain estimates. */\n>>       double evGain = std::max(yGain, iqMeanGain);\n>> @@ -333,9 +334,8 @@ void Agc::process(IPAContext &context, const \n>> ipu3_uapi_stats_3a *stats)\n>>        */\n>>       double yGain = 1.0;\n>>       double yTarget = kRelativeLuminanceTarget;\n>> -\n>>       for (unsigned int i = 0; i < 8; i++) {\n>> -        double yValue = estimateLuminance(context.frameContext,\n>> +        double yValue = estimateLuminance(context.prevFrameContext,\n>>                             context.configuration.grid.bdsGrid,\n>>                             stats, yGain);\n>>           double extraGain = std::min(10.0, yTarget / (yValue + .001));\n>> @@ -348,7 +348,7 @@ void Agc::process(IPAContext &context, const \n>> ipu3_uapi_stats_3a *stats)\n>>               break;\n>>       }\n>>   -    computeExposure(context.frameContext, yGain, iqMeanGain);\n>> +    computeExposure(context, yGain, iqMeanGain);\n>>       frameCount_++;\n>>   }\n>>   diff --git a/src/ipa/ipu3/algorithms/agc.h \n>> b/src/ipa/ipu3/algorithms/agc.h\n>> index 96ec7005..1b444b12 100644\n>> --- a/src/ipa/ipu3/algorithms/agc.h\n>> +++ b/src/ipa/ipu3/algorithms/agc.h\n>> @@ -34,7 +34,7 @@ private:\n>>       double measureBrightness(const ipu3_uapi_stats_3a *stats,\n>>                    const ipu3_uapi_grid_config &grid) const;\n>>       void filterExposure();\n>> -    void computeExposure(IPAFrameContext &frameContext, double yGain,\n>> +    void computeExposure(IPAContext &context, double yGain,\n>>                    double iqMeanGain);\n>>       double estimateLuminance(IPAFrameContext &frameContext,\n>>                    const ipu3_uapi_grid_config &grid,\n>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp \n>> b/src/ipa/ipu3/algorithms/awb.cpp\n>> index 1dc27fc9..3c004928 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>> @@ -382,16 +382,17 @@ void Awb::calculateWBGains(const \n>> ipu3_uapi_stats_3a *stats)\n>>   void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a \n>> *stats)\n>>   {\n>>       calculateWBGains(stats);\n>> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n>>         /*\n>>        * Gains are only recalculated if enough zones were detected.\n>>        * The results are cached, so if no results were calculated, we \n>> 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.temperatureK = asyncResults_.temperatureK;\n>> +    frameContext.awb.gains.blue = asyncResults_.blueGain;\n>> +    frameContext.awb.gains.green = asyncResults_.greenGain;\n>> +    frameContext.awb.gains.red = asyncResults_.redGain;\n>> +    frameContext.awb.temperatureK = asyncResults_.temperatureK;\n>>   }\n>>     constexpr uint16_t Awb::threshold(float value)\n>> @@ -434,6 +435,7 @@ void Awb::prepare(IPAContext &context, \n>> ipu3_uapi_params *params)\n>>        */\n>>       params->acc_param.bnr = imguCssBnrDefaults;\n>>       Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;\n>> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n>>       params->acc_param.bnr.column_size = bdsOutputSize.width;\n>>       params->acc_param.bnr.opt_center.x_reset = grid.x_start - \n>> (bdsOutputSize.width / 2);\n>>       params->acc_param.bnr.opt_center.y_reset = grid.y_start - \n>> (bdsOutputSize.height / 2);\n>> @@ -442,10 +444,10 @@ void Awb::prepare(IPAContext &context, \n>> ipu3_uapi_params *params)\n>>       params->acc_param.bnr.opt_center_sqr.y_sqr_reset = \n>> params->acc_param.bnr.opt_center.y_reset\n>>                               * \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 * \n>> context.frameContext.awb.gains.green;\n>> -    params->acc_param.bnr.wb_gains.r  = 8192 * \n>> context.frameContext.awb.gains.red;\n>> -    params->acc_param.bnr.wb_gains.b  = 8192 * \n>> context.frameContext.awb.gains.blue;\n>> -    params->acc_param.bnr.wb_gains.gb = 8192 * \n>> context.frameContext.awb.gains.green;\n>> +    params->acc_param.bnr.wb_gains.gr = 8192 * \n>> frameContext.awb.gains.green;\n>> +    params->acc_param.bnr.wb_gains.r  = 8192 * \n>> frameContext.awb.gains.red;\n>> +    params->acc_param.bnr.wb_gains.b  = 8192 * \n>> frameContext.awb.gains.blue;\n>> +    params->acc_param.bnr.wb_gains.gb = 8192 * \n>> frameContext.awb.gains.green;\n>>         LOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << \n>> asyncResults_.temperatureK;\n>>   diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp \n>> b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n>> index 2040eda5..bf710bd1 100644\n>> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp\n>> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n>> @@ -42,7 +42,7 @@ int ToneMapping::configure(IPAContext &context,\n>>                  [[maybe_unused]] const IPAConfigInfo &configInfo)\n>>   {\n>>       /* Initialise tone mapping gamma value. */\n>> -    context.frameContext.toneMapping.gamma = 0.0;\n>> +    context.frameContextQueue.front().toneMapping.gamma = 0.0;\n>>         return 0;\n>>   }\n>> @@ -60,7 +60,7 @@ void ToneMapping::prepare([[maybe_unused]] \n>> 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.frameContextQueue.front().toneMapping.gammaCorrection.lut,\n>>              IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n>>              sizeof(params->acc_param.gamma.gc_lut.lut[0]));\n>>   @@ -80,6 +80,7 @@ void ToneMapping::prepare([[maybe_unused]] \n>> IPAContext &context,\n>>   void ToneMapping::process(IPAContext &context,\n>>                 [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>>   {\n>> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n>>       /*\n>>        * Hardcode gamma to 1.1 as a default for now.\n>>        *\n>> @@ -87,11 +88,11 @@ void ToneMapping::process(IPAContext &context,\n>>        */\n>>       gamma_ = 1.1;\n>>   -    if (context.frameContext.toneMapping.gamma == gamma_)\n>> +    if (frameContext.toneMapping.gamma == gamma_)\n>>           return;\n>>         struct ipu3_uapi_gamma_corr_lut &lut =\n>> -        context.frameContext.toneMapping.gammaCorrection;\n>> +        frameContext.toneMapping.gammaCorrection;\n>>         for (uint32_t i = 0; i < std::size(lut.lut); i++) {\n>>           double j = static_cast<double>(i) / (std::size(lut.lut) - 1);\n>> @@ -101,7 +102,7 @@ void ToneMapping::process(IPAContext &context,\n>>           lut.lut[i] = gamma * 8191;\n>>       }\n>>   -    context.frameContext.toneMapping.gamma = gamma_;\n>> +    frameContext.toneMapping.gamma = gamma_;\n>>   }\n>>     } /* namespace ipa::ipu3::algorithms */\n>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n>> index 86794ac1..f503b93d 100644\n>> --- a/src/ipa/ipu3/ipa_context.cpp\n>> +++ b/src/ipa/ipu3/ipa_context.cpp\n>> @@ -39,6 +39,23 @@ namespace libcamera::ipa::ipu3 {\n>>    * algorithm, but should only be written by its owner.\n>>    */\n>>   +/**\n>> + * \\brief Construct a IPAFrameContext instance\n>> + */\n>> +IPAFrameContext::IPAFrameContext() = default;\n>> +\n>> +/**\n>> + * \\brief Move constructor for IPAFrameContext\n>> + * \\param[in] other The other IPAFrameContext\n>> + */\n>> +IPAFrameContext::IPAFrameContext(IPAFrameContext &&other) = default;\n>> +\n>> +/**\n>> + * \\brief Move assignment operator for IPAFrameContext\n>> + * \\param[in] other The other IPAFrameContext\n>> + */\n>> +IPAFrameContext &IPAFrameContext::operator=(IPAFrameContext &&other) \n>> = default;\n>> +\n>>   /**\n>>    * \\struct IPAContext\n>>    * \\brief Global IPA context data shared between all algorithms\n>> @@ -46,13 +63,11 @@ namespace libcamera::ipa::ipu3 {\n>>    * \\var IPAContext::configuration\n>>    * \\brief The IPA session configuration, immutable during the session\n>>    *\n>> - * \\var IPAContext::frameContext\n>> - * \\brief The frame context for the frame being processed\n>> + * \\var IPAContext::frameContextQueue\n>> + * \\brief A queue of frame contexts to be processed by the IPA\n>>    *\n>> - * \\todo While the frame context is supposed to be per-frame, this\n>> - * single frame context stores data related to both the current frame\n>> - * and the previous frames, with fields being updated as the algorithms\n>> - * are run. This needs to be turned into real per-frame data storage.\n>> + * \\var IPAContext::prevFrameContext\n>> + * \\brief The latest frame context which the IPA has finished \n>> processing\n>>    */\n>>     /**\n>> @@ -86,6 +101,11 @@ namespace libcamera::ipa::ipu3 {\n>>    * \\brief Maximum analogue gain supported with the configured sensor\n>>    */\n>>   +/**\n>> + * \\var IPAFrameContext::frame\n>> + * \\brief Frame number of the corresponding frame context\n>> + */\n>> +\n>>   /**\n>>    * \\var IPAFrameContext::agc\n>>    * \\brief Context for the Automatic Gain Control algorithm\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index c6dc0814..a5e298bd 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -14,6 +14,8 @@\n>>     #include <libcamera/geometry.h>\n>>   +#include <queue>\n>> +\n>>   namespace libcamera {\n>>     namespace ipa::ipu3 {\n>> @@ -34,6 +36,12 @@ struct IPASessionConfiguration {\n>>   };\n>>     struct IPAFrameContext {\n>> +    uint32_t frame;\n>> +\n>> +    IPAFrameContext();\n>> +    IPAFrameContext(IPAFrameContext &&other);\n>> +    IPAFrameContext &operator=(IPAFrameContext &&other);\n>> +\n>>       struct {\n>>           uint32_t exposure;\n>>           double gain;\n>> @@ -62,7 +70,8 @@ struct IPAFrameContext {\n>>     struct IPAContext {\n>>       IPASessionConfiguration configuration;\n>> -    IPAFrameContext frameContext;\n>> +    std::queue<IPAFrameContext> frameContextQueue;\n>> +    IPAFrameContext prevFrameContext;\n>>   };\n>>     } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 3d307708..763dbd7d 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -324,6 +324,8 @@ int IPAIPU3::start()\n>>    */\n>>   void IPAIPU3::stop()\n>>   {\n>> +    while (!context_.frameContextQueue.empty())\n>> +        context_.frameContextQueue.pop();\n>>   }\n>>     /**\n>> @@ -457,6 +459,14 @@ int IPAIPU3::configure(const IPAConfigInfo \n>> &configInfo,\n>>       /* Clean context at configuration */\n>>       context_ = {};\n>>   +    /*\n>> +     * Insert a initial context into the queue to faciliate\n>> +     * algo->configure() below.\n>> +     */\n>> +    IPAFrameContext initContext;\n>> +    initContext.frame = 0;\n>> +    context_.frameContextQueue.push(std::move(initContext));\n>> +\n>>       calculateBdsGrid(configInfo.bdsOutputSize);\n>>         lineDuration_ = sensorInfo_.lineLength * 1.0s / \n>> sensorInfo_.pixelRate;\n>> @@ -547,8 +557,9 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>>           const ipu3_uapi_stats_3a *stats =\n>>               reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n>>   -        context_.frameContext.sensor.exposure = \n>> event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n>> -        context_.frameContext.sensor.gain = \n>> camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n>> +        IPAFrameContext &curFrameContext = \n>> context_.frameContextQueue.front();\n>\n> Should we test if frameContextQueue is empty ? As an assert probably \n> as it should never happen ?\n\n\nProbably, yeah.\n\n>\n>> +        curFrameContext.sensor.exposure = \n>> event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n>> +        curFrameContext.sensor.gain = \n>> camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n>>             parseStatistics(event.frame, event.frameTimestamp, stats);\n>>           break;\n>> @@ -571,6 +582,11 @@ void IPAIPU3::processControls([[maybe_unused]] \n>> unsigned int frame,\n>>                     [[maybe_unused]] const ControlList &controls)\n>>   {\n>>       /* \\todo Start processing for 'frame' based on 'controls'. */\n>> +\n>> +    IPAFrameContext newContext;\n>> +    newContext.frame = frame;\n>> +\n>> +    context_.frameContextQueue.push(std::move(newContext));\n>\n> That's why the patch proposed to mark the beginning and and of a frame \n> could make sense. You we create and push it with the frame number in \n> frameStarted and pop it in frameCompleted ?\n\n\nTheoretically makes sense.\n\nHowever, I face frame drops on nautilus here (probably is there on \nsoraka as well) so there's a chance we get frameStart() for X but then \nit can get dropped from cio2 and then frameCompleted() never occurs for X\n\nIt's not a problem per-design per say, but we can't escape the issue as \nwell for now.\n\n>\n>>   }\n>>     /**\n>> @@ -619,6 +635,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>   {\n>>       ControlList ctrls(controls::controls);\n>>   +    context_.prevFrameContext = \n>> std::move(context_.frameContextQueue.front());\n>> +    context_.frameContextQueue.pop();\n>\n> Do we want to pop it here or after the parseStatistics call ?\n\n\nNo, The algo->process() works on the .front() of the queue (which is the \nframe to be processed and algo values to be computed to setControls() \njust right below) so I need to pop it here only. If you / me / someone \ndecides to move the algo->process() out of the parseStatistics (not a \nbad idea to do it anyway!) we can pop it then after the parseStatistics \ncall.\n\n>> +\n>>       for (auto const &algo : algorithms_)\n>>           algo->process(context_, stats);\n>>   @@ -628,11 +647,11 @@ void IPAIPU3::parseStatistics(unsigned int \n>> frame,\n>>       int64_t frameDuration = (defVBlank_ + \n>> sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>();\n>>       ctrls.set(controls::FrameDuration, frameDuration);\n>>   -    ctrls.set(controls::AnalogueGain, \n>> context_.frameContext.sensor.gain);\n>> +    ctrls.set(controls::AnalogueGain, \n>> context_.prevFrameContext.sensor.gain);\n>>   -    ctrls.set(controls::ColourTemperature, \n>> context_.frameContext.awb.temperatureK);\n>> +    ctrls.set(controls::ColourTemperature, \n>> context_.prevFrameContext.awb.temperatureK);\n>>   -    ctrls.set(controls::ExposureTime, \n>> context_.frameContext.sensor.exposure * \n>> lineDuration_.get<std::micro>());\n>> +    ctrls.set(controls::ExposureTime, \n>> context_.prevFrameContext.sensor.exposure * \n>> lineDuration_.get<std::micro>());\n>>         /*\n>>        * \\todo The Metadata provides a path to getting extended data\n>> @@ -661,8 +680,9 @@ void IPAIPU3::setControls(unsigned int frame)\n>>       IPU3Action op;\n>>       op.op = ActionSetSensorControls;\n>>   -    exposure_ = context_.frameContext.agc.exposure;\n>> -    gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n>> +    IPAFrameContext &context = context_.frameContextQueue.front();\n>> +    exposure_ = context.agc.exposure;\n>> +    gain_ = camHelper_->gainCode(context.agc.gain);\n>>         ControlList ctrls(ctrls_);\n>>       ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6AD28BDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 17 Dec 2021 09:22:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1276608A2;\n\tFri, 17 Dec 2021 10:22:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B15F60223\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 17 Dec 2021 10:22:57 +0100 (CET)","from [192.168.1.103] (unknown [106.214.119.121])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1AE1DAF2;\n\tFri, 17 Dec 2021 10:22:54 +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=\"ogo+Iup3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639732976;\n\tbh=kbhE8LMv1bgTfJIvn4FBP5CmPUktqZ7R2Xn0pwzoZMk=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ogo+Iup3y0tbRJfYZdTF1XhMUy2Q//W76rFwtcIJina/5bxnnT9JmPTn18cQYNw9S\n\tRkodOu+zxEH5WitZG3raslGiKm9X43sJVbAHaKQ4XC0MimkOLuWZTohvF9Rzy+ZbxM\n\tPrYndpEYKtLEcLJro+GvNdtohnep16s+G6e2x/8s=","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211214184323.571771-1-umang.jain@ideasonboard.com>\n\t<8bf65a87-14cf-f816-2313-30a7e0ff6130@ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<28f3d251-3c01-dbc9-5fc1-6ae99230e055@ideasonboard.com>","Date":"Fri, 17 Dec 2021 14:52:40 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<8bf65a87-14cf-f816-2313-30a7e0ff6130@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Add a IPAFrameContext\n\tqueue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21821,"web_url":"https://patchwork.libcamera.org/comment/21821/","msgid":"<163974211938.2512616.4618165849137286670@Monstersaurus>","date":"2021-12-17T11:55:19","subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Add a IPAFrameContext\n\tqueue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2021-12-17 09:22:40)\n> Hi JM Thanks for the review\n> \n> On 12/17/21 2:33 PM, Jean-Michel Hautbois wrote:\n> > Hi Umang,\n> >\n> > Thanks for the patch !\n> >\n> > On 14/12/2021 19:43, Umang Jain wrote:\n> >> Having a single IPAFrameContext queue is limiting especially when\n> >> we need to preserve per-frame controls. Right now, we are not processing\n> >> any controls on the IPA side (processControls()) but sooner or later\n> >> we need to preserve the controls setting for the frames in a sequential\n> >> fashion. Since IPAIPU3::processControls() is executed on\n> >> IPU3CameraData::queuePendingRequests() code path, we need to store the\n> >> incoming control setting in a separate IPAFrameContext and push that\n> >> into the queue.\n> >>\n> >> The controls for the next frame to be processed by the IPA are retrieved\n> >> back from the queue in parseStatistics() and set via setControls().\n> >> Meanwhile, the last IPAFrameContext is preserved as prevFrameContext to\n> >> populate the metadata (for ActionMetadataReady).\n> >>\n> >\n> > In order to ease the flow reading, maybe could we pick this patch ?\n> > https://patchwork.libcamera.org/patch/14484/\n> \n> \n> OKay, I see it has one R-b tag so fine by me\n> \n> I think I should also port the IPU3Event/Actions to respective functions \n> as we have for vimc IPA interface. As reported earlier, I have a WIP \n> branch on it already. So it makes sense to complete that first and then \n> introduce the frameStart() and frameCompleted() as per your patch.\n> \n> >\n> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> >> ---\n> >> Changes in v2:\n> >> - Fix over-exposed image stream issue explained in v1.\n> >>    The issue was non-initialized data being used in the frameContext.\n> >>   ---\n> >>   src/ipa/ipu3/algorithms/agc.cpp          | 18 ++++++-------\n> >>   src/ipa/ipu3/algorithms/agc.h            |  2 +-\n> >>   src/ipa/ipu3/algorithms/awb.cpp          | 18 +++++++------\n> >>   src/ipa/ipu3/algorithms/tone_mapping.cpp | 11 ++++----\n> >>   src/ipa/ipu3/ipa_context.cpp             | 32 +++++++++++++++++-----\n> >>   src/ipa/ipu3/ipa_context.h               | 11 +++++++-\n> >>   src/ipa/ipu3/ipu3.cpp                    | 34 +++++++++++++++++++-----\n> >>   7 files changed, 89 insertions(+), 37 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp \n> >> b/src/ipa/ipu3/algorithms/agc.cpp\n> >> index 8d6f18f6..b57ca215 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> >> @@ -99,8 +99,9 @@ int Agc::configure(IPAContext &context, const \n> >> IPAConfigInfo &configInfo)\n> >>       maxAnalogueGain_ = \n> >> std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);\n> >>         /* Configure the default exposure and gain. */\n> >> -    context.frameContext.agc.gain = minAnalogueGain_;\n> >> -    context.frameContext.agc.exposure = minShutterSpeed_ / \n> >> lineDuration_;\n> >> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n> >> +    frameContext.agc.gain = minAnalogueGain_;\n> >> +    frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;\n> >>         return 0;\n> >>   }\n> >> @@ -178,12 +179,12 @@ void Agc::filterExposure()\n> >>    * \\param[in] yGain The gain calculated based on the relative \n> >> luminance target\n> >>    * \\param[in] iqMeanGain The gain calculated based on the relative \n> >> luminance target\n> >>    */\n> >> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,\n> >> -              double iqMeanGain)\n> >> +void Agc::computeExposure(IPAContext &context, double yGain, double \n> >> iqMeanGain)\n> >>   {\n> >\n> > You need to change the associated doc ;-).\n> \n> \n> ops, yeah\n> \n> >\n> >>       /* Get the effective exposure and gain applied on the sensor. */\n> >> -    uint32_t exposure = frameContext.sensor.exposure;\n> >> -    double analogueGain = frameContext.sensor.gain;\n> >> +    uint32_t exposure = context.prevFrameContext.sensor.exposure;\n> >> +    double analogueGain = context.prevFrameContext.sensor.gain;\n> >> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n> >\n> > I would move this line juste before the assignement at the end of the \n> > function ?\n> >\n> >>         /* Use the highest of the two gain estimates. */\n> >>       double evGain = std::max(yGain, iqMeanGain);\n> >> @@ -333,9 +334,8 @@ void Agc::process(IPAContext &context, const \n> >> ipu3_uapi_stats_3a *stats)\n> >>        */\n> >>       double yGain = 1.0;\n> >>       double yTarget = kRelativeLuminanceTarget;\n> >> -\n> >>       for (unsigned int i = 0; i < 8; i++) {\n> >> -        double yValue = estimateLuminance(context.frameContext,\n> >> +        double yValue = estimateLuminance(context.prevFrameContext,\n> >>                             context.configuration.grid.bdsGrid,\n> >>                             stats, yGain);\n> >>           double extraGain = std::min(10.0, yTarget / (yValue + .001));\n> >> @@ -348,7 +348,7 @@ void Agc::process(IPAContext &context, const \n> >> ipu3_uapi_stats_3a *stats)\n> >>               break;\n> >>       }\n> >>   -    computeExposure(context.frameContext, yGain, iqMeanGain);\n> >> +    computeExposure(context, yGain, iqMeanGain);\n> >>       frameCount_++;\n> >>   }\n> >>   diff --git a/src/ipa/ipu3/algorithms/agc.h \n> >> b/src/ipa/ipu3/algorithms/agc.h\n> >> index 96ec7005..1b444b12 100644\n> >> --- a/src/ipa/ipu3/algorithms/agc.h\n> >> +++ b/src/ipa/ipu3/algorithms/agc.h\n> >> @@ -34,7 +34,7 @@ private:\n> >>       double measureBrightness(const ipu3_uapi_stats_3a *stats,\n> >>                    const ipu3_uapi_grid_config &grid) const;\n> >>       void filterExposure();\n> >> -    void computeExposure(IPAFrameContext &frameContext, double yGain,\n> >> +    void computeExposure(IPAContext &context, double yGain,\n> >>                    double iqMeanGain);\n> >>       double estimateLuminance(IPAFrameContext &frameContext,\n> >>                    const ipu3_uapi_grid_config &grid,\n> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp \n> >> b/src/ipa/ipu3/algorithms/awb.cpp\n> >> index 1dc27fc9..3c004928 100644\n> >> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> >> @@ -382,16 +382,17 @@ void Awb::calculateWBGains(const \n> >> ipu3_uapi_stats_3a *stats)\n> >>   void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a \n> >> *stats)\n> >>   {\n> >>       calculateWBGains(stats);\n> >> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n> >>         /*\n> >>        * Gains are only recalculated if enough zones were detected.\n> >>        * The results are cached, so if no results were calculated, we \n> >> 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.temperatureK = asyncResults_.temperatureK;\n> >> +    frameContext.awb.gains.blue = asyncResults_.blueGain;\n> >> +    frameContext.awb.gains.green = asyncResults_.greenGain;\n> >> +    frameContext.awb.gains.red = asyncResults_.redGain;\n> >> +    frameContext.awb.temperatureK = asyncResults_.temperatureK;\n> >>   }\n> >>     constexpr uint16_t Awb::threshold(float value)\n> >> @@ -434,6 +435,7 @@ void Awb::prepare(IPAContext &context, \n> >> ipu3_uapi_params *params)\n> >>        */\n> >>       params->acc_param.bnr = imguCssBnrDefaults;\n> >>       Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;\n> >> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n> >>       params->acc_param.bnr.column_size = bdsOutputSize.width;\n> >>       params->acc_param.bnr.opt_center.x_reset = grid.x_start - \n> >> (bdsOutputSize.width / 2);\n> >>       params->acc_param.bnr.opt_center.y_reset = grid.y_start - \n> >> (bdsOutputSize.height / 2);\n> >> @@ -442,10 +444,10 @@ void Awb::prepare(IPAContext &context, \n> >> ipu3_uapi_params *params)\n> >>       params->acc_param.bnr.opt_center_sqr.y_sqr_reset = \n> >> params->acc_param.bnr.opt_center.y_reset\n> >>                               * \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 * \n> >> context.frameContext.awb.gains.green;\n> >> -    params->acc_param.bnr.wb_gains.r  = 8192 * \n> >> context.frameContext.awb.gains.red;\n> >> -    params->acc_param.bnr.wb_gains.b  = 8192 * \n> >> context.frameContext.awb.gains.blue;\n> >> -    params->acc_param.bnr.wb_gains.gb = 8192 * \n> >> context.frameContext.awb.gains.green;\n> >> +    params->acc_param.bnr.wb_gains.gr = 8192 * \n> >> frameContext.awb.gains.green;\n> >> +    params->acc_param.bnr.wb_gains.r  = 8192 * \n> >> frameContext.awb.gains.red;\n> >> +    params->acc_param.bnr.wb_gains.b  = 8192 * \n> >> frameContext.awb.gains.blue;\n> >> +    params->acc_param.bnr.wb_gains.gb = 8192 * \n> >> frameContext.awb.gains.green;\n> >>         LOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << \n> >> asyncResults_.temperatureK;\n> >>   diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp \n> >> b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> >> index 2040eda5..bf710bd1 100644\n> >> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp\n> >> @@ -42,7 +42,7 @@ int ToneMapping::configure(IPAContext &context,\n> >>                  [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >>   {\n> >>       /* Initialise tone mapping gamma value. */\n> >> -    context.frameContext.toneMapping.gamma = 0.0;\n> >> +    context.frameContextQueue.front().toneMapping.gamma = 0.0;\n> >>         return 0;\n> >>   }\n> >> @@ -60,7 +60,7 @@ void ToneMapping::prepare([[maybe_unused]] \n> >> 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.frameContextQueue.front().toneMapping.gammaCorrection.lut,\n> >>              IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n> >>              sizeof(params->acc_param.gamma.gc_lut.lut[0]));\n> >>   @@ -80,6 +80,7 @@ void ToneMapping::prepare([[maybe_unused]] \n> >> IPAContext &context,\n> >>   void ToneMapping::process(IPAContext &context,\n> >>                 [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> >>   {\n> >> +    IPAFrameContext &frameContext = context.frameContextQueue.front();\n> >>       /*\n> >>        * Hardcode gamma to 1.1 as a default for now.\n> >>        *\n> >> @@ -87,11 +88,11 @@ void ToneMapping::process(IPAContext &context,\n> >>        */\n> >>       gamma_ = 1.1;\n> >>   -    if (context.frameContext.toneMapping.gamma == gamma_)\n> >> +    if (frameContext.toneMapping.gamma == gamma_)\n> >>           return;\n> >>         struct ipu3_uapi_gamma_corr_lut &lut =\n> >> -        context.frameContext.toneMapping.gammaCorrection;\n> >> +        frameContext.toneMapping.gammaCorrection;\n> >>         for (uint32_t i = 0; i < std::size(lut.lut); i++) {\n> >>           double j = static_cast<double>(i) / (std::size(lut.lut) - 1);\n> >> @@ -101,7 +102,7 @@ void ToneMapping::process(IPAContext &context,\n> >>           lut.lut[i] = gamma * 8191;\n> >>       }\n> >>   -    context.frameContext.toneMapping.gamma = gamma_;\n> >> +    frameContext.toneMapping.gamma = gamma_;\n> >>   }\n> >>     } /* namespace ipa::ipu3::algorithms */\n> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp\n> >> index 86794ac1..f503b93d 100644\n> >> --- a/src/ipa/ipu3/ipa_context.cpp\n> >> +++ b/src/ipa/ipu3/ipa_context.cpp\n> >> @@ -39,6 +39,23 @@ namespace libcamera::ipa::ipu3 {\n> >>    * algorithm, but should only be written by its owner.\n> >>    */\n> >>   +/**\n> >> + * \\brief Construct a IPAFrameContext instance\n> >> + */\n> >> +IPAFrameContext::IPAFrameContext() = default;\n> >> +\n> >> +/**\n> >> + * \\brief Move constructor for IPAFrameContext\n> >> + * \\param[in] other The other IPAFrameContext\n> >> + */\n> >> +IPAFrameContext::IPAFrameContext(IPAFrameContext &&other) = default;\n> >> +\n> >> +/**\n> >> + * \\brief Move assignment operator for IPAFrameContext\n> >> + * \\param[in] other The other IPAFrameContext\n> >> + */\n> >> +IPAFrameContext &IPAFrameContext::operator=(IPAFrameContext &&other) \n> >> = default;\n> >> +\n> >>   /**\n> >>    * \\struct IPAContext\n> >>    * \\brief Global IPA context data shared between all algorithms\n> >> @@ -46,13 +63,11 @@ namespace libcamera::ipa::ipu3 {\n> >>    * \\var IPAContext::configuration\n> >>    * \\brief The IPA session configuration, immutable during the session\n> >>    *\n> >> - * \\var IPAContext::frameContext\n> >> - * \\brief The frame context for the frame being processed\n> >> + * \\var IPAContext::frameContextQueue\n> >> + * \\brief A queue of frame contexts to be processed by the IPA\n> >>    *\n> >> - * \\todo While the frame context is supposed to be per-frame, this\n> >> - * single frame context stores data related to both the current frame\n> >> - * and the previous frames, with fields being updated as the algorithms\n> >> - * are run. This needs to be turned into real per-frame data storage.\n> >> + * \\var IPAContext::prevFrameContext\n> >> + * \\brief The latest frame context which the IPA has finished \n> >> processing\n> >>    */\n> >>     /**\n> >> @@ -86,6 +101,11 @@ namespace libcamera::ipa::ipu3 {\n> >>    * \\brief Maximum analogue gain supported with the configured sensor\n> >>    */\n> >>   +/**\n> >> + * \\var IPAFrameContext::frame\n> >> + * \\brief Frame number of the corresponding frame context\n> >> + */\n> >> +\n> >>   /**\n> >>    * \\var IPAFrameContext::agc\n> >>    * \\brief Context for the Automatic Gain Control algorithm\n> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> >> index c6dc0814..a5e298bd 100644\n> >> --- a/src/ipa/ipu3/ipa_context.h\n> >> +++ b/src/ipa/ipu3/ipa_context.h\n> >> @@ -14,6 +14,8 @@\n> >>     #include <libcamera/geometry.h>\n> >>   +#include <queue>\n> >> +\n> >>   namespace libcamera {\n> >>     namespace ipa::ipu3 {\n> >> @@ -34,6 +36,12 @@ struct IPASessionConfiguration {\n> >>   };\n> >>     struct IPAFrameContext {\n> >> +    uint32_t frame;\n> >> +\n> >> +    IPAFrameContext();\n> >> +    IPAFrameContext(IPAFrameContext &&other);\n> >> +    IPAFrameContext &operator=(IPAFrameContext &&other);\n> >> +\n> >>       struct {\n> >>           uint32_t exposure;\n> >>           double gain;\n> >> @@ -62,7 +70,8 @@ struct IPAFrameContext {\n> >>     struct IPAContext {\n> >>       IPASessionConfiguration configuration;\n> >> -    IPAFrameContext frameContext;\n> >> +    std::queue<IPAFrameContext> frameContextQueue;\n> >> +    IPAFrameContext prevFrameContext;\n> >>   };\n> >>     } /* namespace ipa::ipu3 */\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 3d307708..763dbd7d 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -324,6 +324,8 @@ int IPAIPU3::start()\n> >>    */\n> >>   void IPAIPU3::stop()\n> >>   {\n> >> +    while (!context_.frameContextQueue.empty())\n> >> +        context_.frameContextQueue.pop();\n> >>   }\n> >>     /**\n> >> @@ -457,6 +459,14 @@ int IPAIPU3::configure(const IPAConfigInfo \n> >> &configInfo,\n> >>       /* Clean context at configuration */\n> >>       context_ = {};\n> >>   +    /*\n> >> +     * Insert a initial context into the queue to faciliate\n> >> +     * algo->configure() below.\n> >> +     */\n> >> +    IPAFrameContext initContext;\n> >> +    initContext.frame = 0;\n> >> +    context_.frameContextQueue.push(std::move(initContext));\n> >> +\n> >>       calculateBdsGrid(configInfo.bdsOutputSize);\n> >>         lineDuration_ = sensorInfo_.lineLength * 1.0s / \n> >> sensorInfo_.pixelRate;\n> >> @@ -547,8 +557,9 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> >>           const ipu3_uapi_stats_3a *stats =\n> >>               reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n> >>   -        context_.frameContext.sensor.exposure = \n> >> event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> >> -        context_.frameContext.sensor.gain = \n> >> camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n> >> +        IPAFrameContext &curFrameContext = \n> >> context_.frameContextQueue.front();\n> >\n> > Should we test if frameContextQueue is empty ? As an assert probably \n> > as it should never happen ?\n> \n> \n> Probably, yeah.\n> \n> >\n> >> +        curFrameContext.sensor.exposure = \n> >> event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();\n> >> +        curFrameContext.sensor.gain = \n> >> camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());\n> >>             parseStatistics(event.frame, event.frameTimestamp, stats);\n> >>           break;\n> >> @@ -571,6 +582,11 @@ void IPAIPU3::processControls([[maybe_unused]] \n> >> unsigned int frame,\n> >>                     [[maybe_unused]] const ControlList &controls)\n> >>   {\n> >>       /* \\todo Start processing for 'frame' based on 'controls'. */\n> >> +\n> >> +    IPAFrameContext newContext;\n> >> +    newContext.frame = frame;\n> >> +\n> >> +    context_.frameContextQueue.push(std::move(newContext));\n> >\n> > That's why the patch proposed to mark the beginning and and of a frame \n> > could make sense. You we create and push it with the frame number in \n> > frameStarted and pop it in frameCompleted ?\n> \n> \n> Theoretically makes sense.\n> \n> However, I face frame drops on nautilus here (probably is there on \n> soraka as well) so there's a chance we get frameStart() for X but then \n> it can get dropped from cio2 and then frameCompleted() never occurs for X\n> \n> It's not a problem per-design per say, but we can't escape the issue as \n> well for now.\n\nThat's precisely why we should have explicit event handler functions I\nthink.\n\nIn frameStarted() a new frameContext can be added to the queue.\nIn frameCompleted() it should handle cleaning up all outdated\nframeContexts.\n\nAnd any access to retrieve a frameContext should ensure that it has got\nthe right one, or that the API to get a frameContext (a helper) gets the\ncorrect one that matches the frame ID explictily. Don't just pop off the\ntop of the queue.\n\n\n/**\n * \\frame The frame number of the frame that has now completed.\n */\nframeCompleted(unsigned int frame)\n{\n\n\twhile (fc = context_.frameContextQueue.front())\n\t{\n\t\tif (fc.frameNumber =< frame) {\n\t\t\t/* drop this old frame now */\n\t\t}\n\n\t\t/* Keep newer frames */\n\t\tif (fc.frameNumber > frame)\n\t\t\tbreak;\n\t}\n\n}\n\nand any time you want to access/assign a frame context you should have a\nhelper which ensures you get the correct one from the queue, that\nmatches the frame id being processed. This could even be handled with a\nmap perhaps, or just walking the queue to make sure you only return one\nwith the correct frame index.\n\n\n> >\n> >>   }\n> >>     /**\n> >> @@ -619,6 +635,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> >>   {\n> >>       ControlList ctrls(controls::controls);\n> >>   +    context_.prevFrameContext = \n> >> std::move(context_.frameContextQueue.front());\n> >> +    context_.frameContextQueue.pop();\n> >\n> > Do we want to pop it here or after the parseStatistics call ?\n> \n> \n> No, The algo->process() works on the .front() of the queue (which is the \n> frame to be processed and algo values to be computed to setControls() \n> just right below) so I need to pop it here only. If you / me / someone \n> decides to move the algo->process() out of the parseStatistics (not a \n> bad idea to do it anyway!) we can pop it then after the parseStatistics \n> call.\n\nWe shouldn't make assumptions then on which one we pop off. Lets try to\nbe explicit, and make a helper around it to be sure, as above.\n\n\n> >> +\n> >>       for (auto const &algo : algorithms_)\n> >>           algo->process(context_, stats);\n> >>   @@ -628,11 +647,11 @@ void IPAIPU3::parseStatistics(unsigned int \n> >> frame,\n> >>       int64_t frameDuration = (defVBlank_ + \n> >> sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>();\n> >>       ctrls.set(controls::FrameDuration, frameDuration);\n> >>   -    ctrls.set(controls::AnalogueGain, \n> >> context_.frameContext.sensor.gain);\n> >> +    ctrls.set(controls::AnalogueGain, \n> >> context_.prevFrameContext.sensor.gain);\n> >>   -    ctrls.set(controls::ColourTemperature, \n> >> context_.frameContext.awb.temperatureK);\n> >> +    ctrls.set(controls::ColourTemperature, \n> >> context_.prevFrameContext.awb.temperatureK);\n> >>   -    ctrls.set(controls::ExposureTime, \n> >> context_.frameContext.sensor.exposure * \n> >> lineDuration_.get<std::micro>());\n> >> +    ctrls.set(controls::ExposureTime, \n> >> context_.prevFrameContext.sensor.exposure * \n> >> lineDuration_.get<std::micro>());\n> >>         /*\n> >>        * \\todo The Metadata provides a path to getting extended data\n> >> @@ -661,8 +680,9 @@ void IPAIPU3::setControls(unsigned int frame)\n> >>       IPU3Action op;\n> >>       op.op = ActionSetSensorControls;\n> >>   -    exposure_ = context_.frameContext.agc.exposure;\n> >> -    gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n> >> +    IPAFrameContext &context = context_.frameContextQueue.front();\n> >> +    exposure_ = context.agc.exposure;\n> >> +    gain_ = camHelper_->gainCode(context.agc.gain);\n> >>         ControlList ctrls(ctrls_);\n> >>       ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> >>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 1EFA2BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 17 Dec 2021 11:55:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3566D608A5;\n\tFri, 17 Dec 2021 12:55:23 +0100 (CET)","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 2A74B60114\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 17 Dec 2021 12:55:22 +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 8EF9392A;\n\tFri, 17 Dec 2021 12:55:21 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aAegrR/C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639742121;\n\tbh=lRLSnjmN7uYb/+HXINNaNoFyPyUGrKHqB8JDXhWvNho=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=aAegrR/CC5pcwVXyzayqEaYKfEylU3NRtcJmK2RnZm9T96fU2TmCvizANzRLbNKFA\n\tm3XdvCtDG1v1gQC3XRDXX1dck7H6ojufZosJqdl5IXHVGlAMOWaZnHKFX9d7GQKw0X\n\txeIih0gayfnOXS8kCg/QS9ik9rPS3Rzs2w16i0AQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<28f3d251-3c01-dbc9-5fc1-6ae99230e055@ideasonboard.com>","References":"<20211214184323.571771-1-umang.jain@ideasonboard.com>\n\t<8bf65a87-14cf-f816-2313-30a7e0ff6130@ideasonboard.com>\n\t<28f3d251-3c01-dbc9-5fc1-6ae99230e055@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tUmang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 17 Dec 2021 11:55:19 +0000","Message-ID":"<163974211938.2512616.4618165849137286670@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] ipa: ipu3: Add a IPAFrameContext\n\tqueue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]