[libcamera-devel] [PATCH 08/20] libcamera: pipeline: simple: converter: Decouple input and output completion

[Kieran Bingham]

On 31/01/2021 22:46, Laurent Pinchart wrote:
> The SimpleConverter API signals completion of input and output buffer
> pairs. This unnecessarily delays requeueing the input buffer to the
> video capture queue until the output buffer completes, and also delays
> signalling request completion until the input buffer completes. While
> this shouldn't cause large delays in practice, it will also not scale
> when multi-stream support will be added to the converter class.
> 
> To address the current issue and prepare for the future, decouple
> signalling of input and output buffers completion.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/converter.cpp | 24 +++++--------------
>  src/libcamera/pipeline/simple/converter.h   | 11 ++++-----
>  src/libcamera/pipeline/simple/simple.cpp    | 26 +++++++++++++--------
>  3 files changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index f782fbc63b09..8324baedc198 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -45,8 +45,8 @@ SimpleConverter::SimpleConverter(MediaDevice *media)
>  		return;
>  	}
>  
> -	m2m_->output()->bufferReady.connect(this, &SimpleConverter::outputBufferReady);
> -	m2m_->capture()->bufferReady.connect(this, &SimpleConverter::captureBufferReady);
> +	m2m_->output()->bufferReady.connect(this, &SimpleConverter::m2mInputBufferReady);
> +	m2m_->capture()->bufferReady.connect(this, &SimpleConverter::m2mOutputBufferReady);
>  }
>  
>  std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
> @@ -247,26 +247,14 @@ int SimpleConverter::queueBuffers(FrameBuffer *input, FrameBuffer *output)
>  	return 0;
>  }
>  
> -void SimpleConverter::captureBufferReady(FrameBuffer *buffer)
> +void SimpleConverter::m2mInputBufferReady(FrameBuffer *buffer)
>  {
> -	if (!outputDoneQueue_.empty()) {
> -		FrameBuffer *other = outputDoneQueue_.front();
> -		outputDoneQueue_.pop();
> -		bufferReady.emit(other, buffer);
> -	} else {
> -		captureDoneQueue_.push(buffer);
> -	}
> +	inputBufferReady.emit(buffer);
>  }
>  
> -void SimpleConverter::outputBufferReady(FrameBuffer *buffer)
> +void SimpleConverter::m2mOutputBufferReady(FrameBuffer *buffer)
>  {
> -	if (!captureDoneQueue_.empty()) {
> -		FrameBuffer *other = captureDoneQueue_.front();
> -		captureDoneQueue_.pop();
> -		bufferReady.emit(buffer, other);
> -	} else {
> -		outputDoneQueue_.push(buffer);
> -	}
> +	outputBufferReady.emit(buffer);
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> index 780bfa8f7832..739b24df0200 100644
> --- a/src/libcamera/pipeline/simple/converter.h
> +++ b/src/libcamera/pipeline/simple/converter.h
> @@ -9,7 +9,6 @@
>  #define __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
>  
>  #include <memory>
> -#include <queue>
>  #include <tuple>
>  #include <vector>
>  
> @@ -47,17 +46,15 @@ public:
>  
>  	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
>  
> -	Signal<FrameBuffer *, FrameBuffer *> bufferReady;
> +	Signal<FrameBuffer *> inputBufferReady;
> +	Signal<FrameBuffer *> outputBufferReady;
>  
>  private:
> -	void captureBufferReady(FrameBuffer *buffer);
> -	void outputBufferReady(FrameBuffer *buffer);
> +	void m2mInputBufferReady(FrameBuffer *buffer);
> +	void m2mOutputBufferReady(FrameBuffer *buffer);
>  
>  	std::unique_ptr<V4L2M2MDevice> m2m_;
>  
> -	std::queue<FrameBuffer *> captureDoneQueue_;
> -	std::queue<FrameBuffer *> outputDoneQueue_;
> -
>  	unsigned int inputBufferCount_;
>  	unsigned int outputBufferCount_;
>  };
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 20a4ebca94fd..7f9c57234256 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -144,7 +144,8 @@ private:
>  	}
>  
>  	void bufferReady(FrameBuffer *buffer);
> -	void converterDone(FrameBuffer *input, FrameBuffer *output);
> +	void converterInputDone(FrameBuffer *buffer);
> +	void converterOutputDone(FrameBuffer *buffer);
>  
>  	MediaDevice *media_;
>  	std::map<const MediaEntity *, std::unique_ptr<V4L2VideoDevice>> videos_;
> @@ -768,7 +769,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  				<< "Failed to create converter, disabling format conversion";
>  			converter_.reset();
>  		} else {
> -			converter_->bufferReady.connect(this, &SimplePipelineHandler::converterDone);
> +			converter_->inputBufferReady.connect(this, &SimplePipelineHandler::converterInputDone);
> +			converter_->outputBufferReady.connect(this, &SimplePipelineHandler::converterOutputDone);
>  		}
>  	}
>  
> @@ -925,19 +927,23 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
>  	completeRequest(request);
>  }
>  
> -void SimplePipelineHandler::converterDone(FrameBuffer *input,
> -					  FrameBuffer *output)
> +void SimplePipelineHandler::converterInputDone(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	SimpleCameraData *data = cameraData(activeCamera_);
>  
> -	/* Complete the request. */
> -	Request *request = output->request();
> -	completeBuffer(request, output);
> -	completeRequest(request);
> -
>  	/* Queue the input buffer back for capture. */
> -	data->video_->queueBuffer(input);
> +	data->video_->queueBuffer(buffer);
> +}
> +
> +void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer)
> +{
> +	ASSERT(activeCamera_);
> +
> +	/* Complete the request. */
> +	Request *request = buffer->request();
> +	completeBuffer(request, buffer);
> +	completeRequest(request);
>  }

I assume we still expect to maintain a one-in-one-out principle on the
convertor though (no interlacing/de-interacing type buffers though here).

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

>  
>  REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
> 

-- 
Regards
--
Kieran