[libcamera-devel] [PATCH v2] ipa: ipu3: Add a IPAFrameContext queue
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Fri Dec 17 10:03:42 CET 2021
Hi Umang,
Thanks for the patch !
On 14/12/2021 19:43, Umang Jain wrote:
> Having a single IPAFrameContext queue is limiting especially when
> we need to preserve per-frame controls. Right now, we are not processing
> any controls on the IPA side (processControls()) but sooner or later
> we need to preserve the controls setting for the frames in a sequential
> fashion. Since IPAIPU3::processControls() is executed on
> IPU3CameraData::queuePendingRequests() code path, we need to store the
> incoming control setting in a separate IPAFrameContext and push that
> into the queue.
>
> The controls for the next frame to be processed by the IPA are retrieved
> back from the queue in parseStatistics() and set via setControls().
> Meanwhile, the last IPAFrameContext is preserved as prevFrameContext to
> populate the metadata (for ActionMetadataReady).
>
In order to ease the flow reading, maybe could we pick this patch ?
https://patchwork.libcamera.org/patch/14484/
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> Changes in v2:
> - Fix over-exposed image stream issue explained in v1.
> The issue was non-initialized data being used in the frameContext.
>
> ---
> src/ipa/ipu3/algorithms/agc.cpp | 18 ++++++-------
> src/ipa/ipu3/algorithms/agc.h | 2 +-
> src/ipa/ipu3/algorithms/awb.cpp | 18 +++++++------
> src/ipa/ipu3/algorithms/tone_mapping.cpp | 11 ++++----
> src/ipa/ipu3/ipa_context.cpp | 32 +++++++++++++++++-----
> src/ipa/ipu3/ipa_context.h | 11 +++++++-
> src/ipa/ipu3/ipu3.cpp | 34 +++++++++++++++++++-----
> 7 files changed, 89 insertions(+), 37 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 8d6f18f6..b57ca215 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -99,8 +99,9 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain);
>
> /* Configure the default exposure and gain. */
> - context.frameContext.agc.gain = minAnalogueGain_;
> - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;
> + IPAFrameContext &frameContext = context.frameContextQueue.front();
> + frameContext.agc.gain = minAnalogueGain_;
> + frameContext.agc.exposure = minShutterSpeed_ / lineDuration_;
>
> return 0;
> }
> @@ -178,12 +179,12 @@ void Agc::filterExposure()
> * \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(IPAFrameContext &frameContext, double yGain,
> - double iqMeanGain)
> +void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)
> {
You need to change the associated doc ;-).
> /* Get the effective exposure and gain applied on the sensor. */
> - uint32_t exposure = frameContext.sensor.exposure;
> - double analogueGain = frameContext.sensor.gain;
> + uint32_t exposure = context.prevFrameContext.sensor.exposure;
> + double analogueGain = context.prevFrameContext.sensor.gain;
> + IPAFrameContext &frameContext = context.frameContextQueue.front();
I would move this line juste before the assignement at the end of the
function ?
>
> /* Use the highest of the two gain estimates. */
> double evGain = std::max(yGain, iqMeanGain);
> @@ -333,9 +334,8 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> */
> double yGain = 1.0;
> double yTarget = kRelativeLuminanceTarget;
> -
> for (unsigned int i = 0; i < 8; i++) {
> - double yValue = estimateLuminance(context.frameContext,
> + double yValue = estimateLuminance(context.prevFrameContext,
> context.configuration.grid.bdsGrid,
> stats, yGain);
> double extraGain = std::min(10.0, yTarget / (yValue + .001));
> @@ -348,7 +348,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> break;
> }
>
> - computeExposure(context.frameContext, yGain, iqMeanGain);
> + computeExposure(context, yGain, iqMeanGain);
> frameCount_++;
> }
>
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 96ec7005..1b444b12 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -34,7 +34,7 @@ private:
> double measureBrightness(const ipu3_uapi_stats_3a *stats,
> const ipu3_uapi_grid_config &grid) const;
> void filterExposure();
> - void computeExposure(IPAFrameContext &frameContext, double yGain,
> + void computeExposure(IPAContext &context, double yGain,
> double iqMeanGain);
> double estimateLuminance(IPAFrameContext &frameContext,
> const ipu3_uapi_grid_config &grid,
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 1dc27fc9..3c004928 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -382,16 +382,17 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> {
> calculateWBGains(stats);
> + IPAFrameContext &frameContext = context.frameContextQueue.front();
>
> /*
> * Gains are only recalculated if enough zones were detected.
> * The results are cached, so if no results were calculated, we set the
> * cached values from asyncResults_ here.
> */
> - context.frameContext.awb.gains.blue = asyncResults_.blueGain;
> - context.frameContext.awb.gains.green = asyncResults_.greenGain;
> - context.frameContext.awb.gains.red = asyncResults_.redGain;
> - context.frameContext.awb.temperatureK = asyncResults_.temperatureK;
> + frameContext.awb.gains.blue = asyncResults_.blueGain;
> + frameContext.awb.gains.green = asyncResults_.greenGain;
> + frameContext.awb.gains.red = asyncResults_.redGain;
> + frameContext.awb.temperatureK = asyncResults_.temperatureK;
> }
>
> constexpr uint16_t Awb::threshold(float value)
> @@ -434,6 +435,7 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
> */
> params->acc_param.bnr = imguCssBnrDefaults;
> Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;
> + IPAFrameContext &frameContext = context.frameContextQueue.front();
> params->acc_param.bnr.column_size = bdsOutputSize.width;
> params->acc_param.bnr.opt_center.x_reset = grid.x_start - (bdsOutputSize.width / 2);
> params->acc_param.bnr.opt_center.y_reset = grid.y_start - (bdsOutputSize.height / 2);
> @@ -442,10 +444,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
> params->acc_param.bnr.opt_center_sqr.y_sqr_reset = params->acc_param.bnr.opt_center.y_reset
> * params->acc_param.bnr.opt_center.y_reset;
> /* Convert to u3.13 fixed point values */
> - params->acc_param.bnr.wb_gains.gr = 8192 * context.frameContext.awb.gains.green;
> - params->acc_param.bnr.wb_gains.r = 8192 * context.frameContext.awb.gains.red;
> - params->acc_param.bnr.wb_gains.b = 8192 * context.frameContext.awb.gains.blue;
> - params->acc_param.bnr.wb_gains.gb = 8192 * context.frameContext.awb.gains.green;
> + params->acc_param.bnr.wb_gains.gr = 8192 * frameContext.awb.gains.green;
> + params->acc_param.bnr.wb_gains.r = 8192 * frameContext.awb.gains.red;
> + params->acc_param.bnr.wb_gains.b = 8192 * frameContext.awb.gains.blue;
> + params->acc_param.bnr.wb_gains.gb = 8192 * frameContext.awb.gains.green;
>
> LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
>
> diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> index 2040eda5..bf710bd1 100644
> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> @@ -42,7 +42,7 @@ int ToneMapping::configure(IPAContext &context,
> [[maybe_unused]] const IPAConfigInfo &configInfo)
> {
> /* Initialise tone mapping gamma value. */
> - context.frameContext.toneMapping.gamma = 0.0;
> + context.frameContextQueue.front().toneMapping.gamma = 0.0;
>
> return 0;
> }
> @@ -60,7 +60,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
> {
> /* Copy the calculated LUT into the parameters buffer. */
> memcpy(params->acc_param.gamma.gc_lut.lut,
> - context.frameContext.toneMapping.gammaCorrection.lut,
> + context.frameContextQueue.front().toneMapping.gammaCorrection.lut,
> IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
> sizeof(params->acc_param.gamma.gc_lut.lut[0]));
>
> @@ -80,6 +80,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
> void ToneMapping::process(IPAContext &context,
> [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> {
> + IPAFrameContext &frameContext = context.frameContextQueue.front();
> /*
> * Hardcode gamma to 1.1 as a default for now.
> *
> @@ -87,11 +88,11 @@ void ToneMapping::process(IPAContext &context,
> */
> gamma_ = 1.1;
>
> - if (context.frameContext.toneMapping.gamma == gamma_)
> + if (frameContext.toneMapping.gamma == gamma_)
> return;
>
> struct ipu3_uapi_gamma_corr_lut &lut =
> - context.frameContext.toneMapping.gammaCorrection;
> + frameContext.toneMapping.gammaCorrection;
>
> for (uint32_t i = 0; i < std::size(lut.lut); i++) {
> double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
> @@ -101,7 +102,7 @@ void ToneMapping::process(IPAContext &context,
> lut.lut[i] = gamma * 8191;
> }
>
> - context.frameContext.toneMapping.gamma = gamma_;
> + frameContext.toneMapping.gamma = gamma_;
> }
>
> } /* namespace ipa::ipu3::algorithms */
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 86794ac1..f503b93d 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -39,6 +39,23 @@ namespace libcamera::ipa::ipu3 {
> * algorithm, but should only be written by its owner.
> */
>
> +/**
> + * \brief Construct a IPAFrameContext instance
> + */
> +IPAFrameContext::IPAFrameContext() = default;
> +
> +/**
> + * \brief Move constructor for IPAFrameContext
> + * \param[in] other The other IPAFrameContext
> + */
> +IPAFrameContext::IPAFrameContext(IPAFrameContext &&other) = default;
> +
> +/**
> + * \brief Move assignment operator for IPAFrameContext
> + * \param[in] other The other IPAFrameContext
> + */
> +IPAFrameContext &IPAFrameContext::operator=(IPAFrameContext &&other) = default;
> +
> /**
> * \struct IPAContext
> * \brief Global IPA context data shared between all algorithms
> @@ -46,13 +63,11 @@ 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::frameContextQueue
> + * \brief A queue of frame contexts to be processed by the IPA
> *
> - * \todo While the frame context is supposed to be per-frame, this
> - * single frame context stores data related to both the current frame
> - * and the previous frames, with fields being updated as the algorithms
> - * are run. This needs to be turned into real per-frame data storage.
> + * \var IPAContext::prevFrameContext
> + * \brief The latest frame context which the IPA has finished processing
> */
>
> /**
> @@ -86,6 +101,11 @@ namespace libcamera::ipa::ipu3 {
> * \brief Maximum analogue gain supported with the configured sensor
> */
>
> +/**
> + * \var IPAFrameContext::frame
> + * \brief Frame number of the corresponding frame context
> + */
> +
> /**
> * \var IPAFrameContext::agc
> * \brief Context for the Automatic Gain Control algorithm
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index c6dc0814..a5e298bd 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -14,6 +14,8 @@
>
> #include <libcamera/geometry.h>
>
> +#include <queue>
> +
> namespace libcamera {
>
> namespace ipa::ipu3 {
> @@ -34,6 +36,12 @@ struct IPASessionConfiguration {
> };
>
> struct IPAFrameContext {
> + uint32_t frame;
> +
> + IPAFrameContext();
> + IPAFrameContext(IPAFrameContext &&other);
> + IPAFrameContext &operator=(IPAFrameContext &&other);
> +
> struct {
> uint32_t exposure;
> double gain;
> @@ -62,7 +70,8 @@ struct IPAFrameContext {
>
> struct IPAContext {
> IPASessionConfiguration configuration;
> - IPAFrameContext frameContext;
> + std::queue<IPAFrameContext> frameContextQueue;
> + IPAFrameContext prevFrameContext;
> };
>
> } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 3d307708..763dbd7d 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -324,6 +324,8 @@ int IPAIPU3::start()
> */
> void IPAIPU3::stop()
> {
> + while (!context_.frameContextQueue.empty())
> + context_.frameContextQueue.pop();
> }
>
> /**
> @@ -457,6 +459,14 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> /* Clean context at configuration */
> context_ = {};
>
> + /*
> + * Insert a initial context into the queue to faciliate
> + * algo->configure() below.
> + */
> + IPAFrameContext initContext;
> + initContext.frame = 0;
> + context_.frameContextQueue.push(std::move(initContext));
> +
> calculateBdsGrid(configInfo.bdsOutputSize);
>
> lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
> @@ -547,8 +557,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>());
> + IPAFrameContext &curFrameContext = context_.frameContextQueue.front();
Should we test if frameContextQueue is empty ? As an assert probably as
it should never happen ?
> + curFrameContext.sensor.exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> + curFrameContext.sensor.gain = camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>
> parseStatistics(event.frame, event.frameTimestamp, stats);
> break;
> @@ -571,6 +582,11 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
> [[maybe_unused]] const ControlList &controls)
> {
> /* \todo Start processing for 'frame' based on 'controls'. */
> +
> + IPAFrameContext newContext;
> + newContext.frame = frame;
> +
> + context_.frameContextQueue.push(std::move(newContext));
That's why the patch proposed to mark the beginning and and of a frame
could make sense. You we create and push it with the frame number in
frameStarted and pop it in frameCompleted ?
> }
>
> /**
> @@ -619,6 +635,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> {
> ControlList ctrls(controls::controls);
>
> + context_.prevFrameContext = std::move(context_.frameContextQueue.front());
> + context_.frameContextQueue.pop();
Do we want to pop it here or after the parseStatistics call ?
> +
> for (auto const &algo : algorithms_)
> algo->process(context_, stats);
>
> @@ -628,11 +647,11 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>();
> ctrls.set(controls::FrameDuration, frameDuration);
>
> - ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
> + ctrls.set(controls::AnalogueGain, context_.prevFrameContext.sensor.gain);
>
> - ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);
> + ctrls.set(controls::ColourTemperature, context_.prevFrameContext.awb.temperatureK);
>
> - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>());
> + ctrls.set(controls::ExposureTime, context_.prevFrameContext.sensor.exposure * lineDuration_.get<std::micro>());
>
> /*
> * \todo The Metadata provides a path to getting extended data
> @@ -661,8 +680,9 @@ void IPAIPU3::setControls(unsigned int frame)
> IPU3Action op;
> op.op = ActionSetSensorControls;
>
> - exposure_ = context_.frameContext.agc.exposure;
> - gain_ = camHelper_->gainCode(context_.frameContext.agc.gain);
> + IPAFrameContext &context = context_.frameContextQueue.front();
> + exposure_ = context.agc.exposure;
> + gain_ = camHelper_->gainCode(context.agc.gain);
>
> ControlList ctrls(ctrls_);
> ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>
More information about the libcamera-devel
mailing list