[PATCH 1/4] libipa: FrameContext: Move init() to FrameContext

[Dan Scally]

Hi Jacopo

On 16/10/2024 18:03, Jacopo Mondi wrote:
> The FCtQueue structure initializes a new FrameContext using its init()
> function. In case of request underrun, where a FrameContext is
> initialized without application's controls being supplied, the
> FrameContext needs to be initialized to default values.
>
> In order to allow the single IPAs to initialize a FrameContext to
> the desired default values, move the init() function to the FrameContext
> structure, which each IPA derives to a per-IPA type.
>
> In this way each IPA can override the FrameContext::init() function
> and initialize the FrameContext to the desired default values.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>   src/ipa/libipa/fc_queue.cpp    | 10 ++++++++++
>   src/ipa/libipa/fc_queue.h      | 16 ++++++++--------
>   src/ipa/rkisp1/ipa_context.cpp |  5 +++++
>   src/ipa/rkisp1/ipa_context.h   |  2 ++
>   4 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> index 0365e9197748..fa2454fd5706 100644
> --- a/src/ipa/libipa/fc_queue.cpp
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -38,6 +38,16 @@ namespace ipa {
>    * \brief The frame number
>    */
>   
> +/**
> + * \fn FrameContext::init()
> + * \brief Initialize a frame context
> + * \param[in] frameNum The frame number to assign to this FrameContext
> + *
> + * This function initializes a frame context by assigning it a frame number.
> + * The single IPA modules are expected to override this function to initialize
> + * their derived FrameContext implementation to their desired default values.
I think this comes across almost as meaning that it's a requirement for them to override the 
function - the code makes it optional though so I think that this could be made a bit clearer. Up to 
you.
> + */
> +
>   /**
>    * \class FCQueue
>    * \brief A support class for managing FrameContext instances in IPA modules
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> index 24d9e82b727d..b1e8bc1485d4 100644
> --- a/src/ipa/libipa/fc_queue.h
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -22,6 +22,12 @@ template<typename FrameContext>
>   class FCQueue;
>   
>   struct FrameContext {
> +protected:
> +	virtual void init(const uint32_t frameNum)
> +	{
> +		frame = frameNum;
> +	}


This doesn't clear the contents of the FrameContext like FCQueue::init() did, which would leave 
values from the last time this FC was picked out of the queue in there. I'm not sure that that could 
cause any problems that aren't already there so I'm not too worried about that, but I thought I'd 
mention it anyway.


Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>

> +
>   private:
>   	template<typename T> friend class FCQueue;
>   	uint32_t frame;
> @@ -61,7 +67,7 @@ public:
>   			LOG(FCQueue, Warning)
>   				<< "Frame " << frame << " already initialised";
>   		else
> -			init(frameContext, frame);
> +			frameContext.init(frame);
>   
>   		return frameContext;
>   	}
> @@ -98,18 +104,12 @@ public:
>   		LOG(FCQueue, Warning)
>   			<< "Obtained an uninitialised FrameContext for " << frame;
>   
> -		init(frameContext, frame);
> +		frameContext.init(frame);
>   
>   		return frameContext;
>   	}
>   
>   private:
> -	void init(FrameContext &frameContext, const uint32_t frame)
> -	{
> -		frameContext = {};
> -		frameContext.frame = frame;
> -	}
> -
>   	std::vector<FrameContext> contexts_;
>   };
>   
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 14d0c02a2b32..4e4fe5f4ae96 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -417,6 +417,11 @@ namespace libcamera::ipa::rkisp1 {
>    * \brief Analogue gain multiplier
>    */
>   
> +void IPAFrameContext::init(const uint32_t frameNum)
> +{
> +	FrameContext::init(frameNum);
> +}
> +
>   /**
>    * \struct IPAContext
>    * \brief Global IPA context data shared between all algorithms
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index e274d9b01e1c..51e04bc30627 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -177,6 +177,8 @@ struct IPAFrameContext : public FrameContext {
>   	struct {
>   		Matrix<float, 3, 3> ccm;
>   	} ccm;
> +
> +	void init(const uint32_t frame) override;
>   };
>   
>   struct IPAContext {