[libcamera-devel,v3,3/3] ipa: ipu3: Put IPAFrameContext(s) in a ring buffer
diff mbox series

Message ID 20220517191833.333122-4-umang.jain@ideasonboard.com
State Accepted
Headers show
Series
  • IPAFrameContext Ring buffer
Related show

Commit Message

Umang Jain May 17, 2022, 7:18 p.m. UTC 
Instead of having one frame context constantly being updated,
this patch aims to introduce per-frame IPAFrameContext which
are stored in a ring buffer. Whenever a request is queued, a new
IPAFrameContext is created and inserted into the ring buffer.

The IPAFrameContext structure itself has been slightly extended
to store a frame id and a ControlList for incoming frame
controls (sent in by the application). The next step would be to
read and set these controls whenever the request is actually queued
to the hardware.

Since now we are working in multiples of IPAFrameContext, the
Algorithm::process() will actually take in a IPAFrameContext pointer
(as opposed to a nullptr while preparing for this change).

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
 src/ipa/ipu3/algorithms/agc.h   |  4 ++--
 src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
 src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
 src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
 5 files changed, 55 insertions(+), 20 deletions(-)

Comments

Jacopo Mondi May 18, 2022, 7:47 a.m. UTC | #1 
Hi Umang,

On Tue, May 17, 2022 at 09:18:33PM +0200, Umang Jain via libcamera-devel wrote:
> Instead of having one frame context constantly being updated,
> this patch aims to introduce per-frame IPAFrameContext which
> are stored in a ring buffer. Whenever a request is queued, a new
> IPAFrameContext is created and inserted into the ring buffer.
>
> The IPAFrameContext structure itself has been slightly extended
> to store a frame id and a ControlList for incoming frame
> controls (sent in by the application). The next step would be to
> read and set these controls whenever the request is actually queued
> to the hardware.
>
> Since now we are working in multiples of IPAFrameContext, the
> Algorithm::process() will actually take in a IPAFrameContext pointer
> (as opposed to a nullptr while preparing for this change).
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>  src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>  src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
>  src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
>  src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
>  5 files changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 383a8deb..f16be534 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
>   * \param[in] yGain The gain calculated based on the relative luminance target
>   * \param[in] iqMeanGain The gain calculated based on the relative luminance target
>   */
> -void Agc::computeExposure(IPAContext &context, double yGain,
> -			  double iqMeanGain)
> +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> +			  double yGain, double iqMeanGain)
>  {
>  	const IPASessionConfiguration &configuration = context.configuration;
> -	IPAFrameContext &frameContext = context.frameContext;
>  	/* Get the effective exposure and gain applied on the sensor. */
> -	uint32_t exposure = frameContext.sensor.exposure;
> -	double analogueGain = frameContext.sensor.gain;
> +	uint32_t exposure = frameContext->sensor.exposure;
> +	double analogueGain = frameContext->sensor.gain;
>
>  	/* Use the highest of the two gain estimates. */
>  	double evGain = std::max(yGain, iqMeanGain);
> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo
>  			break;
>  	}
>
> -	computeExposure(context, yGain, iqMeanGain);
> +	computeExposure(context, frameContext, yGain, iqMeanGain);
>  	frameCount_++;
>  }
>
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 219a1a96..105ae0f2 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -35,8 +35,8 @@ private:
>  	double measureBrightness(const ipu3_uapi_stats_3a *stats,
>  				 const ipu3_uapi_grid_config &grid) const;
>  	utils::Duration filterExposure(utils::Duration currentExposure);
> -	void computeExposure(IPAContext &context, double yGain,
> -			     double iqMeanGain);
> +	void computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> +			     double yGain, double iqMeanGain);
>  	double estimateLuminance(IPAActiveState &activeState,
>  				 const ipu3_uapi_grid_config &grid,
>  				 const ipu3_uapi_stats_3a *stats,
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 06eb2776..81b30600 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>   * \var IPAContext::configuration
>   * \brief The IPA session configuration, immutable during the session
>   *
> - * \var IPAContext::frameContext
> - * \brief The frame context for the frame being processed
> + * \var IPAContext::frameContexts
> + * \brief Ring buffer of the IPAFrameContext(s)
>   *
>   * \var IPAContext::activeState
>   * \brief The current state of IPA algorithms
> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
>   */
>
>  /**
> + * \brief Default constructor for IPAFrameContext
> + */
> +IPAFrameContext::IPAFrameContext() = default;
> +
> +/**
> + * \brief Construct a IPAFrameContext instance
> + */
> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> +	: frame(id), frameControls(reqControls)
> +{
> +	sensor = {};
> +}
> +
> +/**
> + * \var IPAFrameContext::frame
> + * \brief The frame number
> + *
> + * \var IPAFrameContext::frameControls
> + * \brief Controls sent in by the application while queuing the request
> + *
>   * \var IPAFrameContext::sensor
>   * \brief Effective sensor values that were applied for the frame
>   *
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 8d681131..42e11141 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -8,16 +8,22 @@
>
>  #pragma once
>
> +#include <array>
> +
>  #include <linux/intel-ipu3.h>
>
>  #include <libcamera/base/utils.h>
>
> +#include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
>
>  namespace libcamera {
>
>  namespace ipa::ipu3 {
>
> +/* Maximum number of frame contexts to be held */
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
>  struct IPASessionConfiguration {
>  	struct {
>  		ipu3_uapi_grid_config bdsGrid;
> @@ -71,17 +77,23 @@ struct IPAActiveState {
>  };
>
>  struct IPAFrameContext {
> +	IPAFrameContext();
> +	IPAFrameContext(uint32_t id, const ControlList &reqControls);
> +
>  	struct {
>  		uint32_t exposure;
>  		double gain;
>  	} sensor;
> +
> +	uint32_t frame;
> +	ControlList frameControls;
>  };
>
>  struct IPAContext {
>  	IPASessionConfiguration configuration;
>  	IPAActiveState activeState;
>
> -	IPAFrameContext frameContext;
> +	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>  };
>
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 16e5028f..4322f96b 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
>  	}
>
>  	/* Clean context */
> -	context_ = {};
> +	context_.configuration = {};
>  	context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>
>  	/* Construct our Algorithms */
> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>
>  	/* Clean IPAActiveState at each reconfiguration. */
>  	context_.activeState = {};
> -	context_.frameContext = {};
> +	IPAFrameContext initFrameContext;
> +	context_.frameContexts.fill(initFrameContext);
>
>  	if (!validateSensorControls()) {
>  		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  	const ipu3_uapi_stats_3a *stats =
>  		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>
> -	context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> +
> +	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>
>  	double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>  	int32_t vBlank = context_.configuration.sensor.defVBlank;
>  	ControlList ctrls(controls::controls);
>
>  	for (auto const &algo : algorithms_)
> -		algo->process(context_, nullptr, stats);
> +		algo->process(context_, &frameContext, stats);
>
>  	setControls(frame);
>
> @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  	int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
>  	ctrls.set(controls::FrameDuration, frameDuration);
>
> -	ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> +	ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>
>  	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>
> -	ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> +	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>
>  	/*
>  	 * \todo The Metadata provides a path to getting extended data
> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>   * Parse the request to handle any IPA-managed controls that were set from the
>   * application such as manual sensor settings.
>   */
> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> -			   [[maybe_unused]] const ControlList &controls)
> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>  {
>  	/* \todo Start processing for 'frame' based on 'controls'. */
> +	IPAFrameContext newFrameContext(frame, controls);
> +	context_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;

Just a little note, which now is not that relevant as we have a
limited number of controls, but this goes through at least 2 copies a
the 'controls' list.

Constructor:

IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
	: frame(id), frameControls(reqControls)

And the implicitely defined copy assignment operator.

I wonder if
	context_.frameContexts[frame % kMaxFrameContexts] = {frame, controls};

would spare a copy

The rest looks good to me
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j


>  }
>
>  /**
> --
> 2.31.0
>
Jean-Michel Hautbois May 18, 2022, 9:08 a.m. UTC | #2 
Hi Umang,

On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:
> Instead of having one frame context constantly being updated,
> this patch aims to introduce per-frame IPAFrameContext which
> are stored in a ring buffer. Whenever a request is queued, a new
> IPAFrameContext is created and inserted into the ring buffer.
> 
> The IPAFrameContext structure itself has been slightly extended
> to store a frame id and a ControlList for incoming frame
> controls (sent in by the application). The next step would be to
> read and set these controls whenever the request is actually queued
> to the hardware.
> 
> Since now we are working in multiples of IPAFrameContext, the
> Algorithm::process() will actually take in a IPAFrameContext pointer
> (as opposed to a nullptr while preparing for this change).
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
>   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
>   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
>   5 files changed, 55 insertions(+), 20 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 383a8deb..f16be534 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
>    * \param[in] yGain The gain calculated based on the relative luminance target
>    * \param[in] iqMeanGain The gain calculated based on the relative luminance target
>    */
> -void Agc::computeExposure(IPAContext &context, double yGain,
> -			  double iqMeanGain)
> +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> +			  double yGain, double iqMeanGain)
>   {
>   	const IPASessionConfiguration &configuration = context.configuration;
> -	IPAFrameContext &frameContext = context.frameContext;
>   	/* Get the effective exposure and gain applied on the sensor. */
> -	uint32_t exposure = frameContext.sensor.exposure;
> -	double analogueGain = frameContext.sensor.gain;
> +	uint32_t exposure = frameContext->sensor.exposure;
> +	double analogueGain = frameContext->sensor.gain;
>   
>   	/* Use the highest of the two gain estimates. */
>   	double evGain = std::max(yGain, iqMeanGain);
> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo
>   			break;
>   	}
>   
> -	computeExposure(context, yGain, iqMeanGain);
> +	computeExposure(context, frameContext, yGain, iqMeanGain);
>   	frameCount_++;
>   }
>   
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 219a1a96..105ae0f2 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -35,8 +35,8 @@ private:
>   	double measureBrightness(const ipu3_uapi_stats_3a *stats,
>   				 const ipu3_uapi_grid_config &grid) const;
>   	utils::Duration filterExposure(utils::Duration currentExposure);
> -	void computeExposure(IPAContext &context, double yGain,
> -			     double iqMeanGain);
> +	void computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> +			     double yGain, double iqMeanGain);
>   	double estimateLuminance(IPAActiveState &activeState,
>   				 const ipu3_uapi_grid_config &grid,
>   				 const ipu3_uapi_stats_3a *stats,
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 06eb2776..81b30600 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>    * \var IPAContext::configuration
>    * \brief The IPA session configuration, immutable during the session
>    *
> - * \var IPAContext::frameContext
> - * \brief The frame context for the frame being processed
> + * \var IPAContext::frameContexts
> + * \brief Ring buffer of the IPAFrameContext(s)
>    *
>    * \var IPAContext::activeState
>    * \brief The current state of IPA algorithms
> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
>    */
>   
>   /**
> + * \brief Default constructor for IPAFrameContext
> + */
> +IPAFrameContext::IPAFrameContext() = default;
> +
> +/**
> + * \brief Construct a IPAFrameContext instance
> + */
> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> +	: frame(id), frameControls(reqControls)
> +{
> +	sensor = {};
> +}
> +
> +/**
> + * \var IPAFrameContext::frame
> + * \brief The frame number
> + *
> + * \var IPAFrameContext::frameControls
> + * \brief Controls sent in by the application while queuing the request
> + *
>    * \var IPAFrameContext::sensor
>    * \brief Effective sensor values that were applied for the frame
>    *
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 8d681131..42e11141 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -8,16 +8,22 @@
>   
>   #pragma once
>   
> +#include <array>
> +
>   #include <linux/intel-ipu3.h>
>   
>   #include <libcamera/base/utils.h>
>   
> +#include <libcamera/controls.h>
>   #include <libcamera/geometry.h>
>   
>   namespace libcamera {
>   
>   namespace ipa::ipu3 {
>   
> +/* Maximum number of frame contexts to be held */
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
>   struct IPASessionConfiguration {
>   	struct {
>   		ipu3_uapi_grid_config bdsGrid;
> @@ -71,17 +77,23 @@ struct IPAActiveState {
>   };
>   
>   struct IPAFrameContext {
> +	IPAFrameContext();
> +	IPAFrameContext(uint32_t id, const ControlList &reqControls);
> +
>   	struct {
>   		uint32_t exposure;
>   		double gain;
>   	} sensor;
> +
> +	uint32_t frame;
> +	ControlList frameControls;
>   };
>   
>   struct IPAContext {
>   	IPASessionConfiguration configuration;
>   	IPAActiveState activeState;
>   
> -	IPAFrameContext frameContext;
> +	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>   };
>   
>   } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 16e5028f..4322f96b 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
>   	}
>   
>   	/* Clean context */
> -	context_ = {};
> +	context_.configuration = {};
>   	context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>   
>   	/* Construct our Algorithms */
> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   
>   	/* Clean IPAActiveState at each reconfiguration. */
>   	context_.activeState = {};
> -	context_.frameContext = {};
> +	IPAFrameContext initFrameContext;
> +	context_.frameContexts.fill(initFrameContext);
>   
>   	if (!validateSensorControls()) {
>   		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>   	const ipu3_uapi_stats_3a *stats =
>   		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>   
> -	context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -	context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];

Shouldn't the frame be "double-checked", verifying that 
(frameContext->frame == frame) before passing it to the algorithms somehow ?

Apart from that:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> +
> +	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>   
>   	double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>   	int32_t vBlank = context_.configuration.sensor.defVBlank;
>   	ControlList ctrls(controls::controls);
>   
>   	for (auto const &algo : algorithms_)
> -		algo->process(context_, nullptr, stats);
> +		algo->process(context_, &frameContext, stats);
>   
>   	setControls(frame);
>   
> @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>   	int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
>   	ctrls.set(controls::FrameDuration, frameDuration);
>   
> -	ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> +	ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>   
>   	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>   
> -	ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> +	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>   
>   	/*
>   	 * \todo The Metadata provides a path to getting extended data
> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>    * Parse the request to handle any IPA-managed controls that were set from the
>    * application such as manual sensor settings.
>    */
> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> -			   [[maybe_unused]] const ControlList &controls)
> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>   {
>   	/* \todo Start processing for 'frame' based on 'controls'. */
> +	IPAFrameContext newFrameContext(frame, controls);
> +	context_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;
>   }
>   
>   /**
Umang Jain May 18, 2022, 9:39 a.m. UTC | #3 
Hi JM,

On 5/18/22 11:08, Jean-Michel Hautbois wrote:
> Hi Umang,
>
> On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:
>> Instead of having one frame context constantly being updated,
>> this patch aims to introduce per-frame IPAFrameContext which
>> are stored in a ring buffer. Whenever a request is queued, a new
>> IPAFrameContext is created and inserted into the ring buffer.
>>
>> The IPAFrameContext structure itself has been slightly extended
>> to store a frame id and a ControlList for incoming frame
>> controls (sent in by the application). The next step would be to
>> read and set these controls whenever the request is actually queued
>> to the hardware.
>>
>> Since now we are working in multiples of IPAFrameContext, the
>> Algorithm::process() will actually take in a IPAFrameContext pointer
>> (as opposed to a nullptr while preparing for this change).
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>>   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
>>   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
>>   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
>>   5 files changed, 55 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp 
>> b/src/ipa/ipu3/algorithms/agc.cpp
>> index 383a8deb..f16be534 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -183,14 +183,13 @@ utils::Duration 
>> Agc::filterExposure(utils::Duration exposureValue)
>>    * \param[in] yGain The gain calculated based on the relative 
>> luminance target
>>    * \param[in] iqMeanGain The gain calculated based on the relative 
>> luminance target
>>    */
>> -void Agc::computeExposure(IPAContext &context, double yGain,
>> -              double iqMeanGain)
>> +void Agc::computeExposure(IPAContext &context, IPAFrameContext 
>> *frameContext,
>> +              double yGain, double iqMeanGain)
>>   {
>>       const IPASessionConfiguration &configuration = 
>> context.configuration;
>> -    IPAFrameContext &frameContext = context.frameContext;
>>       /* Get the effective exposure and gain applied on the sensor. */
>> -    uint32_t exposure = frameContext.sensor.exposure;
>> -    double analogueGain = frameContext.sensor.gain;
>> +    uint32_t exposure = frameContext->sensor.exposure;
>> +    double analogueGain = frameContext->sensor.gain;
>>         /* Use the highest of the two gain estimates. */
>>       double evGain = std::max(yGain, iqMeanGain);
>> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, 
>> [[maybe_unused]] IPAFrameContext *frameCo
>>               break;
>>       }
>>   -    computeExposure(context, yGain, iqMeanGain);
>> +    computeExposure(context, frameContext, yGain, iqMeanGain);
>>       frameCount_++;
>>   }
>>   diff --git a/src/ipa/ipu3/algorithms/agc.h 
>> b/src/ipa/ipu3/algorithms/agc.h
>> index 219a1a96..105ae0f2 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -35,8 +35,8 @@ private:
>>       double measureBrightness(const ipu3_uapi_stats_3a *stats,
>>                    const ipu3_uapi_grid_config &grid) const;
>>       utils::Duration filterExposure(utils::Duration currentExposure);
>> -    void computeExposure(IPAContext &context, double yGain,
>> -                 double iqMeanGain);
>> +    void computeExposure(IPAContext &context, IPAFrameContext 
>> *frameContext,
>> +                 double yGain, double iqMeanGain);
>>       double estimateLuminance(IPAActiveState &activeState,
>>                    const ipu3_uapi_grid_config &grid,
>>                    const ipu3_uapi_stats_3a *stats,
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 06eb2776..81b30600 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>>    * \var IPAContext::configuration
>>    * \brief The IPA session configuration, immutable during the session
>>    *
>> - * \var IPAContext::frameContext
>> - * \brief The frame context for the frame being processed
>> + * \var IPAContext::frameContexts
>> + * \brief Ring buffer of the IPAFrameContext(s)
>>    *
>>    * \var IPAContext::activeState
>>    * \brief The current state of IPA algorithms
>> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
>>    */
>>     /**
>> + * \brief Default constructor for IPAFrameContext
>> + */
>> +IPAFrameContext::IPAFrameContext() = default;
>> +
>> +/**
>> + * \brief Construct a IPAFrameContext instance
>> + */
>> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList 
>> &reqControls)
>> +    : frame(id), frameControls(reqControls)
>> +{
>> +    sensor = {};
>> +}
>> +
>> +/**
>> + * \var IPAFrameContext::frame
>> + * \brief The frame number
>> + *
>> + * \var IPAFrameContext::frameControls
>> + * \brief Controls sent in by the application while queuing the request
>> + *
>>    * \var IPAFrameContext::sensor
>>    * \brief Effective sensor values that were applied for the frame
>>    *
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 8d681131..42e11141 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -8,16 +8,22 @@
>>     #pragma once
>>   +#include <array>
>> +
>>   #include <linux/intel-ipu3.h>
>>     #include <libcamera/base/utils.h>
>>   +#include <libcamera/controls.h>
>>   #include <libcamera/geometry.h>
>>     namespace libcamera {
>>     namespace ipa::ipu3 {
>>   +/* Maximum number of frame contexts to be held */
>> +static constexpr uint32_t kMaxFrameContexts = 16;
>> +
>>   struct IPASessionConfiguration {
>>       struct {
>>           ipu3_uapi_grid_config bdsGrid;
>> @@ -71,17 +77,23 @@ struct IPAActiveState {
>>   };
>>     struct IPAFrameContext {
>> +    IPAFrameContext();
>> +    IPAFrameContext(uint32_t id, const ControlList &reqControls);
>> +
>>       struct {
>>           uint32_t exposure;
>>           double gain;
>>       } sensor;
>> +
>> +    uint32_t frame;
>> +    ControlList frameControls;
>>   };
>>     struct IPAContext {
>>       IPASessionConfiguration configuration;
>>       IPAActiveState activeState;
>>   -    IPAFrameContext frameContext;
>> +    std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>>   };
>>     } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 16e5028f..4322f96b 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
>>       }
>>         /* Clean context */
>> -    context_ = {};
>> +    context_.configuration = {};
>>       context_.configuration.sensor.lineDuration = 
>> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>>         /* Construct our Algorithms */
>> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo 
>> &configInfo,
>>         /* Clean IPAActiveState at each reconfiguration. */
>>       context_.activeState = {};
>> -    context_.frameContext = {};
>> +    IPAFrameContext initFrameContext;
>> +    context_.frameContexts.fill(initFrameContext);
>>         if (!validateSensorControls()) {
>>           LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t 
>> frame,
>>       const ipu3_uapi_stats_3a *stats =
>>           reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>>   -    context_.frameContext.sensor.exposure = 
>> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -    context_.frameContext.sensor.gain = 
>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +    IPAFrameContext &frameContext = context_.frameContexts[frame % 
>> kMaxFrameContexts];
>
> Shouldn't the frame be "double-checked", verifying that 
> (frameContext->frame == frame) before passing it to the algorithms 
> somehow ?


Well, we can surely verify that but we know in advance that the 
verification is subjected to failure with known cases (for e.g. frame 
drop) so I am not very keen putting the verification in. If you had seen 
the v2, I had a ASSERT similar to this basically (and has some 
discussion on this there)

>
> Apart from that:
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>


Thanks for reviewing!

>
>> +
>> +    frameContext.sensor.exposure = 
>> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +    frameContext.sensor.gain = 
>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>>         double lineDuration = 
>> context_.configuration.sensor.lineDuration.get<std::micro>();
>>       int32_t vBlank = context_.configuration.sensor.defVBlank;
>>       ControlList ctrls(controls::controls);
>>         for (auto const &algo : algorithms_)
>> -        algo->process(context_, nullptr, stats);
>> +        algo->process(context_, &frameContext, stats);
>>         setControls(frame);
>>   @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const 
>> uint32_t frame,
>>       int64_t frameDuration = (vBlank + 
>> sensorInfo_.outputSize.height) * lineDuration;
>>       ctrls.set(controls::FrameDuration, frameDuration);
>>   -    ctrls.set(controls::AnalogueGain, 
>> context_.frameContext.sensor.gain);
>> +    ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>>         ctrls.set(controls::ColourTemperature, 
>> context_.activeState.awb.temperatureK);
>>   -    ctrls.set(controls::ExposureTime, 
>> context_.frameContext.sensor.exposure * lineDuration);
>> +    ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * 
>> lineDuration);
>>         /*
>>        * \todo The Metadata provides a path to getting extended data
>> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t 
>> frame,
>>    * Parse the request to handle any IPA-managed controls that were 
>> set from the
>>    * application such as manual sensor settings.
>>    */
>> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
>> -               [[maybe_unused]] const ControlList &controls)
>> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList 
>> &controls)
>>   {
>>       /* \todo Start processing for 'frame' based on 'controls'. */
>> +    IPAFrameContext newFrameContext(frame, controls);
>> +    context_.frameContexts[frame % kMaxFrameContexts] = 
>> newFrameContext;
>>   }
>>     /**
Umang Jain May 18, 2022, 10:34 a.m. UTC | #4 
Hi Jacopo,

Thanks for reviewing.

On 5/18/22 09:47, Jacopo Mondi wrote:
> Hi Umang,
>
> On Tue, May 17, 2022 at 09:18:33PM +0200, Umang Jain via libcamera-devel wrote:
>> Instead of having one frame context constantly being updated,
>> this patch aims to introduce per-frame IPAFrameContext which
>> are stored in a ring buffer. Whenever a request is queued, a new
>> IPAFrameContext is created and inserted into the ring buffer.
>>
>> The IPAFrameContext structure itself has been slightly extended
>> to store a frame id and a ControlList for incoming frame
>> controls (sent in by the application). The next step would be to
>> read and set these controls whenever the request is actually queued
>> to the hardware.
>>
>> Since now we are working in multiples of IPAFrameContext, the
>> Algorithm::process() will actually take in a IPAFrameContext pointer
>> (as opposed to a nullptr while preparing for this change).
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>>   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
>>   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
>>   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
>>   5 files changed, 55 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 383a8deb..f16be534 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
>>    * \param[in] yGain The gain calculated based on the relative luminance target
>>    * \param[in] iqMeanGain The gain calculated based on the relative luminance target
>>    */
>> -void Agc::computeExposure(IPAContext &context, double yGain,
>> -			  double iqMeanGain)
>> +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,
>> +			  double yGain, double iqMeanGain)
>>   {
>>   	const IPASessionConfiguration &configuration = context.configuration;
>> -	IPAFrameContext &frameContext = context.frameContext;
>>   	/* Get the effective exposure and gain applied on the sensor. */
>> -	uint32_t exposure = frameContext.sensor.exposure;
>> -	double analogueGain = frameContext.sensor.gain;
>> +	uint32_t exposure = frameContext->sensor.exposure;
>> +	double analogueGain = frameContext->sensor.gain;
>>
>>   	/* Use the highest of the two gain estimates. */
>>   	double evGain = std::max(yGain, iqMeanGain);
>> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo
>>   			break;
>>   	}
>>
>> -	computeExposure(context, yGain, iqMeanGain);
>> +	computeExposure(context, frameContext, yGain, iqMeanGain);
>>   	frameCount_++;
>>   }
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
>> index 219a1a96..105ae0f2 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -35,8 +35,8 @@ private:
>>   	double measureBrightness(const ipu3_uapi_stats_3a *stats,
>>   				 const ipu3_uapi_grid_config &grid) const;
>>   	utils::Duration filterExposure(utils::Duration currentExposure);
>> -	void computeExposure(IPAContext &context, double yGain,
>> -			     double iqMeanGain);
>> +	void computeExposure(IPAContext &context, IPAFrameContext *frameContext,
>> +			     double yGain, double iqMeanGain);
>>   	double estimateLuminance(IPAActiveState &activeState,
>>   				 const ipu3_uapi_grid_config &grid,
>>   				 const ipu3_uapi_stats_3a *stats,
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 06eb2776..81b30600 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>>    * \var IPAContext::configuration
>>    * \brief The IPA session configuration, immutable during the session
>>    *
>> - * \var IPAContext::frameContext
>> - * \brief The frame context for the frame being processed
>> + * \var IPAContext::frameContexts
>> + * \brief Ring buffer of the IPAFrameContext(s)
>>    *
>>    * \var IPAContext::activeState
>>    * \brief The current state of IPA algorithms
>> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
>>    */
>>
>>   /**
>> + * \brief Default constructor for IPAFrameContext
>> + */
>> +IPAFrameContext::IPAFrameContext() = default;
>> +
>> +/**
>> + * \brief Construct a IPAFrameContext instance
>> + */
>> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>> +	: frame(id), frameControls(reqControls)
>> +{
>> +	sensor = {};
>> +}
>> +
>> +/**
>> + * \var IPAFrameContext::frame
>> + * \brief The frame number
>> + *
>> + * \var IPAFrameContext::frameControls
>> + * \brief Controls sent in by the application while queuing the request
>> + *
>>    * \var IPAFrameContext::sensor
>>    * \brief Effective sensor values that were applied for the frame
>>    *
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 8d681131..42e11141 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -8,16 +8,22 @@
>>
>>   #pragma once
>>
>> +#include <array>
>> +
>>   #include <linux/intel-ipu3.h>
>>
>>   #include <libcamera/base/utils.h>
>>
>> +#include <libcamera/controls.h>
>>   #include <libcamera/geometry.h>
>>
>>   namespace libcamera {
>>
>>   namespace ipa::ipu3 {
>>
>> +/* Maximum number of frame contexts to be held */
>> +static constexpr uint32_t kMaxFrameContexts = 16;
>> +
>>   struct IPASessionConfiguration {
>>   	struct {
>>   		ipu3_uapi_grid_config bdsGrid;
>> @@ -71,17 +77,23 @@ struct IPAActiveState {
>>   };
>>
>>   struct IPAFrameContext {
>> +	IPAFrameContext();
>> +	IPAFrameContext(uint32_t id, const ControlList &reqControls);
>> +
>>   	struct {
>>   		uint32_t exposure;
>>   		double gain;
>>   	} sensor;
>> +
>> +	uint32_t frame;
>> +	ControlList frameControls;
>>   };
>>
>>   struct IPAContext {
>>   	IPASessionConfiguration configuration;
>>   	IPAActiveState activeState;
>>
>> -	IPAFrameContext frameContext;
>> +	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>>   };
>>
>>   } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 16e5028f..4322f96b 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
>>   	}
>>
>>   	/* Clean context */
>> -	context_ = {};
>> +	context_.configuration = {};
>>   	context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>>
>>   	/* Construct our Algorithms */
>> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>
>>   	/* Clean IPAActiveState at each reconfiguration. */
>>   	context_.activeState = {};
>> -	context_.frameContext = {};
>> +	IPAFrameContext initFrameContext;
>> +	context_.frameContexts.fill(initFrameContext);
>>
>>   	if (!validateSensorControls()) {
>>   		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>   	const ipu3_uapi_stats_3a *stats =
>>   		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>>
>> -	context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -	context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
>> +
>> +	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>>
>>   	double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
>>   	int32_t vBlank = context_.configuration.sensor.defVBlank;
>>   	ControlList ctrls(controls::controls);
>>
>>   	for (auto const &algo : algorithms_)
>> -		algo->process(context_, nullptr, stats);
>> +		algo->process(context_, &frameContext, stats);
>>
>>   	setControls(frame);
>>
>> @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>   	int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
>>   	ctrls.set(controls::FrameDuration, frameDuration);
>>
>> -	ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
>> +	ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>>
>>   	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>>
>> -	ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
>> +	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
>>
>>   	/*
>>   	 * \todo The Metadata provides a path to getting extended data
>> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>    * Parse the request to handle any IPA-managed controls that were set from the
>>    * application such as manual sensor settings.
>>    */
>> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
>> -			   [[maybe_unused]] const ControlList &controls)
>> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>>   {
>>   	/* \todo Start processing for 'frame' based on 'controls'. */
>> +	IPAFrameContext newFrameContext(frame, controls);
>> +	context_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;
> Just a little note, which now is not that relevant as we have a
> limited number of controls, but this goes through at least 2 copies a
> the 'controls' list.
>
> Constructor:
>
> IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> 	: frame(id), frameControls(reqControls)
>
> And the implicitely defined copy assignment operator.
>
> I wonder if
> 	context_.frameContexts[frame % kMaxFrameContexts] = {frame, controls};
>
> would spare a copy


I am also wondering. I think your version is still doing two copies as 
per my understanding.

One would be through the constructor as you said above.

The other one would be while assigning `context_.frameContexts[frame % 
kMaxFrameContexts]` as operator= would do a member-wise copy, does my 
inference make sense?

But nevertheless,
Your version is a more clear to read hence I would keep this anyway.

>
> The rest looks good to me
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>     j
>
>
>>   }
>>
>>   /**
>> --
>> 2.31.0
>>
Kieran Bingham May 18, 2022, 10:44 a.m. UTC | #5 
Quoting Umang Jain via libcamera-devel (2022-05-18 10:39:11)
> Hi JM,
> 
> On 5/18/22 11:08, Jean-Michel Hautbois wrote:
> > Hi Umang,
> >
> > On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:
> >> Instead of having one frame context constantly being updated,
> >> this patch aims to introduce per-frame IPAFrameContext which
> >> are stored in a ring buffer. Whenever a request is queued, a new
> >> IPAFrameContext is created and inserted into the ring buffer.
> >>
> >> The IPAFrameContext structure itself has been slightly extended
> >> to store a frame id and a ControlList for incoming frame
> >> controls (sent in by the application). The next step would be to
> >> read and set these controls whenever the request is actually queued
> >> to the hardware.
> >>
> >> Since now we are working in multiples of IPAFrameContext, the
> >> Algorithm::process() will actually take in a IPAFrameContext pointer
> >> (as opposed to a nullptr while preparing for this change).
> >>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
> >>   src/ipa/ipu3/algorithms/agc.h   |  4 ++--
> >>   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
> >>   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
> >>   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
> >>   5 files changed, 55 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp 
> >> b/src/ipa/ipu3/algorithms/agc.cpp
> >> index 383a8deb..f16be534 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -183,14 +183,13 @@ utils::Duration 
> >> Agc::filterExposure(utils::Duration exposureValue)
> >>    * \param[in] yGain The gain calculated based on the relative 
> >> luminance target
> >>    * \param[in] iqMeanGain The gain calculated based on the relative 
> >> luminance target
> >>    */
> >> -void Agc::computeExposure(IPAContext &context, double yGain,
> >> -              double iqMeanGain)
> >> +void Agc::computeExposure(IPAContext &context, IPAFrameContext 
> >> *frameContext,
> >> +              double yGain, double iqMeanGain)
> >>   {
> >>       const IPASessionConfiguration &configuration = 
> >> context.configuration;
> >> -    IPAFrameContext &frameContext = context.frameContext;
> >>       /* Get the effective exposure and gain applied on the sensor. */
> >> -    uint32_t exposure = frameContext.sensor.exposure;
> >> -    double analogueGain = frameContext.sensor.gain;
> >> +    uint32_t exposure = frameContext->sensor.exposure;
> >> +    double analogueGain = frameContext->sensor.gain;
> >>         /* Use the highest of the two gain estimates. */
> >>       double evGain = std::max(yGain, iqMeanGain);
> >> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, 
> >> [[maybe_unused]] IPAFrameContext *frameCo
> >>               break;
> >>       }
> >>   -    computeExposure(context, yGain, iqMeanGain);
> >> +    computeExposure(context, frameContext, yGain, iqMeanGain);
> >>       frameCount_++;
> >>   }
> >>   diff --git a/src/ipa/ipu3/algorithms/agc.h 
> >> b/src/ipa/ipu3/algorithms/agc.h
> >> index 219a1a96..105ae0f2 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.h
> >> +++ b/src/ipa/ipu3/algorithms/agc.h
> >> @@ -35,8 +35,8 @@ private:
> >>       double measureBrightness(const ipu3_uapi_stats_3a *stats,
> >>                    const ipu3_uapi_grid_config &grid) const;
> >>       utils::Duration filterExposure(utils::Duration currentExposure);
> >> -    void computeExposure(IPAContext &context, double yGain,
> >> -                 double iqMeanGain);
> >> +    void computeExposure(IPAContext &context, IPAFrameContext 
> >> *frameContext,
> >> +                 double yGain, double iqMeanGain);
> >>       double estimateLuminance(IPAActiveState &activeState,
> >>                    const ipu3_uapi_grid_config &grid,
> >>                    const ipu3_uapi_stats_3a *stats,
> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >> index 06eb2776..81b30600 100644
> >> --- a/src/ipa/ipu3/ipa_context.cpp
> >> +++ b/src/ipa/ipu3/ipa_context.cpp
> >> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
> >>    * \var IPAContext::configuration
> >>    * \brief The IPA session configuration, immutable during the session
> >>    *
> >> - * \var IPAContext::frameContext
> >> - * \brief The frame context for the frame being processed
> >> + * \var IPAContext::frameContexts
> >> + * \brief Ring buffer of the IPAFrameContext(s)
> >>    *
> >>    * \var IPAContext::activeState
> >>    * \brief The current state of IPA algorithms
> >> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
> >>    */
> >>     /**
> >> + * \brief Default constructor for IPAFrameContext
> >> + */
> >> +IPAFrameContext::IPAFrameContext() = default;
> >> +
> >> +/**
> >> + * \brief Construct a IPAFrameContext instance
> >> + */
> >> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList 
> >> &reqControls)
> >> +    : frame(id), frameControls(reqControls)
> >> +{
> >> +    sensor = {};
> >> +}
> >> +
> >> +/**
> >> + * \var IPAFrameContext::frame
> >> + * \brief The frame number
> >> + *
> >> + * \var IPAFrameContext::frameControls
> >> + * \brief Controls sent in by the application while queuing the request
> >> + *
> >>    * \var IPAFrameContext::sensor
> >>    * \brief Effective sensor values that were applied for the frame
> >>    *
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index 8d681131..42e11141 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -8,16 +8,22 @@
> >>     #pragma once
> >>   +#include <array>
> >> +
> >>   #include <linux/intel-ipu3.h>
> >>     #include <libcamera/base/utils.h>
> >>   +#include <libcamera/controls.h>
> >>   #include <libcamera/geometry.h>
> >>     namespace libcamera {
> >>     namespace ipa::ipu3 {
> >>   +/* Maximum number of frame contexts to be held */
> >> +static constexpr uint32_t kMaxFrameContexts = 16;
> >> +
> >>   struct IPASessionConfiguration {
> >>       struct {
> >>           ipu3_uapi_grid_config bdsGrid;
> >> @@ -71,17 +77,23 @@ struct IPAActiveState {
> >>   };
> >>     struct IPAFrameContext {
> >> +    IPAFrameContext();
> >> +    IPAFrameContext(uint32_t id, const ControlList &reqControls);
> >> +
> >>       struct {
> >>           uint32_t exposure;
> >>           double gain;
> >>       } sensor;
> >> +
> >> +    uint32_t frame;
> >> +    ControlList frameControls;
> >>   };
> >>     struct IPAContext {
> >>       IPASessionConfiguration configuration;
> >>       IPAActiveState activeState;
> >>   -    IPAFrameContext frameContext;
> >> +    std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> >>   };
> >>     } /* namespace ipa::ipu3 */
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index 16e5028f..4322f96b 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
> >>       }
> >>         /* Clean context */
> >> -    context_ = {};
> >> +    context_.configuration = {};
> >>       context_.configuration.sensor.lineDuration = 
> >> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
> >>         /* Construct our Algorithms */
> >> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo 
> >> &configInfo,
> >>         /* Clean IPAActiveState at each reconfiguration. */
> >>       context_.activeState = {};
> >> -    context_.frameContext = {};
> >> +    IPAFrameContext initFrameContext;
> >> +    context_.frameContexts.fill(initFrameContext);
> >>         if (!validateSensorControls()) {
> >>           LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> >> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t 
> >> frame,
> >>       const ipu3_uapi_stats_3a *stats =
> >>           reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> >>   -    context_.frameContext.sensor.exposure = 
> >> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> -    context_.frameContext.sensor.gain = 
> >> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >> +    IPAFrameContext &frameContext = context_.frameContexts[frame % 
> >> kMaxFrameContexts];
> >
> > Shouldn't the frame be "double-checked", verifying that 
> > (frameContext->frame == frame) before passing it to the algorithms 
> > somehow ?
> 
> 
> Well, we can surely verify that but we know in advance that the 
> verification is subjected to failure with known cases (for e.g. frame 
> drop) so I am not very keen putting the verification in. If you had seen 
> the v2, I had a ASSERT similar to this basically (and has some 
> discussion on this there)

I do think some sort of check is warranted though, because the CIO2
might be free-running with no requests coming in - so I can envisage we
could easily end up overflowing the ring buffer in a (bad) scenario - so
we should at least report that.

I would expect that in that scenario the CIO2 will run but the IMGU
might not - so we wouldn't get any statistics for that frame.

It's something to think about later anyway ... but a check/print would
be beneficial at least. For now it could still operate anyway, but with
a warning print.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> >
> > Apart from that:
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> 
> Thanks for reviewing!
> 
> >
> >> +
> >> +    frameContext.sensor.exposure = 
> >> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> +    frameContext.sensor.gain = 
> >> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >>         double lineDuration = 
> >> context_.configuration.sensor.lineDuration.get<std::micro>();
> >>       int32_t vBlank = context_.configuration.sensor.defVBlank;
> >>       ControlList ctrls(controls::controls);
> >>         for (auto const &algo : algorithms_)
> >> -        algo->process(context_, nullptr, stats);
> >> +        algo->process(context_, &frameContext, stats);
> >>         setControls(frame);
> >>   @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const 
> >> uint32_t frame,
> >>       int64_t frameDuration = (vBlank + 
> >> sensorInfo_.outputSize.height) * lineDuration;
> >>       ctrls.set(controls::FrameDuration, frameDuration);
> >>   -    ctrls.set(controls::AnalogueGain, 
> >> context_.frameContext.sensor.gain);
> >> +    ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
> >>         ctrls.set(controls::ColourTemperature, 
> >> context_.activeState.awb.temperatureK);
> >>   -    ctrls.set(controls::ExposureTime, 
> >> context_.frameContext.sensor.exposure * lineDuration);
> >> +    ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * 
> >> lineDuration);
> >>         /*
> >>        * \todo The Metadata provides a path to getting extended data
> >> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t 
> >> frame,
> >>    * Parse the request to handle any IPA-managed controls that were 
> >> set from the
> >>    * application such as manual sensor settings.
> >>    */
> >> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> >> -               [[maybe_unused]] const ControlList &controls)
> >> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList 
> >> &controls)
> >>   {
> >>       /* \todo Start processing for 'frame' based on 'controls'. */
> >> +    IPAFrameContext newFrameContext(frame, controls);
> >> +    context_.frameContexts[frame % kMaxFrameContexts] = 
> >> newFrameContext;
> >>   }
> >>     /**
Jacopo Mondi May 18, 2022, 10:59 a.m. UTC | #6 
Hi Umang,

On Wed, May 18, 2022 at 12:34:15PM +0200, Umang Jain wrote:
> Hi Jacopo,
>
> Thanks for reviewing.
>
> On 5/18/22 09:47, Jacopo Mondi wrote:
> > Hi Umang,
> >
> > On Tue, May 17, 2022 at 09:18:33PM +0200, Umang Jain via libcamera-devel wrote:
> > > Instead of having one frame context constantly being updated,
> > > this patch aims to introduce per-frame IPAFrameContext which
> > > are stored in a ring buffer. Whenever a request is queued, a new
> > > IPAFrameContext is created and inserted into the ring buffer.
> > >
> > > The IPAFrameContext structure itself has been slightly extended
> > > to store a frame id and a ControlList for incoming frame
> > > controls (sent in by the application). The next step would be to
> > > read and set these controls whenever the request is actually queued
> > > to the hardware.
> > >
> > > Since now we are working in multiples of IPAFrameContext, the
> > > Algorithm::process() will actually take in a IPAFrameContext pointer
> > > (as opposed to a nullptr while preparing for this change).
> > >
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >   src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
> > >   src/ipa/ipu3/algorithms/agc.h   |  4 ++--
> > >   src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
> > >   src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
> > >   src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
> > >   5 files changed, 55 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > index 383a8deb..f16be534 100644
> > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > @@ -183,14 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
> > >    * \param[in] yGain The gain calculated based on the relative luminance target
> > >    * \param[in] iqMeanGain The gain calculated based on the relative luminance target
> > >    */
> > > -void Agc::computeExposure(IPAContext &context, double yGain,
> > > -			  double iqMeanGain)
> > > +void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> > > +			  double yGain, double iqMeanGain)
> > >   {
> > >   	const IPASessionConfiguration &configuration = context.configuration;
> > > -	IPAFrameContext &frameContext = context.frameContext;
> > >   	/* Get the effective exposure and gain applied on the sensor. */
> > > -	uint32_t exposure = frameContext.sensor.exposure;
> > > -	double analogueGain = frameContext.sensor.gain;
> > > +	uint32_t exposure = frameContext->sensor.exposure;
> > > +	double analogueGain = frameContext->sensor.gain;
> > >
> > >   	/* Use the highest of the two gain estimates. */
> > >   	double evGain = std::max(yGain, iqMeanGain);
> > > @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo
> > >   			break;
> > >   	}
> > >
> > > -	computeExposure(context, yGain, iqMeanGain);
> > > +	computeExposure(context, frameContext, yGain, iqMeanGain);
> > >   	frameCount_++;
> > >   }
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > > index 219a1a96..105ae0f2 100644
> > > --- a/src/ipa/ipu3/algorithms/agc.h
> > > +++ b/src/ipa/ipu3/algorithms/agc.h
> > > @@ -35,8 +35,8 @@ private:
> > >   	double measureBrightness(const ipu3_uapi_stats_3a *stats,
> > >   				 const ipu3_uapi_grid_config &grid) const;
> > >   	utils::Duration filterExposure(utils::Duration currentExposure);
> > > -	void computeExposure(IPAContext &context, double yGain,
> > > -			     double iqMeanGain);
> > > +	void computeExposure(IPAContext &context, IPAFrameContext *frameContext,
> > > +			     double yGain, double iqMeanGain);
> > >   	double estimateLuminance(IPAActiveState &activeState,
> > >   				 const ipu3_uapi_grid_config &grid,
> > >   				 const ipu3_uapi_stats_3a *stats,
> > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > > index 06eb2776..81b30600 100644
> > > --- a/src/ipa/ipu3/ipa_context.cpp
> > > +++ b/src/ipa/ipu3/ipa_context.cpp
> > > @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
> > >    * \var IPAContext::configuration
> > >    * \brief The IPA session configuration, immutable during the session
> > >    *
> > > - * \var IPAContext::frameContext
> > > - * \brief The frame context for the frame being processed
> > > + * \var IPAContext::frameContexts
> > > + * \brief Ring buffer of the IPAFrameContext(s)
> > >    *
> > >    * \var IPAContext::activeState
> > >    * \brief The current state of IPA algorithms
> > > @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
> > >    */
> > >
> > >   /**
> > > + * \brief Default constructor for IPAFrameContext
> > > + */
> > > +IPAFrameContext::IPAFrameContext() = default;
> > > +
> > > +/**
> > > + * \brief Construct a IPAFrameContext instance
> > > + */
> > > +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > +	: frame(id), frameControls(reqControls)
> > > +{
> > > +	sensor = {};
> > > +}
> > > +
> > > +/**
> > > + * \var IPAFrameContext::frame
> > > + * \brief The frame number
> > > + *
> > > + * \var IPAFrameContext::frameControls
> > > + * \brief Controls sent in by the application while queuing the request
> > > + *
> > >    * \var IPAFrameContext::sensor
> > >    * \brief Effective sensor values that were applied for the frame
> > >    *
> > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > index 8d681131..42e11141 100644
> > > --- a/src/ipa/ipu3/ipa_context.h
> > > +++ b/src/ipa/ipu3/ipa_context.h
> > > @@ -8,16 +8,22 @@
> > >
> > >   #pragma once
> > >
> > > +#include <array>
> > > +
> > >   #include <linux/intel-ipu3.h>
> > >
> > >   #include <libcamera/base/utils.h>
> > >
> > > +#include <libcamera/controls.h>
> > >   #include <libcamera/geometry.h>
> > >
> > >   namespace libcamera {
> > >
> > >   namespace ipa::ipu3 {
> > >
> > > +/* Maximum number of frame contexts to be held */
> > > +static constexpr uint32_t kMaxFrameContexts = 16;
> > > +
> > >   struct IPASessionConfiguration {
> > >   	struct {
> > >   		ipu3_uapi_grid_config bdsGrid;
> > > @@ -71,17 +77,23 @@ struct IPAActiveState {
> > >   };
> > >
> > >   struct IPAFrameContext {
> > > +	IPAFrameContext();
> > > +	IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > > +
> > >   	struct {
> > >   		uint32_t exposure;
> > >   		double gain;
> > >   	} sensor;
> > > +
> > > +	uint32_t frame;
> > > +	ControlList frameControls;
> > >   };
> > >
> > >   struct IPAContext {
> > >   	IPASessionConfiguration configuration;
> > >   	IPAActiveState activeState;
> > >
> > > -	IPAFrameContext frameContext;
> > > +	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> > >   };
> > >
> > >   } /* namespace ipa::ipu3 */
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index 16e5028f..4322f96b 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
> > >   	}
> > >
> > >   	/* Clean context */
> > > -	context_ = {};
> > > +	context_.configuration = {};
> > >   	context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
> > >
> > >   	/* Construct our Algorithms */
> > > @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > >
> > >   	/* Clean IPAActiveState at each reconfiguration. */
> > >   	context_.activeState = {};
> > > -	context_.frameContext = {};
> > > +	IPAFrameContext initFrameContext;
> > > +	context_.frameContexts.fill(initFrameContext);
> > >
> > >   	if (!validateSensorControls()) {
> > >   		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > > @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >   	const ipu3_uapi_stats_3a *stats =
> > >   		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> > >
> > > -	context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > -	context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > > +	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> > > +
> > > +	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > +	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > >
> > >   	double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
> > >   	int32_t vBlank = context_.configuration.sensor.defVBlank;
> > >   	ControlList ctrls(controls::controls);
> > >
> > >   	for (auto const &algo : algorithms_)
> > > -		algo->process(context_, nullptr, stats);
> > > +		algo->process(context_, &frameContext, stats);
> > >
> > >   	setControls(frame);
> > >
> > > @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >   	int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
> > >   	ctrls.set(controls::FrameDuration, frameDuration);
> > >
> > > -	ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> > > +	ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
> > >
> > >   	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
> > >
> > > -	ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
> > > +	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
> > >
> > >   	/*
> > >   	 * \todo The Metadata provides a path to getting extended data
> > > @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > >    * Parse the request to handle any IPA-managed controls that were set from the
> > >    * application such as manual sensor settings.
> > >    */
> > > -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
> > > -			   [[maybe_unused]] const ControlList &controls)
> > > +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> > >   {
> > >   	/* \todo Start processing for 'frame' based on 'controls'. */
> > > +	IPAFrameContext newFrameContext(frame, controls);
> > > +	context_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;
> > Just a little note, which now is not that relevant as we have a
> > limited number of controls, but this goes through at least 2 copies a
> > the 'controls' list.
> >
> > Constructor:
> >
> > IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > 	: frame(id), frameControls(reqControls)
> >
> > And the implicitely defined copy assignment operator.
> >
> > I wonder if
> > 	context_.frameContexts[frame % kMaxFrameContexts] = {frame, controls};
> >
> > would spare a copy
>
>
> I am also wondering. I think your version is still doing two copies as per
> my understanding.
>
> One would be through the constructor as you said above.

Ah, {frame, controls} would construct an instance anyway, you're
right.

I assume that's a recurring problem, how do you re-initialize elements in
a container without going through their copy operators (which require
an instance of the same type to be passed in as parameter) ?

>
> The other one would be while assigning `context_.frameContexts[frame %
> kMaxFrameContexts]` as operator= would do a member-wise copy, does my
> inference make sense?
>
> But nevertheless,
> Your version is a more clear to read hence I would keep this anyway.
>
> >
> > The rest looks good to me
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Thanks
> >     j
> >
> >
> > >   }
> > >
> > >   /**
> > > --
> > > 2.31.0
> > >
Umang Jain May 18, 2022, 11:32 a.m. UTC | #7 
Hi Kieran,

On 5/18/22 12:44, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2022-05-18 10:39:11)
>> Hi JM,
>>
>> On 5/18/22 11:08, Jean-Michel Hautbois wrote:
>>> Hi Umang,
>>>
>>> On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:
>>>> Instead of having one frame context constantly being updated,
>>>> this patch aims to introduce per-frame IPAFrameContext which
>>>> are stored in a ring buffer. Whenever a request is queued, a new
>>>> IPAFrameContext is created and inserted into the ring buffer.
>>>>
>>>> The IPAFrameContext structure itself has been slightly extended
>>>> to store a frame id and a ControlList for incoming frame
>>>> controls (sent in by the application). The next step would be to
>>>> read and set these controls whenever the request is actually queued
>>>> to the hardware.
>>>>
>>>> Since now we are working in multiples of IPAFrameContext, the
>>>> Algorithm::process() will actually take in a IPAFrameContext pointer
>>>> (as opposed to a nullptr while preparing for this change).
>>>>
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> ---
>>>>    src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>>>>    src/ipa/ipu3/algorithms/agc.h   |  4 ++--
>>>>    src/ipa/ipu3/ipa_context.cpp    | 24 ++++++++++++++++++++++--
>>>>    src/ipa/ipu3/ipa_context.h      | 14 +++++++++++++-
>>>>    src/ipa/ipu3/ipu3.cpp           | 22 +++++++++++++---------
>>>>    5 files changed, 55 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp
>>>> b/src/ipa/ipu3/algorithms/agc.cpp
>>>> index 383a8deb..f16be534 100644
>>>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>>>> @@ -183,14 +183,13 @@ utils::Duration
>>>> Agc::filterExposure(utils::Duration exposureValue)
>>>>     * \param[in] yGain The gain calculated based on the relative
>>>> luminance target
>>>>     * \param[in] iqMeanGain The gain calculated based on the relative
>>>> luminance target
>>>>     */
>>>> -void Agc::computeExposure(IPAContext &context, double yGain,
>>>> -              double iqMeanGain)
>>>> +void Agc::computeExposure(IPAContext &context, IPAFrameContext
>>>> *frameContext,
>>>> +              double yGain, double iqMeanGain)
>>>>    {
>>>>        const IPASessionConfiguration &configuration =
>>>> context.configuration;
>>>> -    IPAFrameContext &frameContext = context.frameContext;
>>>>        /* Get the effective exposure and gain applied on the sensor. */
>>>> -    uint32_t exposure = frameContext.sensor.exposure;
>>>> -    double analogueGain = frameContext.sensor.gain;
>>>> +    uint32_t exposure = frameContext->sensor.exposure;
>>>> +    double analogueGain = frameContext->sensor.gain;
>>>>          /* Use the highest of the two gain estimates. */
>>>>        double evGain = std::max(yGain, iqMeanGain);
>>>> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context,
>>>> [[maybe_unused]] IPAFrameContext *frameCo
>>>>                break;
>>>>        }
>>>>    -    computeExposure(context, yGain, iqMeanGain);
>>>> +    computeExposure(context, frameContext, yGain, iqMeanGain);
>>>>        frameCount_++;
>>>>    }
>>>>    diff --git a/src/ipa/ipu3/algorithms/agc.h
>>>> b/src/ipa/ipu3/algorithms/agc.h
>>>> index 219a1a96..105ae0f2 100644
>>>> --- a/src/ipa/ipu3/algorithms/agc.h
>>>> +++ b/src/ipa/ipu3/algorithms/agc.h
>>>> @@ -35,8 +35,8 @@ private:
>>>>        double measureBrightness(const ipu3_uapi_stats_3a *stats,
>>>>                     const ipu3_uapi_grid_config &grid) const;
>>>>        utils::Duration filterExposure(utils::Duration currentExposure);
>>>> -    void computeExposure(IPAContext &context, double yGain,
>>>> -                 double iqMeanGain);
>>>> +    void computeExposure(IPAContext &context, IPAFrameContext
>>>> *frameContext,
>>>> +                 double yGain, double iqMeanGain);
>>>>        double estimateLuminance(IPAActiveState &activeState,
>>>>                     const ipu3_uapi_grid_config &grid,
>>>>                     const ipu3_uapi_stats_3a *stats,
>>>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>>>> index 06eb2776..81b30600 100644
>>>> --- a/src/ipa/ipu3/ipa_context.cpp
>>>> +++ b/src/ipa/ipu3/ipa_context.cpp
>>>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>>>>     * \var IPAContext::configuration
>>>>     * \brief The IPA session configuration, immutable during the session
>>>>     *
>>>> - * \var IPAContext::frameContext
>>>> - * \brief The frame context for the frame being processed
>>>> + * \var IPAContext::frameContexts
>>>> + * \brief Ring buffer of the IPAFrameContext(s)
>>>>     *
>>>>     * \var IPAContext::activeState
>>>>     * \brief The current state of IPA algorithms
>>>> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
>>>>     */
>>>>      /**
>>>> + * \brief Default constructor for IPAFrameContext
>>>> + */
>>>> +IPAFrameContext::IPAFrameContext() = default;
>>>> +
>>>> +/**
>>>> + * \brief Construct a IPAFrameContext instance
>>>> + */
>>>> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList
>>>> &reqControls)
>>>> +    : frame(id), frameControls(reqControls)
>>>> +{
>>>> +    sensor = {};
>>>> +}
>>>> +
>>>> +/**
>>>> + * \var IPAFrameContext::frame
>>>> + * \brief The frame number
>>>> + *
>>>> + * \var IPAFrameContext::frameControls
>>>> + * \brief Controls sent in by the application while queuing the request
>>>> + *
>>>>     * \var IPAFrameContext::sensor
>>>>     * \brief Effective sensor values that were applied for the frame
>>>>     *
>>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>>>> index 8d681131..42e11141 100644
>>>> --- a/src/ipa/ipu3/ipa_context.h
>>>> +++ b/src/ipa/ipu3/ipa_context.h
>>>> @@ -8,16 +8,22 @@
>>>>      #pragma once
>>>>    +#include <array>
>>>> +
>>>>    #include <linux/intel-ipu3.h>
>>>>      #include <libcamera/base/utils.h>
>>>>    +#include <libcamera/controls.h>
>>>>    #include <libcamera/geometry.h>
>>>>      namespace libcamera {
>>>>      namespace ipa::ipu3 {
>>>>    +/* Maximum number of frame contexts to be held */
>>>> +static constexpr uint32_t kMaxFrameContexts = 16;
>>>> +
>>>>    struct IPASessionConfiguration {
>>>>        struct {
>>>>            ipu3_uapi_grid_config bdsGrid;
>>>> @@ -71,17 +77,23 @@ struct IPAActiveState {
>>>>    };
>>>>      struct IPAFrameContext {
>>>> +    IPAFrameContext();
>>>> +    IPAFrameContext(uint32_t id, const ControlList &reqControls);
>>>> +
>>>>        struct {
>>>>            uint32_t exposure;
>>>>            double gain;
>>>>        } sensor;
>>>> +
>>>> +    uint32_t frame;
>>>> +    ControlList frameControls;
>>>>    };
>>>>      struct IPAContext {
>>>>        IPASessionConfiguration configuration;
>>>>        IPAActiveState activeState;
>>>>    -    IPAFrameContext frameContext;
>>>> +    std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>>>>    };
>>>>      } /* namespace ipa::ipu3 */
>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>>> index 16e5028f..4322f96b 100644
>>>> --- a/src/ipa/ipu3/ipu3.cpp
>>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>>> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
>>>>        }
>>>>          /* Clean context */
>>>> -    context_ = {};
>>>> +    context_.configuration = {};
>>>>        context_.configuration.sensor.lineDuration =
>>>> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>>>>          /* Construct our Algorithms */
>>>> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo
>>>> &configInfo,
>>>>          /* Clean IPAActiveState at each reconfiguration. */
>>>>        context_.activeState = {};
>>>> -    context_.frameContext = {};
>>>> +    IPAFrameContext initFrameContext;
>>>> +    context_.frameContexts.fill(initFrameContext);
>>>>          if (!validateSensorControls()) {
>>>>            LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>>>> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t
>>>> frame,
>>>>        const ipu3_uapi_stats_3a *stats =
>>>>            reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>>>>    -    context_.frameContext.sensor.exposure =
>>>> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>> -    context_.frameContext.sensor.gain =
>>>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>>>> +    IPAFrameContext &frameContext = context_.frameContexts[frame %
>>>> kMaxFrameContexts];
>>> Shouldn't the frame be "double-checked", verifying that
>>> (frameContext->frame == frame) before passing it to the algorithms
>>> somehow ?
>>
>> Well, we can surely verify that but we know in advance that the
>> verification is subjected to failure with known cases (for e.g. frame
>> drop) so I am not very keen putting the verification in. If you had seen
>> the v2, I had a ASSERT similar to this basically (and has some
>> discussion on this there)
> I do think some sort of check is warranted though, because the CIO2
> might be free-running with no requests coming in - so I can envisage we
> could easily end up overflowing the ring buffer in a (bad) scenario - so
> we should at least report that.
>
> I would expect that in that scenario the CIO2 will run but the IMGU
> might not - so we wouldn't get any statistics for that frame.
>
> It's something to think about later anyway ... but a check/print would
> be beneficial at least. For now it could still operate anyway, but with
> a warning print.


Fair enough, I have introduced a warning:

+       if (frameContext.frame != frame)
+               LOG(IPAIPU3, Warning) << "Frame " << frame << " does not 
match its frame context";

>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>>> Apart from that:
>>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>
>> Thanks for reviewing!
>>
>>>> +
>>>> +    frameContext.sensor.exposure =
>>>> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>> +    frameContext.sensor.gain =
>>>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>>>>          double lineDuration =
>>>> context_.configuration.sensor.lineDuration.get<std::micro>();
>>>>        int32_t vBlank = context_.configuration.sensor.defVBlank;
>>>>        ControlList ctrls(controls::controls);
>>>>          for (auto const &algo : algorithms_)
>>>> -        algo->process(context_, nullptr, stats);
>>>> +        algo->process(context_, &frameContext, stats);
>>>>          setControls(frame);
>>>>    @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const
>>>> uint32_t frame,
>>>>        int64_t frameDuration = (vBlank +
>>>> sensorInfo_.outputSize.height) * lineDuration;
>>>>        ctrls.set(controls::FrameDuration, frameDuration);
>>>>    -    ctrls.set(controls::AnalogueGain,
>>>> context_.frameContext.sensor.gain);
>>>> +    ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>>>>          ctrls.set(controls::ColourTemperature,
>>>> context_.activeState.awb.temperatureK);
>>>>    -    ctrls.set(controls::ExposureTime,
>>>> context_.frameContext.sensor.exposure * lineDuration);
>>>> +    ctrls.set(controls::ExposureTime, frameContext.sensor.exposure *
>>>> lineDuration);
>>>>          /*
>>>>         * \todo The Metadata provides a path to getting extended data
>>>> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t
>>>> frame,
>>>>     * Parse the request to handle any IPA-managed controls that were
>>>> set from the
>>>>     * application such as manual sensor settings.
>>>>     */
>>>> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
>>>> -               [[maybe_unused]] const ControlList &controls)
>>>> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList
>>>> &controls)
>>>>    {
>>>>        /* \todo Start processing for 'frame' based on 'controls'. */
>>>> +    IPAFrameContext newFrameContext(frame, controls);
>>>> +    context_.frameContexts[frame % kMaxFrameContexts] =
>>>> newFrameContext;
>>>>    }
>>>>      /**

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 383a8deb..f16be534 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -183,14 +183,13 @@  utils::Duration Agc::filterExposure(utils::Duration exposureValue)
  * \param[in] yGain The gain calculated based on the relative luminance target
  * \param[in] iqMeanGain The gain calculated based on the relative luminance target
  */
