[libcamera-devel] [PATCH v3 3/3] ipa: ipu3: Put IPAFrameContext(s) in a ring buffer
Umang Jain
umang.jain at ideasonboard.com
Wed May 18 11:39:11 CEST 2022
Hi JM,
On 5/18/22 11:08, Jean-Michel Hautbois wrote:
> Hi Umang,
>
> On 17/05/2022 21:18, Umang Jain via libcamera-devel wrote:
>> Instead of having one frame context constantly being updated,
>> this patch aims to introduce per-frame IPAFrameContext which
>> are stored in a ring buffer. Whenever a request is queued, a new
>> IPAFrameContext is created and inserted into the ring buffer.
>>
>> The IPAFrameContext structure itself has been slightly extended
>> to store a frame id and a ControlList for incoming frame
>> controls (sent in by the application). The next step would be to
>> read and set these controls whenever the request is actually queued
>> to the hardware.
>>
>> Since now we are working in multiples of IPAFrameContext, the
>> Algorithm::process() will actually take in a IPAFrameContext pointer
>> (as opposed to a nullptr while preparing for this change).
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> src/ipa/ipu3/algorithms/agc.cpp | 11 +++++------
>> src/ipa/ipu3/algorithms/agc.h | 4 ++--
>> src/ipa/ipu3/ipa_context.cpp | 24 ++++++++++++++++++++++--
>> src/ipa/ipu3/ipa_context.h | 14 +++++++++++++-
>> src/ipa/ipu3/ipu3.cpp | 22 +++++++++++++---------
>> 5 files changed, 55 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp
>> b/src/ipa/ipu3/algorithms/agc.cpp
>> index 383a8deb..f16be534 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -183,14 +183,13 @@ utils::Duration
>> Agc::filterExposure(utils::Duration exposureValue)
>> * \param[in] yGain The gain calculated based on the relative
>> luminance target
>> * \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, IPAFrameContext
>> *frameContext,
>> + double yGain, double iqMeanGain)
>> {
>> const IPASessionConfiguration &configuration =
>> context.configuration;
>> - IPAFrameContext &frameContext = context.frameContext;
>> /* Get the effective exposure and gain applied on the sensor. */
>> - uint32_t exposure = frameContext.sensor.exposure;
>> - double analogueGain = frameContext.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);
>> @@ -360,7 +359,7 @@ void Agc::process(IPAContext &context,
>> [[maybe_unused]] IPAFrameContext *frameCo
>> break;
>> }
>> - computeExposure(context, yGain, iqMeanGain);
>> + computeExposure(context, frameContext, yGain, iqMeanGain);
>> frameCount_++;
>> }
>> diff --git a/src/ipa/ipu3/algorithms/agc.h
>> b/src/ipa/ipu3/algorithms/agc.h
>> index 219a1a96..105ae0f2 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -35,8 +35,8 @@ private:
>> double measureBrightness(const ipu3_uapi_stats_3a *stats,
>> const ipu3_uapi_grid_config &grid) const;
>> utils::Duration filterExposure(utils::Duration currentExposure);
>> - void computeExposure(IPAContext &context, double yGain,
>> - double iqMeanGain);
>> + void computeExposure(IPAContext &context, IPAFrameContext
>> *frameContext,
>> + double yGain, double iqMeanGain);
>> double estimateLuminance(IPAActiveState &activeState,
>> const ipu3_uapi_grid_config &grid,
>> const ipu3_uapi_stats_3a *stats,
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 06eb2776..81b30600 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -58,8 +58,8 @@ namespace libcamera::ipa::ipu3 {
>> * \var IPAContext::configuration
>> * \brief The IPA session configuration, immutable during the session
>> *
>> - * \var IPAContext::frameContext
>> - * \brief The frame context for the frame being processed
>> + * \var IPAContext::frameContexts
>> + * \brief Ring buffer of the IPAFrameContext(s)
>> *
>> * \var IPAContext::activeState
>> * \brief The current state of IPA algorithms
>> @@ -183,6 +183,26 @@ namespace libcamera::ipa::ipu3 {
>> */
>> /**
>> + * \brief Default constructor for IPAFrameContext
>> + */
>> +IPAFrameContext::IPAFrameContext() = default;
>> +
>> +/**
>> + * \brief Construct a IPAFrameContext instance
>> + */
>> +IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList
>> &reqControls)
>> + : frame(id), frameControls(reqControls)
>> +{
>> + sensor = {};
>> +}
>> +
>> +/**
>> + * \var IPAFrameContext::frame
>> + * \brief The frame number
>> + *
>> + * \var IPAFrameContext::frameControls
>> + * \brief Controls sent in by the application while queuing the request
>> + *
>> * \var IPAFrameContext::sensor
>> * \brief Effective sensor values that were applied for the frame
>> *
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 8d681131..42e11141 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -8,16 +8,22 @@
>> #pragma once
>> +#include <array>
>> +
>> #include <linux/intel-ipu3.h>
>> #include <libcamera/base/utils.h>
>> +#include <libcamera/controls.h>
>> #include <libcamera/geometry.h>
>> namespace libcamera {
>> namespace ipa::ipu3 {
>> +/* Maximum number of frame contexts to be held */
>> +static constexpr uint32_t kMaxFrameContexts = 16;
>> +
>> struct IPASessionConfiguration {
>> struct {
>> ipu3_uapi_grid_config bdsGrid;
>> @@ -71,17 +77,23 @@ struct IPAActiveState {
>> };
>> struct IPAFrameContext {
>> + IPAFrameContext();
>> + IPAFrameContext(uint32_t id, const ControlList &reqControls);
>> +
>> struct {
>> uint32_t exposure;
>> double gain;
>> } sensor;
>> +
>> + uint32_t frame;
>> + ControlList frameControls;
>> };
>> struct IPAContext {
>> IPASessionConfiguration configuration;
>> IPAActiveState activeState;
>> - IPAFrameContext frameContext;
>> + std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>> };
>> } /* namespace ipa::ipu3 */
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 16e5028f..4322f96b 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -313,7 +313,7 @@ int IPAIPU3::init(const IPASettings &settings,
>> }
>> /* Clean context */
>> - context_ = {};
>> + context_.configuration = {};
>> context_.configuration.sensor.lineDuration =
>> sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
>> /* Construct our Algorithms */
>> @@ -456,7 +456,8 @@ int IPAIPU3::configure(const IPAConfigInfo
>> &configInfo,
>> /* Clean IPAActiveState at each reconfiguration. */
>> context_.activeState = {};
>> - context_.frameContext = {};
>> + IPAFrameContext initFrameContext;
>> + context_.frameContexts.fill(initFrameContext);
>> if (!validateSensorControls()) {
>> LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>> @@ -568,15 +569,17 @@ void IPAIPU3::processStatsBuffer(const uint32_t
>> frame,
>> const ipu3_uapi_stats_3a *stats =
>> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>> - context_.frameContext.sensor.exposure =
>> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> - context_.frameContext.sensor.gain =
>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> + IPAFrameContext &frameContext = context_.frameContexts[frame %
>> kMaxFrameContexts];
>
> Shouldn't the frame be "double-checked", verifying that
> (frameContext->frame == frame) before passing it to the algorithms
> somehow ?
Well, we can surely verify that but we know in advance that the
verification is subjected to failure with known cases (for e.g. frame
drop) so I am not very keen putting the verification in. If you had seen
the v2, I had a ASSERT similar to this basically (and has some
discussion on this there)
>
> Apart from that:
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
Thanks for reviewing!
>
>> +
>> + frameContext.sensor.exposure =
>> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> + frameContext.sensor.gain =
>> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> double lineDuration =
>> context_.configuration.sensor.lineDuration.get<std::micro>();
>> int32_t vBlank = context_.configuration.sensor.defVBlank;
>> ControlList ctrls(controls::controls);
>> for (auto const &algo : algorithms_)
>> - algo->process(context_, nullptr, stats);
>> + algo->process(context_, &frameContext, stats);
>> setControls(frame);
>> @@ -584,11 +587,11 @@ void IPAIPU3::processStatsBuffer(const
>> uint32_t frame,
>> int64_t frameDuration = (vBlank +
>> sensorInfo_.outputSize.height) * lineDuration;
>> ctrls.set(controls::FrameDuration, frameDuration);
>> - ctrls.set(controls::AnalogueGain,
>> context_.frameContext.sensor.gain);
>> + ctrls.set(controls::AnalogueGain, frameContext.sensor.gain);
>> ctrls.set(controls::ColourTemperature,
>> context_.activeState.awb.temperatureK);
>> - ctrls.set(controls::ExposureTime,
>> context_.frameContext.sensor.exposure * lineDuration);
>> + ctrls.set(controls::ExposureTime, frameContext.sensor.exposure *
>> lineDuration);
>> /*
>> * \todo The Metadata provides a path to getting extended data
>> @@ -609,10 +612,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t
>> frame,
>> * Parse the request to handle any IPA-managed controls that were
>> set from the
>> * application such as manual sensor settings.
>> */
>> -void IPAIPU3::queueRequest([[maybe_unused]] const uint32_t frame,
>> - [[maybe_unused]] const ControlList &controls)
>> +void IPAIPU3::queueRequest(const uint32_t frame, const ControlList
>> &controls)
>> {
>> /* \todo Start processing for 'frame' based on 'controls'. */
>> + IPAFrameContext newFrameContext(frame, controls);
>> + context_.frameContexts[frame % kMaxFrameContexts] =
>> newFrameContext;
>> }
>> /**
More information about the libcamera-devel
mailing list