[v2,8/9] libipa: agc_mean_luminance: Add exposure compensation support
diff mbox series

Message ID 20250411123641.2144530-9-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Wdr preparations
Related show

Commit Message

Stefan Klug April 11, 2025, 12:36 p.m. UTC
Exposure compensation allows to over- or under-expose an
image by a given value. Add support for that in agc_mean_luminance.

The added exposure compensation can lead to luminance target values that
are close or above saturation and are therefore never reachable. Add a
fix for that by limiting the maximum luminance target to 0.95.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

---

Changes in v2:
- Fixed compiler error that slipped through in v1
- Improved commit message to explain the luminance target limitation
- Collected tag
---
 src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--
 src/ipa/libipa/agc_mean_luminance.h   |  6 ++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

Comments

Kieran Bingham April 12, 2025, 12:55 p.m. UTC | #1
Quoting Stefan Klug (2025-04-11 13:36:36)
> Exposure compensation allows to over- or under-expose an
> image by a given value. Add support for that in agc_mean_luminance.
> 
> The added exposure compensation can lead to luminance target values that
> are close or above saturation and are therefore never reachable. Add a
> fix for that by limiting the maximum luminance target to 0.95.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Fixed compiler error that slipped through in v1
> - Improved commit message to explain the luminance target limitation
> - Collected tag
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--
>  src/ipa/libipa/agc_mean_luminance.h   |  6 ++++++
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 9154f083a510..f6cb7144b31a 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;
>   */
>  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>  
> +/*
> + * Maximum relative luminance target
> + *
> + * This value limits the relative luminance target after applying the exposure
> + * compensation. Targeting a value above this limit results in saturation
> + * and the inability to regulate properly.
> + */
> +static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> +
>  /**
>   * \struct AgcMeanLuminance::AgcConstraint
>   * \brief The boundaries and target for an AeConstraintMode constraint
> @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>   */
>  
>  AgcMeanLuminance::AgcMeanLuminance()
> -       : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> +       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
>  {
>  }
>  
> @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
>         return parseExposureModes(tuningData);
>  }
>  
> +/**
> + * \fn AgcMeanLuminance::setExposureCompensation()
> + * \brief Set the exposure compensation value
> + * \param[in] gain The exposure compensation gain
> + *
> + * This function sets the exposure compensation value to be used in the
> + * AGC calculations. It is expressed as gain instead of EV.
> + */
> +
>  /**
>   * \brief Set the ExposureModeHelper limits for this class
>   * \param[in] minExposureTime Minimum exposure time to allow
> @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
>   */
>  double AgcMeanLuminance::estimateInitialGain() const
>  {
> -       double yTarget = relativeLuminanceTarget_;
> +       double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,
> +                                 kMaxRelativeLuminanceTarget);
>         double yGain = 1.0;
>  
>         /*
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index c41391cb0b73..cad7ef845487 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -44,6 +44,11 @@ public:
>  
>         int parseTuningData(const YamlObject &tuningData);
>  
> +       void setExposureCompensation(double gain)
> +       {
> +               exposureCompensation_ = gain;
> +       }
> +
>         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>                        double minGain, double maxGain);
>  
> @@ -84,6 +89,7 @@ private:
>                                    double gain);
>         utils::Duration filterExposure(utils::Duration exposureValue);
>  
> +       double exposureCompensation_;

This seems like a 'streaming session' configuration parameter, but
storing this here means it will persist even between
stop/start/reconfigure cycles.

Should it be reset at configure() time or such ?

Or do you expect each helper user to reset it at configure time ? Maybe
this means we need to bring the same basic IPA interface to the helpers
directly to maintain the same call hierarchy/events through the whole
architecture (and I think even the metadata/control handling could then
be de-duplicated when the controls modify the helpers rather than
platform specific parameters ...)



>         uint64_t frameCount_;
>         utils::Duration filteredExposure_;
>         double relativeLuminanceTarget_;
> -- 
> 2.43.0
>
Paul Elder May 11, 2025, 10:55 p.m. UTC | #2
Quoting Stefan Klug (2025-04-11 14:36:36)
> Exposure compensation allows to over- or under-expose an
> image by a given value. Add support for that in agc_mean_luminance.
> 
> The added exposure compensation can lead to luminance target values that
> are close or above saturation and are therefore never reachable. Add a
> fix for that by limiting the maximum luminance target to 0.95.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

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

> 
> ---
> 
> Changes in v2:
> - Fixed compiler error that slipped through in v1
> - Improved commit message to explain the luminance target limitation
> - Collected tag
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--
>  src/ipa/libipa/agc_mean_luminance.h   |  6 ++++++
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 9154f083a510..f6cb7144b31a 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;
>   */
>  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>  
> +/*
> + * Maximum relative luminance target
> + *
> + * This value limits the relative luminance target after applying the exposure
> + * compensation. Targeting a value above this limit results in saturation
> + * and the inability to regulate properly.
> + */
> +static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> +
>  /**
>   * \struct AgcMeanLuminance::AgcConstraint
>   * \brief The boundaries and target for an AeConstraintMode constraint
> @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>   */
>  
>  AgcMeanLuminance::AgcMeanLuminance()
> -       : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> +       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
>  {
>  }
>  
> @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
>         return parseExposureModes(tuningData);
>  }
>  
> +/**
> + * \fn AgcMeanLuminance::setExposureCompensation()
> + * \brief Set the exposure compensation value
> + * \param[in] gain The exposure compensation gain
> + *
> + * This function sets the exposure compensation value to be used in the
> + * AGC calculations. It is expressed as gain instead of EV.
> + */
> +
>  /**
>   * \brief Set the ExposureModeHelper limits for this class
>   * \param[in] minExposureTime Minimum exposure time to allow
> @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
>   */
>  double AgcMeanLuminance::estimateInitialGain() const
>  {
> -       double yTarget = relativeLuminanceTarget_;
> +       double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,
> +                                 kMaxRelativeLuminanceTarget);
>         double yGain = 1.0;
>  
>         /*
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index c41391cb0b73..cad7ef845487 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -44,6 +44,11 @@ public:
>  
>         int parseTuningData(const YamlObject &tuningData);
>  
> +       void setExposureCompensation(double gain)
> +       {
> +               exposureCompensation_ = gain;
> +       }
> +
>         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>                        double minGain, double maxGain);
>  
> @@ -84,6 +89,7 @@ private:
>                                    double gain);
>         utils::Duration filterExposure(utils::Duration exposureValue);
>  
> +       double exposureCompensation_;
>         uint64_t frameCount_;
>         utils::Duration filteredExposure_;
>         double relativeLuminanceTarget_;
> -- 
> 2.43.0
>
Laurent Pinchart May 11, 2025, 11:08 p.m. UTC | #3
On Fri, Apr 11, 2025 at 02:36:36PM +0200, Stefan Klug wrote:
> Exposure compensation allows to over- or under-expose an
> image by a given value. Add support for that in agc_mean_luminance.
> 
> The added exposure compensation can lead to luminance target values that
> are close or above saturation and are therefore never reachable. Add a
> fix for that by limiting the maximum luminance target to 0.95.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Fixed compiler error that slipped through in v1
> - Improved commit message to explain the luminance target limitation
> - Collected tag
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--
>  src/ipa/libipa/agc_mean_luminance.h   |  6 ++++++
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 9154f083a510..f6cb7144b31a 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;
>   */
>  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>  
> +/*
> + * Maximum relative luminance target
> + *
> + * This value limits the relative luminance target after applying the exposure
> + * compensation. Targeting a value above this limit results in saturation
> + * and the inability to regulate properly.
> + */
> +static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> +
>  /**
>   * \struct AgcMeanLuminance::AgcConstraint
>   * \brief The boundaries and target for an AeConstraintMode constraint
> @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>   */
>  
>  AgcMeanLuminance::AgcMeanLuminance()
> -	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> +	: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)