-void Agc::computeExposure(IPAContext &context, double yGain,
-			  double iqMeanGain)
+void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext,
+			  double yGain, double iqMeanGain)
 {
 	const IPASessionConfiguration &configuration = context.configuration;
-	IPAFrameContext &frameContext = context.frameContext;
 	/* Get the effective exposure and gain applied on the sensor. */
-	uint32_t exposure = frameContext.sensor.exposure;
-	double analogueGain = frameContext.sensor.gain;
+	uint32_t exposure = frameContext->sensor.exposure;
+	double analogueGain = frameContext->sensor.gain;
 
 	/* Use the highest of the two gain estimates. */
 	double evGain = std::max(yGain, iqMeanGain);
@@ -360,7 +359,7 @@  void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo
 			break;
 	}
 
-	computeExposure(context, yGain, iqMeanGain);
+	computeExposure(context, frameContext, yGain, iqMeanGain);
 	frameCount_++;
 }
 
diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
index 219a1a96..105ae0f2 100644
--- a/src/ipa/ipu3/algorithms/agc.h
+++ b/src/ipa/ipu3/algorithms/agc.h
@@ -35,8 +35,8 @@  private:
 	double measureBrightness(const ipu3_uapi_stats_3a *stats,
 				 const ipu3_uapi_grid_config &grid) const;
 	utils::Duration filterExposure(utils::Duration currentExposure);
