[libcamera-devel] [PATCH 21/22] ipa: ipu3: Move ExposureTime to IPA

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-08 13:13:49)
> Now that we have the exposure time calculated, pass it to the
> controls::ExposureTime and don't use the pipeline handler for it
> anymore. While at it, use the same line duration value for ExposureTime
> and FrameDuration.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp                | 9 +++++++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 --
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index ca3f2417..24c77be8 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -691,12 +691,17 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  
>         setControls(frame);
>  
> +       double lineDuration = sensorInfo_.lineLength
> +                           / (sensorInfo_.pixelRate / 1e6);

Is this something that should be calculated / cached at configure time?

> +
>         /* \todo Use VBlank value calculated from each frame exposure. */
> -       int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> -                               (sensorInfo_.pixelRate / 1e6);
> +       int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height)
> +                             * lineDuration;
>         ctrls.set(controls::FrameDuration, frameDuration);
>  
>         ctrls.set(controls::ColourTemperature, context_.frameContext->awb.temperatureK);

new line here if you're keeping the int32_t calculation split.

> +       int32_t exposureTime = context_.frameContext->agc.exposure * lineDuration;
> +       ctrls.set(controls::ExposureTime, exposureTime);

Otherwise, it could go in one statement, particularly if we take a local
alias pointer for the frameContext as it is likely to be accessed so much:

	IPAFrameContext *frameContext = context_.frameContext;

	ctrls.set(controls::ExposureTime, frameContext->agc.exposure * lineDuration);

>  
>         /*
>          * \todo We should be able to add 'anything' (with a Control) in here to
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5d87f6e5..97003681 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1322,8 +1322,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>         pipe()->completeBuffer(request, buffer);
>  
>         request->metadata().set(controls::draft::PipelineDepth, 3);
> -       /* \todo Move the ExposureTime control to the IPA. */
> -       request->metadata().set(controls::ExposureTime, exposureTime_);

Does the pipeline handler still need an exposureTime_ ? Can it be
removed? or is something else using it?


>         /* \todo Actually apply the scaler crop region to the ImgU. */
>         if (request->controls().contains(controls::ScalerCrop))
>                 cropRegion_ = request->controls().get(controls::ScalerCrop);
> -- 
> 2.32.0
>