[libcamera-devel] [PATCH v1 2/4] ipa: Pass metadata to Algorithm::process() function
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Oct 19 13:36:01 CEST 2022
Quoting Laurent Pinchart via libcamera-devel (2022-10-19 12:04:32)
> Extend the Algorithm::process() function with a metadata control list,
> to be filled by individual algorithms with frame metadata. Update the
> rkisp1 and ipu3 IPA modules accordingly, and drop the dead code in the
> IPARkISP1::prepareMetadata() function while at it.
>
> This only creates the infrastructure, filling metadata in individual
> algorithms will be handled separately.
>
> 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 | 2 ++
> src/ipa/ipu3/algorithms/agc.h | 2 +-
> src/ipa/ipu3/algorithms/awb.cpp | 1 +
> 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 | 3 ++-
> src/ipa/libipa/algorithm.cpp | 5 +++--
> src/ipa/libipa/algorithm.h | 1 +
> src/ipa/rkisp1/algorithms/agc.cpp | 4 +++-
> src/ipa/rkisp1/algorithms/agc.h | 2 +-
> src/ipa/rkisp1/algorithms/awb.cpp | 1 +
> src/ipa/rkisp1/algorithms/awb.h | 2 +-
> src/ipa/rkisp1/rkisp1.cpp | 17 +++--------------
> 16 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp
> index 75632aa39d21..2d728871a63b 100644
> --- a/src/ipa/ipu3/algorithms/af.cpp
> +++ b/src/ipa/ipu3/algorithms/af.cpp
> @@ -408,6 +408,7 @@ bool Af::afIsOutOfFocus(IPAContext &context)
> * \param[in] context The IPA context
> * \param[in] frame The frame context sequence number
> * \param[in] frameContext The current frame context
> + * \param[out] metadata Metadata for the frame, to be filled by the algorithm
> * \param[in] stats The statistics buffer of IPU3
> *
> * Ideally, a clear image also has a relatively higher contrast. So, every
> @@ -423,6 +424,7 @@ bool Af::afIsOutOfFocus(IPAContext &context)
> */
> void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> [[maybe_unused]] IPAFrameContext &frameContext,
> + [[maybe_unused]] ControlList &metadata,
> 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 89d37ac18615..ea7ca647df12 100644
> --- a/src/ipa/ipu3/algorithms/af.h
> +++ b/src/ipa/ipu3/algorithms/af.h
> @@ -35,7 +35,7 @@ public:
> IPAFrameContext &frameContext,
> ipu3_uapi_params *params) override;
> void process(IPAContext &context, const uint32_t frame,
> - IPAFrameContext &frameContext,
> + IPAFrameContext &frameContext, ControlList &metadata,
> 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 a1a3c38ffe84..44dd5c809e26 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -319,6 +319,7 @@ double Agc::estimateLuminance(IPAActiveState &activeState,
> * \param[in] context The shared IPA context
> * \param[in] frame The current frame sequence number
> * \param[in] frameContext The current frame context
> + * \param[out] metadata Metadata for the frame, to be filled by the algorithm
> * \param[in] stats The IPU3 statistics and ISP results
> *
> * Identify the current image brightness, and use that to estimate the optimal
> @@ -326,6 +327,7 @@ double Agc::estimateLuminance(IPAActiveState &activeState,
> */
> void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> IPAFrameContext &frameContext,
> + [[maybe_unused]] ControlList &metadata,
> const ipu3_uapi_stats_3a *stats)
> {
> /*
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 59b4b9843c2f..9106cbefe87a 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -29,7 +29,7 @@ public:
>
> int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> void process(IPAContext &context, const uint32_t frame,
> - IPAFrameContext &frameContext,
> + IPAFrameContext &frameContext, ControlList &metadata,
> const ipu3_uapi_stats_3a *stats) override;
>
> private:
> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index 0dbd7d4c374f..836eb0687181 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -389,6 +389,7 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> */
> void Awb::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> [[maybe_unused]] IPAFrameContext &frameContext,
> + [[maybe_unused]] ControlList &metadata,
> 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 28e2d38a711c..c38c425c3654 100644
> --- a/src/ipa/ipu3/algorithms/awb.h
> +++ b/src/ipa/ipu3/algorithms/awb.h
> @@ -43,7 +43,7 @@ public:
> IPAFrameContext &frameContext,
> ipu3_uapi_params *params) override;
> void process(IPAContext &context, const uint32_t frame,
> - IPAFrameContext &frameContext,
> + IPAFrameContext &frameContext, ControlList &metadata,
> 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 eac3d4064443..6fcc03f3fcf7 100644
> --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp
> +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp
> @@ -78,6 +78,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
> * \param[in] context The shared IPA context
> * \param[in] frame The current frame sequence number
> * \param[in] frameContext The current frame context
> + * \param[out] metadata Metadata for the frame, to be filled by the algorithm
> * \param[in] stats The IPU3 statistics and ISP results
> *
> * The tone mapping look up table is generated as an inverse power curve from
> @@ -85,6 +86,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context,
> */
> void ToneMapping::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> [[maybe_unused]] IPAFrameContext &frameContext,
> + [[maybe_unused]] ControlList &metadata,
> [[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 822e5168df82..ab2a1c000f2f 100644
> --- a/src/ipa/ipu3/algorithms/tone_mapping.h
> +++ b/src/ipa/ipu3/algorithms/tone_mapping.h
> @@ -22,7 +22,7 @@ public:
> void prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext, ipu3_uapi_params *params) override;
> void process(IPAContext &context, const uint32_t frame,
> - IPAFrameContext &frameContext,
> + IPAFrameContext &frameContext, ControlList &metadata,
> const ipu3_uapi_stats_3a *stats) override;
>
> private:
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 891643e057b8..0ccc6bf5c8af 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -631,9 +631,10 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>();
> int32_t vBlank = context_.configuration.sensor.defVBlank;
> ControlList ctrls(controls::controls);
It looks like ctrls() is what is currently used as metadata here.
It gets emitted at the end of this function.
Can we either use ctrls passed in to algo->process() or (preferred)
rename ctrls to metadata here to ensure that /when/ we set metadata in
the algorithms it will reach the pipeline handler?
Probably keep the existing metadata additions in this function still -
converting them can be later, but otherwise this is a partial
implementation for IPU3.
*with that*
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> + ControlList metadata(controls::controls);
>
> for (auto const &algo : algorithms())
> - algo->process(context_, frame, frameContext, stats);
> + algo->process(context_, frame, frameContext, metadata, stats);
I would have had metadata as the last parameter as an 'output'
(inputs, outputs) ... but I bet you've gone alphabetical.
No point changing it though - that's too much work, and this is fine.
> setControls(frame);
>
> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp
> index 55abddab2b0a..a34c583d947e 100644
> --- a/src/ipa/libipa/algorithm.cpp
> +++ b/src/ipa/libipa/algorithm.cpp
> @@ -106,12 +106,13 @@ namespace ipa {
> * \param[in] context The shared IPA context
> * \param[in] frame The frame context sequence number
> * \param[in] frameContext The current frame's context
> + * \param[out] metadata Metadata for the frame, to be filled by the algorithm
> * \param[in] stats The IPA statistics and ISP results
> *
> * This function is called while camera is running for every frame processed by
> * the ISP, to process statistics generated from that frame by the ISP.
> - * Algorithms shall use this data to run calculations and update their state
> - * accordingly.
> + * Algorithms shall use this data to run calculations, update their state
> + * accordingly, and fill the frame metadata.
> *
> * Processing shall not take an undue amount of time, and any extended or
> * computationally expensive calculations or operations must be handled
> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
> index d8601f9ccaff..793247be8129 100644
> --- a/src/ipa/libipa/algorithm.h
> +++ b/src/ipa/libipa/algorithm.h
> @@ -55,6 +55,7 @@ public:
> virtual void process([[maybe_unused]] typename Module::Context &context,
> [[maybe_unused]] const uint32_t frame,
> [[maybe_unused]] typename Module::FrameContext &frameContext,
> + [[maybe_unused]] ControlList &metadata,
> [[maybe_unused]] const typename Module::Stats *stats)
> {
> }
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 04062a364e7f..9de11a2a3bca 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -288,7 +288,9 @@ 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]] const uint32_t frame,
> - IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats)
> + IPAFrameContext &frameContext,
> + [[maybe_unused]] ControlList &metadata,
> + const rkisp1_stat_buffer *stats)
> {
> /*
> * \todo Verify that the exposure and gain applied by the sensor for
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index 9ad5c32fd6f6..99af09167331 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -30,7 +30,7 @@ public:
> IPAFrameContext &frameContext,
> rkisp1_params_cfg *params) override;
> void process(IPAContext &context, const uint32_t frame,
> - IPAFrameContext &frameContext,
> + IPAFrameContext &frameContext, ControlList &metadata,
> const rkisp1_stat_buffer *stats) override;
>
> private:
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 3349948a3acf..9b97db7d5c5a 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -208,6 +208,7 @@ void Awb::queueRequest(IPAContext &context,
> void Awb::process(IPAContext &context,
> [[maybe_unused]] const uint32_t frame,
> IPAFrameContext &frameContext,
> + [[maybe_unused]] ControlList &metadata,
> 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 d76b538288ec..3659b2d5405f 100644
> --- a/src/ipa/rkisp1/algorithms/awb.h
> +++ b/src/ipa/rkisp1/algorithms/awb.h
> @@ -27,7 +27,7 @@ public:
> IPAFrameContext &frameContext,
> const ControlList &controls) override;
> void process(IPAContext &context, const uint32_t frame,
> - IPAFrameContext &frameCtx,
> + IPAFrameContext &frameContext, ControlList &metadata,
> const rkisp1_stat_buffer *stats) override;
>
> private:
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 3f5c1a58695c..bef5211d5c34 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -69,7 +69,6 @@ protected:
>
> private:
> void setControls(unsigned int frame);
> - void prepareMetadata(unsigned int frame, unsigned int aeState);
>
> std::map<unsigned int, FrameBuffer> buffers_;
> std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> @@ -338,14 +337,14 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> frameContext.sensor.gain =
> camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>
> - unsigned int aeState = 0;
> + ControlList metadata(controls::controls);
>
> for (auto const &algo : algorithms())
> - algo->process(context_, frame, frameContext, stats);
> + algo->process(context_, frame, frameContext, metadata, stats);
>
> setControls(frame);
>
> - prepareMetadata(frame, aeState);
> + metadataReady.emit(frame, metadata);
That ties in much better.
> }
>
> void IPARkISP1::setControls(unsigned int frame)
> @@ -366,16 +365,6 @@ void IPARkISP1::setControls(unsigned int frame)
> setSensorControls.emit(frame, ctrls);
> }
>
> -void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)
> -{
> - ControlList ctrls(controls::controls);
> -
> - if (aeState)
> - ctrls.set(controls::AeLocked, aeState == 2);
Phew, that's good to see it all go.
> -
> - metadataReady.emit(frame, ctrls);
> -}
> -
> } /* namespace ipa::rkisp1 */
>
> /*
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list