[libcamera-devel] [RFC PATCH v2 3/3] ipa: algorithm: process() should take in frame number instead of context
Umang Jain
umang.jain at ideasonboard.com
Wed Jun 1 09:58:30 CEST 2022
Hi JM
On 6/1/22 09:56, Jean-Michel Hautbois wrote:
> 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'
> '''
I have mentioned in the cover that the documentation might be 'broken'.
I ran out of time when I was writing this design hence, left fixing
documentation
>
> 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