[v1,2/5] ipa rkisp1: Remove temperatureK from FrameContext
diff mbox series

Message ID 20240712143227.3036702-3-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 
The frame context is used to store data used for processing that frame.
It is later used to either act as input for other algorithms or to fill
the metadata.  For the colour temperature this is not needed, as the
meatadata shall not contain the value that was active when the image was
processed, but the value that was calculated based on the statistics for
that image. This is no functional change.

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

Comments

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

On 12/07/2024 15:32, Stefan Klug wrote:
> The frame context is used to store data used for processing that frame.
> It is later used to either act as input for other algorithms or to fill
> the metadata.  For the colour temperature this is not needed, as the
> meatadata shall not contain the value that was active when the image was
> processed, but the value that was calculated based on the statistics for
> that image. This is no functional change.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>   src/ipa/rkisp1/algorithms/awb.cpp | 7 ++-----
>   src/ipa/rkisp1/ipa_context.h      | 1 -
>   2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 1a5d4776970a..18f750207793 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -277,7 +277,6 @@ void Awb::process(IPAContext &context,
>   	 */
>   	if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold &&
>   	    blueMean < kMeanMinThreshold) {
> -		frameContext.awb.temperatureK = activeState.awb.temperatureK;
>   		return;
>   	}
>   
> @@ -309,21 +308,19 @@ void Awb::process(IPAContext &context,
>   	activeState.awb.gains.automatic.blue = blueGain;
>   	activeState.awb.gains.automatic.green = 1.0;
>   
> -	frameContext.awb.temperatureK = activeState.awb.temperatureK;
> -
>   	metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
>   	metadata.set(controls::ColourGains, {
>   			static_cast<float>(frameContext.awb.gains.red),
>   			static_cast<float>(frameContext.awb.gains.blue)
>   		});
> -	metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
> +	metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
>   
>   	LOG(RkISP1Awb, Debug) << std::showpoint
>   		<< "Means [" << redMean << ", " << greenMean << ", " << blueMean
>   		<< "], gains [" << activeState.awb.gains.automatic.red << ", "
>   		<< activeState.awb.gains.automatic.green << ", "
>   		<< activeState.awb.gains.automatic.blue << "], temp "
> -		<< frameContext.awb.temperatureK << "K";
> +		<< activeState.awb.temperatureK << "K";
>   }
>   
>   REGISTER_IPA_ALGORITHM(Awb, "Awb")
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 1d0e9030af1c..27a9bf62fc16 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -136,7 +136,6 @@ struct IPAFrameContext : public FrameContext {
>   			double blue;
>   		} gains;
>   
> -		unsigned int temperatureK;
>   		bool autoEnabled;
>   	} awb;
>
Kieran Bingham July 14, 2024, 11:47 p.m. UTC | #2 
Quoting Stefan Klug (2024-07-12 15:32:03)
> The frame context is used to store data used for processing that frame.
> It is later used to either act as input for other algorithms or to fill
> the metadata.  For the colour temperature this is not needed, as the
> meatadata shall not contain the value that was active when the image was
> processed, but the value that was calculated based on the statistics for
> that image. This is no functional change.

Does any other algo want to know the current colour temperature though?

Oh, I see - they would get that from the activestate ... ? which would
then always be the most recently calculated 

> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 7 ++-----
>  src/ipa/rkisp1/ipa_context.h      | 1 -
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 1a5d4776970a..18f750207793 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -277,7 +277,6 @@ void Awb::process(IPAContext &context,
>          */
>         if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold &&
>             blueMean < kMeanMinThreshold) {
> -               frameContext.awb.temperatureK = activeState.awb.temperatureK;
>                 return;
>         }

The braces then become redundant here if this is a simple return ...

But other than that..

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

>  
> @@ -309,21 +308,19 @@ void Awb::process(IPAContext &context,
>         activeState.awb.gains.automatic.blue = blueGain;
>         activeState.awb.gains.automatic.green = 1.0;
>  
> -       frameContext.awb.temperatureK = activeState.awb.temperatureK;
> -
>         metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
>         metadata.set(controls::ColourGains, {
>                         static_cast<float>(frameContext.awb.gains.red),
>                         static_cast<float>(frameContext.awb.gains.blue)
>                 });
> -       metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
> +       metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
>  
>         LOG(RkISP1Awb, Debug) << std::showpoint
>                 << "Means [" << redMean << ", " << greenMean << ", " << blueMean
>                 << "], gains [" << activeState.awb.gains.automatic.red << ", "
>                 << activeState.awb.gains.automatic.green << ", "
>                 << activeState.awb.gains.automatic.blue << "], temp "
> -               << frameContext.awb.temperatureK << "K";
> +               << activeState.awb.temperatureK << "K";
>  }
>  
>  REGISTER_IPA_ALGORITHM(Awb, "Awb")
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 1d0e9030af1c..27a9bf62fc16 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -136,7 +136,6 @@ struct IPAFrameContext : public FrameContext {
>                         double blue;
>                 } gains;
>  
> -               unsigned int temperatureK;
>                 bool autoEnabled;
>         } awb;
>  
> -- 
> 2.43.0
>
Paul Elder July 19, 2024, 7:17 a.m. UTC | #3 
On Fri, Jul 12, 2024 at 04:32:03PM +0200, Stefan Klug wrote:
> The frame context is used to store data used for processing that frame.
> It is later used to either act as input for other algorithms or to fill
> the metadata.  For the colour temperature this is not needed, as the
> meatadata shall not contain the value that was active when the image was
> processed, but the value that was calculated based on the statistics for
> that image. This is no functional change.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

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

> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 7 ++-----
>  src/ipa/rkisp1/ipa_context.h      | 1 -
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 1a5d4776970a..18f750207793 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -277,7 +277,6 @@ void Awb::process(IPAContext &context,
>  	 */
>  	if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold &&
>  	    blueMean < kMeanMinThreshold) {
> -		frameContext.awb.temperatureK = activeState.awb.temperatureK;
>  		return;
>  	}
>  
> @@ -309,21 +308,19 @@ void Awb::process(IPAContext &context,
>  	activeState.awb.gains.automatic.blue = blueGain;
>  	activeState.awb.gains.automatic.green = 1.0;
>  
> -	frameContext.awb.temperatureK = activeState.awb.temperatureK;
> -
>  	metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
>  	metadata.set(controls::ColourGains, {
>  			static_cast<float>(frameContext.awb.gains.red),
>  			static_cast<float>(frameContext.awb.gains.blue)
>  		});
> -	metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
> +	metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
>  
>  	LOG(RkISP1Awb, Debug) << std::showpoint
>  		<< "Means [" << redMean << ", " << greenMean << ", " << blueMean
>  		<< "], gains [" << activeState.awb.gains.automatic.red << ", "
>  		<< activeState.awb.gains.automatic.green << ", "
>  		<< activeState.awb.gains.automatic.blue << "], temp "
> -		<< frameContext.awb.temperatureK << "K";
> +		<< activeState.awb.temperatureK << "K";
>  }
>  
>  REGISTER_IPA_ALGORITHM(Awb, "Awb")
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 1d0e9030af1c..27a9bf62fc16 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -136,7 +136,6 @@ struct IPAFrameContext : public FrameContext {
>  			double blue;
>  		} gains;
>  
> -		unsigned int temperatureK;
>  		bool autoEnabled;
>  	} awb;
>  
> -- 
> 2.43.0
>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
index 1a5d4776970a..18f750207793 100644
--- a/src/ipa/rkisp1/algorithms/awb.cpp
+++ b/src/ipa/rkisp1/algorithms/awb.cpp
@@ -277,7 +277,6 @@  void Awb::process(IPAContext &context,
 	 */
 	if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold &&
 	    blueMean < kMeanMinThreshold) {
-		frameContext.awb.temperatureK = activeState.awb.temperatureK;
 		return;
 	}
 
@@ -309,21 +308,19 @@  void Awb::process(IPAContext &context,
 	activeState.awb.gains.automatic.blue = blueGain;
 	activeState.awb.gains.automatic.green = 1.0;
 
-	frameContext.awb.temperatureK = activeState.awb.temperatureK;
-
 	metadata.set(controls::AwbEnable, frameContext.awb.autoEnabled);
 	metadata.set(controls::ColourGains, {
 			static_cast<float>(frameContext.awb.gains.red),
 			static_cast<float>(frameContext.awb.gains.blue)
 		});
-	metadata.set(controls::ColourTemperature, frameContext.awb.temperatureK);
+	metadata.set(controls::ColourTemperature, activeState.awb.temperatureK);
 
 	LOG(RkISP1Awb, Debug) << std::showpoint
 		<< "Means [" << redMean << ", " << greenMean << ", " << blueMean
 		<< "], gains [" << activeState.awb.gains.automatic.red << ", "
 		<< activeState.awb.gains.automatic.green << ", "
 		<< activeState.awb.gains.automatic.blue << "], temp "
-		<< frameContext.awb.temperatureK << "K";
+		<< activeState.awb.temperatureK << "K";
 }
 
 REGISTER_IPA_ALGORITHM(Awb, "Awb")
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 1d0e9030af1c..27a9bf62fc16 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -136,7 +136,6 @@  struct IPAFrameContext : public FrameContext {
 			double blue;
 		} gains;
 
-		unsigned int temperatureK;
 		bool autoEnabled;
 	} awb;