[PATCH 1/3] ipa: ipu3: Refer to integration time instead of shutter speed

[Laurent Pinchart]

Hi Naush,

On Wed, May 08, 2024 at 03:02:34PM +0100, Naushir Patuck wrote:
> On Wed, 8 May 2024 at 14:33, Laurent Pinchart wrote:
> > On Thu, Apr 18, 2024 at 01:46:30PM +0100, Daniel Scally wrote:
> > > Within the IPU3 IPA module there are multiple references to min and
> > > max shutter speeds. In calculation and usage however those variables
> > > reflect integration time rather than shutter speed - this difference
> > > in terminology is particularly problematic because the minimum
> > > shutter speed is equivalent to the maximum integration time.
> > >
> > > Replace references to shutter speed with integration time.
> >
> > I like this (I may be tempted to reply with U+2661), and I'd like the
> > change to be applied globally through libcamera. "Integration time" even
> > deserves a place in a glossary in my opinion.
> 
> I might regret asking this... :)
> 
> What's the difference between the two?  I always treat them as equivalent
> in value.

Unless you have a mechanical shutter, in which case the shutter speed
could have a different meaning, it's mostly a matter of vocabulary.
Expressing a shutter "speed" in seconds would make most physics teachers
cry. The current code base uses "maximum speed" to indicate the maximum
integration time, which should be the minimum speed. Using "speed" is
just confusing.

Going for time units, we could use "exposure time", which is a common
term. I dislike it as it's often shortened to "exposure", which also has
other meanings. "Integration time" is less subject to confusion.

> > > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > ---
> > >  src/ipa/ipu3/algorithms/agc.cpp | 12 ++++++------
> > >  src/ipa/ipu3/algorithms/agc.h   |  4 ++--
> > >  src/ipa/ipu3/ipa_context.cpp    |  8 ++++----
> > >  src/ipa/ipu3/ipa_context.h      |  4 ++--
> > >  src/ipa/ipu3/ipu3.cpp           |  8 ++++----
> > >  5 files changed, 18 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > index 46fc3b33..a59b73fb 100644
> > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > @@ -51,7 +51,7 @@ LOG_DEFINE_CATEGORY(IPU3Agc)
> > >  static constexpr double kMinAnalogueGain = 1.0;
> > >
> > >  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> > > -static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> > > +static constexpr utils::Duration kMaxIntegrationTime = 60ms;
> > >
> > >  /* Histogram constants */
> > >  static constexpr uint32_t knumHistogramBins = 256;
> > > @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10;
> > >  static constexpr double kRelativeLuminanceTarget = 0.16;
> > >
> > >  Agc::Agc()
> > > -     : minShutterSpeed_(0s), maxShutterSpeed_(0s)
> > > +     : minIntegrationTime_(0s), maxIntegrationTime_(0s)
> > >  {
> > >  }
> > >
> > > @@ -114,9 +114,9 @@ int Agc::configure(IPAContext &context,
> > >       stride_ = configuration.grid.stride;
> > >       bdsGrid_ = configuration.grid.bdsGrid;
> > >
> > > -     minShutterSpeed_ = configuration.agc.minShutterSpeed;
> > > -     maxShutterSpeed_ = std::min(configuration.agc.maxShutterSpeed,
> > > -                                 kMaxShutterSpeed);
> > > +     minIntegrationTime_ = configuration.agc.minIntegrationTime;
> > > +     maxIntegrationTime_ = std::min(configuration.agc.maxIntegrationTime,
> > > +                                    kMaxIntegrationTime);
> > >
> > >       minAnalogueGain_ = std::max(configuration.agc.minAnalogueGain, kMinAnalogueGain);
> > >       maxAnalogueGain_ = configuration.agc.maxAnalogueGain;
> > > @@ -129,7 +129,7 @@ int Agc::configure(IPAContext &context,
> > >       context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > >
> > >       /* \todo Run this again when FrameDurationLimits is passed in */
> > > -     configureExposureModeHelpers(minShutterSpeed_, maxShutterSpeed_,
> > > +     configureExposureModeHelpers(minIntegrationTime_, maxIntegrationTime_,
> > >                                    minAnalogueGain_, maxAnalogueGain_);
> > >
> > >       resetFrameCount();
> > > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > > index 945d1846..631dea52 100644
> > > --- a/src/ipa/ipu3/algorithms/agc.h
> > > +++ b/src/ipa/ipu3/algorithms/agc.h
> > > @@ -42,8 +42,8 @@ private:
> > >       Histogram parseStatistics(const ipu3_uapi_stats_3a *stats,
> > >                                 const ipu3_uapi_grid_config &grid);
> > >
> > > -     utils::Duration minShutterSpeed_;
> > > -     utils::Duration maxShutterSpeed_;
> > > +     utils::Duration minIntegrationTime_;
> > > +     utils::Duration maxIntegrationTime_;
> > >
> > >       double minAnalogueGain_;
> > >       double maxAnalogueGain_;
> > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > > index c4fb5642..72f7cec9 100644
> > > --- a/src/ipa/ipu3/ipa_context.cpp
> > > +++ b/src/ipa/ipu3/ipa_context.cpp
> > > @@ -92,11 +92,11 @@ namespace libcamera::ipa::ipu3 {
> > >   * \var IPASessionConfiguration::agc
> > >   * \brief AGC parameters configuration of the IPA
> > >   *
> > > - * \var IPASessionConfiguration::agc.minShutterSpeed
> > > - * \brief Minimum shutter speed supported with the configured sensor
> > > + * \var IPASessionConfiguration::agc.minIntegrationTime
> > > + * \brief Minimum integration time supported with the configured sensor
> > >   *
> > > - * \var IPASessionConfiguration::agc.maxShutterSpeed
> > > - * \brief Maximum shutter speed supported with the configured sensor
> > > + * \var IPASessionConfiguration::agc.maxIntegrationTime
> > > + * \brief Maximum integration time supported with the configured sensor
> > >   *
> > >   * \var IPASessionConfiguration::agc.minAnalogueGain
> > >   * \brief Minimum analogue gain supported with the configured sensor
> > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > index a92cb6ce..6b1ffdd1 100644
> > > --- a/src/ipa/ipu3/ipa_context.h
> > > +++ b/src/ipa/ipu3/ipa_context.h
> > > @@ -33,8 +33,8 @@ struct IPASessionConfiguration {
> > >       } af;
> > >
> > >       struct {
> > > -             utils::Duration minShutterSpeed;
> > > -             utils::Duration maxShutterSpeed;
> > > +             utils::Duration minIntegrationTime;
> > > +             utils::Duration maxIntegrationTime;
> > >               double minAnalogueGain;
> > >               double maxAnalogueGain;
> > >       } agc;
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index 4809eb60..f13cc394 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -217,13 +217,13 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)
> > >
> > >       /*
> > >        * When the AGC computes the new exposure values for a frame, it needs
> > > -      * to know the limits for shutter speed and analogue gain.
> > > +      * to know the limits for integration time and analogue gain.
> > >        * As it depends on the sensor, update it with the controls.
> > >        *
> > > -      * \todo take VBLANK into account for maximum shutter speed
> > > +      * \todo take VBLANK into account for maximum integration time
> > >        */
> > > -     context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;
> > > -     context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;
> > > +     context_.configuration.agc.minIntegrationTime = minExposure * context_.configuration.sensor.lineDuration;
> > > +     context_.configuration.agc.maxIntegrationTime = maxExposure * context_.configuration.sensor.lineDuration;
> > >       context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
> > >       context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> > >  }

-- 
Regards,

Laurent Pinchart