[libcamera-devel] [PATCH v3 03/14] ipa: ipu3: Use sensor controls to update frameContext

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-11 11:05:54)
> The pipeline handler populates the new sensorControls ControlList, to
> have the effective exposure and gain values for the current frame. This
> is done when a statistics buffer is received.
> 
> Make those values the frameContext::sensor values for the frame when the
> EventStatReady event is received.
> 
> AGC also needs to use frameContext.sensor as its input values and
> frameContext.agc as its output values. Modify computeExposure by passing
> it the frameContext instead of individual exposure and gain values.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 19 ++++++++++---------
>  src/ipa/ipu3/algorithms/agc.h   |  2 +-
>  src/ipa/ipu3/ipa_context.cpp    | 11 +++++++++++
>  src/ipa/ipu3/ipa_context.h      |  5 +++++
>  src/ipa/ipu3/ipu3.cpp           |  4 ++++
>  5 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index b5d736c1..825b35d4 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -169,10 +169,9 @@ void Agc::filterExposure()
>  
>  /**
>   * \brief Estimate the new exposure and gain values
> - * \param[inout] exposure The exposure value reference as a number of lines
> - * \param[inout] gain The gain reference to be updated
> + * \param[inout] frameContext The shared IPA frame Context
>   */
> -void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
> +void Agc::computeExposure(IPAFrameContext &frameContext)
>  {
>         /* Algorithm initialization should wait for first valid frames */
>         /* \todo - have a number of frames given by DelayedControls ?
> @@ -189,6 +188,10 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>                 return;
>         }
>  
> +       /* Get the effective exposure and gain applied on the sensor. */
> +       uint32_t &exposure = frameContext.sensor.exposure;
> +       double &analogueGain = frameContext.sensor.gain;

I don't think these need to be references now, but it doesn't
particularly matter.

Perhaps removing the & would make it clear that the
frameContext.sensor.exposure and frameContext.sensor.gain isn't going to
be modified in this function (which the & could otherwise imply).


> +
>         /* Estimate the gain needed to have the proportion wanted */
>         double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
> @@ -233,8 +236,9 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>                             << shutterTime << " and "
>                             << stepGain;
>  
> -       exposure = shutterTime / lineDuration_;
> -       analogueGain = stepGain;
> +       /* Update the estimated exposure and gain. */
> +       frameContext.agc.exposure = shutterTime / lineDuration_;
> +       frameContext.agc.gain = stepGain;
>  
>         /*
>          * Update the exposure value for the next process call.
> @@ -257,11 +261,8 @@ void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>   */
>  void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  {
> -       /* Get the latest exposure and gain applied */
> -       uint32_t &exposure = context.frameContext.agc.exposure;
> -       double &analogueGain = context.frameContext.agc.gain;
>         measureBrightness(stats, context.configuration.grid.bdsGrid);
> -       computeExposure(exposure, analogueGain);
> +       computeExposure(context.frameContext);

That's a lot cleaner...

>         frameCount_++;
>  }
>  
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 69e0b831..f0db25ee 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -34,7 +34,7 @@ private:
>         void measureBrightness(const ipu3_uapi_stats_3a *stats,
>                                const ipu3_uapi_grid_config &grid);
>         void filterExposure();
> -       void computeExposure(uint32_t &exposure, double &gain);
> +       void computeExposure(IPAFrameContext &frameContext);
>  
>         uint64_t frameCount_;
>         uint64_t lastFrame_;
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 2355a9c7..a7ff957d 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -119,6 +119,17 @@ namespace libcamera::ipa::ipu3 {
>   * \brief White balance gain for B channel
>   */
>  
> +/**
> + * \var IPAFrameContext::sensor
> + * \brief Effective sensor values
> + *
> + * \var IPAFrameContext::sensor.exposure
> + * \brief Exposure time expressed as a number of lines
> + *
> + * \var IPAFrameContext::sensor.gain
> + * \brief Analogue gain multiplier
> + */
> +
>  /**
>   * \var IPAFrameContext::toneMapping
>   * \brief Context for ToneMapping and Gamma control
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 1e46c61a..a5a19800 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -47,6 +47,11 @@ struct IPAFrameContext {
>                 } gains;
>         } awb;
>  
> +       struct {
> +               uint32_t exposure;
> +               double gain;
> +       } sensor;
> +
>         struct {
>                 double gamma;
>                 struct ipu3_uapi_gamma_corr_lut gammaCorrection;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index bcc3863b..dee002a5 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -549,6 +549,10 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>                 const ipu3_uapi_stats_3a *stats =
>                         reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>  
> +               /* \todo move those into processControls ? */

I still haven't understood why we want to move these into
processControls.

I think that is supposed to deal with processing controls from the
Request?

For everything else


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +               context_.frameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +               context_.frameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +
>                 parseStatistics(event.frame, event.frameTimestamp, stats);
>                 break;
>         }
> -- 
> 2.32.0
>