Line wrap please.

>  {
>  }
>  
> @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
>  	return parseExposureModes(tuningData);
>  }
>  
> +/**
> + * \fn AgcMeanLuminance::setExposureCompensation()
> + * \brief Set the exposure compensation value
> + * \param[in] gain The exposure compensation gain
> + *
> + * This function sets the exposure compensation value to be used in the
> + * AGC calculations. It is expressed as gain instead of EV.

What's the rationale for expressing it as a gain instead of EV ?

> + */
> +
>  /**
>   * \brief Set the ExposureModeHelper limits for this class
>   * \param[in] minExposureTime Minimum exposure time to allow
> @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
>   */
>  double AgcMeanLuminance::estimateInitialGain() const
>  {
> -	double yTarget = relativeLuminanceTarget_;
> +	double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,
> +				  kMaxRelativeLuminanceTarget);
>  	double yGain = 1.0;
>  
>  	/*
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index c41391cb0b73..cad7ef845487 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -44,6 +44,11 @@ public:
>  
>  	int parseTuningData(const YamlObject &tuningData);
>  
> +	void setExposureCompensation(double gain)
> +	{
> +		exposureCompensation_ = gain;
> +	}
> +
>  	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>  		       double minGain, double maxGain);
>  
> @@ -84,6 +89,7 @@ private:
>  				   double gain);
>  	utils::Duration filterExposure(utils::Duration exposureValue);
>  
> +	double exposureCompensation_;
>  	uint64_t frameCount_;
>  	utils::Duration filteredExposure_;
>  	double relativeLuminanceTarget_;
Stefan Klug June 23, 2025, 4:02 p.m. UTC | #4
Hi Laurent,

Thank you for the review.

Quoting Laurent Pinchart (2025-05-12 01:08:17)
> On Fri, Apr 11, 2025 at 02:36:36PM +0200, Stefan Klug wrote:
> > Exposure compensation allows to over- or under-expose an
> > image by a given value. Add support for that in agc_mean_luminance.
> > 
> > The added exposure compensation can lead to luminance target values that
> > are close or above saturation and are therefore never reachable. Add a
> > fix for that by limiting the maximum luminance target to 0.95.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Fixed compiler error that slipped through in v1
> > - Improved commit message to explain the luminance target limitation
> > - Collected tag
> > ---
> >  src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--
> >  src/ipa/libipa/agc_mean_luminance.h   |  6 ++++++
> >  2 files changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 9154f083a510..f6cb7144b31a 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;
> >   */
> >  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> >  
> > +/*
> > + * Maximum relative luminance target
> > + *
> > + * This value limits the relative luminance target after applying the exposure
> > + * compensation. Targeting a value above this limit results in saturation
> > + * and the inability to regulate properly.
> > + */
> > +static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > +
> >  /**
> >   * \struct AgcMeanLuminance::AgcConstraint
> >   * \brief The boundaries and target for an AeConstraintMode constraint
> > @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> >   */
> >  
> >  AgcMeanLuminance::AgcMeanLuminance()
> > -     : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> > +     : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> 
> Line wrap please.
> 
> >  {
> >  }
> >  
> > @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> >       return parseExposureModes(tuningData);
> >  }
> >  
> > +/**
> > + * \fn AgcMeanLuminance::setExposureCompensation()
> > + * \brief Set the exposure compensation value
> > + * \param[in] gain The exposure compensation gain
> > + *
> > + * This function sets the exposure compensation value to be used in the
> > + * AGC calculations. It is expressed as gain instead of EV.
> 
> What's the rationale for expressing it as a gain instead of EV ?

I still owe you an answer on this one. The idea was that everything
inside AgcMeanLuminance is handled as gains. To me EV is more of a
photograpy related unit than a technical unit. But other than that,
I don't have a real argument. So I can as well express that as EV. Maybe
that is even cleaner...

Opinions?

Best regards,
Stefan

> 
> > + */
> > +
> >  /**
> >   * \brief Set the ExposureModeHelper limits for this class
> >   * \param[in] minExposureTime Minimum exposure time to allow
> > @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
> >   */
> >  double AgcMeanLuminance::estimateInitialGain() const
> >  {
> > -     double yTarget = relativeLuminanceTarget_;
> > +     double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > +                               kMaxRelativeLuminanceTarget);
> >       double yGain = 1.0;
> >  
> >       /*
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index c41391cb0b73..cad7ef845487 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -44,6 +44,11 @@ public:
> >  
> >       int parseTuningData(const YamlObject &tuningData);
> >  
> > +     void setExposureCompensation(double gain)
> > +     {
> > +             exposureCompensation_ = gain;
> > +     }
> > +
> >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                      double minGain, double maxGain);
> >  
> > @@ -84,6 +89,7 @@ private:
> >                                  double gain);
> >       utils::Duration filterExposure(utils::Duration exposureValue);
> >  
> > +     double exposureCompensation_;
> >       uint64_t frameCount_;
> >       utils::Duration filteredExposure_;
> >       double relativeLuminanceTarget_;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Stefan Klug June 23, 2025, 4:08 p.m. UTC | #5
Hi Kieran,

Thank you for the review.

Quoting Kieran Bingham (2025-04-12 14:55:00)
> Quoting Stefan Klug (2025-04-11 13:36:36)
> > Exposure compensation allows to over- or under-expose an
> > image by a given value. Add support for that in agc_mean_luminance.
> > 
> > The added exposure compensation can lead to luminance target values that
> > are close or above saturation and are therefore never reachable. Add a
> > fix for that by limiting the maximum luminance target to 0.95.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Fixed compiler error that slipped through in v1
> > - Improved commit message to explain the luminance target limitation
> > - Collected tag
> > ---
> >  src/ipa/libipa/agc_mean_luminance.cpp | 23 +++++++++++++++++++++--
> >  src/ipa/libipa/agc_mean_luminance.h   |  6 ++++++
> >  2 files changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 9154f083a510..f6cb7144b31a 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -44,6 +44,15 @@ static constexpr uint32_t kNumStartupFrames = 10;
> >   */
> >  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> >  
> > +/*
> > + * Maximum relative luminance target
> > + *
> > + * This value limits the relative luminance target after applying the exposure
> > + * compensation. Targeting a value above this limit results in saturation
> > + * and the inability to regulate properly.
> > + */
> > +static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > +
> >  /**
> >   * \struct AgcMeanLuminance::AgcConstraint
> >   * \brief The boundaries and target for an AeConstraintMode constraint
> > @@ -134,7 +143,7 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> >   */
> >  
> >  AgcMeanLuminance::AgcMeanLuminance()
> > -       : frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> > +       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
> >  {
> >  }
> >  
> > @@ -369,6 +378,15 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> >         return parseExposureModes(tuningData);
> >  }
> >  
> > +/**
> > + * \fn AgcMeanLuminance::setExposureCompensation()
> > + * \brief Set the exposure compensation value
> > + * \param[in] gain The exposure compensation gain
> > + *
> > + * This function sets the exposure compensation value to be used in the
> > + * AGC calculations. It is expressed as gain instead of EV.
> > + */
> > +
> >  /**
> >   * \brief Set the ExposureModeHelper limits for this class
> >   * \param[in] minExposureTime Minimum exposure time to allow
> > @@ -425,7 +443,8 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
> >   */
> >  double AgcMeanLuminance::estimateInitialGain() const
> >  {
> > -       double yTarget = relativeLuminanceTarget_;
> > +       double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > +                                 kMaxRelativeLuminanceTarget);
> >         double yGain = 1.0;
> >  
> >         /*
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index c41391cb0b73..cad7ef845487 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -44,6 +44,11 @@ public:
> >  
> >         int parseTuningData(const YamlObject &tuningData);
> >  
> > +       void setExposureCompensation(double gain)
> > +       {
> > +               exposureCompensation_ = gain;
> > +       }
> > +
> >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                        double minGain, double maxGain);
> >  
> > @@ -84,6 +89,7 @@ private:
> >                                    double gain);
> >         utils::Duration filterExposure(utils::Duration exposureValue);
> >  
> > +       double exposureCompensation_;
> 
> This seems like a 'streaming session' configuration parameter, but
> storing this here means it will persist even between
> stop/start/reconfigure cycles.
> 
> Should it be reset at configure() time or such ?
> 
> Or do you expect each helper user to reset it at configure time ? Maybe
> this means we need to bring the same basic IPA interface to the helpers
> directly to maintain the same call hierarchy/events through the whole
> architecture (and I think even the metadata/control handling could then
> be de-duplicated when the controls modify the helpers rather than
> platform specific parameters ...)

I don't know if I'd like to push the same interface to the helpers. I
like to think of them as small building blocks for the actual pipeline.
But then when we want to have all the pipelines to behave similarly it
might make sense... oh my.

Regarding the actual issue. In the actual implementation (next patch)
the function gets called unconditionally with data from the frame
context. So I don't think there is a practical problem here.

Best regards,
Stefan

> 
> 
> 
> >         uint64_t frameCount_;
> >         utils::Duration filteredExposure_;
> >         double relativeLuminanceTarget_;
> > -- 
> > 2.43.0
> >

Patch
diff mbox series

diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index 9154f083a510..f6cb7144b31a 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -44,6 +44,15 @@  static constexpr uint32_t kNumStartupFrames = 10;
  */
 static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
 
