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

[Laurent Pinchart]

Hi Kieran,

On Fri, Feb 19, 2021 at 05:33:23PM +0000, Kieran Bingham wrote:
> 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).

Correct. Until someone needs this feature of couse :-)

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >  
> >  REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
> > 

-- 
Regards,

Laurent Pinchart