[05/10] ipa: ipu3: Parse and store Agc stats
diff mbox series

Message ID 20240322131451.3092931-6-dan.scally@ideasonboard.com
State Superseded
Headers show
Series
  • Centralise Agc into libipa
Related show

Commit Message

Dan Scally March 22, 2024, 1:14 p.m. UTC
In preparation for switching to a derivation of MeanLuminanceAgc, add
a function to parse and store the statistics for easy retrieval in an
overriding estimateLuminance() function.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++
 src/ipa/ipu3/algorithms/agc.h   |  8 ++++++++
 2 files changed, 41 insertions(+)

Comments

Stefan Klug March 26, 2024, 11:03 a.m. UTC | #1
Hi Daniel,

Thanks for the patch.

On Fri, Mar 22, 2024 at 01:14:46PM +0000, Daniel Scally wrote:
> In preparation for switching to a derivation of MeanLuminanceAgc, add
> a function to parse and store the statistics for easy retrieval in an
> overriding estimateLuminance() function.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++
>  src/ipa/ipu3/algorithms/agc.h   |  8 ++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 606a237a..ee9a42cf 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -142,6 +142,39 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>  	return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>  }
>  
> +void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
> +			  const ipu3_uapi_grid_config &grid)
> +{
> +	uint32_t hist[knumHistogramBins] = { 0 };
> +
> +	reds_.clear();
> +	greens_.clear();
> +	blues_.clear();
> +
> +	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> +		for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
> +			uint32_t cellPosition = cellY * stride_ + cellX;
> +
> +			const ipu3_uapi_awb_set_item *cell =
> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> +					&stats->awb_raw_buffer.meta_data[cellPosition]);
> +
> +			reds_.push_back(cell->R_avg);
> +			greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2);
> +			blues_.push_back(cell->B_avg);
> +
> +			/*
> +			 * Store the average green value to estimate the
> +			 * brightness. Even the overexposed pixels are
> +			 * taken into account.
> +			 */
> +			hist[(cell->Gr_avg + cell->Gb_avg) / 2]++;

Why are only the greens taken into account?  Ahh its just a copy of
existing code, that gets removed in patch 7/10. Still should that be
changed to hist[r*0.299 + (gr + gb) / 2 * 0.587 + b * 0.114]++ ?

Or am I missing something here?

> +		}
> +	}
> +
> +	hist_ = Histogram(Span<uint32_t>(hist));
> +}
> +
>  /**
>   * \brief Apply a filter on the exposure value to limit the speed of changes
>   * \param[in] exposureValue The target exposure from the AGC algorithm
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 9d6e3ff1..7ed0ef7a 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -13,6 +13,8 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libipa/histogram.h"
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
> @@ -43,6 +45,8 @@ private:
>  				 const ipu3_uapi_grid_config &grid,
>  				 const ipu3_uapi_stats_3a *stats,
>  				 double gain);
> +	void parseStatistics(const ipu3_uapi_stats_3a *stats,
> +			     const ipu3_uapi_grid_config &grid);
>  
>  	uint64_t frameCount_;
>  
> @@ -55,6 +59,10 @@ private:
>  	utils::Duration filteredExposure_;
>  
>  	uint32_t stride_;
> +	std::vector<uint8_t> reds_;
> +	std::vector<uint8_t> blues_;
> +	std::vector<uint8_t> greens_;
> +	Histogram hist_;
>  };
>  
>  } /* namespace ipa::ipu3::algorithms */
> -- 
> 2.34.1
>
Dan Scally March 26, 2024, 12:39 p.m. UTC | #2
Hi Stefan

