[PATCH 4/4] ipa: rkisp1: Initialize FrameContext.agc.meteringMode

[Stefan Klug]

Hi Jacopo,

Thank you for the patch. 

On Wed, Oct 16, 2024 at 07:03:45PM +0200, Jacopo Mondi wrote:
> The RkISP1 AGC algorithms assumes the metering mode to be
> "MeteringMatrix" and pre-fill an array of weights associated
> with it stored in meteringModes_[MeteringMatrix] when intializing
> the algorithm in parseMeteringModes().
> 
> It laters fetches the weights when computing parameters using the
> FrameContext.agc.meteringMode as index of the meteringModes_ array.
> 
> When a Request gets queued to the algorithm, the
> FrameContext.agc.meteringMode index is populated from the algorithm's
> active state, and optionally updated if the application has specified
> a different metering mode.
> 
> In case of Request underrun however, a non-initialized FrameContext
> gets provided to the algorithm, causing an (unhandled) exception when
> accessing the meteringModes_ array.
> 
> Fix this by intializing the AGC metering mode to a supported value
> coming from the ActiveState in IPAFrameContext::init().
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 2 ++
>  src/ipa/rkisp1/ipa_context.cpp    | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 17d074d9c03e..dd7e9468741e 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -175,6 +175,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  		static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
>  	context.activeState.agc.exposureMode =
>  		static_cast<controls::AeExposureModeEnum>(exposureModeHelpers().begin()->first);
> +
> +	/* Use the metering matrix mode by default. */
>  	context.activeState.agc.meteringMode =
>  		static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first);
>  
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 2dad42b3154f..e88609137345 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -421,6 +421,11 @@ void IPAFrameContext::init(const uint32_t frameNum,
>  			   const ActiveState &activeState)
>  {
>  	FrameContext::init(frameNum, activeState);
> +
> +	const IPAActiveState *rkisp1ActiveState =
> +		reinterpret_cast<const IPAActiveState *>(&activeState);
> +
> +	agc.meteringMode = rkisp1ActiveState->agc.meteringMode;

I don't particularly like that the IPAFrameContext needs that knowledge
which should be internal to the agc algorithm. We could add a
initContext() function to the algorithm base class. But then we need to
loop over all algorithms here, which doesn't feel efficient for a single
case. But on the other hand if we assume that request underruns become
more common, then the other algorithms need to be able to continue with
current data without a prior queueRequest() call. While writing this,
yes I think we should forward that to every algorithm. Otherwise we
might end up with strange effects because the algorithms use the data
from the frame context that was reused without proper initialization.

I hope it is somehow understandable.

Cheers,
Stefan

>  }
>  
>  /**
> -- 
> 2.47.0
>