[libcamera-devel] ipa: ipu3: Rectify gain value reporting in request metadata

14878 diff mbox series

Report value of sensor's gain pertaining to the current request
completion, as that is the gain value the request completed with.
The Agc algorithm processes the gain value for incoming next request
hence it should not be reported in request's metadata.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Umang,

On 30/11/2021 06:22, Umang Jain wrote:
> Report value of sensor's gain pertaining to the current request
> completion, as that is the gain value the request completed with.
> The Agc algorithm processes the gain value for incoming next request
> hence it should not be reported in request's metadata.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Thanks for the explanation in previous patch.
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> ---
>   src/ipa/ipu3/ipu3.cpp | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b0c75541..76182587 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -628,7 +628,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>   	int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>();
>   	ctrls.set(controls::FrameDuration, frameDuration);
>   
> -	ctrls.set(controls::AnalogueGain, context_.frameContext.agc.gain);
> +	ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);
>   
>   	ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);
>   
>

Quoting Umang Jain (2021-11-30 05:22:53)
> Report value of sensor's gain pertaining to the current request
> completion, as that is the gain value the request completed with.
> The Agc algorithm processes the gain value for incoming next request
> hence it should not be reported in request's metadata.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index b0c75541..76182587 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -628,7 +628,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>         int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>();
>         ctrls.set(controls::FrameDuration, frameDuration);
>  
> -       ctrls.set(controls::AnalogueGain, context_.frameContext.agc.gain);
> +       ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain);

Aha, yes that looks like a more reasonable way to align the Gain and
Exposure.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


We could probably add some extra/better comments on the gain/exposure to
make it clear, but this is a valid fix on it's own, and if we add extra
documentation - we should align it with both the IPU3 and the RKISP now.

--
Kieran


>  
>         ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);
>  
> -- 
> 2.31.0
>