[libcamera-devel] [PATCH v3 02/17] ipa: rkisp1: Rename frameContext to activeState
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Aug 24 11:27:02 CEST 2022
Quoting Laurent Pinchart (2022-08-24 01:37:02)
> On Thu, Aug 18, 2022 at 05:07:27PM +0100, Kieran Bingham via libcamera-devel wrote:
> > Quoting Jacopo Mondi via libcamera-devel (2022-08-18 10:43:55)
> > > From: Kieran Bingham via libcamera-devel <libcamera-devel at lists.libcamera.org>
> > >
> > > Move the existing frame context structure to the IPAActiveState.
> > > This structure should store the most up to date results for the IPA
> > > which may be shared to other algorithms that operate on the data.
> >
> > Laurent queried this patch in v2.
> >
> > I suggested we add the following to this commit message:
> >
> > """
> > All existing algorithm state is moved to the ActiveState and the
> > algorithms should be updated individually to use the FrameContext
> > directly where appropriate.
> > """
>
> I'd like to make it clear (partly to check that my understanding is
> correct :-)) that this commit is about moving the existing frame context
> aside to make space for the new one. How about
>
> --------
> The RkISP1 IPA module creates a single instance of its IPAFrameContext
> structure, effectively using it more as an active state than a per-frame
> context. To prepare for the introduction of a real per-frame context,
> move all the members of the IPAFrameContext structure to a new
> IPAActiveState structure. The IPAFrameContext becomes effectively
> unused at runtime, but can't be removed yet as it is still required as a
> template argument to the Module and Algorithm classes.
>
> The new IPAActiveState structure is not foreseen to stay, its members
> should move to either the new per-frame context that will be introduced,
> or to the individual algorithm classes.
Not foreseen might be too strong. I keep saying, I believe there may be
good cause for it ActiveState to exist. But until we get progress to
actually implement something real (blocked effectively by this series) I
can't quantify exactly what.
My expectations are that things like Modes (Is AWB enabled or in manual)
are going to be in a shared IPAActiveState - as these may need to be
retrieved or queried (or adjusted in the case of disabling a mode so
that AF can operate) based on an active state, not a frame context.
Otherwise, the first paragraph is fine with me.
> --------
>
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > src/ipa/rkisp1/algorithms/agc.cpp | 20 ++++++++--------
> > > src/ipa/rkisp1/algorithms/awb.cpp | 34 ++++++++++++++--------------
> > > src/ipa/rkisp1/algorithms/blc.cpp | 2 +-
> > > src/ipa/rkisp1/algorithms/cproc.cpp | 4 ++--
> > > src/ipa/rkisp1/algorithms/dpcc.cpp | 2 +-
> > > src/ipa/rkisp1/algorithms/filter.cpp | 4 ++--
> > > src/ipa/rkisp1/algorithms/gsl.cpp | 2 +-
> > > src/ipa/rkisp1/algorithms/lsc.cpp | 2 +-
> > > src/ipa/rkisp1/ipa_context.h | 7 ++++--
> > > src/ipa/rkisp1/rkisp1.cpp | 12 +++++-----
> > > 10 files changed, 46 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index a1bb7d972926..483e941fe427 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -73,8 +73,8 @@ Agc::Agc()
> > > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > {
> > > /* Configure the default exposure and gain. */
> > > - context.frameContext.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> > > - context.frameContext.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
> > > + context.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> > > + context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
> > >
> > > /*
> > > * According to the RkISP1 documentation:
> > > @@ -98,7 +98,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
> > > context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
> > >
> > > - /* \todo Use actual frame index by populating it in the frameContext. */
> > > + /* \todo Use actual frame index by populating it in the activeState. */
>
> That will go later in the series, but we could still keep it accurate in
> the meantime by writing
>
> /*
> * \todo Use the upcoming per-frame context API that will provide a
> * frame index
> */
>
> > > frameCount_ = 0;
> > > return 0;
> > > }
> > > @@ -140,18 +140,18 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue)
> > >
> > > /**
> > > * \brief Estimate the new exposure and gain values
> > > - * \param[inout] frameContext The shared IPA frame Context
> > > + * \param[inout] context The shared IPA Context
>
> And add to the commit message
>
> While at it, fix a typo in the documentation of the
> Agc::computeExposure() function that incorrectly refers to the frame
> context instead of the global context.
>
> > > * \param[in] yGain The gain calculated on the current brightness level
> > > * \param[in] iqMeanGain The gain calculated based on the relative luminance target
> > > */
> > > void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)
> > > {
> > > IPASessionConfiguration &configuration = context.configuration;
> > > - IPAFrameContext &frameContext = context.frameContext;
> > > + IPAActiveState &activeState = context.activeState;
> > >
> > > /* Get the effective exposure and gain applied on the sensor. */
> > > - uint32_t exposure = frameContext.sensor.exposure;
> > > - double analogueGain = frameContext.sensor.gain;
> > > + uint32_t exposure = activeState.sensor.exposure;
> > > + double analogueGain = activeState.sensor.gain;
> > >
> > > /* Use the highest of the two gain estimates. */
> > > double evGain = std::max(yGain, iqMeanGain);
> > > @@ -216,8 +216,8 @@ void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain)
> > > << stepGain;
> > >
> > > /* Update the estimated exposure and gain. */
> > > - frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration;
> > > - frameContext.agc.gain = stepGain;
> > > + activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration;
> > > + activeState.agc.gain = stepGain;
> > > }
> > >
> > > /**
> > > @@ -324,7 +324,7 @@ void Agc::process(IPAContext &context,
> > > */
> > > void Agc::prepare(IPAContext &context, rkisp1_params_cfg *params)
> > > {
> > > - if (context.frameContext.frameCount > 0)
> > > + if (context.activeState.frameCount > 0)
> > > return;
> > >
> > > /* Configure the measurement window. */
> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > index 9f00364d12b1..427aaeb1e955 100644
> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > @@ -35,9 +35,9 @@ LOG_DEFINE_CATEGORY(RkISP1Awb)
> > > int Awb::configure(IPAContext &context,
> > > const IPACameraSensorInfo &configInfo)
> > > {
> > > - context.frameContext.awb.gains.red = 1.0;
> > > - context.frameContext.awb.gains.blue = 1.0;
> > > - context.frameContext.awb.gains.green = 1.0;
> > > + context.activeState.awb.gains.red = 1.0;
> > > + context.activeState.awb.gains.blue = 1.0;
> > > + context.activeState.awb.gains.green = 1.0;
> > >
> > > /*
> > > * Define the measurement window for AWB as a centered rectangle
> > > @@ -72,16 +72,16 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)
> > > */
> > > void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params)
> > > {
> > > - params->others.awb_gain_config.gain_green_b = 256 * context.frameContext.awb.gains.green;
> > > - params->others.awb_gain_config.gain_blue = 256 * context.frameContext.awb.gains.blue;
> > > - params->others.awb_gain_config.gain_red = 256 * context.frameContext.awb.gains.red;
> > > - params->others.awb_gain_config.gain_green_r = 256 * context.frameContext.awb.gains.green;
> > > + params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green;
> > > + params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue;
> > > + params->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red;
> > > + params->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green;
> > >
> > > /* Update the gains. */
> > > params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN;
> > >
> > > /* If we already have configured the gains and window, return. */
> > > - if (context.frameContext.frameCount > 0)
> > > + if (context.activeState.frameCount > 0)
> > > return;
> > >
> > > /* Configure the gains to apply. */
> > > @@ -125,7 +125,7 @@ void Awb::process([[maybe_unused]] IPAContext &context,
> > > {
> > > const rkisp1_cif_isp_stat *params = &stats->params;
> > > const rkisp1_cif_isp_awb_stat *awb = ¶ms->awb;
> > > - IPAFrameContext &frameContext = context.frameContext;
> > > + IPAActiveState &activeState = context.activeState;
> > >
> > > /* Get the YCbCr mean values */
> > > double yMean = awb->awb_mean[0].mean_y_or_g;
> > > @@ -157,22 +157,22 @@ void Awb::process([[maybe_unused]] IPAContext &context,
> > >
> > > /* Filter the values to avoid oscillations. */
> > > double speed = 0.2;
> > > - redGain = speed * redGain + (1 - speed) * frameContext.awb.gains.red;
> > > - blueGain = speed * blueGain + (1 - speed) * frameContext.awb.gains.blue;
> > > + redGain = speed * redGain + (1 - speed) * activeState.awb.gains.red;
> > > + blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue;
> > >
> > > /*
> > > * Gain values are unsigned integer value, range 0 to 4 with 8 bit
> > > * fractional part.
> > > */
> > > - frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> > > - frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> > > + activeState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);
> > > + activeState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);
> > > /* Hardcode the green gain to 1.0. */
> > > - frameContext.awb.gains.green = 1.0;
> > > + activeState.awb.gains.green = 1.0;
> > >
> > > - frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
> > > + activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
> > >
> > > - LOG(RkISP1Awb, Debug) << "Gain found for red: " << context.frameContext.awb.gains.red
> > > - << " and for blue: " << context.frameContext.awb.gains.blue;
> > > + LOG(RkISP1Awb, Debug) << "Gain found for red: " << context.activeState.awb.gains.red
> > > + << " and for blue: " << context.activeState.awb.gains.blue;
> > > }
> > >
> > > REGISTER_IPA_ALGORITHM(Awb, "Awb")
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > index a58569fa2dc2..4d55a2d529cb 100644
> > > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > > @@ -68,7 +68,7 @@ int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,
> > > void BlackLevelCorrection::prepare(IPAContext &context,
> > > rkisp1_params_cfg *params)
> > > {
> > > - if (context.frameContext.frameCount > 0)
> > > + if (context.activeState.frameCount > 0)
> > > return;
> > >
> > > if (!tuningParameters_)
> > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> > > index bca5ab6907d6..a3b778d1c908 100644
> > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> > > @@ -40,7 +40,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
> > > [[maybe_unused]] const uint32_t frame,
> > > const ControlList &controls)
> > > {
> > > - auto &cproc = context.frameContext.cproc;
> > > + auto &cproc = context.activeState.cproc;
> > >
> > > const auto &brightness = controls.get(controls::Brightness);
> > > if (brightness) {
> > > @@ -73,7 +73,7 @@ void ColorProcessing::queueRequest(IPAContext &context,
> > > void ColorProcessing::prepare(IPAContext &context,
> > > rkisp1_params_cfg *params)
> > > {
> > > - auto &cproc = context.frameContext.cproc;
> > > + auto &cproc = context.activeState.cproc;
> > >
> > > /* Check if the algorithm configuration has been updated. */
> > > if (!cproc.updateParams)
> > > diff --git a/src/ipa/rkisp1/algorithms/dpcc.cpp b/src/ipa/rkisp1/algorithms/dpcc.cpp
> > > index 69bc651eaf08..93d37b1dae44 100644
> > > --- a/src/ipa/rkisp1/algorithms/dpcc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/dpcc.cpp
> > > @@ -234,7 +234,7 @@ int DefectPixelClusterCorrection::init([[maybe_unused]] IPAContext &context,
> > > void DefectPixelClusterCorrection::prepare(IPAContext &context,
> > > rkisp1_params_cfg *params)
> > > {
> > > - if (context.frameContext.frameCount > 0)
> > > + if (context.activeState.frameCount > 0)
> > > return;
> > >
> > > if (!initialized_)
> > > diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp
> > > index 8ca10fd1ee9d..bc7fc1f32f34 100644
> > > --- a/src/ipa/rkisp1/algorithms/filter.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/filter.cpp
> > > @@ -46,7 +46,7 @@ void Filter::queueRequest(IPAContext &context,
> > > [[maybe_unused]] const uint32_t frame,
> > > const ControlList &controls)
> > > {
> > > - auto &filter = context.frameContext.filter;
> > > + auto &filter = context.activeState.filter;
> > >
> > > const auto &sharpness = controls.get(controls::Sharpness);
> > > if (sharpness) {
> > > @@ -87,7 +87,7 @@ void Filter::queueRequest(IPAContext &context,
> > > */
> > > void Filter::prepare(IPAContext &context, rkisp1_params_cfg *params)
> > > {
> > > - auto &filter = context.frameContext.filter;
> > > + auto &filter = context.activeState.filter;
> > >
> > > /* Check if the algorithm configuration has been updated. */
> > > if (!filter.updateParams)
> > > diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp
> > > index 2fd1a23d3a9b..dd9974627cd2 100644
> > > --- a/src/ipa/rkisp1/algorithms/gsl.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/gsl.cpp
> > > @@ -121,7 +121,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,
> > > void GammaSensorLinearization::prepare(IPAContext &context,
> > > rkisp1_params_cfg *params)
> > > {
> > > - if (context.frameContext.frameCount > 0)
> > > + if (context.activeState.frameCount > 0)
> > > return;
> > >
> > > if (!initialized_)
> > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > > index 05c8c0dab5c8..4ed467086199 100644
> > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > > @@ -125,7 +125,7 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> > > void LensShadingCorrection::prepare(IPAContext &context,
> > > rkisp1_params_cfg *params)
> > > {
> > > - if (context.frameContext.frameCount > 0)
> > > + if (context.activeState.frameCount > 0)
> > > return;
> > >
> > > if (!initialized_)
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 2bdb6a81d7c9..a8daeca487ae 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -41,7 +41,7 @@ struct IPASessionConfiguration {
> > > } hw;
> > > };
> > >
> > > -struct IPAFrameContext {
> > > +struct IPAActiveState {
> > > struct {
> > > uint32_t exposure;
> > > double gain;
> > > @@ -78,9 +78,12 @@ struct IPAFrameContext {
> > > unsigned int frameCount;
> > > };
> > >
> > > +struct IPAFrameContext {
> > > +};
> > > +
> > > struct IPAContext {
> > > IPASessionConfiguration configuration;
> > > - IPAFrameContext frameContext;
> > > + IPAActiveState activeState;
> > > };
>
> The documentation in ipa_context.cpp needs to be updated too.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> This patch could go the very front of the series.
>
> > >
> > > } /* namespace ipa::rkisp1 */
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 17d42d38eb45..88e6a2b23eed 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -246,7 +246,7 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
> > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> > >
> > > - context_.frameContext.frameCount = 0;
> > > + context_.activeState.frameCount = 0;
> > >
> > > for (auto const &algo : algorithms()) {
> > > int ret = algo->configure(context_, info);
> > > @@ -306,7 +306,7 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> > > algo->prepare(context_, params);
> > >
> > > paramsBufferReady.emit(frame);
> > > - context_.frameContext.frameCount++;
> > > + context_.activeState.frameCount++;
> > > }
> > >
> > > void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
> > > @@ -316,9 +316,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> > > reinterpret_cast<rkisp1_stat_buffer *>(
> > > mappedBuffers_.at(bufferId).planes()[0].data());
> > >
> > > - context_.frameContext.sensor.exposure =
> > > + context_.activeState.sensor.exposure =
> > > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > > - context_.frameContext.sensor.gain =
> > > + context_.activeState.sensor.gain =
> > > camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> > >
> > > unsigned int aeState = 0;
> > > @@ -333,8 +333,8 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> > >
> > > void IPARkISP1::setControls(unsigned int frame)
> > > {
> > > - uint32_t exposure = context_.frameContext.agc.exposure;
> > > - uint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);
> > > + uint32_t exposure = context_.activeState.agc.exposure;
> > > + uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain);
> > >
> > > ControlList ctrls(ctrls_);
> > > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list