[libcamera-devel] [PATCH v2 13/14] ipa: ipu3: Move ExposureTime to IPA

[Kieran Bingham]

Quoting Jean-Michel Hautbois (2021-11-10 19:59:00)
> 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                |  6 ++++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +---------------
>  2 files changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 68a97e8e..9220fea5 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -629,12 +629,14 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>         setControls(frame);
>  
>         /* \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_.count();

Same comments as previous patch regarding .count() vs .get<std::nano>(),
but it's optional. Though here it looks like it reads nicely to express
that it's in nanoseconds...

>         ctrls.set(controls::FrameDuration, frameDuration);
>  
>         ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);
>  
> +       ctrls.set(controls::ExposureTime, context_.frameContext.agc.exposure * lineDuration_.count());
> +

I wonder if there should be any specific grouping or ordering in how we
set the metadata controls here ...

I guess it would end up being either alphabetical if sorted by control
name, or grouped if things like exposure/gain should be kept together -
or otherwise grouped by 'feature'?

>         /*
>          * \todo We should be able to add 'anything' (with a Control) in here to
>          * get information to say.
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 3fcfa777..fe37e9c7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -47,7 +47,7 @@ class IPU3CameraData : public Camera::Private
>  {
>  public:
>         IPU3CameraData(PipelineHandler *pipe)
> -               : Camera::Private(pipe), exposureTime_(0), supportsFlips_(false)
> +               : Camera::Private(pipe), supportsFlips_(false)
>         {
>         }
>  
> @@ -67,7 +67,6 @@ public:
>         Stream vfStream_;
>         Stream rawStream_;
>  
> -       uint32_t exposureTime_;
>         Rectangle cropRegion_;
>         bool supportsFlips_;
>         Transform rotationTransform_;
> @@ -1049,17 +1048,6 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)
>  
>         controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
>  
> -       /*
> -        * \todo Report the actual exposure time, use the default for the
> -        * moment.
> -        */
> -       const auto exposureInfo = data->ipaControls_.find(&controls::ExposureTime);
> -       if (exposureInfo == data->ipaControls_.end()) {
> -               LOG(IPU3, Error) << "Exposure control not initialized by the IPA";
> -               return -EINVAL;
> -       }
> -       data->exposureTime_ = exposureInfo->second.def().get<int32_t>();
> -
>         /* Add the IPA registered controls to list of camera controls. */
>         for (const auto &ipaControl : data->ipaControls_)
>                 controls[ipaControl.first] = ipaControl.second;
> @@ -1321,8 +1309,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_);

I'd sneak a blank separator line in here at this point ;-) But hey
that's me and my blank lines ....

Nothing non-trivial in the comments, so when considered how you see fit:


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


>         /* \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
>