[3/5] fixup: ipa: rkisp1: Remove bespoke Agc functions
diff mbox series

Message ID 20240405144729.2992219-4-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: rkisp1: Improve AGC (plumbing)
Related show

Commit Message

Paul Elder April 5, 2024, 2:47 p.m. UTC
Remove some constants that should have been removed, and restore a
fairly informative comment that got removed.

Although the removal of one of the constants also removes a todo
comment, this will be addressed imminently so it should be fine.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 32 +++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

Comments

Umang Jain April 12, 2024, 8:24 a.m. UTC | #1
Hi Paul

Thank you for the patch.

On 05/04/24 8:17 pm, Paul Elder wrote:
> Remove some constants that should have been removed, and restore a
> fairly informative comment that got removed.
>
> Although the removal of one of the constants also removes a todo
> comment, this will be addressed imminently so it should be fine.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>   src/ipa/rkisp1/algorithms/agc.cpp | 32 +++++++++++++++++++++++++------
>   1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 7220f00a..1dfc4aaa 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -39,12 +39,6 @@ namespace ipa::rkisp1::algorithms {
>   
>   LOG_DEFINE_CATEGORY(RkISP1Agc)
>   
> -/* Minimum limit for analogue gain value */
> -static constexpr double kMinAnalogueGain = 1.0;
> -
> -/* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> -static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> -
>   int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
>   			    const char *prop)
>   {
> @@ -315,6 +309,32 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,
>   					       context.hw->numHistogramBins), 4);
>   }
>   
> +/**
> + * \brief Estimate the relative luminance of the frame with a given gain
> + * \param[in] expMeans The mean luminance values, from the RkISP1 statistics
> + * \param[in] gain The gain to apply to the frame

The documentation states two input function parameters but looking at 
this patch, estimateLuminance() has only gain parameter ?

Did you miss to update the function signature here?
> + *
> + * This function estimates the average relative luminance of the frame that
> + * would be output by the sensor if an additional \a gain was applied.
> + *
> + * The estimation is based on the AE statistics for the current frame. Y
> + * averages for all cells are first multiplied by the gain, and then saturated
> + * to approximate the sensor behaviour at high brightness values. The
> + * approximation is quite rough, as it doesn't take into account non-linearities
> + * when approaching saturation. In this case, saturating after the conversion to
> + * YUV doesn't take into account the fact that the R, G and B components
> + * contribute differently to the relative luminance.
> + *
> + * \todo Have a dedicated YUV algorithm ?
> + *
> + * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
> + * theoretical perfect reflector of 100% reference white.
> + *
> + * More detailed information can be found in:
> + * https://en.wikipedia.org/wiki/Relative_luminance
> + *
> + * \return The relative luminance
> + */
>   double Agc::estimateLuminance(double gain)
>   {
>   	double ySum = 0.0;
Dan Scally April 18, 2024, 11:52 a.m. UTC | #2
Hello Umang, Paul

On 12/04/2024 09:24, Umang Jain wrote:
> Hi Paul
>
> Thank you for the patch.
>
> On 05/04/24 8:17 pm, Paul Elder wrote:
>> Remove some constants that should have been removed, and restore a
>> fairly informative comment that got removed.
>>
>> Although the removal of one of the constants also removes a todo
>> comment, this will be addressed imminently so it should be fine.
>>
>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>> ---
>>   src/ipa/rkisp1/algorithms/agc.cpp | 32 +++++++++++++++++++++++++------
>>   1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>> index 7220f00a..1dfc4aaa 100644
>> --- a/src/ipa/rkisp1/algorithms/agc.cpp
>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>> @@ -39,12 +39,6 @@ namespace ipa::rkisp1::algorithms {
>>     LOG_DEFINE_CATEGORY(RkISP1Agc)
>>   -/* Minimum limit for analogue gain value */
>> -static constexpr double kMinAnalogueGain = 1.0;
>> -
>> -/* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
>> -static constexpr utils::Duration kMaxShutterSpeed = 60ms;
>> -
>>   int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
>>                   const char *prop)
>>   {
>> @@ -315,6 +309,32 @@ void Agc::parseStatistics(const rkisp1_stat_buffer *stats,
>>                              context.hw->numHistogramBins), 4);
>>   }
>>   +/**
>> + * \brief Estimate the relative luminance of the frame with a given gain
>> + * \param[in] expMeans The mean luminance values, from the RkISP1 statistics
>> + * \param[in] gain The gain to apply to the frame
>
> The documentation states two input function parameters but looking at this patch, 
> estimateLuminance() has only gain parameter ?
>
> Did you miss to update the function signature here?

Thank you for the fixes - I squashed this into v2 of the centralised AGC series, but dropped the 
expMeans parameter from the doxygen comment here.
>> + *
>> + * This function estimates the average relative luminance of the frame that
>> + * would be output by the sensor if an additional \a gain was applied.
>> + *
>> + * The estimation is based on the AE statistics for the current frame. Y
>> + * averages for all cells are first multiplied by the gain, and then saturated
>> + * to approximate the sensor behaviour at high brightness values. The
>> + * approximation is quite rough, as it doesn't take into account non-linearities
>> + * when approaching saturation. In this case, saturating after the conversion to
>> + * YUV doesn't take into account the fact that the R, G and B components
>> + * contribute differently to the relative luminance.
>> + *
>> + * \todo Have a dedicated YUV algorithm ?
>> + *
>> + * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
>> + * theoretical perfect reflector of 100% reference white.
>> + *
>> + * More detailed information can be found in:
>> + * https://en.wikipedia.org/wiki/Relative_luminance
>> + *
>> + * \return The relative luminance
>> + */
>>   double Agc::estimateLuminance(double gain)
>>   {
>>       double ySum = 0.0;
>

Patch
diff mbox series

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 7220f00a..1dfc4aaa 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -39,12 +39,6 @@  namespace ipa::rkisp1::algorithms {
 
 LOG_DEFINE_CATEGORY(RkISP1Agc)
 
-/* Minimum limit for analogue gain value */
-static constexpr double kMinAnalogueGain = 1.0;
-
-/* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
-static constexpr utils::Duration kMaxShutterSpeed = 60ms;
-
 int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
 			    const char *prop)
 {
@@ -315,6 +309,32 @@  void Agc::parseStatistics(const rkisp1_stat_buffer *stats,
 					       context.hw->numHistogramBins), 4);
 }
 
+/**
+ * \brief Estimate the relative luminance of the frame with a given gain
+ * \param[in] expMeans The mean luminance values, from the RkISP1 statistics
+ * \param[in] gain The gain to apply to the frame
+ *
+ * This function estimates the average relative luminance of the frame that
+ * would be output by the sensor if an additional \a gain was applied.
+ *
+ * The estimation is based on the AE statistics for the current frame. Y
+ * averages for all cells are first multiplied by the gain, and then saturated
+ * to approximate the sensor behaviour at high brightness values. The
+ * approximation is quite rough, as it doesn't take into account non-linearities
+ * when approaching saturation. In this case, saturating after the conversion to
+ * YUV doesn't take into account the fact that the R, G and B components
+ * contribute differently to the relative luminance.
+ *
+ * \todo Have a dedicated YUV algorithm ?
+ *
+ * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
+ * theoretical perfect reflector of 100% reference white.
+ *
+ * More detailed information can be found in:
+ * https://en.wikipedia.org/wiki/Relative_luminance
+ *
+ * \return The relative luminance
+ */
 double Agc::estimateLuminance(double gain)
 {
 	double ySum = 0.0;