[v1,5/5] ipa: rkisp1: ccm: Ensure metadata contains valid ccm
diff mbox series

Message ID 20240712143227.3036702-6-stefan.klug@ideasonboard.com
State Accepted
Headers show
Series
  • A few fixes for the rkisp1 ipa
Related show

Commit Message

Stefan Klug July 12, 2024, 2:32 p.m. UTC 
When the colour temperature does not change between frames, the ccm
inside the frame context is not updated and the metadata contains
invalid data. Fix that by caching the ccm inside the active state.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/ccm.cpp | 7 +++++--
 src/ipa/rkisp1/ipa_context.h      | 4 ++++
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Dan Scally July 12, 2024, 3:07 p.m. UTC | #1 
Hi Stefan


On 12/07/2024 15:32, Stefan Klug wrote:
> When the colour temperature does not change between frames, the ccm
> inside the frame context is not updated and the metadata contains
> invalid data. Fix that by caching the ccm inside the active state.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---


Makes sense to me: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   src/ipa/rkisp1/algorithms/ccm.cpp | 7 +++++--
>   src/ipa/rkisp1/ipa_context.h      | 4 ++++
>   2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index 4c4e3f5029a4..e9b1e78b7ec1 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -101,7 +101,7 @@ void Ccm::setParameters(rkisp1_params_cfg *params,
>   /**
>    * \copydoc libcamera::ipa::Algorithm::prepare
>    */
> -void Ccm::prepare(IPAContext &context, const uint32_t frame,
> +void Ccm::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,
>   		  IPAFrameContext &frameContext,
>   		  rkisp1_params_cfg *params)
>   {
> @@ -111,13 +111,16 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>   	 * \todo The colour temperature will likely be noisy, add filtering to
>   	 * avoid updating the CCM matrix all the time.
>   	 */
> -	if (frame > 0 && ct == ct_)
> +	if (frame > 0 && ct == ct_) {
> +		frameContext.ccm.ccm = context.activeState.ccm.ccm;
>   		return;
> +	}
>   
>   	ct_ = ct;
>   	Matrix<float, 3, 3> ccm = ccm_.get(ct);
>   	Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
>   
> +	context.activeState.ccm.ccm = ccm;
>   	frameContext.ccm.ccm = ccm;
>   
>   	setParameters(params, ccm, offsets);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 27a9bf62fc16..061efc0c578e 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -97,6 +97,10 @@ struct IPAActiveState {
>   		bool autoEnabled;
>   	} awb;
>   
> +	struct {
> +		Matrix<float, 3, 3> ccm;
> +	} ccm;
> +
>   	struct {
>   		int8_t brightness;
>   		uint8_t contrast;
Kieran Bingham July 15, 2024, 2:54 a.m. UTC | #2 
Quoting Stefan Klug (2024-07-12 15:32:06)
> When the colour temperature does not change between frames, the ccm
> inside the frame context is not updated and the metadata contains
> invalid data. Fix that by caching the ccm inside the active state.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/ccm.cpp | 7 +++++--
>  src/ipa/rkisp1/ipa_context.h      | 4 ++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index 4c4e3f5029a4..e9b1e78b7ec1 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -101,7 +101,7 @@ void Ccm::setParameters(rkisp1_params_cfg *params,
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void Ccm::prepare(IPAContext &context, const uint32_t frame,
> +void Ccm::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,

Is this maybe_unused required? You've added usage, not removed it ... it
certainly looks used to me ?

>                   IPAFrameContext &frameContext,
>                   rkisp1_params_cfg *params)
>  {
> @@ -111,13 +111,16 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>          * \todo The colour temperature will likely be noisy, add filtering to
>          * avoid updating the CCM matrix all the time.
>          */
> -       if (frame > 0 && ct == ct_)
> +       if (frame > 0 && ct == ct_) {
> +               frameContext.ccm.ccm = context.activeState.ccm.ccm;
>                 return;
> +       }
>  
>         ct_ = ct;
>         Matrix<float, 3, 3> ccm = ccm_.get(ct);
>         Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
>  
> +       context.activeState.ccm.ccm = ccm;

Should we save a copy - and fill the context.activeState.ccm.ccm
directly from the ccm_.get ? (Though that might then look unbalanced
against the offsets).

Or otherwise, as this is only accessed internally here - what about
moving Matrix<float, 3, 3> ccm; and Matrix<int16_t, 3, 1> offsets; to
the private class instance ? Then there's no extra copy ...

But if you still prefer this route - then

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

>         frameContext.ccm.ccm = ccm;
>  
>         setParameters(params, ccm, offsets);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 27a9bf62fc16..061efc0c578e 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -97,6 +97,10 @@ struct IPAActiveState {
>                 bool autoEnabled;
>         } awb;
>  
> +       struct {
> +               Matrix<float, 3, 3> ccm;
> +       } ccm;
> +
>         struct {
>                 int8_t brightness;
>                 uint8_t contrast;
> -- 
> 2.43.0
>
Paul Elder July 19, 2024, 8:03 a.m. UTC | #3 
On Fri, Jul 12, 2024 at 04:32:06PM +0200, Stefan Klug wrote:
> When the colour temperature does not change between frames, the ccm
> inside the frame context is not updated and the metadata contains
> invalid data. Fix that by caching the ccm inside the active state.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/rkisp1/algorithms/ccm.cpp | 7 +++++--
>  src/ipa/rkisp1/ipa_context.h      | 4 ++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
> index 4c4e3f5029a4..e9b1e78b7ec1 100644
> --- a/src/ipa/rkisp1/algorithms/ccm.cpp
> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp
> @@ -101,7 +101,7 @@ void Ccm::setParameters(rkisp1_params_cfg *params,
>  /**
>   * \copydoc libcamera::ipa::Algorithm::prepare
>   */
> -void Ccm::prepare(IPAContext &context, const uint32_t frame,
> +void Ccm::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,
>  		  IPAFrameContext &frameContext,
>  		  rkisp1_params_cfg *params)
>  {
> @@ -111,13 +111,16 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>  	 * \todo The colour temperature will likely be noisy, add filtering to
>  	 * avoid updating the CCM matrix all the time.
>  	 */
> -	if (frame > 0 && ct == ct_)
> +	if (frame > 0 && ct == ct_) {
> +		frameContext.ccm.ccm = context.activeState.ccm.ccm;
>  		return;
> +	}
>  
>  	ct_ = ct;
>  	Matrix<float, 3, 3> ccm = ccm_.get(ct);
>  	Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
>  
> +	context.activeState.ccm.ccm = ccm;
>  	frameContext.ccm.ccm = ccm;
>  
>  	setParameters(params, ccm, offsets);
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 27a9bf62fc16..061efc0c578e 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -97,6 +97,10 @@ struct IPAActiveState {
>  		bool autoEnabled;
>  	} awb;
>  
> +	struct {
> +		Matrix<float, 3, 3> ccm;
> +	} ccm;
> +
>  	struct {
>  		int8_t brightness;
>  		uint8_t contrast;
> -- 
> 2.43.0
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp
index 4c4e3f5029a4..e9b1e78b7ec1 100644
--- a/src/ipa/rkisp1/algorithms/ccm.cpp
+++ b/src/ipa/rkisp1/algorithms/ccm.cpp
@@ -101,7 +101,7 @@  void Ccm::setParameters(rkisp1_params_cfg *params,
 /**
  * \copydoc libcamera::ipa::Algorithm::prepare
  */
-void Ccm::prepare(IPAContext &context, const uint32_t frame,
+void Ccm::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,
 		  IPAFrameContext &frameContext,
 		  rkisp1_params_cfg *params)
 {
@@ -111,13 +111,16 @@  void Ccm::prepare(IPAContext &context, const uint32_t frame,
 	 * \todo The colour temperature will likely be noisy, add filtering to
 	 * avoid updating the CCM matrix all the time.
 	 */
-	if (frame > 0 && ct == ct_)
+	if (frame > 0 && ct == ct_) {
+		frameContext.ccm.ccm = context.activeState.ccm.ccm;
 		return;
+	}
 
 	ct_ = ct;
 	Matrix<float, 3, 3> ccm = ccm_.get(ct);
 	Matrix<int16_t, 3, 1> offsets = offsets_.get(ct);
 
+	context.activeState.ccm.ccm = ccm;
 	frameContext.ccm.ccm = ccm;
 
 	setParameters(params, ccm, offsets);
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 27a9bf62fc16..061efc0c578e 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -97,6 +97,10 @@  struct IPAActiveState {
 		bool autoEnabled;
 	} awb;
 
+	struct {
+		Matrix<float, 3, 3> ccm;
+	} ccm;
+
 	struct {
 		int8_t brightness;
 		uint8_t contrast;