[libcamera-devel] [PATCH v2 3/3] ipa: ipu3: agc: Introduce lineDuration in IPASessionConfiguration
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Feb 10 12:23:44 CET 2022
Quoting Jean-Michel Hautbois (2022-02-08 17:09:59)
> Hi Kieran,
>
> On 07/02/2022 16:34, Kieran Bingham wrote:
> > Hi JM,
> >
> > Quoting Jean-Michel Hautbois (2022-02-07 13:59:37)
> >> Instead of having a local cached value for line duration, store it in
> >> the IPASessionConfiguration::agc structure.
> >
> > Looking at my previous comments on this patch in
> > https://patchwork.libcamera.org/patch/14778/, I was suggesting that I
> > believed this line duration should be in the sensor specific structure,
> > not the AGC specific structure.
> >
> > I'm sorry my comment then was not clear, perhaps I should have made it
> > more direct.
> >
> > Is the lineDuration specific to the AGC algorithm in any way that I have
> > not seen or understood?
> >
> > In rkisp1/ipa_context.h, you have:
> >
> > struct IPASessionConfiguration {
> > ...
> > struct {
> > utils::Duration lineDuration;
> > } sensor;
> > ...
> > };
> >
> >
> >> While at it, configure the default analogue gain and shutter speed to
> >> controlled fixed values.
> >>
> >> The latter is set to be 10ms as it will in most cases be close to the
> >> one needed, making the AGC faster to converge.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >> src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++++----------
> >> src/ipa/ipu3/algorithms/agc.h | 3 +--
> >> src/ipa/ipu3/ipa_context.cpp | 3 +++
> >> src/ipa/ipu3/ipa_context.h | 1 +
> >> 4 files changed, 20 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index a929085c..137c36cf 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10;
> >> static constexpr double kRelativeLuminanceTarget = 0.16;
> >>
> >> Agc::Agc()
> >> - : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s),
> >> + : frameCount_(0), minShutterSpeed_(0s),
> >> maxShutterSpeed_(0s), filteredExposure_(0s)
> >> {
> >> }
> >> @@ -85,11 +85,14 @@ Agc::Agc()
> >> */
> >> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >> {
> >> - stride_ = context.configuration.grid.stride;
> >> + IPASessionConfiguration &configuration = context.configuration;
> >> + IPAFrameContext &frameContext = context.frameContext;
> >> +
> >> + stride_ = configuration.grid.stride;
> >>
> >> /* \todo use the IPAContext to provide the limits */
> >> - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
> >> - / configInfo.sensorInfo.pixelRate;
> >> + configuration.agc.lineDuration = configInfo.sensorInfo.lineLength * 1.0s
> >> + / configInfo.sensorInfo.pixelRate;
> >
> > And in RKISP1 this is calculated during the IPA configure, before it
> > calls configure on the algorithms. Should we do that here to align them?
> >
>
> I noticed that lineDuration_ is cached and used in updateControls()
> which is called in configure()... and in init() (so, before it is set ?)...
> We might have an issue there ?
Ok - so we have two instances of lineDuration_ ... so they should be
moved to a single one in the sensor specific structure ...
It should be initialised during init() before it's used, and updated
in configure() before any algorithms are called? Is that right?
> >>
> >> minShutterSpeed_ = context.configuration.agc.minShutterSpeed;
> >> maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed,
> >> @@ -99,8 +102,8 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >> maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
> >>
> >> /* Configure the default exposure and gain. */
> >> - context.frameContext.agc.gain = minAnalogueGain_;
> >> - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;
> >> + frameContext.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain);
> >> + frameContext.agc.exposure = 10ms / configuration.agc.lineDuration;
> >>
> >> return 0;
> >> }
> >> @@ -182,9 +185,11 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
> >> * \param[in] yGain The gain calculated based on the relative luminance target
> >> * \param[in] iqMeanGain The gain calculated based on the relative luminance target
> >> */
> >> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
> >> +void Agc::computeExposure(IPAContext &context, double yGain,
> >> double iqMeanGain)
> >> {
> >> + const IPASessionConfiguration &configuration = context.configuration;
> >> + IPAFrameContext &frameContext = context.frameContext;
> >> /* Get the effective exposure and gain applied on the sensor. */
> >> uint32_t exposure = frameContext.sensor.exposure;
> >> double analogueGain = frameContext.sensor.gain;
> >> @@ -200,7 +205,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
> >> /* extracted from Rpi::Agc::computeTargetExposure */
> >>
> >> /* Calculate the shutter time in seconds */
> >> - utils::Duration currentShutter = exposure * lineDuration_;
> >> + utils::Duration currentShutter = exposure * configuration.agc.lineDuration;
> >>
> >> /*
> >> * Update the exposure value for the next computation using the values
> >> @@ -247,7 +252,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
> >> << stepGain;
> >>
> >> /* Update the estimated exposure and gain. */
> >> - frameContext.agc.exposure = shutterTime / lineDuration_;
> >> + frameContext.agc.exposure = shutterTime / configuration.agc.lineDuration;
> >> frameContext.agc.gain = stepGain;
> >> }
> >>
> >> @@ -354,7 +359,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >> break;
> >> }
> >>
> >> - computeExposure(context.frameContext, yGain, iqMeanGain);
> >> + computeExposure(context, yGain, iqMeanGain);
> >> frameCount_++;
> >> }
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> >> index 84bfe045..ad705605 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.h
> >> +++ b/src/ipa/ipu3/algorithms/agc.h
> >> @@ -34,7 +34,7 @@ private:
> >> double measureBrightness(const ipu3_uapi_stats_3a *stats,
> >> const ipu3_uapi_grid_config &grid) const;
> >> utils::Duration filterExposure(utils::Duration currentExposure);
> >> - void computeExposure(IPAFrameContext &frameContext, double yGain,
> >> + void computeExposure(IPAContext &context, double yGain,
> >> double iqMeanGain);
> >> double estimateLuminance(IPAFrameContext &frameContext,
> >> const ipu3_uapi_grid_config &grid,
> >> @@ -43,7 +43,6 @@ private:
> >>
> >> uint64_t frameCount_;
> >>
> >> - utils::Duration lineDuration_;
> >> utils::Duration minShutterSpeed_;
> >> utils::Duration maxShutterSpeed_;
> >>
> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >> index 86794ac1..ace9c66f 100644
> >> --- a/src/ipa/ipu3/ipa_context.cpp
> >> +++ b/src/ipa/ipu3/ipa_context.cpp
> >> @@ -84,6 +84,9 @@ namespace libcamera::ipa::ipu3 {
> >> *
> >> * \var IPASessionConfiguration::agc.maxAnalogueGain
> >> * \brief Maximum analogue gain supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.lineDuration
> >> + * \brief Line duration in microseconds
> >> */
> >>
> >> /**
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index c6dc0814..7696fd14 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -30,6 +30,7 @@ struct IPASessionConfiguration {
> >> utils::Duration maxShutterSpeed;
> >> double minAnalogueGain;
> >> double maxAnalogueGain;
> >> + utils::Duration lineDuration;
> >> } agc;
> >> };
> >>
> >> --
> >> 2.32.0
> >>
More information about the libcamera-devel
mailing list