[v3,04/19] libipa: exposure_mode_helper: Take exposure/gain quantization into account
diff mbox series

Message ID 20250815102945.1602071-5-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Implement WDR algorithm
Related show

Commit Message

Stefan Klug Aug. 15, 2025, 10:29 a.m. UTC 
In ExposureModeHelper::splitExposure() the quantization of exposure time
and gain is not taken into account. This can lead to visible flicker
when the quantization steps are too big. As a preparation to fixing
that, add a function to set the sensor line length and the current
sensor mode helper and extend the clampXXX functions to return the
quantization error.

By default the exposure time quantization is assumed to be 1us and gain
is assumed to not be quantized at all.

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

---

Changes in v3:
- Collected tags
- Renamed lineLength to lineDuration
- Remove the "no functional changes" sentence, as the returned
  exposureTime/gain are now quantized, which happened outside of this
class before. So that is actually a functional change.
---
 src/ipa/libipa/exposure_mode_helper.cpp | 49 ++++++++++++++++++++-----
 src/ipa/libipa/exposure_mode_helper.h   | 10 ++++-
 2 files changed, 48 insertions(+), 11 deletions(-)

Comments

Paul Elder Sept. 5, 2025, 10:43 a.m. UTC | #1 
Quoting Stefan Klug (2025-08-15 19:29:24)
> In ExposureModeHelper::splitExposure() the quantization of exposure time
> and gain is not taken into account. This can lead to visible flicker
> when the quantization steps are too big. As a preparation to fixing
> that, add a function to set the sensor line length and the current
> sensor mode helper and extend the clampXXX functions to return the
> quantization error.
> 
> By default the exposure time quantization is assumed to be 1us and gain
> is assumed to not be quantized at all.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> ---
> 
> Changes in v3:
> - Collected tags
> - Renamed lineLength to lineDuration
> - Remove the "no functional changes" sentence, as the returned
>   exposureTime/gain are now quantized, which happened outside of this
> class before. So that is actually a functional change.
> ---
>  src/ipa/libipa/exposure_mode_helper.cpp | 49 ++++++++++++++++++++-----
>  src/ipa/libipa/exposure_mode_helper.h   | 10 ++++-
>  2 files changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> index 0c1e99e31a47..b18c30a6291c 100644
> --- a/src/ipa/libipa/exposure_mode_helper.cpp
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -70,18 +70,37 @@ namespace ipa {
>   * the runtime limits set through setLimits() instead.
>   */
>  ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages)
> +       : lineDuration_(1us), minExposureTime_(0us), maxExposureTime_(0us),
> +         minGain_(0), maxGain_(0), sensorHelper_(nullptr)
>  {
> -       minExposureTime_ = 0us;
> -       maxExposureTime_ = 0us;
> -       minGain_ = 0;
> -       maxGain_ = 0;
> -
>         for (const auto &[s, g] : stages) {
>                 exposureTimes_.push_back(s);
>                 gains_.push_back(g);
>         }
>  }
>  
> +/**
> + * \brief Configure sensor details
> + * \param[in] lineDuration The current line length of the sensor
> + * \param[in] sensorHelper The sensor helper
> + *
> + * This function sets the line length and sensor helper. These are used in
> + * splitExposure() to take the quantization of the exposure and gain into
> + * account.
> + *
> + * When this is not called, is is assumed that exposure is in micro second

s/this is not/this has not been/

s/is is/it is/

> + * granularity and gain has no quantization at all.
> + *
> + * The caller has to ensure that sensorHelper is valid for the lifetime of
> + * ExposureModeHelper or the next call to configure.

"or the next call to configure" is a bit confusing. Do you mean that the
sensorHelper must be valid until the next configure() call or configure() must
be called again if ExposureModeHelper dies?


Paul

> + */
> +void ExposureModeHelper::configure(utils::Duration lineDuration,
> +                                  const CameraSensorHelper *sensorHelper)
> +{
> +       lineDuration_ = lineDuration;
> +       sensorHelper_ = sensorHelper;
> +}
> +
>  /**
>   * \brief Set the exposure time and gain limits
>   * \param[in] minExposureTime The minimum exposure time supported
> @@ -108,14 +127,26 @@ void ExposureModeHelper::setLimits(utils::Duration minExposureTime,
>         maxGain_ = maxGain;
>  }
>  
> -utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime) const
> +utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime,
> +                                                     double *quantizationGain) const
>  {
> -       return std::clamp(exposureTime, minExposureTime_, maxExposureTime_);
> +       utils::Duration clamped;
> +       utils::Duration exp;
> +
> +       clamped = std::clamp(exposureTime, minExposureTime_, maxExposureTime_);
> +       exp = static_cast<long>(clamped / lineDuration_) * lineDuration_;
> +       if (quantizationGain)
> +               *quantizationGain = clamped / exp;
> +
> +       return exp;
>  }
>  
> -double ExposureModeHelper::clampGain(double gain) const
> +double ExposureModeHelper::clampGain(double gain, double *quantizationGain) const
>  {
> -       return std::clamp(gain, minGain_, maxGain_);
> +       double clamped = std::clamp(gain, minGain_, maxGain_);
> +       if (!sensorHelper_)
> +               return clamped;
> +       return sensorHelper_->quantizeGain(clamped, quantizationGain);
>  }
>  
>  /**
> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> index c5be1b6703a8..ac7e8da95c6c 100644
> --- a/src/ipa/libipa/exposure_mode_helper.h
> +++ b/src/ipa/libipa/exposure_mode_helper.h
> @@ -14,6 +14,8 @@
>  #include <libcamera/base/span.h>
>  #include <libcamera/base/utils.h>
>  
> +#include "camera_sensor_helper.h"
> +
>  namespace libcamera {
>  
>  namespace ipa {
> @@ -24,6 +26,7 @@ public:
>         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
>         ~ExposureModeHelper() = default;
>  
> +       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
>         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>                        double minGain, double maxGain);
>  
> @@ -36,16 +39,19 @@ public:
>         double maxGain() const { return maxGain_; }
>  
>  private:
> -       utils::Duration clampExposureTime(utils::Duration exposureTime) const;
> -       double clampGain(double gain) const;
> +       utils::Duration clampExposureTime(utils::Duration exposureTime,
> +                                         double *quantizationGain = nullptr) const;
> +       double clampGain(double gain, double *quantizationGain = nullptr) const;
>  
>         std::vector<utils::Duration> exposureTimes_;
>         std::vector<double> gains_;
>  
> +       utils::Duration lineDuration_;
>         utils::Duration minExposureTime_;
>         utils::Duration maxExposureTime_;
>         double minGain_;
>         double maxGain_;
> +       const CameraSensorHelper *sensorHelper_;
>  };
>  
>  } /* namespace ipa */
> -- 
> 2.48.1
>
Stefan Klug Sept. 18, 2025, 1:24 p.m. UTC | #2 
Hi Paul,

