[libcamera-devel] [PATCH v4 19/32] ipa: rkisp1: agc: Store per-frame information in frame context
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 23 00:18:09 CEST 2022
Quoting Laurent Pinchart (2022-09-22 23:07:45)
> On Thu, Sep 22, 2022 at 01:10:18PM +0100, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2022-09-22 11:17:02)
> > > On Thu, Sep 22, 2022 at 09:59:23AM +0100, Kieran Bingham wrote:
> > > > Quoting Jacopo Mondi (2022-09-21 21:13:53)
> > > > > 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.
>
> I'll add a todo comment. Note that it doesn't matter that much if we
> receive what we've asked, as long as the values we get correspond to
> what has been applied to the frame. That's the important part to ensure
> the algorithm is stable. Synchronizing the values computed by the
> algorithm with a particular frame is only important in manual mode.
>
> > > > > > > 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;
> >
> > So perhaps what this should be setting is
> > 'frameContext.agc.desiredExposure'
> >
> > In otherwords, at the time of processing this image, the AGC ran and
> > determined it would prefer to have an exposure of X
> >
> > But currently this reads to me as...
> >
> > This frame's Exposure is X
> >
> > Which is completely wrong.
>
> The exposure and gain members are documented as
>
> * The exposure and gain are the latest values computed by the AGC algorithm.
>
> I don't want to call them desiredExposure and desiredGain, as we would
> then have to prefix pretty much everything with either "desired" or
> "computed".
>
> > > > > > > +
> > > > > >
> > > > > > 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..
> >
> > If we end up prepareing two frames for the ISP at 'the same time' ...
> > then yes, I would expect them to have the same values, as that is the
> > 'most up to date information we have at that time'.
>
> That matches my understanding.
>
> > > > 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 will " ... do this is fine, but I don't think we do right now. That
> > was my only highlight. I think this is fine for us to keep building on
> > top of though.
>
> That may well be the case. I think this patch brings us to a state where
> we can start testing the interaction with DelayedControls. I do expect
> it to hurt, but that's a required step in the healing process :-)
>
> > > > 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 ?
> >
> > Aha, I meant 'prepare' is called before the frame is processed by the
> > ISP, and process() is called after the frame is processed by the ISP...
> >
> > > > 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 ?
> >
> > For automatic - no, it shouldn't matter. As far as I'm aware, we go with
> > 'the most up to date information we have at the time we are preparing
> > that frame for the ISP to process' ... i.e. the current ActiveState.
>
> That's right, the latest values we have are the best, as they correspond
> to the latest statistics, and thus the newest available information
> about the scene.
>
> > > > 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.
> >
> > Ack.
> >
> > > > > > > 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'.
>
> Or rather in the process() function of the AGC, which is called just
> below.
>
> > > > > > >
> > > > > > > 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 ?
> >
> > No ... I don't think we do right now.
> >
> > Ohh - it's worse. It's all currently wrong ;-)
> >
> > IPARkISP1::processStatsBuffer(frame) {
> > ...
> > setControls(frame);
> > ...
> > }
> >
> >
> > So - we're calling 'setControls' which then gets the FrameContext
> > 'frame'.
> >
> > That is setting 'what the AGC' would like the sensor to be as soon as it
> > can be.
> >
> > But it shouldn't be parsing
> > FrameContext.agc.exposure
> >
> > To me - that is 'what the exposure *is* of the frame related to
> > Framecontext.
>
> That's stored in sensor.exposure (and sensor.gain).
>
> > If we extend this with a new entry (referenced at the top of this mail)
> > such as :
> >
> > FrameContext.agc.desiredExposure
> >
> > Or such - then I'd be happier with this in the short term. But I really
> > don't want to mix up "What the AGC would like" and "What the Frame
> > exposure is" in a single field of the frame context.
>
> That we agree on, but those are already stored in two separate fields
> :-)
Aha - perfect, I'd missed / forgotten about the sensor structure. Ok
then - move along, nothing to see here ... ;-)
--
Kieran
>
> > > > > > 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