+/*
+ * Maximum relative luminance target
+ *
+ * This value limits the relative luminance target after applying the exposure
+ * compensation. Targeting a value above this limit results in saturation
+ * and the inability to regulate properly.
+ */
+static constexpr double kMaxRelativeLuminanceTarget = 0.95;
+
 /**
  * \struct AgcMeanLuminance::AgcConstraint
  * \brief The boundaries and target for an AeConstraintMode constraint
@@ -134,7 +143,7 @@  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
  */
 
 AgcMeanLuminance::AgcMeanLuminance()
-	: frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
+	: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s), relativeLuminanceTarget_(0)
 {
 }
 
@@ -369,6 +378,15 @@  int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
 	return parseExposureModes(tuningData);
 }
 
+/**
+ * \fn AgcMeanLuminance::setExposureCompensation()
+ * \brief Set the exposure compensation value
+ * \param[in] gain The exposure compensation gain
+ *
+ * This function sets the exposure compensation value to be used in the
+ * AGC calculations. It is expressed as gain instead of EV.
+ */
+
 /**
  * \brief Set the ExposureModeHelper limits for this class
  * \param[in] minExposureTime Minimum exposure time to allow
@@ -425,7 +443,8 @@  void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
  */
 double AgcMeanLuminance::estimateInitialGain() const
 {
-	double yTarget = relativeLuminanceTarget_;
+	double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_,
+				  kMaxRelativeLuminanceTarget);
 	double yGain = 1.0;
 
 	/*
diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
index c41391cb0b73..cad7ef845487 100644
--- a/src/ipa/libipa/agc_mean_luminance.h
+++ b/src/ipa/libipa/agc_mean_luminance.h
@@ -44,6 +44,11 @@  public:
 
 	int parseTuningData(const YamlObject &tuningData);
 
+	void setExposureCompensation(double gain)
+	{
+		exposureCompensation_ = gain;
+	}
+
 	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
 		       double minGain, double maxGain);
 
@@ -84,6 +89,7 @@  private:
 				   double gain);
 	utils::Duration filterExposure(utils::Duration exposureValue);
 
+	double exposureCompensation_;
 	uint64_t frameCount_;
 	utils::Duration filteredExposure_;
 	double relativeLuminanceTarget_;