[libcamera-devel] [PATCH v5 03/14] ipa: ipu3: Use sensor controls to update frameContext
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 17 14:59:07 CET 2021
Hi Jean-Michel,
On Tue, Nov 16, 2021 at 08:52:02AM +0100, Jean-Michel Hautbois wrote:
> On 15/11/2021 16:36, Laurent Pinchart wrote:
> > On Sat, Nov 13, 2021 at 09:49:36AM +0100, Jean-Michel Hautbois wrote:
> >> 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>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> >> Reviewed-by: Paul Elder <paul.elder 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 | 3 +++
> >> 5 files changed, 30 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index b5d736c1..5723f6f3 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;
> >> +
> >> /* 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);
> >> 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
> >
> > I'd expand this a bit
> >
> > * \brief The sensor control values that were used to capture this frame
> >
> > (in a follow-up patch as the series has been merged)
> >
> > Although, looking at the code, this structure now stores both the
> > control values used to capture the frame (before calling
> > computeExposure()), and the values for the next frame (after calling
> > computeExposure()). It could be nice to split it in two, to make the
> > code and documentation clearer.
>
> Having a IPACurrentFrameContext and IPANextFrameContext ? Isn't having
> IPAFrameContext.sensor for input and IPAFrameContext.agc for output
> enough to split ? Or did I misunderstand you :-) ?
I've missed that the have two separate fields (agc and sensor) in the
frame context. Please ignore the second part of the comment. The
proposal to expand the brief still holds.
> >> + *
> >> + * \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..38e86e58 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -549,6 +549,9 @@ void IPAIPU3::processEvent(const IPU3Event &event)
> >> const ipu3_uapi_stats_3a *stats =
> >> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
> >>
> >> + 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>());
> >
> > Could you shorten the lines ? The second one is above our hard limit.
>
> I can but checkstyle isnt happy (I did, tbh)
>
> >> +
> >> parseStatistics(event.frame, event.frameTimestamp, stats);
> >> break;
> >> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list