[libcamera-devel] [PATCH v4 03/32] ipa: libipa: Pass a reference instead of pointer to Algorithm::process()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 20 21:06:26 CEST 2022
Hi Kieran,
On Tue, Sep 20, 2022 at 02:25:20PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:31)
> > Frame contexts will become the core component of IPA modules, always
> > available to functions of the algorithms. To indicate and prepare for
> > this, turn the frame context pointer passed to Algorithm::process() into
> > a reference.
>
> Excellent.
>
> > The RkISP1 IPA module doesn't use frame contexts yet, so pass a dummy
> > context for now.
> >
> > While at it, drop an unneeded [[maybe_unused]] from Agc::process().
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/ipa/ipu3/algorithms/af.cpp | 2 +-
> > src/ipa/ipu3/algorithms/af.h | 2 +-
> > src/ipa/ipu3/algorithms/agc.cpp | 8 ++++----
> > src/ipa/ipu3/algorithms/agc.h | 4 ++--
> > src/ipa/ipu3/algorithms/awb.cpp | 2 +-
> > src/ipa/ipu3/algorithms/awb.h | 2 +-
> > src/ipa/ipu3/algorithms/tone_mapping.cpp | 2 +-
> > src/ipa/ipu3/algorithms/tone_mapping.h | 2 +-
> > src/ipa/ipu3/ipu3.cpp | 2 +-
> > src/ipa/libipa/algorithm.h | 2 +-
> > src/ipa/rkisp1/algorithms/agc.cpp | 3 ++-
> > src/ipa/rkisp1/algorithms/agc.h | 2 +-
> > src/ipa/rkisp1/algorithms/awb.cpp | 2 +-
> > src/ipa/rkisp1/algorithms/awb.h | 2 +-
> > src/ipa/rkisp1/rkisp1.cpp | 5 ++++-
> > 15 files changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> > index 9127c24f1287..5ab64bf88313 100644
> > --- a/src/ipa/ipu3/algorithms/af.cpp
> > +++ b/src/ipa/ipu3/algorithms/af.cpp
> > @@ -417,7 +417,7 @@ bool Af::afIsOutOfFocus(IPAContext &context)
> > *
> > * [1] Hill Climbing Algorithm, https://en.wikipedia.org/wiki/Hill_climbing
> > */
> > -void Af::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext,
> > +void Af::process(IPAContext &context, [[maybe_unused]] IPAFrameContext &frameContext,
> > const ipu3_uapi_stats_3a *stats)
> > {
> > /* Evaluate the AF buffer length */
> > diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> > index 9b93594898bb..29117e8bdd0d 100644
> > --- a/src/ipa/ipu3/algorithms/af.h
> > +++ b/src/ipa/ipu3/algorithms/af.h
> > @@ -32,7 +32,7 @@ public:
> >
> > void prepare(IPAContext &context, ipu3_uapi_params *params) override;
> > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > - void process(IPAContext &context, IPAFrameContext *frameContext,
> > + void process(IPAContext &context, IPAFrameContext &frameContext,
> > const ipu3_uapi_stats_3a *stats) override;
> >
> > private:
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index ed4809d98007..f8ca29640a44 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -183,13 +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, IPAFrameContext *frameContext,
> > +void Agc::computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > double yGain, double iqMeanGain)
> > {
> > const IPASessionConfiguration &configuration = context.configuration;
> > /* 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);
> > @@ -323,7 +323,7 @@ double Agc::estimateLuminance(IPAActiveState &activeState,
> > * Identify the current image brightness, and use that to estimate the optimal
> > * new exposure and gain for the scene.
> > */
> > -void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext,
> > +void Agc::process(IPAContext &context, IPAFrameContext &frameContext,
> > const ipu3_uapi_stats_3a *stats)
> > {
> > /*
> > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> > index 105ae0f2aac6..876fbbb6b585 100644
> > --- a/src/ipa/ipu3/algorithms/agc.h
> > +++ b/src/ipa/ipu3/algorithms/agc.h
> > @@ -28,14 +28,14 @@ public:
> > ~Agc() = default;
> >
> > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > - void process(IPAContext &context, IPAFrameContext *frameContext,
> > + void process(IPAContext &context, IPAFrameContext &frameContext,
> > const ipu3_uapi_stats_3a *stats) override;
> >
> > 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, IPAFrameContext *frameContext,
> > + void computeExposure(IPAContext &context, IPAFrameContext &frameContext,
> > double yGain, double iqMeanGain);
> > double estimateLuminance(IPAActiveState &activeState,
> > const ipu3_uapi_grid_config &grid,
> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > index b658ee546b87..128f5d9d5351 100644
> > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > @@ -387,7 +387,7 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> > /**
> > * \copydoc libcamera::ipa::Algorithm::process
> > */
> > -void Awb::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext,
> > +void Awb::process(IPAContext &context, [[maybe_unused]] IPAFrameContext &frameContext,
> > const ipu3_uapi_stats_3a *stats)
> > {
> > calculateWBGains(stats);
> > diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h
> > index 0acd21480845..ebbb2d58be7e 100644
> > --- a/src/ipa/ipu3/algorithms/awb.h
> > +++ b/src/ipa/ipu3/algorithms/awb.h
> > @@ -40,7 +40,7 @@ public:
> >
> > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > void prepare(IPAContext &context, ipu3_uapi_params *params) override;
> > - void process(IPAContext &context, IPAFrameContext *frameContext,
> > + void process(IPAContext &context, IPAFrameContext &frameContext,
> > const ipu3_uapi_stats_3a *stats) override;
> >
> > private:
> > diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> > index c21647e8c51b..24faba9be2b2 100644
> > --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> > +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> > @@ -78,7 +78,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
> > * The tone mapping look up table is generated as an inverse power curve from
> > * our gamma setting.
> > */
> > -void ToneMapping::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext,
> > +void ToneMapping::process(IPAContext &context, [[maybe_unused]] IPAFrameContext &frameContext,
> > [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> > {
> > /*
> > diff --git a/src/ipa/ipu3/algorithms/tone_mapping.h b/src/ipa/ipu3/algorithms/tone_mapping.h
> > index d7d4800628f2..5be62f6ee9bd 100644
> > --- a/src/ipa/ipu3/algorithms/tone_mapping.h
> > +++ b/src/ipa/ipu3/algorithms/tone_mapping.h
> > @@ -20,7 +20,7 @@ public:
> >
> > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > void prepare(IPAContext &context, ipu3_uapi_params *params) override;
> > - void process(IPAContext &context, IPAFrameContext *frameContext,
> > + void process(IPAContext &context, IPAFrameContext &frameContext,
> > const ipu3_uapi_stats_3a *stats) override;
> >
> > private:
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index e3c1e8160759..f0c92507533c 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -616,7 +616,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> > ControlList ctrls(controls::controls);
> >
> > for (auto const &algo : algorithms())
> > - algo->process(context_, &frameContext, stats);
> > + algo->process(context_, frameContext, stats);
> >
> > setControls(frame);
> >
> > diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
> > index ccc659a63e3b..0fe3d772963a 100644
> > --- a/src/ipa/libipa/algorithm.h
> > +++ b/src/ipa/libipa/algorithm.h
> > @@ -49,7 +49,7 @@ public:
> > }
> >
> > virtual void process([[maybe_unused]] typename Module::Context &context,
> > - [[maybe_unused]] typename Module::FrameContext *frameContext,
> > + [[maybe_unused]] typename Module::FrameContext &frameContext,
> > [[maybe_unused]] const typename Module::Stats *stats)
> > {
> > }
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 708c36ed1af8..7642796d69a3 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -275,13 +275,14 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
> > /**
> > * \brief Process RkISP1 statistics, and run AGC operations
> > * \param[in] context The shared IPA context
> > + * \param[in] frame The frame context sequence number
>
> I don't think this is in yet ... Perhaps this is for a later patch.
Indeed. This is missing documentation of the frameContext argument
though. I'll fix that, with a "while at it" comment in the commit
message.
> With that,
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > * \param[in] stats The RKISP1 statistics and ISP results
> > *
> > * Identify the current image brightness, and use that to estimate the optimal
> > * new exposure and gain for the scene.
> > */
> > void Agc::process(IPAContext &context,
> > - [[maybe_unused]] IPAFrameContext *frameContext,
> > + [[maybe_unused]] IPAFrameContext &frameContext,
> > const rkisp1_stat_buffer *stats)
> > {
> > const rkisp1_cif_isp_stat *params = &stats->params;
> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > index 1c9818b7db2a..6a5723ecbcf8 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -27,7 +27,7 @@ public:
> >
> > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> > void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> > - void process(IPAContext &context, IPAFrameContext *frameContext,
> > + void process(IPAContext &context, IPAFrameContext &frameContext,
> > const rkisp1_stat_buffer *stats) override;
> >
> > private:
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index b12df21de4aa..2bc5d3aa16ab 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -152,7 +152,7 @@ void Awb::queueRequest(IPAContext &context,
> > * \copydoc libcamera::ipa::Algorithm::process
> > */
> > void Awb::process([[maybe_unused]] IPAContext &context,
> > - [[maybe_unused]] IPAFrameContext *frameCtx,
> > + [[maybe_unused]] IPAFrameContext &frameCtx,
> > const rkisp1_stat_buffer *stats)
> > {
> > const rkisp1_cif_isp_stat *params = &stats->params;
> > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h
> > index 4917267bb99d..fc221acea617 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.h
> > +++ b/src/ipa/rkisp1/algorithms/awb.h
> > @@ -23,7 +23,7 @@ public:
> > void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> > void queueRequest(IPAContext &context, const uint32_t frame,
> > const ControlList &controls) override;
> > - void process(IPAContext &context, IPAFrameContext *frameCtx,
> > + void process(IPAContext &context, IPAFrameContext &frameCtx,
> > const rkisp1_stat_buffer *stats) override;
> >
> > private:
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 3ff1eb48f605..3bc6d4dd2784 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -326,8 +326,11 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >
> > unsigned int aeState = 0;
> >
> > + /* \todo Obtain the frame context to pass to process from the FCQueue */
> > + IPAFrameContext frameContext;
> > +
> > for (auto const &algo : algorithms())
> > - algo->process(context_, nullptr, stats);
> > + algo->process(context_, frameContext, stats);
> >
> > setControls(frame);
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list