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

[Naushir Patuck]

On Wed, 8 May 2024 at 14:33, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Dan,
>
> Thank you for the patch.
>
> 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.


>
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240508/1062b5bb/attachment.htm>