[libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store per-frame information in frame context
Jacopo Mondi
jacopo at jmondi.org
Thu Sep 22 12:17:02 CEST 2022
Hi Kieran
On Thu, Sep 22, 2022 at 09:59:23AM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2022-09-21 21:13:53)
> > 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()).
>
> What are you distinguising between 'the last frame' and the 'last value
> computed by AGC?'.
>
> process() is currently always the last step where the AGC actually runs,
> so the ActiveState reports the 'best'/'most-up-to-date' desired values
> that the AGC would like to use for automatic mode.
That's my point, if process() has been called two consecutive times
(n, n + 1) we store in the FrameContext 'n' the values computed for
frame 'n + 1'
I'm not even sure that's an issue, just making sure this is known..
>
> However - the issue I have here is that for Sensor controls - this point
> in the time line is too late to apply those values.
>
On this point I'm not concerned, but I might be missing something.
We'll make sure we process frames early enough, and the values we want
to have applied at frame X will be computed here and correctly applied
in advance, so what we report here should be what will actually be
realized for that frame ?
> We can make adjustments to the gains on the ISP here - but not the
> sensor.
>
> We (already know that we) need to identify a way to ensure we
> syncrhonise DelayedControls actions both here on the IPA side along with
> the Pipeline Handler side.
>
> That could be either communicating with DelayedControls, or changing the
> current implementation (I've thought about 'scheduled controls' in the
> past, but it's all still WIP).
>
>
> > 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 ?
>
>
> prepare() is to generate configuration for the ISP.
> process() reports the statistics of the ISP after completion.
>
> For any given frame, prepare can only ever be called before that frame,
> and for the same frame process will be after.
>
I was using IPU3 as a refernce as there fillParmsBuffer() (which calls
algo->prepare()) is called on the CIO2 buffer ready event, so the
assumption that prepare() is called before that frame is produced does
not hold there ?
>
> I don't believe we enforce any requirement for them to be called in
> lockstep, and at the overall design level - we shouldn't enforce that.
>
> You could have (not specifically on RKISP)
>
> Frame 1 Frame 2 Frame 3 Frame 4
>
> prepare(1)
> prepare(2)
> process(1)
> prepare(3)
Looking at the RkISP1 AWB:
Exactly, here in prepare(3) we store in the FrameContext(3) the automatic gains
computed on process(1). I guess that's ok but
> process(2)
> process(3)
> prepare(4)
here in prepare(4) we'll use the ones computed in process(3)
So there's no actually guarantee of which frames the auto gains we
store in the FrameContext() have been computed on. I guess that's not
much relevant for automatic modes ?
> process(4)
>
>
>
>
>
> > >
> > >
> > > Is prepare() called before the sensor starts in the inline-ISP case with
> > > RKISP?
>
> That part I don't know yet. But even if prepare() is called just before
> the sensor starts for a given frame - it's absolutely not currently guaranteed that
> the values applied to the sensor match the ActiveState values. So
> putting those values in the frameContext is misleading.
>
> In theory we 'could' identify what is applied on the sensor and make up
> the difference with an ISP digital gain to 'speed' up the convergence ?
> But either way - we 'must' track what the sensor *actually* gets
> configured with here in the FrameContext.
>
> I think all that can be on top of these patches though - as long as we
> highlight the issue here.
>
Absolutely, nothing blocking the series here, it's already a move in
the right direction and if we have to improve we should be doing that
on top.
> > >
> > >
> > > > 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 ? :)
>
> Because the current implementations as I understand it mean that we are
> calling setControls for a given frame, and those controls will then be
> 'applied' in 2 or 3 frames time. So ... not actaully the frame
> referenced by the FrameContext.
How so ? Don't we assume we operate early enough to have the here
computed values are applied in time to have them realized at the right
frame ?
>
>
> > > 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