Thank you for the review.

Quoting Paul Elder (2025-09-05 12:43:00)
> Quoting Stefan Klug (2025-08-15 19:29:24)
> > In ExposureModeHelper::splitExposure() the quantization of exposure time
> > and gain is not taken into account. This can lead to visible flicker
> > when the quantization steps are too big. As a preparation to fixing
> > that, add a function to set the sensor line length and the current
> > sensor mode helper and extend the clampXXX functions to return the
> > quantization error.
> > 
> > By default the exposure time quantization is assumed to be 1us and gain
> > is assumed to not be quantized at all.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v3:
> > - Collected tags
> > - Renamed lineLength to lineDuration
> > - Remove the "no functional changes" sentence, as the returned
> >   exposureTime/gain are now quantized, which happened outside of this
> > class before. So that is actually a functional change.
> > ---
> >  src/ipa/libipa/exposure_mode_helper.cpp | 49 ++++++++++++++++++++-----
> >  src/ipa/libipa/exposure_mode_helper.h   | 10 ++++-
> >  2 files changed, 48 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > index 0c1e99e31a47..b18c30a6291c 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -70,18 +70,37 @@ namespace ipa {
> >   * the runtime limits set through setLimits() instead.
> >   */
> >  ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages)
> > +       : lineDuration_(1us), minExposureTime_(0us), maxExposureTime_(0us),
> > +         minGain_(0), maxGain_(0), sensorHelper_(nullptr)
> >  {
> > -       minExposureTime_ = 0us;
> > -       maxExposureTime_ = 0us;
> > -       minGain_ = 0;
> > -       maxGain_ = 0;
> > -
> >         for (const auto &[s, g] : stages) {
> >                 exposureTimes_.push_back(s);
> >                 gains_.push_back(g);
> >         }
> >  }
> >  
> > +/**
> > + * \brief Configure sensor details
> > + * \param[in] lineDuration The current line length of the sensor
> > + * \param[in] sensorHelper The sensor helper
> > + *
> > + * This function sets the line length and sensor helper. These are used in
> > + * splitExposure() to take the quantization of the exposure and gain into
> > + * account.
> > + *
> > + * When this is not called, is is assumed that exposure is in micro second
> 
> s/this is not/this has not been/
> 
> s/is is/it is/
> 
> > + * granularity and gain has no quantization at all.
> > + *
> > + * The caller has to ensure that sensorHelper is valid for the lifetime of
> > + * ExposureModeHelper or the next call to configure.
> 
> "or the next call to configure" is a bit confusing. Do you mean that the
> sensorHelper must be valid until the next configure() call or configure() must
> be called again if ExposureModeHelper dies?

I really don't like that part. I replaced it now with
"ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the
caller has to ensure that sensorHelper is valid until the next call to
configure()."

Best regards,
Stefan

> 
> 
> Paul
> 
> > + */
> > +void ExposureModeHelper::configure(utils::Duration lineDuration,
> > +                                  const CameraSensorHelper *sensorHelper)
> > +{
> > +       lineDuration_ = lineDuration;
> > +       sensorHelper_ = sensorHelper;
> > +}
> > +
> >  /**
> >   * \brief Set the exposure time and gain limits
> >   * \param[in] minExposureTime The minimum exposure time supported
> > @@ -108,14 +127,26 @@ void ExposureModeHelper::setLimits(utils::Duration minExposureTime,
> >         maxGain_ = maxGain;
> >  }
> >  
> > -utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime) const
> > +utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime,
> > +                                                     double *quantizationGain) const
> >  {
> > -       return std::clamp(exposureTime, minExposureTime_, maxExposureTime_);
> > +       utils::Duration clamped;
> > +       utils::Duration exp;
> > +
> > +       clamped = std::clamp(exposureTime, minExposureTime_, maxExposureTime_);
> > +       exp = static_cast<long>(clamped / lineDuration_) * lineDuration_;
> > +       if (quantizationGain)
> > +               *quantizationGain = clamped / exp;
> > +
> > +       return exp;
> >  }
> >  
> > -double ExposureModeHelper::clampGain(double gain) const
> > +double ExposureModeHelper::clampGain(double gain, double *quantizationGain) const
> >  {
> > -       return std::clamp(gain, minGain_, maxGain_);
> > +       double clamped = std::clamp(gain, minGain_, maxGain_);
> > +       if (!sensorHelper_)
> > +               return clamped;
> > +       return sensorHelper_->quantizeGain(clamped, quantizationGain);
> >  }
> >  
> >  /**
> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> > index c5be1b6703a8..ac7e8da95c6c 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.h
> > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > @@ -14,6 +14,8 @@
> >  #include <libcamera/base/span.h>
> >  #include <libcamera/base/utils.h>
> >  
> > +#include "camera_sensor_helper.h"
> > +
> >  namespace libcamera {
> >  
> >  namespace ipa {
> > @@ -24,6 +26,7 @@ public:
> >         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
> >         ~ExposureModeHelper() = default;
> >  
> > +       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
> >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                        double minGain, double maxGain);
> >  
> > @@ -36,16 +39,19 @@ public:
> >         double maxGain() const { return maxGain_; }
> >  
> >  private:
> > -       utils::Duration clampExposureTime(utils::Duration exposureTime) const;
> > -       double clampGain(double gain) const;
> > +       utils::Duration clampExposureTime(utils::Duration exposureTime,
> > +                                         double *quantizationGain = nullptr) const;
> > +       double clampGain(double gain, double *quantizationGain = nullptr) const;
> >  
> >         std::vector<utils::Duration> exposureTimes_;
> >         std::vector<double> gains_;
> >  
> > +       utils::Duration lineDuration_;
> >         utils::Duration minExposureTime_;
> >         utils::Duration maxExposureTime_;
> >         double minGain_;
> >         double maxGain_;
> > +       const CameraSensorHelper *sensorHelper_;
> >  };
> >  
> >  } /* namespace ipa */
> > -- 
> > 2.48.1
> >

