[libcamera-devel] [PATCH v3 9/9] ipu3: Fixes frame delay

[Umang Jain]

Hello,

On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:
> Like the shipped ipu3 HAL, handle the frame delay with one extra input,


Can you point to the shipped ipu3 HAL frame delay handling code here for 
reference?

> needed for the first frame and StillCapture's non-sequential frame
> requests.
>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>   src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++++++-----
>   1 file changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e26c2736..8689cf8b 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -90,6 +90,9 @@ public:
>   
>   	ControlInfoMap ipaControls_;
>   
> +	bool firstRequest_ = true;
> +	int lastRequestSequence_ = -1;


this is only used under hasCapture so maybe lastCaptureRequestSeq_ is 
more appropriate?

> +
>   private:
>   	void metadataReady(unsigned int id, const ControlList &metadata);
>   	void paramsBufferReady(unsigned int id);
> @@ -565,6 +568,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>   	CIO2Device *cio2 = &data->cio2_;
>   	V4L2DeviceFormat outputFormat;
>   
> +	data->firstRequest_ = true;
> +	data->lastRequestSequence_ = -1;
> +


Does this needs to be reset on every start()/stop() cycles?

>   	ImgUDevice::PipeConfig imguConfig1 = config->imguConfig1();
>   	useImgu1_ = !imguConfig1.isNull();
>   
> @@ -1427,6 +1433,11 @@ void IPU3CameraData::paramsBufferReady(unsigned int id)
>   	if (!info)
>   		return;
>   
> +	int yuvCount = firstRequest_ ? 2 : 1;
> +	int stillCount = firstRequest_ || (lastRequestSequence_ + 1) != static_cast<int>(info->request->sequence()) ? 2 : 1;
> +


This is getting a bit non-trivial to follow. Can you probably include a 
brief comment here to explain some rationale?

As per my understanding, if the lastRequestSequence_ was a StillCapture, 
stillCount will be 1 for current request capture, and it will 2 for all 
other cases?

We need stillCount to be 2 (usually) because .... ?

> +	firstRequest_ = false;
> +
>   	bool hasYuv = false;
>   	bool hasCapture = false;
>   	/* Queue all buffers from the request aimed for the ImgU. */
> @@ -1436,33 +1447,53 @@ void IPU3CameraData::paramsBufferReady(unsigned int id)
>   
>   		if (stream == &outStream_) {
>   			hasYuv = true;
> -			imgu0_->output_->queueBuffer(outbuffer);
> +
> +			for (int i = 0; i < yuvCount; ++i) {
> +				bufferReturnCounters[outbuffer] += 1;
> +				imgu0_->output_->queueBuffer(outbuffer);


Have you checked / discussed what happens if you re-queue the same 
buffer on videodevice twice (before even dequeuing it)? I am not sure, 
because the current documentation doesn't specify this case I think.

I've asked around.

> +			}
>   		} else if (stream == &vfStream_) {
>   			hasYuv = true;
> -			imgu0_->viewfinder_->queueBuffer(outbuffer);
> +
> +			for (int i = 0; i < yuvCount; ++i) {
> +				bufferReturnCounters[outbuffer] += 1;
> +				imgu0_->viewfinder_->queueBuffer(outbuffer);
> +			}
>   		} else if (stream == &outCaptureStream_) {
>   			hasCapture = true;
>   
> -			imgu1_->output_->queueBuffer(outbuffer);
> +			for (int i = 0; i < stillCount; ++i) {
> +				bufferReturnCounters[outbuffer] += 1;
> +				imgu1_->output_->queueBuffer(outbuffer);
> +			}
>   		}
>   	}
>   
>   	if (hasYuv) {
> -		bufferReturnCounters[info->rawBuffer] += 1;
> -
> -		imgu0_->param_->queueBuffer(info->paramBuffer);
> -		imgu0_->stat_->queueBuffer(info->statBuffer);
> -		imgu0_->input_->queueBuffer(info->rawBuffer);
> +		for (int i = 0; i < yuvCount; ++i) {
> +			bufferReturnCounters[info->paramBuffer] += 1;
> +			bufferReturnCounters[info->statBuffer] += 1;
> +			bufferReturnCounters[info->rawBuffer] += 1;
> +
> +			imgu0_->param_->queueBuffer(info->paramBuffer);
> +			imgu0_->stat_->queueBuffer(info->statBuffer);
> +			imgu0_->input_->queueBuffer(info->rawBuffer);
> +		}
>   	} else {
>   		info->paramDequeued = true;
>   		info->metadataProcessed = true;
>   	}
>   
>   	if (hasCapture) {
> -		bufferReturnCounters[info->rawBuffer] += 1;
> +		lastRequestSequence_ = info->request->sequence();
>   
> -		imgu1_->param_->queueBuffer(info->captureParamBuffer);
> -		imgu1_->input_->queueBuffer(info->rawBuffer);
> +		for (int i = 0; i < stillCount; ++i) {
> +			bufferReturnCounters[info->captureParamBuffer] += 1;
> +			bufferReturnCounters[info->rawBuffer] += 1;
> +
> +			imgu1_->param_->queueBuffer(info->captureParamBuffer);
> +			imgu1_->input_->queueBuffer(info->rawBuffer);
> +		}
>   	} else {
>   		info->captureParamDequeued = true;
>   	}
> @@ -1503,6 +1534,11 @@ void IPU3CameraData::tryReturnBuffer(FrameBuffer *buffer)
>    */
>   void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>   {
> +	if (--bufferReturnCounters[buffer] > 0)
> +		return;
> +
> +	bufferReturnCounters.erase(buffer);
> +
>   	IPU3Frames::Info *info = frameInfos_.find(buffer);
>   	if (!info)
>   		return;
> @@ -1569,6 +1605,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>   
>   void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>   {
> +	if (--bufferReturnCounters[buffer] > 0)
> +		return;
> +
> +	bufferReturnCounters.erase(buffer);
> +
>   	IPU3Frames::Info *info = frameInfos_.find(buffer);
>   	if (!info)
>   		return;
> @@ -1589,6 +1630,11 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>   
>   void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer)
>   {
> +	if (--bufferReturnCounters[buffer] > 0)
> +		return;
> +
> +	bufferReturnCounters.erase(buffer);
> +
>   	IPU3Frames::Info *info = frameInfos_.find(buffer);
>   	if (!info)
>   		return;
> @@ -1609,6 +1655,11 @@ void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer)
>   
>   void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>   {
> +	if (--bufferReturnCounters[buffer] > 0)
> +		return;
> +
> +	bufferReturnCounters.erase(buffer);
> +
>   	IPU3Frames::Info *info = frameInfos_.find(buffer);
>   	if (!info)
>   		return;