-	void computeExposure(IPAContext &context, double yGain,
-			     double iqMeanGain);
+	void computeExposure(IPAContext &context, IPAFrameContext *frameContext,
+			     double yGain, double iqMeanGain);
 	double estimateLuminance(IPAActiveState &activeState,
 				 const ipu3_uapi_grid_config &grid,
 				 const ipu3_uapi_stats_3a *stats,
diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
index 06eb2776..81b30600 100644
--- a/src/ipa/ipu3/ipa_context.cpp
+++ b/src/ipa/ipu3/ipa_context.cpp
@@ -58,8 +58,8 @@  namespace libcamera::ipa::ipu3 {
  * \var IPAContext::configuration
  * \brief The IPA session configuration, immutable during the session
  *
- * \var IPAContext::frameContext
- * \brief The frame context for the frame being processed
+ * \var IPAContext::frameContexts
+ * \brief Ring buffer of the IPAFrameContext(s)
  *
  * \var IPAContext::activeState
  * \brief The current state of IPA algorithms
@@ -183,6 +183,26 @@  namespace libcamera::ipa::ipu3 {
  */
 
 /**
+ * \brief Default constructor for IPAFrameContext
+ */
+IPAFrameContext::IPAFrameContext() = default;
+
+/**
+ * \brief Construct a IPAFrameContext instance
+ */
+IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
+	: frame(id), frameControls(reqControls)
+{
+	sensor = {};
+}
+
+/**
+ * \var IPAFrameContext::frame
+ * \brief The frame number
+ *
+ * \var IPAFrameContext::frameControls
+ * \brief Controls sent in by the application while queuing the request
+ *
  * \var IPAFrameContext::sensor
  * \brief Effective sensor values that were applied for the frame
  *
diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
index 8d681131..42e11141 100644
--- a/src/ipa/ipu3/ipa_context.h
+++ b/src/ipa/ipu3/ipa_context.h
@@ -8,16 +8,22 @@ 
 
 #pragma once
 
+#include <array>
+
 #include <linux/intel-ipu3.h>
 
 #include <libcamera/base/utils.h>
 
+#include <libcamera/controls.h>
 #include <libcamera/geometry.h>
 
 namespace libcamera {
 
 namespace ipa::ipu3 {
 
+/* Maximum number of frame contexts to be held */
+static constexpr uint32_t kMaxFrameContexts = 16;
+
 struct IPASessionConfiguration {
 	struct {
 		ipu3_uapi_grid_config bdsGrid;
@@ -71,17 +77,23 @@  struct IPAActiveState {
 };
 
 struct IPAFrameContext {
+	IPAFrameContext();
+	IPAFrameContext(uint32_t id, const ControlList &reqControls);
+
 	struct {
 		uint32_t exposure;
 		double gain;
 	} sensor;
+
+	uint32_t frame;
+	ControlList frameControls;
 };
 
 struct IPAContext {
 	IPASessionConfiguration configuration;
 	IPAActiveState activeState;
 
-	IPAFrameContext frameContext;
+	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
 };
 
 } /* namespace ipa::ipu3 */
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 16e5028f..4322f96b 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -313,7 +313,7 @@  int IPAIPU3::init(const IPASettings &settings,
 	}
 
 	/* Clean context */
-	context_ = {};
+	context_.configuration = {};
 	context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
 
 	/* Construct our Algorithms */
@@ -456,7 +456,8 @@  int IPAIPU3::configure(const IPAConfigInfo &configInfo,
 
 	/* Clean IPAActiveState at each reconfiguration. */
 	context_.activeState = {};
-	context_.frameContext = {};
+	IPAFrameContext initFrameContext;
+	context_.frameContexts.fill(initFrameContext);
 
 	if (!validateSensorControls()) {
 		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
@@ -568,15 +569,17 @@  void IPAIPU3::processStatsBuffer(const uint32_t frame,
 	const ipu3_uapi_stats_3a *stats =
 		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
 
-	context_.frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
-	context_.frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
+	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
+
+	frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
+	frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
 
 	double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
 	int32_t vBlank = context_.configuration.sensor.defVBlank;
 	ControlList ctrls(controls::controls);
 
 	for (auto const &algo : algorithms_)
-		algo->process(context_, nullptr, stats);
+		algo->process(context_, &frameContext, stats);
 
 	setControls(frame);
 
@@ -584,11 +587,11 @@  void IPAIPU3::processStatsBuffer(const uint32_t frame,
 	int64_t frameDuration = (vBlank + sensorInfo_.outputSize.height) * lineDuration;
 	ctrls.set(controls::FrameDuration, frameDuration);
 
-	ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
+	ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
 
 	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
 
-	ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration);
+	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
 
 	/*
 	 * \todo The Metadata provides a path to getting extended data
@@ -609,10 +612,11 @@  void IPAIPU3::processStatsBuffer(const uint32_t frame,
  * Parse the request to handle any IPA-managed controls that were set from the
  * application such as manual sensor settings.
  */
-void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
-			   [[maybe_unused]] const ControlList &controls)
+void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
 {
 	/* \todo Start processing for 'frame' based on 'controls'. */
+	IPAFrameContext newFrameContext(frame, controls);
+	context_.frameContexts[frame % kMaxFrameContexts] = newFrameContext;
 }
 
 /**