[libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store per-frame information in frame context
Jacopo Mondi
jacopo at jmondi.org
Wed Sep 21 22:13:53 CEST 2022
Hi Laurent, Kieran,
On Wed, Sep 21, 2022 at 01:07:21AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:47)
> > Rework the algorithm's usage of the active state to store the value of
> > controls for the last queued request in the queueRequest() function, and
> > store a copy of the values in the corresponding frame context.
> >
> > The frame context is used in the prepare() function to populate the ISP
> > parameters with values corresponding to the right frame.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/ipa/rkisp1/algorithms/agc.cpp | 18 ++++++++-----
> > src/ipa/rkisp1/algorithms/agc.h | 3 ++-
> > src/ipa/rkisp1/ipa_context.cpp | 43 ++++++++++++++++++++-----------
> > src/ipa/rkisp1/ipa_context.h | 14 ++++++----
> > src/ipa/rkisp1/rkisp1.cpp | 9 ++++---
> > 5 files changed, 55 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 4124c3272bc7..e7198169d11b 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -144,17 +144,19 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
> > /**
> > * \brief Estimate the new exposure and gain values
> > * \param[inout] context The shared IPA Context
> > + * \param[in] frameContext The FrameContext for this frame
> > * \param[in] yGain The gain calculated on the current brightness level
> > * \param[in] iqMeanGain The gain calculated based on the relative luminance target
> > */
> > -void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)
> > +void Agc::computeExposure(IPAContext &context, RkISP1FrameContext &frameContext,
> > + double yGain, double iqMeanGain)
> > {
> > IPASessionConfiguration &configuration = context.configuration;
> > IPAActiveState &activeState = context.activeState;
> >
> > /* Get the effective exposure and gain applied on the sensor. */
> > - uint32_t exposure = activeState.sensor.exposure;
> > - double analogueGain = activeState.sensor.gain;
> > + uint32_t exposure = frameContext.sensor.exposure;
> > + double analogueGain = frameContext.sensor.gain;
> >
> > /* Use the highest of the two gain estimates. */
> > double evGain = std::max(yGain, iqMeanGain);
> > @@ -285,7 +287,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
> > * new exposure and gain for the scene.
> > */
> > void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > - [[maybe_unused]] RkISP1FrameContext &frameContext,
> > + RkISP1FrameContext &frameContext,
> > const rkisp1_stat_buffer *stats)
> > {
> > const rkisp1_cif_isp_stat *params = &stats->params;
> > @@ -319,7 +321,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > break;
> > }
> >
> > - computeExposure(context, yGain, iqMeanGain);
> > + computeExposure(context, frameContext, yGain, iqMeanGain);
>
> It may be useful to compare somewhere here the values for gain/exposure
> that we expected to be set on the sensor, as stored in our frame
> context, with the values that could be retrieved from the
> DelayedControls implementation to make sure they are
> consistent/coherrent.
>
>
> > frameCount_++;
> > }
> >
> > @@ -327,9 +329,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > * \copydoc libcamera::ipa::Algorithm::prepare
> > */
> > void Agc::prepare(IPAContext &context, const uint32_t frame,
> > - [[maybe_unused]] RkISP1FrameContext &frameContext,
> > - rkisp1_params_cfg *params)
> > + RkISP1FrameContext &frameContext, rkisp1_params_cfg *params)
> > {
> > + frameContext.agc.exposure = context.activeState.agc.exposure;
> > + frameContext.agc.gain = context.activeState.agc.gain;
> > +
>
> This may be a little dubious ... due to the delays inherrent with
> setting a gain and exposure...
>
> If they are not known to be exactly the values used by the sensor for
> this frame, but we still need to store them here, then I'd add a todo or
> a warning note...
Do I read the code and the comment here right, and this mean we report
as frameContext.agc (== the agc values computed for this frame) the
last values computed by the AGC algorithm overall, which might be
different than the ones computed for the last frame, if we have two
consecutive calls to process() (which calls computeExposure()).
Am I wrong thinking that prepare() and process() are synched to events
with different clocks ? prepare() is called in the "frame ready" call
path, which process() in the ISP "stats ready" call path. Do we have
guarantees that the two are in sync ?
>
>
> Is prepare() called before the sensor starts in the inline-ISP case with
> RKISP?
>
>
> > if (frame > 0)
> > return;
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > index be8932040c8e..d6c6fb130fc1 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -34,7 +34,8 @@ public:
> > const rkisp1_stat_buffer *stats) override;
> >
> > private:
> > - void computeExposure(IPAContext &Context, double yGain, double iqMeanGain);
> > + void computeExposure(IPAContext &Context, RkISP1FrameContext &frameContext,
> > + double yGain, double iqMeanGain);
> > utils::Duration filterExposure(utils::Duration exposureValue);
> > double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
> > double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 211428717838..88e0631c8d39 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -104,16 +104,13 @@ namespace libcamera::ipa::rkisp1 {
> > * \var IPAActiveState::agc
> > * \brief State for the Automatic Gain Control algorithm
> > *
> > - * The exposure and gain determined are expected to be applied to the sensor
> > - * at the earliest opportunity.
> > + * The exposure and gain are the latest values computed by the AGC algorithm.
> > *
> > * \var IPAActiveState::agc.exposure
> > * \brief Exposure time expressed as a number of lines
> > *
> > * \var IPAActiveState::agc.gain
> > * \brief Analogue gain multiplier
> > - *
> > - * The gain should be adapted to the sensor specific gain code before applying.
> > */
> >
> > /**
> > @@ -181,17 +178,6 @@ namespace libcamera::ipa::rkisp1 {
> > * \brief Indicates if ISP parameters need to be updated
> > */
> >
> > -/**
> > - * \var IPAActiveState::sensor
> > - * \brief Effective sensor values
> > - *
> > - * \var IPAActiveState::sensor.exposure
> > - * \brief Exposure time expressed as a number of lines
> > - *
> > - * \var IPAActiveState::sensor.gain
> > - * \brief Analogue gain multiplier
> > - */
> > -
> > /**
> > * \struct RkISP1FrameContext
> > * \brief Per-frame context for algorithms
> > @@ -199,6 +185,33 @@ namespace libcamera::ipa::rkisp1 {
> > * \todo Populate the frame context for all algorithms
> > */
> >
> > +/**
> > + * \var RkISP1FrameContext::agc
> > + * \brief Automatic Gain Control parameters for this frame
> > + *
> > + * The exposure and gain are provided by the AGC algorithm, and are to be
> > + * applied to the sensor in order to take effect for this frame.
> > + *
> > + * \var RkISP1FrameContext::agc.exposure
> > + * \brief Exposure time expressed as a number of lines
> > + *
> > + * \var RkISP1FrameContext::agc.gain
> > + * \brief Analogue gain multiplier
> > + *
> > + * The gain should be adapted to the sensor specific gain code before applying.
> > + */
> > +
> > +/**
> > + * \var RkISP1FrameContext::sensor
> > + * \brief Sensor configuration that used been used for this frame
> > + *
> > + * \var RkISP1FrameContext::sensor.exposure
> > + * \brief Exposure time expressed as a number of lines
> > + *
> > + * \var RkISP1FrameContext::sensor.gain
> > + * \brief Analogue gain multiplier
> > + */
> > +
> > /**
> > * \struct IPAContext
> > * \brief Global IPA context data shared between all algorithms
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 4e38ef40ed3c..ecf993cd22d7 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -82,14 +82,18 @@ struct IPAActiveState {
> > uint8_t sharpness;
> > bool updateParams;
> > } filter;
> > -
> > - struct {
> > - uint32_t exposure;
> > - double gain;
> > - } sensor;
> > };
> >
> > struct RkISP1FrameContext : public FrameContext {
> > + struct {
> > + uint32_t exposure;
> > + double gain;
> > + } agc;
> > +
> > + struct {
> > + uint32_t exposure;
> > + double gain;
> > + } sensor;
> > };
> >
> > struct IPAContext {
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 4f6e0b0f87b9..a23cfc7fb24d 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -334,9 +334,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> > reinterpret_cast<rkisp1_stat_buffer *>(
> > mappedBuffers_.at(bufferId).planes()[0].data());
> >
> > - context_.activeState.sensor.exposure =
> > + frameContext.sensor.exposure =
> > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > - context_.activeState.sensor.gain =
> > + frameContext.sensor.gain =
> > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>
> Aha - good - so here we're getting the 'effective' values that were set
> at the sensor, before we do any processing with them.
>
> So this is the point that would be interesting to compare 'what we asked
> for' ... 'vs what we actually got'.
>
>
> >
> > unsigned int aeState = 0;
> > @@ -351,8 +351,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >
> > void IPARkISP1::setControls(unsigned int frame)
> > {
> > - uint32_t exposure = context_.activeState.agc.exposure;
> > - uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
> > + RkISP1FrameContext &frameContext = context_.frameContexts.get(frame);
> > + uint32_t exposure = frameContext.agc.exposure;
> > + uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);
>
> So this is the part where we need to figure out more regarding PFC ...
> but that's a separate topic for the moment.
>
I might have missed why ? :)
> So while this patch won't meet our PFC requriements - I think that's
> fine and this helps build what we need to get there.
>
>
> I'm a bit more hesitant here (hence leaving this to come back to) ...
> but I don't think this is 'worse' than where we already are - and
> progresses the frame context conversions.
>
> Perhaps with some more notes / a todo on the fact that we need to do
> more about handling of setting exposure / gains for a specific frame...
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >
> > ControlList ctrls(ctrls_);
> > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
More information about the libcamera-devel
mailing list