[libcamera-devel] [RFC PATCH v2 3/3] ipa: algorithm: process() should take in frame number instead of context
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Wed Jun 1 09:56:42 CEST 2022
Hi Umang,
Thanks for the patch !
On 27/05/2022 21:17, Umang Jain via libcamera-devel wrote:
> This will enable algorithms to extract a frame context from the FCQueue
> using the frame number. This is required because algorithms can operate
> on different frames at different points in time (according to frame latency
> they need to respect).
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> src/ipa/ipu3/algorithms/af.cpp | 2 +-
> src/ipa/ipu3/algorithms/af.h | 2 +-
> src/ipa/ipu3/algorithms/agc.cpp | 13 +++++++------
> src/ipa/ipu3/algorithms/agc.h | 4 ++--
> src/ipa/ipu3/algorithms/algorithm.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.cpp | 2 +-
> src/ipa/libipa/algorithm.h | 6 +++---
> src/ipa/rkisp1/algorithms/agc.cpp | 3 +--
> src/ipa/rkisp1/algorithms/agc.h | 2 +-
> src/ipa/rkisp1/algorithms/algorithm.h | 5 ++---
> src/ipa/rkisp1/algorithms/awb.cpp | 2 +-
> src/ipa/rkisp1/algorithms/awb.h | 2 +-
> src/ipa/rkisp1/rkisp1.cpp | 2 +-
> 18 files changed, 29 insertions(+), 30 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index d07521a0..24462c6b 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -420,7 +420,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]] uint32_t frame,
> const ipu3_uapi_stats_3a *stats)
> {
You need to document it properly:
'''
/home/jm/libcamera/src/ipa/ipu3/algorithms/af.cpp:408: warning: argument
'frameContext' of command @param is not found in the argument list of
libcamera::ipa::ipu3::algorithms::Af::process(IPAContext &context,
uint32_t frame, const ipu3_uapi_stats_3a *stats)
/home/jm/libcamera/src/ipa/ipu3/algorithms/af.cpp:408: warning: The
following parameter of
libcamera::ipa::ipu3::algorithms::Af::process(IPAContext &context,
uint32_t frame, const ipu3_uapi_stats_3a *stats) is not documented:
parameter 'frame'
'''
Same for agc and tonemapping functions below :-).
With it:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> /* Evaluate the AF buffer length */
> diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h
> index ccf015f3..69aeb3e2 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, uint32_t frame,
> 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 f16be534..41766007 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -183,13 +183,14 @@ 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,
> - double yGain, double iqMeanGain)
> +void Agc::computeExposure(IPAContext &context, uint32_t frame, 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;
> + IPAFrameContext &frameContext = context.frameContexts.get(frame);
> + 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 +324,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, [[maybe_unused]] uint32_t frame,
> const ipu3_uapi_stats_3a *stats)
> {
> /*
> @@ -359,7 +360,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameCo
> break;
> }
>
> - computeExposure(context, frameContext, yGain, iqMeanGain);
> + computeExposure(context, frame, yGain, iqMeanGain);
> frameCount_++;
> }
>
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 105ae0f2..8ef2a2d8 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, uint32_t frame,
> 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, uint32_t frame,
> double yGain, double iqMeanGain);
> double estimateLuminance(IPAActiveState &activeState,
> const ipu3_uapi_grid_config &grid,
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> index 234b2bd7..3e0c60d3 100644
> --- a/src/ipa/ipu3/algorithms/algorithm.h
> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> @@ -17,8 +17,8 @@ namespace libcamera {
>
> namespace ipa::ipu3 {
>
> -using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAFrameContext,
> - IPAConfigInfo, ipu3_uapi_params,
> +using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAConfigInfo,
> + ipu3_uapi_params,
> ipu3_uapi_stats_3a>;
>
> } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 5c232d92..35f35ebe 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]] uint32_t frame,
> 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 9a50a985..f263540c 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, uint32_t frame,
> 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 f86e79b2..0f70b8aa 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]] uint32_t frame,
> [[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 d7d48006..61474085 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, uint32_t frame,
> const ipu3_uapi_stats_3a *stats) override;
>
> private:
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index e09c5d05..4f33b1a6 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -582,7 +582,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_, frame, stats);
>
> setControls(frame);
>
> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp
> index cce2ed62..c0a9bbeb 100644
> --- a/src/ipa/libipa/algorithm.cpp
> +++ b/src/ipa/libipa/algorithm.cpp
> @@ -64,7 +64,7 @@ namespace ipa {
> * \fn Algorithm::process()
> * \brief Process ISP statistics, and run algorithm operations
> * \param[in] context The shared IPA context
> - * \param[in] frameContext The current frame's context
> + * \param[in] frame The current frame id
> * \param[in] stats The IPA statistics and ISP results
> *
> * This function is called while camera is running for every frame processed by
> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
> index 032a05b5..d1ca0883 100644
> --- a/src/ipa/libipa/algorithm.h
> +++ b/src/ipa/libipa/algorithm.h
> @@ -10,8 +10,8 @@ namespace libcamera {
>
> namespace ipa {
>
> -template<typename Context, typename FrameContext, typename Config,
> - typename Params, typename Stats>
> +template<typename Context, typename Config, typename Params, typename Stats>
> +
> class Algorithm
> {
> public:
> @@ -29,7 +29,7 @@ public:
> }
>
> virtual void process([[maybe_unused]] Context &context,
> - [[maybe_unused]] FrameContext *frameContext,
> + [[maybe_unused]] uint32_t frame,
> [[maybe_unused]] const Stats *stats)
> {
> }
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index b5a184d9..9418b533 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -280,8 +280,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
> * 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, [[maybe_unused]] uint32_t frame,
> 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 22c02779..a749f1c3 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, uint32_t frame,
> const rkisp1_stat_buffer *stats) override;
>
> private:
> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> index 68e3a44e..069c744c 100644
> --- a/src/ipa/rkisp1/algorithms/algorithm.h
> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> @@ -19,9 +19,8 @@ namespace libcamera {
>
> namespace ipa::rkisp1 {
>
> -using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAFrameContext,
> - IPACameraSensorInfo, rkisp1_params_cfg,
> - rkisp1_stat_buffer>;
> +using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPACameraSensorInfo,
> + rkisp1_params_cfg, rkisp1_stat_buffer>;
>
> } /* namespace ipa::rkisp1 */
>
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 88441382..1e7fd11a 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]] uint32_t frame,
> 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 7647842f..82921f34 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, uint32_t frame,
> const rkisp1_stat_buffer *stats) override;
>
> private:
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index c818a6d7..ad33d817 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -272,7 +272,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> unsigned int aeState = 0;
>
> for (auto const &algo : algorithms_)
> - algo->process(context_, nullptr, stats);
> + algo->process(context_, frame, stats);
>
> setControls(frame);
>
More information about the libcamera-devel
mailing list