[libcamera-devel] [PATCH v5 03/14] ipa: ipu3: Use sensor controls to update frameContext
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Tue Nov 16 08:52:02 CET 2021
Hi Laurent,
On 15/11/2021 16:36, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> 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 :-) ?
>
>> + *
>> + * \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;
>> }
>
More information about the libcamera-devel
mailing list