<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 8 May 2024 at 14:33, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Dan,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Apr 18, 2024 at 01:46:30PM +0100, Daniel Scally wrote:<br>
> Within the IPU3 IPA module there are multiple references to min and<br>
> max shutter speeds. In calculation and usage however those variables<br>
> reflect integration time rather than shutter speed - this difference<br>
> in terminology is particularly problematic because the minimum<br>
> shutter speed is equivalent to the maximum integration time.<br>
> <br>
> Replace references to shutter speed with integration time.<br>
<br>
I like this (I may be tempted to reply with U+2661), and I'd like the<br>
change to be applied globally through libcamera. "Integration time" even<br>
deserves a place in a glossary in my opinion.<br></blockquote><div><br></div><div>I might regret asking this... :)</div><div><br></div><div>What's the difference between the two?  I always treat them as equivalent in value.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Signed-off-by: Daniel Scally <<a href="mailto:dan.scally@ideasonboard.com" target="_blank">dan.scally@ideasonboard.com</a>><br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> ---<br>
>  src/ipa/ipu3/algorithms/agc.cpp | 12 ++++++------<br>
>  src/ipa/ipu3/algorithms/agc.h   |  4 ++--<br>
>  src/ipa/ipu3/ipa_context.cpp    |  8 ++++----<br>
>  src/ipa/ipu3/ipa_context.h      |  4 ++--<br>
>  src/ipa/ipu3/ipu3.cpp           |  8 ++++----<br>
>  5 files changed, 18 insertions(+), 18 deletions(-)<br>
> <br>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp<br>
> index 46fc3b33..a59b73fb 100644<br>
> --- a/src/ipa/ipu3/algorithms/agc.cpp<br>
> +++ b/src/ipa/ipu3/algorithms/agc.cpp<br>
> @@ -51,7 +51,7 @@ LOG_DEFINE_CATEGORY(IPU3Agc)<br>
>  static constexpr double kMinAnalogueGain = 1.0;<br>
>  <br>
>  /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */<br>
> -static constexpr utils::Duration kMaxShutterSpeed = 60ms;<br>
> +static constexpr utils::Duration kMaxIntegrationTime = 60ms;<br>
>  <br>
>  /* Histogram constants */<br>
>  static constexpr uint32_t knumHistogramBins = 256;<br>
> @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10;<br>
>  static constexpr double kRelativeLuminanceTarget = 0.16;<br>
>  <br>
>  Agc::Agc()<br>
> -     : minShutterSpeed_(0s), maxShutterSpeed_(0s)<br>
> +     : minIntegrationTime_(0s), maxIntegrationTime_(0s)<br>
>  {<br>
>  }<br>
>  <br>
> @@ -114,9 +114,9 @@ int Agc::configure(IPAContext &context,<br>
>       stride_ = configuration.grid.stride;<br>
>       bdsGrid_ = configuration.grid.bdsGrid;<br>
>  <br>
> -     minShutterSpeed_ = configuration.agc.minShutterSpeed;<br>
> -     maxShutterSpeed_ = std::min(configuration.agc.maxShutterSpeed,<br>
> -                                 kMaxShutterSpeed);<br>
> +     minIntegrationTime_ = configuration.agc.minIntegrationTime;<br>
> +     maxIntegrationTime_ = std::min(configuration.agc.maxIntegrationTime,<br>
> +                                    kMaxIntegrationTime);<br>
>  <br>
>       minAnalogueGain_ = std::max(configuration.agc.minAnalogueGain, kMinAnalogueGain);<br>
>       maxAnalogueGain_ = configuration.agc.maxAnalogueGain;<br>
> @@ -129,7 +129,7 @@ int Agc::configure(IPAContext &context,<br>
>       context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;<br>
>  <br>
>       /* \todo Run this again when FrameDurationLimits is passed in */<br>
> -     configureExposureModeHelpers(minShutterSpeed_, maxShutterSpeed_,<br>
> +     configureExposureModeHelpers(minIntegrationTime_, maxIntegrationTime_,<br>
>                                    minAnalogueGain_, maxAnalogueGain_);<br>
>  <br>
>       resetFrameCount();<br>
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h<br>
> index 945d1846..631dea52 100644<br>
> --- a/src/ipa/ipu3/algorithms/agc.h<br>
> +++ b/src/ipa/ipu3/algorithms/agc.h<br>
> @@ -42,8 +42,8 @@ private:<br>
>       Histogram parseStatistics(const ipu3_uapi_stats_3a *stats,<br>
>                                 const ipu3_uapi_grid_config &grid);<br>
>  <br>
> -     utils::Duration minShutterSpeed_;<br>
> -     utils::Duration maxShutterSpeed_;<br>
> +     utils::Duration minIntegrationTime_;<br>
> +     utils::Duration maxIntegrationTime_;<br>
>  <br>
>       double minAnalogueGain_;<br>
>       double maxAnalogueGain_;<br>
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp<br>
> index c4fb5642..72f7cec9 100644<br>
> --- a/src/ipa/ipu3/ipa_context.cpp<br>
> +++ b/src/ipa/ipu3/ipa_context.cpp<br>
> @@ -92,11 +92,11 @@ namespace libcamera::ipa::ipu3 {<br>
>   * \var IPASessionConfiguration::agc<br>
>   * \brief AGC parameters configuration of the IPA<br>
>   *<br>
> - * \var IPASessionConfiguration::agc.minShutterSpeed<br>
> - * \brief Minimum shutter speed supported with the configured sensor<br>
> + * \var IPASessionConfiguration::agc.minIntegrationTime<br>
> + * \brief Minimum integration time supported with the configured sensor<br>
>   *<br>
> - * \var IPASessionConfiguration::agc.maxShutterSpeed<br>
> - * \brief Maximum shutter speed supported with the configured sensor<br>
> + * \var IPASessionConfiguration::agc.maxIntegrationTime<br>
> + * \brief Maximum integration time supported with the configured sensor<br>
>   *<br>
>   * \var IPASessionConfiguration::agc.minAnalogueGain<br>
>   * \brief Minimum analogue gain supported with the configured sensor<br>
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h<br>
> index a92cb6ce..6b1ffdd1 100644<br>
> --- a/src/ipa/ipu3/ipa_context.h<br>
> +++ b/src/ipa/ipu3/ipa_context.h<br>
> @@ -33,8 +33,8 @@ struct IPASessionConfiguration {<br>
>       } af;<br>
>  <br>
>       struct {<br>
> -             utils::Duration minShutterSpeed;<br>
> -             utils::Duration maxShutterSpeed;<br>
> +             utils::Duration minIntegrationTime;<br>
> +             utils::Duration maxIntegrationTime;<br>
>               double minAnalogueGain;<br>
>               double maxAnalogueGain;<br>
>       } agc;<br>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp<br>
> index 4809eb60..f13cc394 100644<br>
> --- a/src/ipa/ipu3/ipu3.cpp<br>
> +++ b/src/ipa/ipu3/ipu3.cpp<br>
> @@ -217,13 +217,13 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls)<br>
>  <br>
>       /*<br>
>        * When the AGC computes the new exposure values for a frame, it needs<br>
> -      * to know the limits for shutter speed and analogue gain.<br>
> +      * to know the limits for integration time and analogue gain.<br>
>        * As it depends on the sensor, update it with the controls.<br>
>        *<br>
> -      * \todo take VBLANK into account for maximum shutter speed<br>
> +      * \todo take VBLANK into account for maximum integration time<br>
>        */<br>
> -     context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration;<br>
> -     context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration;<br>
> +     context_.configuration.agc.minIntegrationTime = minExposure * context_.configuration.sensor.lineDuration;<br>
> +     context_.configuration.agc.maxIntegrationTime = maxExposure * context_.configuration.sensor.lineDuration;<br>
>       context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);<br>
>       context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);<br>
>  }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>