On 26/03/2024 11:03, Stefan Klug wrote:
> Hi Daniel,
>
> Thanks for the patch.
>
> On Fri, Mar 22, 2024 at 01:14:46PM +0000, Daniel Scally wrote:
>> In preparation for switching to a derivation of MeanLuminanceAgc, add
>> a function to parse and store the statistics for easy retrieval in an
>> overriding estimateLuminance() function.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++
>>   src/ipa/ipu3/algorithms/agc.h   |  8 ++++++++
>>   2 files changed, 41 insertions(+)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 606a237a..ee9a42cf 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -142,6 +142,39 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>>   	return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>>   }
>>   
>> +void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
>> +			  const ipu3_uapi_grid_config &grid)
>> +{
>> +	uint32_t hist[knumHistogramBins] = { 0 };
>> +
>> +	reds_.clear();
>> +	greens_.clear();
>> +	blues_.clear();
>> +
>> +	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
>> +		for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
>> +			uint32_t cellPosition = cellY * stride_ + cellX;
>> +
>> +			const ipu3_uapi_awb_set_item *cell =
>> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
>> +					&stats->awb_raw_buffer.meta_data[cellPosition]);
>> +
>> +			reds_.push_back(cell->R_avg);
>> +			greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2);
>> +			blues_.push_back(cell->B_avg);
>> +
>> +			/*
>> +			 * Store the average green value to estimate the
>> +			 * brightness. Even the overexposed pixels are
>> +			 * taken into account.
>> +			 */
>> +			hist[(cell->Gr_avg + cell->Gb_avg) / 2]++;
> Why are only the greens taken into account?  Ahh its just a copy of
> existing code, that gets removed in patch 7/10. Still should that be
> changed to hist[r*0.299 + (gr + gb) / 2 * 0.587 + b * 0.114]++ ?
>
> Or am I missing something here?


No you're missing nothing; I think that that's the right thing to do but I wanted to keep this 
series as closely as possible to the existing behaviour to make it easier to integrate. I can add a 
patch on top to switch this though?

>
>> +		}
>> +	}
>> +
>> +	hist_ = Histogram(Span<uint32_t>(hist));
>> +}
>> +
>>   /**
>>    * \brief Apply a filter on the exposure value to limit the speed of changes
>>    * \param[in] exposureValue The target exposure from the AGC algorithm
>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
>> index 9d6e3ff1..7ed0ef7a 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -13,6 +13,8 @@
>>   
>>   #include <libcamera/geometry.h>
>>   
>> +#include "libipa/histogram.h"
>> +
>>   #include "algorithm.h"
>>   
>>   namespace libcamera {
>> @@ -43,6 +45,8 @@ private:
>>   				 const ipu3_uapi_grid_config &grid,
>>   				 const ipu3_uapi_stats_3a *stats,
>>   				 double gain);
>> +	void parseStatistics(const ipu3_uapi_stats_3a *stats,
>> +			     const ipu3_uapi_grid_config &grid);
>>   
>>   	uint64_t frameCount_;
>>   
>> @@ -55,6 +59,10 @@ private:
>>   	utils::Duration filteredExposure_;
>>   
>>   	uint32_t stride_;
>> +	std::vector<uint8_t> reds_;
>> +	std::vector<uint8_t> blues_;
>> +	std::vector<uint8_t> greens_;
>> +	Histogram hist_;
>>   };
>>   
>>   } /* namespace ipa::ipu3::algorithms */
>> -- 
>> 2.34.1
>>
Laurent Pinchart April 5, 2024, 11:39 p.m. UTC | #3
Hi Dan,

Thank you for the patch.

On Fri, Mar 22, 2024 at 01:14:46PM +0000, Daniel Scally wrote:
> In preparation for switching to a derivation of MeanLuminanceAgc, add
> a function to parse and store the statistics for easy retrieval in an
> overriding estimateLuminance() function.

This is hard to review separately from 06/10, I would squash the two
patches together.

> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 33 +++++++++++++++++++++++++++++++++
>  src/ipa/ipu3/algorithms/agc.h   |  8 ++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 606a237a..ee9a42cf 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -142,6 +142,39 @@ double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>  	return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>  }
>  
> +void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
> +			  const ipu3_uapi_grid_config &grid)
> +{
> +	uint32_t hist[knumHistogramBins] = { 0 };
> +
> +	reds_.clear();
> +	greens_.clear();
> +	blues_.clear();
> +
> +	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> +		for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
> +			uint32_t cellPosition = cellY * stride_ + cellX;
> +
> +			const ipu3_uapi_awb_set_item *cell =
> +				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> +					&stats->awb_raw_buffer.meta_data[cellPosition]);
> +
> +			reds_.push_back(cell->R_avg);
> +			greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2);
> +			blues_.push_back(cell->B_avg);
> +
> +			/*
> +			 * Store the average green value to estimate the
> +			 * brightness. Even the overexposed pixels are
> +			 * taken into account.
> +			 */
> +			hist[(cell->Gr_avg + cell->Gb_avg) / 2]++;
> +		}
> +	}
> +
> +	hist_ = Histogram(Span<uint32_t>(hist));
> +}
> +
>  /**
>   * \brief Apply a filter on the exposure value to limit the speed of changes
>   * \param[in] exposureValue The target exposure from the AGC algorithm
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 9d6e3ff1..7ed0ef7a 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -13,6 +13,8 @@
>  
>  #include <libcamera/geometry.h>
>  
> +#include "libipa/histogram.h"
> +
>  #include "algorithm.h"
>  
>  namespace libcamera {
> @@ -43,6 +45,8 @@ private:
>  				 const ipu3_uapi_grid_config &grid,
>  				 const ipu3_uapi_stats_3a *stats,
>  				 double gain);
> +	void parseStatistics(const ipu3_uapi_stats_3a *stats,
> +			     const ipu3_uapi_grid_config &grid);
>  
>  	uint64_t frameCount_;
>  
> @@ -55,6 +59,10 @@ private:
>  	utils::Duration filteredExposure_;
>  
>  	uint32_t stride_;
> +	std::vector<uint8_t> reds_;
> +	std::vector<uint8_t> blues_;
> +	std::vector<uint8_t> greens_;
> +	Histogram hist_;
>  };
>  
>  } /* namespace ipa::ipu3::algorithms */

Patch
diff mbox series

diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
index 606a237a..ee9a42cf 100644
--- a/src/ipa/ipu3/algorithms/agc.cpp
+++ b/src/ipa/ipu3/algorithms/agc.cpp
@@ -142,6 +142,39 @@  double Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
 	return Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
 }
 
+void Agc::parseStatistics(const ipu3_uapi_stats_3a *stats,
+			  const ipu3_uapi_grid_config &grid)
+{
+	uint32_t hist[knumHistogramBins] = { 0 };
+
+	reds_.clear();
+	greens_.clear();
+	blues_.clear();
+
+	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
+		for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
+			uint32_t cellPosition = cellY * stride_ + cellX;
+
+			const ipu3_uapi_awb_set_item *cell =
+				reinterpret_cast<const ipu3_uapi_awb_set_item *>(
+					&stats->awb_raw_buffer.meta_data[cellPosition]);
+
+			reds_.push_back(cell->R_avg);
+			greens_.push_back((cell->Gr_avg + cell->Gb_avg) / 2);
+			blues_.push_back(cell->B_avg);
+
+			/*
+			 * Store the average green value to estimate the
+			 * brightness. Even the overexposed pixels are
+			 * taken into account.
+			 */
+			hist[(cell->Gr_avg + cell->Gb_avg) / 2]++;
+		}
+	}
+
+	hist_ = Histogram(Span<uint32_t>(hist));
+}
+
 /**
  * \brief Apply a filter on the exposure value to limit the speed of changes
  * \param[in] exposureValue The target exposure from the AGC algorithm
diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
index 9d6e3ff1..7ed0ef7a 100644
--- a/src/ipa/ipu3/algorithms/agc.h
+++ b/src/ipa/ipu3/algorithms/agc.h
@@ -13,6 +13,8 @@ 
 
 #include <libcamera/geometry.h>
 
+#include "libipa/histogram.h"
+
 #include "algorithm.h"
 
 namespace libcamera {
@@ -43,6 +45,8 @@  private:
 				 const ipu3_uapi_grid_config &grid,
 				 const ipu3_uapi_stats_3a *stats,
 				 double gain);
+	void parseStatistics(const ipu3_uapi_stats_3a *stats,
+			     const ipu3_uapi_grid_config &grid);
 
 	uint64_t frameCount_;
 
@@ -55,6 +59,10 @@  private:
 	utils::Duration filteredExposure_;
 
 	uint32_t stride_;
+	std::vector<uint8_t> reds_;
+	std::vector<uint8_t> blues_;
+	std::vector<uint8_t> greens_;
+	Histogram hist_;
 };
 
 } /* namespace ipa::ipu3::algorithms */