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

[Jean-Michel Hautbois]

Hi Kieran,

On 08/11/2021 17:23, Kieran Bingham wrote:
> 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?

It can, indeed !

>> +
>>          /* \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);
> 

Sure.

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

Good catch. It is set in PipelineHandlerIPU3::updateControls() and never 
used anymore, so it should be removed from the pipeline handler now :-).

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