[10/19] libcamera: software_isp: Call Algorithm::process

20401 diff mbox series

This patch adds Algorithm::process call for the defined algorithms.
This is preparation only since there are currently no Algorithm based
algorithms defined.

As software ISP currently doesn't produce any metadata, a dummy and
unused metadata instance is created to satisfy Algorithm::process API.
This should be changed in future.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/soft_simple.cpp | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Hi Milan

On 26/06/24 12:50 pm, Milan Zamazal wrote:
> This patch adds Algorithm::process call for the defined algorithms.
> This is preparation only since there are currently no Algorithm based
> algorithms defined.
>
> As software ISP currently doesn't produce any metadata, a dummy and
> unused metadata instance is created to satisfy Algorithm::process API.
> This should be changed in future.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   src/ipa/simple/soft_simple.cpp | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 6ddb4004..c0cb6769 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -265,10 +265,21 @@ void IPASoftSimple::stop()
>   }
>   
>   void IPASoftSimple::processStats(
> -	[[maybe_unused]] const uint32_t frame,
> +	const uint32_t frame,
>   	[[maybe_unused]] const uint32_t bufferId,
>   	const ControlList &sensorControls)
>   {
> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);

I was trying to infer from the previous patches where the frameContexts 
were allocated per frame.. only to find out it is done in later patch 12/19.

the Algorithms::* patches ideally would be arranged in the order of 
their logical calling sequencing ... but it's not the end of the world. :)

So far looks good to me.

> +	/*
> +	 * \todo Software ISP currently doesn't produce any metadata so it is
> +	 * possible to use a dummy metadata instance here. But metadata should be
> +	 * properly handled in future.
> +	 */
> +	ControlList metadata(controls::controls);
> +	for (auto const &algo : algorithms()) {
> +		algo->process(context_, frame, frameContext, stats_, metadata);
> +	}
> +
>   	SwIspStats::Histogram histogram = stats_->yHistogram;
>   	if (ignoreUpdates_ > 0)
>   		blackLevel_.update(histogram);

Hi Milan,

Thank you for the patch.

On 26/06/24 12:50 pm, Milan Zamazal wrote:
> This patch adds Algorithm::process call for the defined algorithms.
> This is preparation only since there are currently no Algorithm based
> algorithms defined.
>
> As software ISP currently doesn't produce any metadata, a dummy and
> unused metadata instance is created to satisfy Algorithm::process API.
> This should be changed in future.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   src/ipa/simple/soft_simple.cpp | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 6ddb4004..c0cb6769 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -265,10 +265,21 @@ void IPASoftSimple::stop()
>   }
>   
>   void IPASoftSimple::processStats(
> -	[[maybe_unused]] const uint32_t frame,
> +	const uint32_t frame,
>   	[[maybe_unused]] const uint32_t bufferId,
>   	const ControlList &sensorControls)
>   {
> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> +	/*
> +	 * \todo Software ISP currently doesn't produce any metadata so it is
> +	 * possible to use a dummy metadata instance here. But metadata should be
> +	 * properly handled in future.
> +	 */

Can be possibly reworded to:

         /*
          * Software ISP currently does not produce any metadata. Use a 
empty
          * ControlList for now.
          *
          * \todo Implement proper metadata handling
          */

> +	ControlList metadata(controls::controls);
> +	for (auto const &algo : algorithms()) {
> +		algo->process(context_, frame, frameContext, stats_, metadata);
> +	}

{...} can be dropped

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> +
>   	SwIspStats::Histogram histogram = stats_->yHistogram;
>   	if (ignoreUpdates_ > 0)
>   		blackLevel_.update(histogram);