| Message ID | 20240405144729.2992219-4-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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;
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; >
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;
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(-)