[libcamera-devel] [PATCH v3 1/4] ipa: ipu3: Separate out frame context queue as a distinct class

[Jacopo Mondi]

Hi Umang,

On Mon, Jun 20, 2022 at 02:10:21AM +0530, Umang Jain via libcamera-devel wrote:
> There are cases where we need more checks and balance to be carried out
> by the frame context queue class. For that, separate it out as a
> distinct class on which we can build upon.
>
> For now, a minimialistic implementation is provided with .get(frame)
> helper which returns a IPAFrameContext pointer for the required frame.
> Going ahead FCQueue::get() can be extended to access/modify the frame
> context queue.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.cpp | 65 ++++++++++++++++++++++++++++++++++--
>  src/ipa/ipu3/ipa_context.h   | 13 +++++++-
>  src/ipa/ipu3/ipu3.cpp        | 18 +++++-----
>  3 files changed, 84 insertions(+), 12 deletions(-)
>
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 13cdb835..36d20b1b 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -7,12 +7,18 @@
>
>  #include "ipa_context.h"
>
> +#include <libcamera/base/log.h>
> +
>  /**
>   * \file ipa_context.h
>   * \brief Context and state information shared between the algorithms
>   */
>
> -namespace libcamera::ipa::ipu3 {
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAIPU3)
> +
> +namespace ipa::ipu3 {
>
>  /**
>   * \struct IPASessionConfiguration
> @@ -211,4 +217,59 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>   * \brief Analogue gain multiplier
>   */
>
> -} /* namespace libcamera::ipa::ipu3 */
> +/**
> + * \class FCQueue
> + * \brief A FIFO circular queue holding IPAFrameContext(s)
> + *
> + * FCQueue holds all the IPAFrameContext(s) related to frames required
> + * to be processed by the IPA at a given point.
> + */
> +
> +/**
> + * \brief FCQueue constructor
> + */
> +FCQueue::FCQueue()
> +{
> +	reset();
> +}
> +
> +/**
> + * \brief Retrieve the IPAFrameContext for the frame
> + * \param[in] frame Frame number for which the IPAFrameContext needs to
> + * retrieved
> + *
> + * \return Pointer to the IPAFrameContext
> + */
> +IPAFrameContext *FCQueue::get(uint32_t frame)
> +{
> +	IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> +
> +	if (frame != frameContext.frame) {
> +		LOG(IPAIPU3, Warning)
> +			<< "Got wrong frame context for frame" << frame
> +			<< " or frame context doesn't exist yet";
> +	}
> +
> +	return &frameContext;
> +}
> +
> +/**
> + * \brief Sets the size of the FCQueue
> + * \param[in] size The intended size of the FCQueue
> + */
> +void FCQueue::setSize(uint32_t size)

Will the size be changed at run time or is it fixed ? Because if
that's the case this could be made a compile-time parameter..

> +{
> +	resize(size);
> +}
> +
> +/**
> + * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
> + */
> +void FCQueue::reset()
> +{
> +	clear();
> +}

If reset() doesn't get augmented, I would rather use clear() directly.
Even more so when you'll move inherit privately and decide which
std::vector<> functions to expose.

> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 42e11141..e487d34b 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -89,11 +89,22 @@ struct IPAFrameContext {
>  	ControlList frameControls;
>  };
>
> +class FCQueue : public std::vector<IPAFrameContext>

Is there anything that prevents you from inheriting privately already?
>From a quick look I don't see usages of the std::vector<> functions on
frameContexts (I might have missed some though)

> +{
> +public:
> +	FCQueue();
> +
> +	void reset();
> +	void setSize(uint32_t size);
> +
> +	IPAFrameContext *get(uint32_t frame);
> +};
> +
>  struct IPAContext {
>  	IPASessionConfiguration configuration;
>  	IPAActiveState activeState;
>
> -	std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> +	FCQueue frameContexts;
>  };
>
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672..d770c254 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -456,8 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>
>  	/* Clean IPAActiveState at each reconfiguration. */
>  	context_.activeState = {};
> -	IPAFrameContext initFrameContext;
> -	context_.frameContexts.fill(initFrameContext);
> +	context_.frameContexts.reset();
> +	context_.frameContexts.setSize(kMaxFrameContexts);
>
>  	if (!validateSensorControls()) {
>  		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -569,20 +569,20 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>  	const ipu3_uapi_stats_3a *stats =
>  		reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>
> -	IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> +	IPAFrameContext *frameContext = context_.frameContexts.get(frame);
>
> -	if (frameContext.frame != frame)
> +	if (frameContext->frame != frame)
>  		LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
>
> -	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>());
> +	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_, &frameContext, stats);
> +		algo->process(context_, frameContext, stats);
>
>  	setControls(frame);
>
> @@ -590,11 +590,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, frameContext.sensor.gain);
> +	ctrls.set(controls::AnalogueGain, frameContext->sensor.gain);
>
>  	ctrls.set(controls::ColourTemperature, context_.activeState.awb.temperatureK);
>
> -	ctrls.set(controls::ExposureTime, frameContext.sensor.exposure * lineDuration);
> +	ctrls.set(controls::ExposureTime, frameContext->sensor.exposure * lineDuration);
>
>  	/*
>  	 * \todo The Metadata provides a path to getting extended data
> --
> 2.31.1
>