Patch
diff mbox series 

diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
index 0c1e99e31a47..b18c30a6291c 100644
--- a/src/ipa/libipa/exposure_mode_helper.cpp
+++ b/src/ipa/libipa/exposure_mode_helper.cpp
@@ -70,18 +70,37 @@  namespace ipa {
  * the runtime limits set through setLimits() instead.
  */
 ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages)
+	: lineDuration_(1us), minExposureTime_(0us), maxExposureTime_(0us),
+	  minGain_(0), maxGain_(0), sensorHelper_(nullptr)
 {
-	minExposureTime_ = 0us;
-	maxExposureTime_ = 0us;
-	minGain_ = 0;
-	maxGain_ = 0;
-
 	for (const auto &[s, g] : stages) {
 		exposureTimes_.push_back(s);
 		gains_.push_back(g);
 	}
 }
 
+/**
+ * \brief Configure sensor details
+ * \param[in] lineDuration The current line length of the sensor
+ * \param[in] sensorHelper The sensor helper
+ *
+ * This function sets the line length and sensor helper. These are used in
+ * splitExposure() to take the quantization of the exposure and gain into
+ * account.
+ *
+ * When this is not called, is is assumed that exposure is in micro second
+ * granularity and gain has no quantization at all.
+ *
+ * The caller has to ensure that sensorHelper is valid for the lifetime of
+ * ExposureModeHelper or the next call to configure.
+ */
+void ExposureModeHelper::configure(utils::Duration lineDuration,
+				   const CameraSensorHelper *sensorHelper)
+{
+	lineDuration_ = lineDuration;
+	sensorHelper_ = sensorHelper;
+}
+
 /**
  * \brief Set the exposure time and gain limits
  * \param[in] minExposureTime The minimum exposure time supported
@@ -108,14 +127,26 @@  void ExposureModeHelper::setLimits(utils::Duration minExposureTime,
 	maxGain_ = maxGain;
 }
 
-utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime) const
+utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTime,
+						      double *quantizationGain) const
 {
-	return std::clamp(exposureTime, minExposureTime_, maxExposureTime_);
+	utils::Duration clamped;
+	utils::Duration exp;
+
+	clamped = std::clamp(exposureTime, minExposureTime_, maxExposureTime_);
+	exp = static_cast<long>(clamped / lineDuration_) * lineDuration_;
+	if (quantizationGain)
+		*quantizationGain = clamped / exp;
+
+	return exp;
 }
 
-double ExposureModeHelper::clampGain(double gain) const
+double ExposureModeHelper::clampGain(double gain, double *quantizationGain) const
 {
-	return std::clamp(gain, minGain_, maxGain_);
+	double clamped = std::clamp(gain, minGain_, maxGain_);
+	if (!sensorHelper_)
+		return clamped;
+	return sensorHelper_->quantizeGain(clamped, quantizationGain);
 }
 
 /**
diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
index c5be1b6703a8..ac7e8da95c6c 100644
--- a/src/ipa/libipa/exposure_mode_helper.h
+++ b/src/ipa/libipa/exposure_mode_helper.h
@@ -14,6 +14,8 @@ 
 #include <libcamera/base/span.h>
 #include <libcamera/base/utils.h>
 
+#include "camera_sensor_helper.h"
+
 namespace libcamera {
 
 namespace ipa {
@@ -24,6 +26,7 @@  public:
 	ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
 	~ExposureModeHelper() = default;
 
+	void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
 	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
 		       double minGain, double maxGain);
 
@@ -36,16 +39,19 @@  public:
 	double maxGain() const { return maxGain_; }
 
 private:
-	utils::Duration clampExposureTime(utils::Duration exposureTime) const;
-	double clampGain(double gain) const;
+	utils::Duration clampExposureTime(utils::Duration exposureTime,
+					  double *quantizationGain = nullptr) const;
+	double clampGain(double gain, double *quantizationGain = nullptr) const;
 
 	std::vector<utils::Duration> exposureTimes_;
 	std::vector<double> gains_;
 
+	utils::Duration lineDuration_;
 	utils::Duration minExposureTime_;
 	utils::Duration maxExposureTime_;
 	double minGain_;
 	double maxGain_;
+	const CameraSensorHelper *sensorHelper_;
 };
 
 } /* namespace ipa */