[1/4] libipa: FrameContext: Move init() to FrameContext
diff mbox series

Message ID 20241016170348.715993-2-jacopo.mondi@ideasonboard.com
State New
Headers show
Series
  • libipa: Initialize FrameContext with ActiveState
Related show

Commit Message

Jacopo Mondi Oct. 16, 2024, 5:03 p.m. UTC 
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@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(-)

Comments

Stefan Klug Oct. 28, 2024, 9:57 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch. 

On Wed, Oct 16, 2024 at 07:03:42PM +0200, 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@ideasonboard.com>

Looks good to me.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Best regards,
Stefan

> ---
>  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.
> + */
> +
>  /**
>   * \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;
> +	}
> +
>  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 {
> -- 
> 2.47.0
>
Dan Scally Oct. 28, 2024, 10:07 a.m. UTC | #2 
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@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@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 {

Patch
diff mbox series 

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.
+ */
+
 /**
  * \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;
+	}
+
 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 {