[libcamera-devel] [RFC PATCH 06/12] ipa: libipa: Provide a common base for FrameContexts
Umang Jain
umang.jain at ideasonboard.com
Fri Jul 22 23:03:49 CEST 2022
Hi Kieran,
On 7/21/22 17:43, Kieran Bingham via libcamera-devel wrote:
> Provide a common IPAFrameContext as a base for IPA modules to inherit
> from.
>
> This will allow having a common set of parameters for every FrameContext.
>
> We still have to use a templated instance of the FCQueue though to make
> sure we allocate the correct types.
>
> Both the RKISP1 and the IPU3 create IPA specific FrameContext structures
> which inherit from the IPAFrameContext.
>
> While adjusting interface to Algorithm::process() the FrameContext is
> converted from a pointer to a reference.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
Looks good!
Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> src/ipa/ipu3/algorithms/af.cpp | 3 ++-
> src/ipa/ipu3/algorithms/af.h | 2 +-
> src/ipa/ipu3/algorithms/agc.cpp | 9 ++++----
> src/ipa/ipu3/algorithms/agc.h | 4 ++--
> src/ipa/ipu3/algorithms/awb.cpp | 3 ++-
> src/ipa/ipu3/algorithms/awb.h | 2 +-
> src/ipa/ipu3/algorithms/tone_mapping.cpp | 3 ++-
> src/ipa/ipu3/algorithms/tone_mapping.h | 2 +-
> src/ipa/ipu3/ipa_context.cpp | 26 +++++-------------------
> src/ipa/ipu3/ipa_context.h | 5 ++---
> src/ipa/ipu3/ipu3.cpp | 6 +++---
> src/ipa/ipu3/module.h | 2 +-
> src/ipa/libipa/algorithm.h | 2 +-
> src/ipa/libipa/fc_queue.cpp | 21 +++++++++++++++++++
> src/ipa/libipa/fc_queue.h | 6 ++++++
> src/ipa/rkisp1/algorithms/agc.cpp | 2 +-
> src/ipa/rkisp1/algorithms/agc.h | 2 +-
> src/ipa/rkisp1/algorithms/awb.cpp | 2 +-
> src/ipa/rkisp1/algorithms/awb.h | 2 +-
> src/ipa/rkisp1/ipa_context.h | 4 +++-
> src/ipa/rkisp1/module.h | 2 +-
> src/ipa/rkisp1/rkisp1.cpp | 5 ++++-
> 22 files changed, 67 insertions(+), 48 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index d07521a090ac..fcbf8e557b10 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -420,7 +420,8 @@ 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]] IPU3FrameContext &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 ccf015f3f8f5..7a5aeb1b6bf4 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, IPU3FrameContext &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 5bc64ae52214..92ea6249798d 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, IPU3FrameContext &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,8 @@ 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,
> + IPU3FrameContext &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..96585f48933f 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, IPU3FrameContext &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, IPU3FrameContext &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 704267222a31..45f8e5f29119 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -387,7 +387,8 @@ 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]] IPU3FrameContext &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..28e39620fd59 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, IPU3FrameContext &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 f86e79b24a67..977741c5a4d4 100644
> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> @@ -78,7 +78,8 @@ 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]] IPU3FrameContext &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..8cce38c742a6 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, IPU3FrameContext &frameContext,
> const ipu3_uapi_stats_3a *stats) override;
>
> private:
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index dd139cb4c868..536d76ecd63b 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 {
> * most recently computed by the IPA algorithms.
> */
>
> -/**
> - * \struct IPAFrameContext
> - * \brief Context for a frame
> - *
> - * The frame context stores data specific to a single frame processed by the
> - * IPA. Each frame processed by the IPA has a context associated with it,
> - * accessible through the IPAContext structure.
> - *
> - * Fields in the frame context should reflect values and controls
> - * associated with the specific frame as requested by the application, and
> - * as configured by the hardware. Fields can be read by algorithms to
> - * determine if they should update any specific action for this frame, and
> - * finally to update the metadata control lists when the frame is fully
> - * completed.
> - */
> -
> /**
> * \struct IPAContext
> * \brief Global IPA context data shared between all algorithms
> @@ -181,16 +165,16 @@ namespace libcamera::ipa::ipu3 {
> */
>
> /**
> - * \var IPAFrameContext::frame
> - * \brief The frame number
> + * \struct IPU3FrameContext
> + * \copybrief libcamera::ipa::IPAFrameContext
> *
> - * \var IPAFrameContext::sensor
> + * \var IPU3FrameContext::sensor
> * \brief Effective sensor values that were applied for the frame
> *
> - * \var IPAFrameContext::sensor.exposure
> + * \var IPU3FrameContext::sensor.exposure
> * \brief Exposure time expressed as a number of lines
> *
> - * \var IPAFrameContext::sensor.gain
> + * \var IPU3FrameContext::sensor.gain
> * \brief Analogue gain multiplier
> */
>
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 193fc44a821a..c4aa4c3f4f6a 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -73,8 +73,7 @@ struct IPAActiveState {
> } toneMapping;
> };
>
> -struct IPAFrameContext {
> - uint32_t frame;
> +struct IPU3FrameContext : public IPAFrameContext {
>
> struct {
> uint32_t exposure;
> @@ -86,7 +85,7 @@ struct IPAContext {
> IPASessionConfiguration configuration;
> IPAActiveState activeState;
>
> - FCQueue<IPAFrameContext> frameContexts;
> + FCQueue<IPU3FrameContext> frameContexts;
> };
>
> } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 55e82cd404f4..489e51bc6cf7 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -570,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> const ipu3_uapi_stats_3a *stats =
> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>
> - IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> + IPU3FrameContext &frameContext = context_.frameContexts.get(frame);
>
> if (frameContext.frame != frame)
> LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> @@ -583,7 +583,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);
>
> @@ -619,7 +619,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> {
> /* \todo Start processing for 'frame' based on 'controls'. */
> - IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> + IPU3FrameContext &frameContext = context_.frameContexts.initialise(frame);
>
> /* \todo Implement queueRequest to each algorithm. */
> (void)frameContext;
> diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h
> index d94fc4594871..6d0d50f615d8 100644
> --- a/src/ipa/ipu3/module.h
> +++ b/src/ipa/ipu3/module.h
> @@ -19,7 +19,7 @@ namespace libcamera {
>
> namespace ipa::ipu3 {
>
> -using Module = ipa::Module<IPAContext, IPAFrameContext, IPAConfigInfo,
> +using Module = ipa::Module<IPAContext, IPU3FrameContext, IPAConfigInfo,
> ipu3_uapi_params, ipu3_uapi_stats_3a>;
>
> } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
> index 2a8871d831a3..adbcf69adb4f 100644
> --- a/src/ipa/libipa/algorithm.h
> +++ b/src/ipa/libipa/algorithm.h
> @@ -41,7 +41,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/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> index ecb47f287350..627c2fa3301e 100644
> --- a/src/ipa/libipa/fc_queue.cpp
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -55,6 +55,27 @@ namespace ipa {
> * the Request when it completes.
> */
>
> +/**
> + * \struct IPAFrameContext
> + * \brief Context for a frame
> + *
> + * The frame context stores data specific to a single frame processed by the
> + * IPA. Each frame processed by the IPA has a context associated with it,
> + * accessible through the Frame Context Queue.
> + *
> + * Fields in the frame context should reflect values and controls associated
> + * with the specific frame as requested by the application, and as configured by
> + * the hardware. Fields can be read by algorithms to determine if they should
> + * update any specific action for this frame, and finally to update the metadata
> + * control lists when the frame is fully completed.
> + *
> + * \var IPAFrameContext::frame
> + * \brief The frame number
> + *
> + * \var IPAFrameContext::error
> + * \brief The error flags associated with the frame
> + */
> +
> /**
> * \fn FCQueue::clear()
> * \brief Reinitialise all data on the queue
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> index 82ce36811926..6c7b8bf11ab2 100644
> --- a/src/ipa/libipa/fc_queue.h
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -13,6 +13,7 @@
> #include <libcamera/base/log.h>
>
> #include <libcamera/controls.h>
> +#include <libcamera/request.h>
>
> namespace libcamera {
>
> @@ -27,6 +28,11 @@ namespace ipa {
> */
> static constexpr uint32_t kMaxFrameContexts = 16;
>
> +struct IPAFrameContext {
> + unsigned int frame;
> + Request::ErrorFlags error;
> +};
> +
> template<typename FrameContext>
> class FCQueue : public std::array<FrameContext, kMaxFrameContexts>
> {
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 483e941fe427..40d27963ef68 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -281,7 +281,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
> * new exposure and gain for the scene.
> */
> void Agc::process(IPAContext &context,
> - [[maybe_unused]] IPAFrameContext *frameContext,
> + [[maybe_unused]] RKISP1FrameContext &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 22c02779d311..1a291bdf3636 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -29,7 +29,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, RKISP1FrameContext &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 427aaeb1e955..3beafb73ca33 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -120,7 +120,7 @@ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params)
> * \copydoc libcamera::ipa::Algorithm::process
> */
> void Awb::process([[maybe_unused]] IPAContext &context,
> - [[maybe_unused]] IPAFrameContext *frameCtx,
> + [[maybe_unused]] RKISP1FrameContext &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 7647842f6609..259b85eebdb4 100644
> --- a/src/ipa/rkisp1/algorithms/awb.h
> +++ b/src/ipa/rkisp1/algorithms/awb.h
> @@ -23,7 +23,7 @@ public:
>
> int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
> - void process(IPAContext &context, IPAFrameContext *frameCtx,
> + void process(IPAContext &context, RKISP1FrameContext &frameCtx,
> const rkisp1_stat_buffer *stats) override;
>
> private:
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 341fef2e68fe..a64dbc75fdd2 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -14,6 +14,8 @@
>
> #include <libcamera/geometry.h>
>
> +#include <libipa/fc_queue.h>
> +
> namespace libcamera {
>
> namespace ipa::rkisp1 {
> @@ -64,7 +66,7 @@ struct IPAActiveState {
> unsigned int frameCount;
> };
>
> -struct IPAFrameContext {
> +struct RKISP1FrameContext : public IPAFrameContext {
> };
>
> struct IPAContext {
> diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h
> index 89f83208a75c..2568c624df0f 100644
> --- a/src/ipa/rkisp1/module.h
> +++ b/src/ipa/rkisp1/module.h
> @@ -19,7 +19,7 @@ namespace libcamera {
>
> namespace ipa::rkisp1 {
>
> -using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,
> +using Module = ipa::Module<IPAContext, RKISP1FrameContext, IPACameraSensorInfo,
> rkisp1_params_cfg, rkisp1_stat_buffer>;
>
> } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 9629ccbf4247..7481e67e70f6 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -301,8 +301,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 */
> + RKISP1FrameContext frameContext;
> +
> for (auto const &algo : algorithms())
> - algo->process(context_, nullptr, stats);
> + algo->process(context_, frameContext, stats);
>
> setControls(frame);
>
More information about the libcamera-devel
mailing list