[libcamera-devel,09/20] libcamera: pipeline: simple: converter: Add multi-stream support

11088 diff mbox series

While the M2M device backing the converter doesn't support multiple
streams natively, it can be run once per stream to produce multiple
outputs from the same input, with different output formats and sizes.

To support this, create a class to model a stream and move control of
the M2M device to the Stream class. The SimpleConverter class then
creates stream instances and iterates over them. Each stream needs its
own instance of the V4L2M2MDevice, to support different output
configurations. The SimpleConverter class retains a device instance to
support the query operations.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/converter.cpp | 294 ++++++++++++++------
 src/libcamera/pipeline/simple/converter.h   |  44 ++-
 src/libcamera/pipeline/simple/simple.cpp    |   6 +-
 3 files changed, 250 insertions(+), 94 deletions(-)

On 31/01/2021 22:46, Laurent Pinchart wrote:
> While the M2M device backing the converter doesn't support multiple
> streams natively, it can be run once per stream to produce multiple
> outputs from the same input, with different output formats and sizes.

Well, that's going to get a bit more fun ;-)

> To support this, create a class to model a stream and move control of
> the M2M device to the Stream class. The SimpleConverter class then
> creates stream instances and iterates over them. Each stream needs its
> own instance of the V4L2M2MDevice, to support different output
> configurations. The SimpleConverter class retains a device instance to
> support the query operations.

This reminds me of a discussion on linux-media, where having multiple
opens of an M2M device gets quite difficult to distinguish between the
different opened contexts in the kernel debug messages. Each queue has a
different type, but the separate open contexts are harder to distinguish.

I hope our multiple streams log outputs can be clearly separated
(presumably with a stream index)?

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/converter.cpp | 294 ++++++++++++++------
>  src/libcamera/pipeline/simple/converter.h   |  44 ++-
>  src/libcamera/pipeline/simple/simple.cpp    |   6 +-
>  3 files changed, 250 insertions(+), 94 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index 8324baedc198..3db162d9edb8 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -17,12 +17,157 @@
>  
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/utils.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
>  namespace libcamera {
>  
>  LOG_DECLARE_CATEGORY(SimplePipeline)
>  
> +/* -----------------------------------------------------------------------------
> + * SimpleConverter::Stream
> + */
> +
> +SimpleConverter::Stream::Stream(SimpleConverter *converter)
> +	: converter_(converter)
> +{
> +	m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_);
> +
> +	m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);
> +	m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);
> +
> +	int ret = m2m_->open();
> +	if (ret < 0)
> +		m2m_.reset();
> +}
> +
> +int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
> +				       const StreamConfiguration &outputCfg)
> +{
> +	V4L2PixelFormat videoFormat =
> +		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> +
> +	V4L2DeviceFormat format;
> +	format.fourcc = videoFormat;
> +	format.size = inputCfg.size;
> +	format.planesCount = 1;
> +	format.planes[0].bpl = inputCfg.stride;
> +
> +	int ret = m2m_->output()->setFormat(&format);
> +	if (ret < 0) {
> +		LOG(SimplePipeline, Error)
> +			<< "Failed to set input format: " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> +	    format.planes[0].bpl != inputCfg.stride) {
> +		LOG(SimplePipeline, Error)
> +			<< "Input format not supported";
> +		return -EINVAL;
> +	}
> +
> +	/* Set the pixel format and size on the output. */
> +	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> +	format = {};
> +	format.fourcc = videoFormat;
> +	format.size = outputCfg.size;
> +
> +	ret = m2m_->capture()->setFormat(&format);
> +	if (ret < 0) {
> +		LOG(SimplePipeline, Error)
> +			<< "Failed to set output format: " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> +		LOG(SimplePipeline, Error)
> +			<< "Output format not supported";
> +		return -EINVAL;
> +	}
> +
> +	inputBufferCount_ = inputCfg.bufferCount;
> +	outputBufferCount_ = outputCfg.bufferCount;
> +
> +	return 0;
> +}
> +
> +int SimpleConverter::Stream::exportBuffers(unsigned int count,
> +					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	return m2m_->capture()->exportBuffers(count, buffers);
> +}
> +
> +int SimpleConverter::Stream::start()
> +{
> +	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> +	if (ret < 0) {
> +		stop();
> +		return ret;
> +	}
> +
> +	ret = m2m_->output()->streamOn();
> +	if (ret < 0) {
> +		stop();
> +		return ret;
> +	}
> +
> +	ret = m2m_->capture()->streamOn();
> +	if (ret < 0) {
> +		stop();
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void SimpleConverter::Stream::stop()
> +{
> +	m2m_->capture()->streamOff();
> +	m2m_->output()->streamOff();
> +	m2m_->capture()->releaseBuffers();
> +	m2m_->output()->releaseBuffers();
> +}
> +
> +int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,
> +					  FrameBuffer *output)
> +{
> +	int ret = m2m_->output()->queueBuffer(input);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = m2m_->capture()->queueBuffer(output);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)
> +{
> +	auto it = converter_->queue_.find(buffer);
> +	if (it == converter_->queue_.end())
> +		return;
> +
> +	if (!--it->second) {
> +		converter_->inputBufferReady.emit(buffer);
> +		converter_->queue_.erase(it);
> +	}
> +}
> +
> +void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)
> +{
> +	converter_->outputBufferReady.emit(buffer);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * SimpleConverter
> + */
> +
>  SimpleConverter::SimpleConverter(MediaDevice *media)
>  {
>  	/*
> @@ -37,16 +182,14 @@ SimpleConverter::SimpleConverter(MediaDevice *media)
>  	if (it == entities.end())
>  		return;
>  
> -	m2m_ = std::make_unique<V4L2M2MDevice>((*it)->deviceNode());
> +	deviceNode_ = (*it)->deviceNode();
>  
> +	m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_);
>  	int ret = m2m_->open();
>  	if (ret < 0) {
>  		m2m_.reset();
>  		return;
>  	}
> -
> -	m2m_->output()->bufferReady.connect(this, &SimpleConverter::m2mInputBufferReady);
> -	m2m_->capture()->bufferReady.connect(this, &SimpleConverter::m2mOutputBufferReady);
>  }
>  
>  std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
> @@ -141,86 +284,54 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
>  }
>  
>  int SimpleConverter::configure(const StreamConfiguration &inputCfg,
> -			       const StreamConfiguration &outputCfg)
> +			       const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
>  {
> -	int ret;
> +	int ret = 0;
>  
> -	V4L2PixelFormat videoFormat =
> -		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> +	streams_.clear();
> +	streams_.reserve(outputCfgs.size());
>  
> -	V4L2DeviceFormat format;
> -	format.fourcc = videoFormat;
> -	format.size = inputCfg.size;
> -	format.planesCount = 1;
> -	format.planes[0].bpl = inputCfg.stride;
> +	for (const StreamConfiguration &outputCfg : outputCfgs) {
> +		Stream &stream = streams_.emplace_back(this);
>  
> -	ret = m2m_->output()->setFormat(&format);
> -	if (ret < 0) {
> -		LOG(SimplePipeline, Error)
> -			<< "Failed to set input format: " << strerror(-ret);
> -		return ret;
> -	}
> +		if (!stream.isValid()) {
> +			LOG(SimplePipeline, Error) << "Failed to create stream";
> +			ret = -EINVAL;
> +			break;
> +		}
>  
> -	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> -	    format.planes[0].bpl != inputCfg.stride) {
> -		LOG(SimplePipeline, Error)
> -			<< "Input format not supported";
> -		return -EINVAL;
> +		ret = stream.configure(inputCfg, outputCfg);
> +		if (ret < 0)
> +			break;
>  	}
>  
> -	/* Set the pixel format and size on the output. */
> -	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> -	format = {};
> -	format.fourcc = videoFormat;
> -	format.size = outputCfg.size;
> -
> -	ret = m2m_->capture()->setFormat(&format);
>  	if (ret < 0) {
> -		LOG(SimplePipeline, Error)
> -			<< "Failed to set output format: " << strerror(-ret);
> +		streams_.clear();
>  		return ret;
>  	}
>  
> -	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> -		LOG(SimplePipeline, Error)
> -			<< "Output format not supported";
> -		return -EINVAL;
> -	}
> -
> -	inputBufferCount_ = inputCfg.bufferCount;
> -	outputBufferCount_ = outputCfg.bufferCount;
> -
>  	return 0;
>  }
>  
> -int SimpleConverter::exportBuffers(unsigned int count,
> +int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,
>  				   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> -	return m2m_->capture()->exportBuffers(count, buffers);
> +	if (output >= streams_.size())
> +		return -EINVAL;
> +
> +	return streams_[output].exportBuffers(count, buffers);
>  }
>  
>  int SimpleConverter::start()
>  {
> -	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> -	if (ret < 0)
> -		return ret;
> +	int ret;
>  
> -	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> -	if (ret < 0) {
> -		stop();
> -		return ret;
> -	}
> -
> -	ret = m2m_->output()->streamOn();
> -	if (ret < 0) {
> -		stop();
> -		return ret;
> -	}
> -
> -	ret = m2m_->capture()->streamOn();
> -	if (ret < 0) {
> -		stop();
> -		return ret;
> +	for (Stream &stream : utils::reverse(streams_)) {

Do we have to start in reverse order?
I understand stopping them in the reverse order ... but starting?


> +		ret = stream.start();
> +		if (ret < 0) {
> +			stop();
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -228,33 +339,48 @@ int SimpleConverter::start()
>  
>  void SimpleConverter::stop()
>  {
> -	m2m_->capture()->streamOff();
> -	m2m_->output()->streamOff();
> -	m2m_->capture()->releaseBuffers();
> -	m2m_->output()->releaseBuffers();
> +	for (Stream &stream : utils::reverse(streams_))
> +		stream.stop();

much nicer ;-)

>  }
>  
> -int SimpleConverter::queueBuffers(FrameBuffer *input, FrameBuffer *output)
> +int SimpleConverter::queueBuffers(FrameBuffer *input,
> +				  const std::map<unsigned int, FrameBuffer *> &outputs)
>  {
> -	int ret = m2m_->output()->queueBuffer(input);
> -	if (ret < 0)
> -		return ret;
> +	unsigned int mask = 0;
> +	int ret;
>  
> -	ret = m2m_->capture()->queueBuffer(output);
> -	if (ret < 0)
> -		return ret;
> +	/* Validate the outputs as a sanity check. */
> +	if (outputs.empty())
> +		return -EINVAL;
> +
> +	for (auto [index, buffer] : outputs) {
> +		if (index >= streams_.size())
> +			return -EINVAL;
> +		if (mask & (1 << index))

I presume mask is checking to see/prevent there being multiple copies of
the same stream in outputs or something?

But this isn't very clear or readable - so a comment at least would help.


> +			return -EINVAL;
> +		if (!buffer)
> +			return -EINVAL;
> +
> +		mask |= 1 << index;
> +	}
> +
> +	/* Queue the input and output buffers to all the streams. */
> +	for (auto [index, buffer] : outputs) {
> +		ret = streams_[index].queueBuffers(input, buffer);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Add the input buffer to the queue, with the number of streams as a
> +	 * reference count. Completion of the input buffer will be signalled by
> +	 * the stream that releases the last reference.
> +	 */
> +	queue_.emplace(std::piecewise_construct,
> +		       std::forward_as_tuple(input),
> +		       std::forward_as_tuple(outputs.size()));
>  
>  	return 0;
>  }
>  
> -void SimpleConverter::m2mInputBufferReady(FrameBuffer *buffer)
> -{
> -	inputBufferReady.emit(buffer);
> -}
> -
> -void SimpleConverter::m2mOutputBufferReady(FrameBuffer *buffer)
> -{
> -	outputBufferReady.emit(buffer);
> -}
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> index 739b24df0200..176978eefe48 100644
> --- a/src/libcamera/pipeline/simple/converter.h
> +++ b/src/libcamera/pipeline/simple/converter.h
> @@ -8,7 +8,10 @@
>  #ifndef __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
>  #define __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
>  
> +#include <functional>
> +#include <map>
>  #include <memory>
> +#include <string>
>  #include <tuple>
>  #include <vector>
>  
> @@ -37,26 +40,53 @@ public:
>  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
>  
>  	int configure(const StreamConfiguration &inputCfg,
> -		      const StreamConfiguration &outputCfg);
> -	int exportBuffers(unsigned int count,
> +		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> +	int exportBuffers(unsigned int ouput, unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  
>  	int start();
>  	void stop();
>  
> -	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> +	int queueBuffers(FrameBuffer *input,
> +			 const std::map<unsigned int, FrameBuffer *> &outputs);
>  
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
>  
>  private:
> -	void m2mInputBufferReady(FrameBuffer *buffer);
> -	void m2mOutputBufferReady(FrameBuffer *buffer);
> +	class Stream
> +	{
> +	public:
> +		Stream(SimpleConverter *converter);
>  
> +		bool isValid() const { return m2m_ != nullptr; }
> +
> +		int configure(const StreamConfiguration &inputCfg,
> +			      const StreamConfiguration &outputCfg);
> +		int exportBuffers(unsigned int count,
> +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +
> +		int start();
> +		void stop();
> +
> +		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> +
> +	private:
> +		void captureBufferReady(FrameBuffer *buffer);
> +		void outputBufferReady(FrameBuffer *buffer);
> +
> +		SimpleConverter *converter_;
> +		std::unique_ptr<V4L2M2MDevice> m2m_;
> +
> +		unsigned int inputBufferCount_;
> +		unsigned int outputBufferCount_;
> +	};
> +
> +	std::string deviceNode_;
>  	std::unique_ptr<V4L2M2MDevice> m2m_;
>  
> -	unsigned int inputBufferCount_;
> -	unsigned int outputBufferCount_;
> +	std::vector<Stream> streams_;
> +	std::map<FrameBuffer *, unsigned int> queue_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 7f9c57234256..b7a890ab772e 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -610,7 +610,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  		inputCfg.stride = captureFormat.planes[0].bpl;
>  		inputCfg.bufferCount = cfg.bufferCount;
>  
> -		ret = converter_->configure(inputCfg, cfg);
> +		ret = converter_->configure(inputCfg, { cfg });
>  		if (ret < 0) {
>  			LOG(SimplePipeline, Error)
>  				<< "Unable to configure converter";
> @@ -636,7 +636,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
>  	 * whether the converter is used or not.
>  	 */
>  	if (useConverter_)
> -		return converter_->exportBuffers(count, buffers);
> +		return converter_->exportBuffers(0, count, buffers);

I was wondering if it would be better to reference the Stream objects
rather than indexes, but I can see that might become confusing with our
Stream *stream we already have ...

And for now, we only have a single stream so index zero is enough for
now ...

Only a few trivial comments.
With those handled as you wish:

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

>  	else
>  		return data->video_->exportBuffers(count, buffers);
>  }
> @@ -917,7 +917,7 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
>  		FrameBuffer *output = converterQueue_.front();
>  		converterQueue_.pop();
>  
> -		converter_->queueBuffers(buffer, output);
> +		converter_->queueBuffers(buffer, { { 0, output } });
>  		return;
>  	}
>  
>

Hi Kieran,

On Mon, Feb 22, 2021 at 01:08:12PM +0000, Kieran Bingham wrote:
> On 31/01/2021 22:46, Laurent Pinchart wrote:
> > While the M2M device backing the converter doesn't support multiple
> > streams natively, it can be run once per stream to produce multiple
> > outputs from the same input, with different output formats and sizes.
> 
> Well, that's going to get a bit more fun ;-)
> 
> > To support this, create a class to model a stream and move control of
> > the M2M device to the Stream class. The SimpleConverter class then
> > creates stream instances and iterates over them. Each stream needs its
> > own instance of the V4L2M2MDevice, to support different output
> > configurations. The SimpleConverter class retains a device instance to
> > support the query operations.
> 
> This reminds me of a discussion on linux-media, where having multiple
> opens of an M2M device gets quite difficult to distinguish between the
> different opened contexts in the kernel debug messages. Each queue has a
> different type, but the separate open contexts are harder to distinguish.
> 
> I hope our multiple streams log outputs can be clearly separated
> (presumably with a stream index)?

Good point... Seems I need to fix that :-)

Beside handling this in the converter, we could print the fd in
V4L2VideoDevice::logPrefix(), do you think that would be useful ?

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/simple/converter.cpp | 294 ++++++++++++++------
> >  src/libcamera/pipeline/simple/converter.h   |  44 ++-
> >  src/libcamera/pipeline/simple/simple.cpp    |   6 +-
> >  3 files changed, 250 insertions(+), 94 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> > index 8324baedc198..3db162d9edb8 100644
> > --- a/src/libcamera/pipeline/simple/converter.cpp
> > +++ b/src/libcamera/pipeline/simple/converter.cpp
> > @@ -17,12 +17,157 @@
> >  
> >  #include "libcamera/internal/log.h"
> >  #include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/utils.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> >  
> >  namespace libcamera {
> >  
> >  LOG_DECLARE_CATEGORY(SimplePipeline)
> >  
> > +/* -----------------------------------------------------------------------------
> > + * SimpleConverter::Stream
> > + */
> > +
> > +SimpleConverter::Stream::Stream(SimpleConverter *converter)
> > +	: converter_(converter)
> > +{
> > +	m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_);
> > +
> > +	m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);
> > +	m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);
> > +
> > +	int ret = m2m_->open();
> > +	if (ret < 0)
> > +		m2m_.reset();
> > +}
> > +
> > +int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
> > +				       const StreamConfiguration &outputCfg)
> > +{
> > +	V4L2PixelFormat videoFormat =
> > +		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> > +
> > +	V4L2DeviceFormat format;
> > +	format.fourcc = videoFormat;
> > +	format.size = inputCfg.size;
> > +	format.planesCount = 1;
> > +	format.planes[0].bpl = inputCfg.stride;
> > +
> > +	int ret = m2m_->output()->setFormat(&format);
> > +	if (ret < 0) {
> > +		LOG(SimplePipeline, Error)
> > +			<< "Failed to set input format: " << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> > +	    format.planes[0].bpl != inputCfg.stride) {
> > +		LOG(SimplePipeline, Error)
> > +			<< "Input format not supported";
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Set the pixel format and size on the output. */
> > +	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> > +	format = {};
> > +	format.fourcc = videoFormat;
> > +	format.size = outputCfg.size;
> > +
> > +	ret = m2m_->capture()->setFormat(&format);
> > +	if (ret < 0) {
> > +		LOG(SimplePipeline, Error)
> > +			<< "Failed to set output format: " << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> > +		LOG(SimplePipeline, Error)
> > +			<< "Output format not supported";
> > +		return -EINVAL;
> > +	}
> > +
> > +	inputBufferCount_ = inputCfg.bufferCount;
> > +	outputBufferCount_ = outputCfg.bufferCount;
> > +
> > +	return 0;
> > +}
> > +
> > +int SimpleConverter::Stream::exportBuffers(unsigned int count,
> > +					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > +	return m2m_->capture()->exportBuffers(count, buffers);
> > +}
> > +
> > +int SimpleConverter::Stream::start()
> > +{
> > +	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> > +	if (ret < 0) {
> > +		stop();
> > +		return ret;
> > +	}
> > +
> > +	ret = m2m_->output()->streamOn();
> > +	if (ret < 0) {
> > +		stop();
> > +		return ret;
> > +	}
> > +
> > +	ret = m2m_->capture()->streamOn();
> > +	if (ret < 0) {
> > +		stop();
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void SimpleConverter::Stream::stop()
> > +{
> > +	m2m_->capture()->streamOff();
> > +	m2m_->output()->streamOff();
> > +	m2m_->capture()->releaseBuffers();
> > +	m2m_->output()->releaseBuffers();
> > +}
> > +
> > +int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,
> > +					  FrameBuffer *output)
> > +{
> > +	int ret = m2m_->output()->queueBuffer(input);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = m2m_->capture()->queueBuffer(output);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)
> > +{
> > +	auto it = converter_->queue_.find(buffer);
> > +	if (it == converter_->queue_.end())
> > +		return;
> > +
> > +	if (!--it->second) {
> > +		converter_->inputBufferReady.emit(buffer);
> > +		converter_->queue_.erase(it);
> > +	}
> > +}
> > +
> > +void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)
> > +{
> > +	converter_->outputBufferReady.emit(buffer);
> > +}
> > +
> > +/* -----------------------------------------------------------------------------
> > + * SimpleConverter
> > + */
> > +
> >  SimpleConverter::SimpleConverter(MediaDevice *media)
> >  {
> >  	/*
> > @@ -37,16 +182,14 @@ SimpleConverter::SimpleConverter(MediaDevice *media)
> >  	if (it == entities.end())
> >  		return;
> >  
> > -	m2m_ = std::make_unique<V4L2M2MDevice>((*it)->deviceNode());
> > +	deviceNode_ = (*it)->deviceNode();
> >  
> > +	m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_);
> >  	int ret = m2m_->open();
> >  	if (ret < 0) {
> >  		m2m_.reset();
> >  		return;
> >  	}
> > -
> > -	m2m_->output()->bufferReady.connect(this, &SimpleConverter::m2mInputBufferReady);
> > -	m2m_->capture()->bufferReady.connect(this, &SimpleConverter::m2mOutputBufferReady);
> >  }
> >  
> >  std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
> > @@ -141,86 +284,54 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
> >  }
> >  
> >  int SimpleConverter::configure(const StreamConfiguration &inputCfg,
> > -			       const StreamConfiguration &outputCfg)
> > +			       const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
> >  {
> > -	int ret;
> > +	int ret = 0;
> >  
> > -	V4L2PixelFormat videoFormat =
> > -		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> > +	streams_.clear();
> > +	streams_.reserve(outputCfgs.size());
> >  
> > -	V4L2DeviceFormat format;
> > -	format.fourcc = videoFormat;
> > -	format.size = inputCfg.size;
> > -	format.planesCount = 1;
> > -	format.planes[0].bpl = inputCfg.stride;
> > +	for (const StreamConfiguration &outputCfg : outputCfgs) {
> > +		Stream &stream = streams_.emplace_back(this);
> >  
> > -	ret = m2m_->output()->setFormat(&format);
> > -	if (ret < 0) {
> > -		LOG(SimplePipeline, Error)
> > -			<< "Failed to set input format: " << strerror(-ret);
> > -		return ret;
> > -	}
> > +		if (!stream.isValid()) {
> > +			LOG(SimplePipeline, Error) << "Failed to create stream";
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> >  
> > -	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> > -	    format.planes[0].bpl != inputCfg.stride) {
> > -		LOG(SimplePipeline, Error)
> > -			<< "Input format not supported";
> > -		return -EINVAL;
> > +		ret = stream.configure(inputCfg, outputCfg);
> > +		if (ret < 0)
> > +			break;
> >  	}
> >  
> > -	/* Set the pixel format and size on the output. */
> > -	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> > -	format = {};
> > -	format.fourcc = videoFormat;
> > -	format.size = outputCfg.size;
> > -
> > -	ret = m2m_->capture()->setFormat(&format);
> >  	if (ret < 0) {
> > -		LOG(SimplePipeline, Error)
> > -			<< "Failed to set output format: " << strerror(-ret);
> > +		streams_.clear();
> >  		return ret;
> >  	}
> >  
> > -	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> > -		LOG(SimplePipeline, Error)
> > -			<< "Output format not supported";
> > -		return -EINVAL;
> > -	}
> > -
> > -	inputBufferCount_ = inputCfg.bufferCount;
> > -	outputBufferCount_ = outputCfg.bufferCount;
> > -
> >  	return 0;
> >  }
> >  
> > -int SimpleConverter::exportBuffers(unsigned int count,
> > +int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,
> >  				   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> > -	return m2m_->capture()->exportBuffers(count, buffers);
> > +	if (output >= streams_.size())
> > +		return -EINVAL;
> > +
> > +	return streams_[output].exportBuffers(count, buffers);
> >  }
> >  
> >  int SimpleConverter::start()
> >  {
> > -	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> > -	if (ret < 0)
> > -		return ret;
> > +	int ret;
> >  
> > -	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> > -	if (ret < 0) {
> > -		stop();
> > -		return ret;
> > -	}
> > -
> > -	ret = m2m_->output()->streamOn();
> > -	if (ret < 0) {
> > -		stop();
> > -		return ret;
> > -	}
> > -
> > -	ret = m2m_->capture()->streamOn();
> > -	if (ret < 0) {
> > -		stop();
> > -		return ret;
> > +	for (Stream &stream : utils::reverse(streams_)) {
> 
> Do we have to start in reverse order?
> I understand stopping them in the reverse order ... but starting?

Indeed, I'll fix that.

> > +		ret = stream.start();
> > +		if (ret < 0) {
> > +			stop();
> > +			return ret;
> > +		}
> >  	}
> >  
> >  	return 0;
> > @@ -228,33 +339,48 @@ int SimpleConverter::start()
> >  
> >  void SimpleConverter::stop()
> >  {
> > -	m2m_->capture()->streamOff();
> > -	m2m_->output()->streamOff();
> > -	m2m_->capture()->releaseBuffers();
> > -	m2m_->output()->releaseBuffers();
> > +	for (Stream &stream : utils::reverse(streams_))
> > +		stream.stop();
> 
> much nicer ;-)

I have to confess I really like how C++ makes this possible. The price
to pay, of course, is total madness after studying templates.

> >  }
> >  
> > -int SimpleConverter::queueBuffers(FrameBuffer *input, FrameBuffer *output)
> > +int SimpleConverter::queueBuffers(FrameBuffer *input,
> > +				  const std::map<unsigned int, FrameBuffer *> &outputs)
> >  {
> > -	int ret = m2m_->output()->queueBuffer(input);
> > -	if (ret < 0)
> > -		return ret;
> > +	unsigned int mask = 0;
> > +	int ret;
> >  
> > -	ret = m2m_->capture()->queueBuffer(output);
> > -	if (ret < 0)
> > -		return ret;
> > +	/* Validate the outputs as a sanity check. */
> > +	if (outputs.empty())
> > +		return -EINVAL;
> > +
> > +	for (auto [index, buffer] : outputs) {
> > +		if (index >= streams_.size())
> > +			return -EINVAL;
> > +		if (mask & (1 << index))
> 
> I presume mask is checking to see/prevent there being multiple copies of
> the same stream in outputs or something?
> 
> But this isn't very clear or readable - so a comment at least would help.

That's correct. I'll add a comment.

> > +			return -EINVAL;
> > +		if (!buffer)
> > +			return -EINVAL;
> > +
> > +		mask |= 1 << index;
> > +	}
> > +
> > +	/* Queue the input and output buffers to all the streams. */
> > +	for (auto [index, buffer] : outputs) {
> > +		ret = streams_[index].queueBuffers(input, buffer);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	/*
> > +	 * Add the input buffer to the queue, with the number of streams as a
> > +	 * reference count. Completion of the input buffer will be signalled by
> > +	 * the stream that releases the last reference.
> > +	 */
> > +	queue_.emplace(std::piecewise_construct,
> > +		       std::forward_as_tuple(input),
> > +		       std::forward_as_tuple(outputs.size()));
> >  
> >  	return 0;
> >  }
> >  
> > -void SimpleConverter::m2mInputBufferReady(FrameBuffer *buffer)
> > -{
> > -	inputBufferReady.emit(buffer);
> > -}
> > -
> > -void SimpleConverter::m2mOutputBufferReady(FrameBuffer *buffer)
> > -{
> > -	outputBufferReady.emit(buffer);
> > -}
> > -
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> > index 739b24df0200..176978eefe48 100644
> > --- a/src/libcamera/pipeline/simple/converter.h
> > +++ b/src/libcamera/pipeline/simple/converter.h
> > @@ -8,7 +8,10 @@
> >  #ifndef __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
> >  #define __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
> >  
> > +#include <functional>
> > +#include <map>
> >  #include <memory>
> > +#include <string>
> >  #include <tuple>
> >  #include <vector>
> >  
> > @@ -37,26 +40,53 @@ public:
> >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
> >  
> >  	int configure(const StreamConfiguration &inputCfg,
> > -		      const StreamConfiguration &outputCfg);
> > -	int exportBuffers(unsigned int count,
> > +		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> > +	int exportBuffers(unsigned int ouput, unsigned int count,
> >  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >  
> >  	int start();
> >  	void stop();
> >  
> > -	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > +	int queueBuffers(FrameBuffer *input,
> > +			 const std::map<unsigned int, FrameBuffer *> &outputs);
> >  
> >  	Signal<FrameBuffer *> inputBufferReady;
> >  	Signal<FrameBuffer *> outputBufferReady;
> >  
> >  private:
> > -	void m2mInputBufferReady(FrameBuffer *buffer);
> > -	void m2mOutputBufferReady(FrameBuffer *buffer);
> > +	class Stream
> > +	{
> > +	public:
> > +		Stream(SimpleConverter *converter);
> >  
> > +		bool isValid() const { return m2m_ != nullptr; }
> > +
> > +		int configure(const StreamConfiguration &inputCfg,
> > +			      const StreamConfiguration &outputCfg);
> > +		int exportBuffers(unsigned int count,
> > +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > +
> > +		int start();
> > +		void stop();
> > +
> > +		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > +
> > +	private:
> > +		void captureBufferReady(FrameBuffer *buffer);
> > +		void outputBufferReady(FrameBuffer *buffer);
> > +
> > +		SimpleConverter *converter_;
> > +		std::unique_ptr<V4L2M2MDevice> m2m_;
> > +
> > +		unsigned int inputBufferCount_;
> > +		unsigned int outputBufferCount_;
> > +	};
> > +
> > +	std::string deviceNode_;
> >  	std::unique_ptr<V4L2M2MDevice> m2m_;
> >  
> > -	unsigned int inputBufferCount_;
> > -	unsigned int outputBufferCount_;
> > +	std::vector<Stream> streams_;
> > +	std::map<FrameBuffer *, unsigned int> queue_;
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 7f9c57234256..b7a890ab772e 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -610,7 +610,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >  		inputCfg.stride = captureFormat.planes[0].bpl;
> >  		inputCfg.bufferCount = cfg.bufferCount;
> >  
> > -		ret = converter_->configure(inputCfg, cfg);
> > +		ret = converter_->configure(inputCfg, { cfg });
> >  		if (ret < 0) {
> >  			LOG(SimplePipeline, Error)
> >  				<< "Unable to configure converter";
> > @@ -636,7 +636,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> >  	 * whether the converter is used or not.
> >  	 */
> >  	if (useConverter_)
> > -		return converter_->exportBuffers(count, buffers);
> > +		return converter_->exportBuffers(0, count, buffers);
> 
> I was wondering if it would be better to reference the Stream objects
> rather than indexes, but I can see that might become confusing with our
> Stream *stream we already have ...

I've thought about that, but it would require exposing the Stream class.
If this was a public API I'd put more effort in the design :-)

> And for now, we only have a single stream so index zero is enough for
> now ...
> 
> Only a few trivial comments.
> With those handled as you wish:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >  	else
> >  		return data->video_->exportBuffers(count, buffers);
> >  }
> > @@ -917,7 +917,7 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
> >  		FrameBuffer *output = converterQueue_.front();
> >  		converterQueue_.pop();
> >  
> > -		converter_->queueBuffers(buffer, output);
> > +		converter_->queueBuffers(buffer, { { 0, output } });
> >  		return;
> >  	}
> >

Hi Laurent and Kieran,

On Tue, Mar 02, 2021 at 12:19:02AM +0200, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Feb 22, 2021 at 01:08:12PM +0000, Kieran Bingham wrote:
> > On 31/01/2021 22:46, Laurent Pinchart wrote:
> > > While the M2M device backing the converter doesn't support multiple
> > > streams natively, it can be run once per stream to produce multiple
> > > outputs from the same input, with different output formats and sizes.
> > 
> > Well, that's going to get a bit more fun ;-)
> > 
> > > To support this, create a class to model a stream and move control of
> > > the M2M device to the Stream class. The SimpleConverter class then
> > > creates stream instances and iterates over them. Each stream needs its
> > > own instance of the V4L2M2MDevice, to support different output
> > > configurations. The SimpleConverter class retains a device instance to
> > > support the query operations.
> > 
> > This reminds me of a discussion on linux-media, where having multiple
> > opens of an M2M device gets quite difficult to distinguish between the
> > different opened contexts in the kernel debug messages. Each queue has a
> > different type, but the separate open contexts are harder to distinguish.
> > 
> > I hope our multiple streams log outputs can be clearly separated
> > (presumably with a stream index)?
> 
> Good point... Seems I need to fix that :-)
> 
> Beside handling this in the converter, we could print the fd in
> V4L2VideoDevice::logPrefix(), do you think that would be useful ?

Ooh yeah I think that's a good idea.

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/simple/converter.cpp | 294 ++++++++++++++------
> > >  src/libcamera/pipeline/simple/converter.h   |  44 ++-
> > >  src/libcamera/pipeline/simple/simple.cpp    |   6 +-
> > >  3 files changed, 250 insertions(+), 94 deletions(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> > > index 8324baedc198..3db162d9edb8 100644
> > > --- a/src/libcamera/pipeline/simple/converter.cpp
> > > +++ b/src/libcamera/pipeline/simple/converter.cpp
> > > @@ -17,12 +17,157 @@
> > >  
> > >  #include "libcamera/internal/log.h"
> > >  #include "libcamera/internal/media_device.h"
> > > +#include "libcamera/internal/utils.h"
> > >  #include "libcamera/internal/v4l2_videodevice.h"
> > >  
> > >  namespace libcamera {
> > >  
> > >  LOG_DECLARE_CATEGORY(SimplePipeline)
> > >  
> > > +/* -----------------------------------------------------------------------------
> > > + * SimpleConverter::Stream
> > > + */
> > > +
> > > +SimpleConverter::Stream::Stream(SimpleConverter *converter)
> > > +	: converter_(converter)
> > > +{
> > > +	m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_);
> > > +
> > > +	m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);
> > > +	m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);
> > > +
> > > +	int ret = m2m_->open();
> > > +	if (ret < 0)
> > > +		m2m_.reset();
> > > +}
> > > +
> > > +int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
> > > +				       const StreamConfiguration &outputCfg)
> > > +{
> > > +	V4L2PixelFormat videoFormat =
> > > +		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> > > +
> > > +	V4L2DeviceFormat format;
> > > +	format.fourcc = videoFormat;
> > > +	format.size = inputCfg.size;
> > > +	format.planesCount = 1;
> > > +	format.planes[0].bpl = inputCfg.stride;
> > > +
> > > +	int ret = m2m_->output()->setFormat(&format);
> > > +	if (ret < 0) {
> > > +		LOG(SimplePipeline, Error)
> > > +			<< "Failed to set input format: " << strerror(-ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> > > +	    format.planes[0].bpl != inputCfg.stride) {
> > > +		LOG(SimplePipeline, Error)
> > > +			<< "Input format not supported";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Set the pixel format and size on the output. */
> > > +	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> > > +	format = {};
> > > +	format.fourcc = videoFormat;
> > > +	format.size = outputCfg.size;

Why doesn't the output need plane information?

> > > +
> > > +	ret = m2m_->capture()->setFormat(&format);
> > > +	if (ret < 0) {
> > > +		LOG(SimplePipeline, Error)
> > > +			<< "Failed to set output format: " << strerror(-ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> > > +		LOG(SimplePipeline, Error)
> > > +			<< "Output format not supported";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	inputBufferCount_ = inputCfg.bufferCount;
> > > +	outputBufferCount_ = outputCfg.bufferCount;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int SimpleConverter::Stream::exportBuffers(unsigned int count,
> > > +					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > +{
> > > +	return m2m_->capture()->exportBuffers(count, buffers);
> > > +}
> > > +
> > > +int SimpleConverter::Stream::start()
> > > +{
> > > +	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> > > +	if (ret < 0) {
> > > +		stop();
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = m2m_->output()->streamOn();
> > > +	if (ret < 0) {
> > > +		stop();
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = m2m_->capture()->streamOn();
> > > +	if (ret < 0) {
> > > +		stop();
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void SimpleConverter::Stream::stop()
> > > +{
> > > +	m2m_->capture()->streamOff();
> > > +	m2m_->output()->streamOff();
> > > +	m2m_->capture()->releaseBuffers();
> > > +	m2m_->output()->releaseBuffers();
> > > +}
> > > +
> > > +int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,
> > > +					  FrameBuffer *output)
> > > +{
> > > +	int ret = m2m_->output()->queueBuffer(input);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = m2m_->capture()->queueBuffer(output);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)
> > > +{
> > > +	auto it = converter_->queue_.find(buffer);
> > > +	if (it == converter_->queue_.end())
> > > +		return;
> > > +
> > > +	if (!--it->second) {
> > > +		converter_->inputBufferReady.emit(buffer);
> > > +		converter_->queue_.erase(it);
> > > +	}
> > > +}
> > > +
> > > +void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)
> > > +{
> > > +	converter_->outputBufferReady.emit(buffer);
> > > +}
> > > +
> > > +/* -----------------------------------------------------------------------------
> > > + * SimpleConverter
> > > + */
> > > +
> > >  SimpleConverter::SimpleConverter(MediaDevice *media)
> > >  {
> > >  	/*
> > > @@ -37,16 +182,14 @@ SimpleConverter::SimpleConverter(MediaDevice *media)
> > >  	if (it == entities.end())
> > >  		return;
> > >  
> > > -	m2m_ = std::make_unique<V4L2M2MDevice>((*it)->deviceNode());
> > > +	deviceNode_ = (*it)->deviceNode();
> > >  
> > > +	m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_);
> > >  	int ret = m2m_->open();
> > >  	if (ret < 0) {
> > >  		m2m_.reset();
> > >  		return;
> > >  	}
> > > -
> > > -	m2m_->output()->bufferReady.connect(this, &SimpleConverter::m2mInputBufferReady);
> > > -	m2m_->capture()->bufferReady.connect(this, &SimpleConverter::m2mOutputBufferReady);
> > >  }
> > >  
> > >  std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
> > > @@ -141,86 +284,54 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
> > >  }
> > >  
> > >  int SimpleConverter::configure(const StreamConfiguration &inputCfg,
> > > -			       const StreamConfiguration &outputCfg)
> > > +			       const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
> > >  {
> > > -	int ret;
> > > +	int ret = 0;
> > >  
> > > -	V4L2PixelFormat videoFormat =
> > > -		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> > > +	streams_.clear();
> > > +	streams_.reserve(outputCfgs.size());
> > >  
> > > -	V4L2DeviceFormat format;
> > > -	format.fourcc = videoFormat;
> > > -	format.size = inputCfg.size;
> > > -	format.planesCount = 1;
> > > -	format.planes[0].bpl = inputCfg.stride;
> > > +	for (const StreamConfiguration &outputCfg : outputCfgs) {
> > > +		Stream &stream = streams_.emplace_back(this);
> > >  
> > > -	ret = m2m_->output()->setFormat(&format);
> > > -	if (ret < 0) {
> > > -		LOG(SimplePipeline, Error)
> > > -			<< "Failed to set input format: " << strerror(-ret);
> > > -		return ret;
> > > -	}
> > > +		if (!stream.isValid()) {
> > > +			LOG(SimplePipeline, Error) << "Failed to create stream";
> > > +			ret = -EINVAL;
> > > +			break;
> > > +		}
> > >  
> > > -	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> > > -	    format.planes[0].bpl != inputCfg.stride) {
> > > -		LOG(SimplePipeline, Error)
> > > -			<< "Input format not supported";
> > > -		return -EINVAL;
> > > +		ret = stream.configure(inputCfg, outputCfg);
> > > +		if (ret < 0)
> > > +			break;
> > >  	}
> > >  
> > > -	/* Set the pixel format and size on the output. */
> > > -	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> > > -	format = {};
> > > -	format.fourcc = videoFormat;
> > > -	format.size = outputCfg.size;
> > > -
> > > -	ret = m2m_->capture()->setFormat(&format);
> > >  	if (ret < 0) {
> > > -		LOG(SimplePipeline, Error)
> > > -			<< "Failed to set output format: " << strerror(-ret);
> > > +		streams_.clear();
> > >  		return ret;
> > >  	}
> > >  
> > > -	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> > > -		LOG(SimplePipeline, Error)
> > > -			<< "Output format not supported";
> > > -		return -EINVAL;
> > > -	}
> > > -
> > > -	inputBufferCount_ = inputCfg.bufferCount;
> > > -	outputBufferCount_ = outputCfg.bufferCount;
> > > -
> > >  	return 0;
> > >  }
> > >  
> > > -int SimpleConverter::exportBuffers(unsigned int count,
> > > +int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,
> > >  				   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > > -	return m2m_->capture()->exportBuffers(count, buffers);
> > > +	if (output >= streams_.size())
> > > +		return -EINVAL;
> > > +
> > > +	return streams_[output].exportBuffers(count, buffers);
> > >  }
> > >  
> > >  int SimpleConverter::start()
> > >  {
> > > -	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> > > -	if (ret < 0)
> > > -		return ret;
> > > +	int ret;
> > >  
> > > -	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> > > -	if (ret < 0) {
> > > -		stop();
> > > -		return ret;
> > > -	}
> > > -
> > > -	ret = m2m_->output()->streamOn();
> > > -	if (ret < 0) {
> > > -		stop();
> > > -		return ret;
> > > -	}
> > > -
> > > -	ret = m2m_->capture()->streamOn();
> > > -	if (ret < 0) {
> > > -		stop();
> > > -		return ret;
> > > +	for (Stream &stream : utils::reverse(streams_)) {
> > 
> > Do we have to start in reverse order?
> > I understand stopping them in the reverse order ... but starting?
> 
> Indeed, I'll fix that.

Ah, I was wondering about the reasoning for this. Now I see.

> > > +		ret = stream.start();
> > > +		if (ret < 0) {
> > > +			stop();
> > > +			return ret;
> > > +		}
> > >  	}
> > >  
> > >  	return 0;
> > > @@ -228,33 +339,48 @@ int SimpleConverter::start()
> > >  
> > >  void SimpleConverter::stop()
> > >  {
> > > -	m2m_->capture()->streamOff();
> > > -	m2m_->output()->streamOff();
> > > -	m2m_->capture()->releaseBuffers();
> > > -	m2m_->output()->releaseBuffers();
> > > +	for (Stream &stream : utils::reverse(streams_))
> > > +		stream.stop();
> > 
> > much nicer ;-)
> 
> I have to confess I really like how C++ makes this possible. The price
> to pay, of course, is total madness after studying templates.
> 
> > >  }
> > >  
> > > -int SimpleConverter::queueBuffers(FrameBuffer *input, FrameBuffer *output)
> > > +int SimpleConverter::queueBuffers(FrameBuffer *input,
> > > +				  const std::map<unsigned int, FrameBuffer *> &outputs)
> > >  {
> > > -	int ret = m2m_->output()->queueBuffer(input);
> > > -	if (ret < 0)
> > > -		return ret;
> > > +	unsigned int mask = 0;
> > > +	int ret;
> > >  
> > > -	ret = m2m_->capture()->queueBuffer(output);
> > > -	if (ret < 0)
> > > -		return ret;
> > > +	/* Validate the outputs as a sanity check. */
> > > +	if (outputs.empty())
> > > +		return -EINVAL;
> > > +
> > > +	for (auto [index, buffer] : outputs) {
> > > +		if (index >= streams_.size())
> > > +			return -EINVAL;
> > > +		if (mask & (1 << index))
> > 
> > I presume mask is checking to see/prevent there being multiple copies of
> > the same stream in outputs or something?
> > 
> > But this isn't very clear or readable - so a comment at least would help.
> 
> That's correct. I'll add a comment.

I thought it was fine, just that maybe mask could be named something
else. But I can't think of anything better, so maybe a comment would be
easier.

> > > +			return -EINVAL;
> > > +		if (!buffer)
> > > +			return -EINVAL;
> > > +
> > > +		mask |= 1 << index;
> > > +	}
> > > +
> > > +	/* Queue the input and output buffers to all the streams. */
> > > +	for (auto [index, buffer] : outputs) {
> > > +		ret = streams_[index].queueBuffers(input, buffer);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Add the input buffer to the queue, with the number of streams as a
> > > +	 * reference count. Completion of the input buffer will be signalled by
> > > +	 * the stream that releases the last reference.
> > > +	 */
> > > +	queue_.emplace(std::piecewise_construct,
> > > +		       std::forward_as_tuple(input),
> > > +		       std::forward_as_tuple(outputs.size()));
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > -void SimpleConverter::m2mInputBufferReady(FrameBuffer *buffer)
> > > -{
> > > -	inputBufferReady.emit(buffer);
> > > -}
> > > -
> > > -void SimpleConverter::m2mOutputBufferReady(FrameBuffer *buffer)
> > > -{
> > > -	outputBufferReady.emit(buffer);
> > > -}
> > > -
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> > > index 739b24df0200..176978eefe48 100644
> > > --- a/src/libcamera/pipeline/simple/converter.h
> > > +++ b/src/libcamera/pipeline/simple/converter.h
> > > @@ -8,7 +8,10 @@
> > >  #ifndef __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
> > >  #define __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
> > >  
> > > +#include <functional>
> > > +#include <map>
> > >  #include <memory>
> > > +#include <string>
> > >  #include <tuple>
> > >  #include <vector>
> > >  
> > > @@ -37,26 +40,53 @@ public:
> > >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
> > >  
> > >  	int configure(const StreamConfiguration &inputCfg,
> > > -		      const StreamConfiguration &outputCfg);
> > > -	int exportBuffers(unsigned int count,
> > > +		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> > > +	int exportBuffers(unsigned int ouput, unsigned int count,
> > >  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > >  
> > >  	int start();
> > >  	void stop();
> > >  
> > > -	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > > +	int queueBuffers(FrameBuffer *input,
> > > +			 const std::map<unsigned int, FrameBuffer *> &outputs);
> > >  
> > >  	Signal<FrameBuffer *> inputBufferReady;
> > >  	Signal<FrameBuffer *> outputBufferReady;
> > >  
> > >  private:
> > > -	void m2mInputBufferReady(FrameBuffer *buffer);
> > > -	void m2mOutputBufferReady(FrameBuffer *buffer);
> > > +	class Stream
> > > +	{
> > > +	public:
> > > +		Stream(SimpleConverter *converter);
> > >  
> > > +		bool isValid() const { return m2m_ != nullptr; }
> > > +
> > > +		int configure(const StreamConfiguration &inputCfg,
> > > +			      const StreamConfiguration &outputCfg);
> > > +		int exportBuffers(unsigned int count,
> > > +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > > +
> > > +		int start();
> > > +		void stop();
> > > +
> > > +		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > > +
> > > +	private:
> > > +		void captureBufferReady(FrameBuffer *buffer);
> > > +		void outputBufferReady(FrameBuffer *buffer);
> > > +
> > > +		SimpleConverter *converter_;
> > > +		std::unique_ptr<V4L2M2MDevice> m2m_;
> > > +
> > > +		unsigned int inputBufferCount_;
> > > +		unsigned int outputBufferCount_;
> > > +	};
> > > +
> > > +	std::string deviceNode_;
> > >  	std::unique_ptr<V4L2M2MDevice> m2m_;
> > >  
> > > -	unsigned int inputBufferCount_;
> > > -	unsigned int outputBufferCount_;
> > > +	std::vector<Stream> streams_;
> > > +	std::map<FrameBuffer *, unsigned int> queue_;
> > >  };
> > >  
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 7f9c57234256..b7a890ab772e 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -610,7 +610,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > >  		inputCfg.stride = captureFormat.planes[0].bpl;
> > >  		inputCfg.bufferCount = cfg.bufferCount;
> > >  
> > > -		ret = converter_->configure(inputCfg, cfg);
> > > +		ret = converter_->configure(inputCfg, { cfg });
> > >  		if (ret < 0) {
> > >  			LOG(SimplePipeline, Error)
> > >  				<< "Unable to configure converter";
> > > @@ -636,7 +636,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> > >  	 * whether the converter is used or not.
> > >  	 */
> > >  	if (useConverter_)
> > > -		return converter_->exportBuffers(count, buffers);
> > > +		return converter_->exportBuffers(0, count, buffers);
> > 
> > I was wondering if it would be better to reference the Stream objects
> > rather than indexes, but I can see that might become confusing with our
> > Stream *stream we already have ...
> 
> I've thought about that, but it would require exposing the Stream class.
> If this was a public API I'd put more effort in the design :-)

Heh. I think indexes are fine. Due to the masking above I guess we're
limited to 31 streams?


Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> > And for now, we only have a single stream so index zero is enough for
> > now ...
> > 
> > Only a few trivial comments.
> > With those handled as you wish:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > >  	else
> > >  		return data->video_->exportBuffers(count, buffers);
> > >  }
> > > @@ -917,7 +917,7 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
> > >  		FrameBuffer *output = converterQueue_.front();
> > >  		converterQueue_.pop();
> > >  
> > > -		converter_->queueBuffers(buffer, output);
> > > +		converter_->queueBuffers(buffer, { { 0, output } });
> > >  		return;
> > >  	}
> > >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

On Tue, Mar 02, 2021 at 09:11:20AM +0900, paul.elder@ideasonboard.com wrote:
> On Tue, Mar 02, 2021 at 12:19:02AM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 22, 2021 at 01:08:12PM +0000, Kieran Bingham wrote:
> > > On 31/01/2021 22:46, Laurent Pinchart wrote:
> > > > While the M2M device backing the converter doesn't support multiple
> > > > streams natively, it can be run once per stream to produce multiple
> > > > outputs from the same input, with different output formats and sizes.
> > > 
> > > Well, that's going to get a bit more fun ;-)
> > > 
> > > > To support this, create a class to model a stream and move control of
> > > > the M2M device to the Stream class. The SimpleConverter class then
> > > > creates stream instances and iterates over them. Each stream needs its
> > > > own instance of the V4L2M2MDevice, to support different output
> > > > configurations. The SimpleConverter class retains a device instance to
> > > > support the query operations.
> > > 
> > > This reminds me of a discussion on linux-media, where having multiple
> > > opens of an M2M device gets quite difficult to distinguish between the
> > > different opened contexts in the kernel debug messages. Each queue has a
> > > different type, but the separate open contexts are harder to distinguish.
> > > 
> > > I hope our multiple streams log outputs can be clearly separated
> > > (presumably with a stream index)?
> > 
> > Good point... Seems I need to fix that :-)
> > 
> > Beside handling this in the converter, we could print the fd in
> > V4L2VideoDevice::logPrefix(), do you think that would be useful ?
> 
> Ooh yeah I think that's a good idea.
> 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  src/libcamera/pipeline/simple/converter.cpp | 294 ++++++++++++++------
> > > >  src/libcamera/pipeline/simple/converter.h   |  44 ++-
> > > >  src/libcamera/pipeline/simple/simple.cpp    |   6 +-
> > > >  3 files changed, 250 insertions(+), 94 deletions(-)
> > > > 
> > > > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> > > > index 8324baedc198..3db162d9edb8 100644
> > > > --- a/src/libcamera/pipeline/simple/converter.cpp
> > > > +++ b/src/libcamera/pipeline/simple/converter.cpp
> > > > @@ -17,12 +17,157 @@
> > > >  
> > > >  #include "libcamera/internal/log.h"
> > > >  #include "libcamera/internal/media_device.h"
> > > > +#include "libcamera/internal/utils.h"
> > > >  #include "libcamera/internal/v4l2_videodevice.h"
> > > >  
> > > >  namespace libcamera {
> > > >  
> > > >  LOG_DECLARE_CATEGORY(SimplePipeline)
> > > >  
> > > > +/* -----------------------------------------------------------------------------
> > > > + * SimpleConverter::Stream
> > > > + */
> > > > +
> > > > +SimpleConverter::Stream::Stream(SimpleConverter *converter)
> > > > +	: converter_(converter)
> > > > +{
> > > > +	m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_);
> > > > +
> > > > +	m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);
> > > > +	m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);
> > > > +
> > > > +	int ret = m2m_->open();
> > > > +	if (ret < 0)
> > > > +		m2m_.reset();
> > > > +}
> > > > +
> > > > +int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
> > > > +				       const StreamConfiguration &outputCfg)
> > > > +{
> > > > +	V4L2PixelFormat videoFormat =
> > > > +		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> > > > +
> > > > +	V4L2DeviceFormat format;
> > > > +	format.fourcc = videoFormat;
> > > > +	format.size = inputCfg.size;
> > > > +	format.planesCount = 1;
> > > > +	format.planes[0].bpl = inputCfg.stride;
> > > > +
> > > > +	int ret = m2m_->output()->setFormat(&format);
> > > > +	if (ret < 0) {
> > > > +		LOG(SimplePipeline, Error)
> > > > +			<< "Failed to set input format: " << strerror(-ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> > > > +	    format.planes[0].bpl != inputCfg.stride) {
> > > > +		LOG(SimplePipeline, Error)
> > > > +			<< "Input format not supported";
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	/* Set the pixel format and size on the output. */
> > > > +	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> > > > +	format = {};
> > > > +	format.fourcc = videoFormat;
> > > > +	format.size = outputCfg.size;
> 
> Why doesn't the output need plane information?

setFormat() will set the number of planes, stride and frame size.
You're right though, we should fix this, but it will require validate()
to also honour stride and frame size, which it doesn't today. We can fix
this on top.

> > > > +
> > > > +	ret = m2m_->capture()->setFormat(&format);
> > > > +	if (ret < 0) {
> > > > +		LOG(SimplePipeline, Error)
> > > > +			<< "Failed to set output format: " << strerror(-ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> > > > +		LOG(SimplePipeline, Error)
> > > > +			<< "Output format not supported";
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	inputBufferCount_ = inputCfg.bufferCount;
> > > > +	outputBufferCount_ = outputCfg.bufferCount;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int SimpleConverter::Stream::exportBuffers(unsigned int count,
> > > > +					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > > +{
> > > > +	return m2m_->capture()->exportBuffers(count, buffers);
> > > > +}
> > > > +
> > > > +int SimpleConverter::Stream::start()
> > > > +{
> > > > +	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> > > > +	if (ret < 0) {
> > > > +		stop();
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = m2m_->output()->streamOn();
> > > > +	if (ret < 0) {
> > > > +		stop();
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = m2m_->capture()->streamOn();
> > > > +	if (ret < 0) {
> > > > +		stop();
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +void SimpleConverter::Stream::stop()
> > > > +{
> > > > +	m2m_->capture()->streamOff();
> > > > +	m2m_->output()->streamOff();
> > > > +	m2m_->capture()->releaseBuffers();
> > > > +	m2m_->output()->releaseBuffers();
> > > > +}
> > > > +
> > > > +int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,
> > > > +					  FrameBuffer *output)
> > > > +{
> > > > +	int ret = m2m_->output()->queueBuffer(input);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = m2m_->capture()->queueBuffer(output);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)
> > > > +{
> > > > +	auto it = converter_->queue_.find(buffer);
> > > > +	if (it == converter_->queue_.end())
> > > > +		return;
> > > > +
> > > > +	if (!--it->second) {
> > > > +		converter_->inputBufferReady.emit(buffer);
> > > > +		converter_->queue_.erase(it);
> > > > +	}
> > > > +}
> > > > +
> > > > +void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)
> > > > +{
> > > > +	converter_->outputBufferReady.emit(buffer);
> > > > +}
> > > > +
> > > > +/* -----------------------------------------------------------------------------
> > > > + * SimpleConverter
> > > > + */
> > > > +
> > > >  SimpleConverter::SimpleConverter(MediaDevice *media)
> > > >  {
> > > >  	/*
> > > > @@ -37,16 +182,14 @@ SimpleConverter::SimpleConverter(MediaDevice *media)
> > > >  	if (it == entities.end())
> > > >  		return;
> > > >  
> > > > -	m2m_ = std::make_unique<V4L2M2MDevice>((*it)->deviceNode());
> > > > +	deviceNode_ = (*it)->deviceNode();
> > > >  
> > > > +	m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_);
> > > >  	int ret = m2m_->open();
> > > >  	if (ret < 0) {
> > > >  		m2m_.reset();
> > > >  		return;
> > > >  	}
> > > > -
> > > > -	m2m_->output()->bufferReady.connect(this, &SimpleConverter::m2mInputBufferReady);
> > > > -	m2m_->capture()->bufferReady.connect(this, &SimpleConverter::m2mOutputBufferReady);
> > > >  }
> > > >  
> > > >  std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
> > > > @@ -141,86 +284,54 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
> > > >  }
> > > >  
> > > >  int SimpleConverter::configure(const StreamConfiguration &inputCfg,
> > > > -			       const StreamConfiguration &outputCfg)
> > > > +			       const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
> > > >  {
> > > > -	int ret;
> > > > +	int ret = 0;
> > > >  
> > > > -	V4L2PixelFormat videoFormat =
> > > > -		m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> > > > +	streams_.clear();
> > > > +	streams_.reserve(outputCfgs.size());
> > > >  
> > > > -	V4L2DeviceFormat format;
> > > > -	format.fourcc = videoFormat;
> > > > -	format.size = inputCfg.size;
> > > > -	format.planesCount = 1;
> > > > -	format.planes[0].bpl = inputCfg.stride;
> > > > +	for (const StreamConfiguration &outputCfg : outputCfgs) {
> > > > +		Stream &stream = streams_.emplace_back(this);
> > > >  
> > > > -	ret = m2m_->output()->setFormat(&format);
> > > > -	if (ret < 0) {
> > > > -		LOG(SimplePipeline, Error)
> > > > -			<< "Failed to set input format: " << strerror(-ret);
> > > > -		return ret;
> > > > -	}
> > > > +		if (!stream.isValid()) {
> > > > +			LOG(SimplePipeline, Error) << "Failed to create stream";
> > > > +			ret = -EINVAL;
> > > > +			break;
> > > > +		}
> > > >  
> > > > -	if (format.fourcc != videoFormat || format.size != inputCfg.size ||
> > > > -	    format.planes[0].bpl != inputCfg.stride) {
> > > > -		LOG(SimplePipeline, Error)
> > > > -			<< "Input format not supported";
> > > > -		return -EINVAL;
> > > > +		ret = stream.configure(inputCfg, outputCfg);
> > > > +		if (ret < 0)
> > > > +			break;
> > > >  	}
> > > >  
> > > > -	/* Set the pixel format and size on the output. */
> > > > -	videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> > > > -	format = {};
> > > > -	format.fourcc = videoFormat;
> > > > -	format.size = outputCfg.size;
> > > > -
> > > > -	ret = m2m_->capture()->setFormat(&format);
> > > >  	if (ret < 0) {
> > > > -		LOG(SimplePipeline, Error)
> > > > -			<< "Failed to set output format: " << strerror(-ret);
> > > > +		streams_.clear();
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	if (format.fourcc != videoFormat || format.size != outputCfg.size) {
> > > > -		LOG(SimplePipeline, Error)
> > > > -			<< "Output format not supported";
> > > > -		return -EINVAL;
> > > > -	}
> > > > -
> > > > -	inputBufferCount_ = inputCfg.bufferCount;
> > > > -	outputBufferCount_ = outputCfg.bufferCount;
> > > > -
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -int SimpleConverter::exportBuffers(unsigned int count,
> > > > +int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,
> > > >  				   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > >  {
> > > > -	return m2m_->capture()->exportBuffers(count, buffers);
> > > > +	if (output >= streams_.size())
> > > > +		return -EINVAL;
> > > > +
> > > > +	return streams_[output].exportBuffers(count, buffers);
> > > >  }
> > > >  
> > > >  int SimpleConverter::start()
> > > >  {
> > > > -	int ret = m2m_->output()->importBuffers(inputBufferCount_);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > > +	int ret;
> > > >  
> > > > -	ret = m2m_->capture()->importBuffers(outputBufferCount_);
> > > > -	if (ret < 0) {
> > > > -		stop();
> > > > -		return ret;
> > > > -	}
> > > > -
> > > > -	ret = m2m_->output()->streamOn();
> > > > -	if (ret < 0) {
> > > > -		stop();
> > > > -		return ret;
> > > > -	}
> > > > -
> > > > -	ret = m2m_->capture()->streamOn();
> > > > -	if (ret < 0) {
> > > > -		stop();
> > > > -		return ret;
> > > > +	for (Stream &stream : utils::reverse(streams_)) {
> > > 
> > > Do we have to start in reverse order?
> > > I understand stopping them in the reverse order ... but starting?
> > 
> > Indeed, I'll fix that.
> 
> Ah, I was wondering about the reasoning for this. Now I see.
> 
> > > > +		ret = stream.start();
> > > > +		if (ret < 0) {
> > > > +			stop();
> > > > +			return ret;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	return 0;
> > > > @@ -228,33 +339,48 @@ int SimpleConverter::start()
> > > >  
> > > >  void SimpleConverter::stop()
> > > >  {
> > > > -	m2m_->capture()->streamOff();
> > > > -	m2m_->output()->streamOff();
> > > > -	m2m_->capture()->releaseBuffers();
> > > > -	m2m_->output()->releaseBuffers();
> > > > +	for (Stream &stream : utils::reverse(streams_))
> > > > +		stream.stop();
> > > 
> > > much nicer ;-)
> > 
> > I have to confess I really like how C++ makes this possible. The price
> > to pay, of course, is total madness after studying templates.
> > 
> > > >  }
> > > >  
> > > > -int SimpleConverter::queueBuffers(FrameBuffer *input, FrameBuffer *output)
> > > > +int SimpleConverter::queueBuffers(FrameBuffer *input,
> > > > +				  const std::map<unsigned int, FrameBuffer *> &outputs)
> > > >  {
> > > > -	int ret = m2m_->output()->queueBuffer(input);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > > +	unsigned int mask = 0;
> > > > +	int ret;
> > > >  
> > > > -	ret = m2m_->capture()->queueBuffer(output);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > > +	/* Validate the outputs as a sanity check. */
> > > > +	if (outputs.empty())
> > > > +		return -EINVAL;
> > > > +
> > > > +	for (auto [index, buffer] : outputs) {
> > > > +		if (index >= streams_.size())
> > > > +			return -EINVAL;
> > > > +		if (mask & (1 << index))
> > > 
> > > I presume mask is checking to see/prevent there being multiple copies of
> > > the same stream in outputs or something?
> > > 
> > > But this isn't very clear or readable - so a comment at least would help.
> > 
> > That's correct. I'll add a comment.
> 
> I thought it was fine, just that maybe mask could be named something
> else. But I can't think of anything better, so maybe a comment would be
> easier.
> 
> > > > +			return -EINVAL;
> > > > +		if (!buffer)
> > > > +			return -EINVAL;
> > > > +
> > > > +		mask |= 1 << index;
> > > > +	}
> > > > +
> > > > +	/* Queue the input and output buffers to all the streams. */
> > > > +	for (auto [index, buffer] : outputs) {
> > > > +		ret = streams_[index].queueBuffers(input, buffer);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Add the input buffer to the queue, with the number of streams as a
> > > > +	 * reference count. Completion of the input buffer will be signalled by
> > > > +	 * the stream that releases the last reference.
> > > > +	 */
> > > > +	queue_.emplace(std::piecewise_construct,
> > > > +		       std::forward_as_tuple(input),
> > > > +		       std::forward_as_tuple(outputs.size()));
> > > >  
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -void SimpleConverter::m2mInputBufferReady(FrameBuffer *buffer)
> > > > -{
> > > > -	inputBufferReady.emit(buffer);
> > > > -}
> > > > -
> > > > -void SimpleConverter::m2mOutputBufferReady(FrameBuffer *buffer)
> > > > -{
> > > > -	outputBufferReady.emit(buffer);
> > > > -}
> > > > -
> > > >  } /* namespace libcamera */
> > > > diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> > > > index 739b24df0200..176978eefe48 100644
> > > > --- a/src/libcamera/pipeline/simple/converter.h
> > > > +++ b/src/libcamera/pipeline/simple/converter.h
> > > > @@ -8,7 +8,10 @@
> > > >  #ifndef __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
> > > >  #define __LIBCAMERA_PIPELINE_SIMPLE_CONVERTER_H__
> > > >  
> > > > +#include <functional>
> > > > +#include <map>
> > > >  #include <memory>
> > > > +#include <string>
> > > >  #include <tuple>
> > > >  #include <vector>
> > > >  
> > > > @@ -37,26 +40,53 @@ public:
> > > >  	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size);
> > > >  
> > > >  	int configure(const StreamConfiguration &inputCfg,
> > > > -		      const StreamConfiguration &outputCfg);
> > > > -	int exportBuffers(unsigned int count,
> > > > +		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> > > > +	int exportBuffers(unsigned int ouput, unsigned int count,
> > > >  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > > >  
> > > >  	int start();
> > > >  	void stop();
> > > >  
> > > > -	int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > > > +	int queueBuffers(FrameBuffer *input,
> > > > +			 const std::map<unsigned int, FrameBuffer *> &outputs);
> > > >  
> > > >  	Signal<FrameBuffer *> inputBufferReady;
> > > >  	Signal<FrameBuffer *> outputBufferReady;
> > > >  
> > > >  private:
> > > > -	void m2mInputBufferReady(FrameBuffer *buffer);
> > > > -	void m2mOutputBufferReady(FrameBuffer *buffer);
> > > > +	class Stream
> > > > +	{
> > > > +	public:
> > > > +		Stream(SimpleConverter *converter);
> > > >  
> > > > +		bool isValid() const { return m2m_ != nullptr; }
> > > > +
> > > > +		int configure(const StreamConfiguration &inputCfg,
> > > > +			      const StreamConfiguration &outputCfg);
> > > > +		int exportBuffers(unsigned int count,
> > > > +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > > > +
> > > > +		int start();
> > > > +		void stop();
> > > > +
> > > > +		int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> > > > +
> > > > +	private:
> > > > +		void captureBufferReady(FrameBuffer *buffer);
> > > > +		void outputBufferReady(FrameBuffer *buffer);
> > > > +
> > > > +		SimpleConverter *converter_;
> > > > +		std::unique_ptr<V4L2M2MDevice> m2m_;
> > > > +
> > > > +		unsigned int inputBufferCount_;
> > > > +		unsigned int outputBufferCount_;
> > > > +	};
> > > > +
> > > > +	std::string deviceNode_;
> > > >  	std::unique_ptr<V4L2M2MDevice> m2m_;
> > > >  
> > > > -	unsigned int inputBufferCount_;
> > > > -	unsigned int outputBufferCount_;
> > > > +	std::vector<Stream> streams_;
> > > > +	std::map<FrameBuffer *, unsigned int> queue_;
> > > >  };
> > > >  
> > > >  } /* namespace libcamera */
> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > > index 7f9c57234256..b7a890ab772e 100644
> > > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > > @@ -610,7 +610,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > > >  		inputCfg.stride = captureFormat.planes[0].bpl;
> > > >  		inputCfg.bufferCount = cfg.bufferCount;
> > > >  
> > > > -		ret = converter_->configure(inputCfg, cfg);
> > > > +		ret = converter_->configure(inputCfg, { cfg });
> > > >  		if (ret < 0) {
> > > >  			LOG(SimplePipeline, Error)
> > > >  				<< "Unable to configure converter";
> > > > @@ -636,7 +636,7 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> > > >  	 * whether the converter is used or not.
> > > >  	 */
> > > >  	if (useConverter_)
> > > > -		return converter_->exportBuffers(count, buffers);
> > > > +		return converter_->exportBuffers(0, count, buffers);
> > > 
> > > I was wondering if it would be better to reference the Stream objects
> > > rather than indexes, but I can see that might become confusing with our
> > > Stream *stream we already have ...
> > 
> > I've thought about that, but it would require exposing the Stream class.
> > If this was a public API I'd put more effort in the design :-)
> 
> Heh. I think indexes are fine. Due to the masking above I guess we're
> limited to 31 streams?

I'd be surprised if a converter had enough bandwidth to run more than 32
passes within a single frame interval :-)

> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> > > And for now, we only have a single stream so index zero is enough for
> > > now ...
> > > 
> > > Only a few trivial comments.
> > > With those handled as you wish:
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > >  	else
> > > >  		return data->video_->exportBuffers(count, buffers);
> > > >  }
> > > > @@ -917,7 +917,7 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)
> > > >  		FrameBuffer *output = converterQueue_.front();
> > > >  		converterQueue_.pop();
> > > >  
> > > > -		converter_->queueBuffers(buffer, output);
> > > > +		converter_->queueBuffers(buffer, { { 0, output } });
> > > >  		return;
> > > >  	}
> > > >

On 02/03/2021 00:33, Laurent Pinchart wrote:
>>>> I was wondering if it would be better to reference the Stream objects
>>>> rather than indexes, but I can see that might become confusing with our
>>>> Stream *stream we already have ...
>>>
>>> I've thought about that, but it would require exposing the Stream class.
>>> If this was a public API I'd put more effort in the design :-)
>>
>> Heh. I think indexes are fine. Due to the masking above I guess we're
>> limited to 31 streams?
> 
> I'd be surprised if a converter had enough bandwidth to run more than 32
> passes within a single frame interval :-)

Given that there is now an internal detail here restricting the number
of streams, even if we're unlikely to create that many, should there be
an assert if someone tries to go over that amount?

We're unlikely to get this high, so it's optional - but you never know
what some crazy person might do ;-)

Perhaps it could also catch bugs if someone is adding streams without
care to remove or some other category of bug though.

Hi Kieran,

On Tue, Mar 02, 2021 at 10:12:31AM +0000, Kieran Bingham wrote:
> On 02/03/2021 00:33, Laurent Pinchart wrote:
> >>>> I was wondering if it would be better to reference the Stream objects
> >>>> rather than indexes, but I can see that might become confusing with our
> >>>> Stream *stream we already have ...
> >>>
> >>> I've thought about that, but it would require exposing the Stream class.
> >>> If this was a public API I'd put more effort in the design :-)
> >>
> >> Heh. I think indexes are fine. Due to the masking above I guess we're
> >> limited to 31 streams?
> > 
> > I'd be surprised if a converter had enough bandwidth to run more than 32
> > passes within a single frame interval :-)
> 
> Given that there is now an internal detail here restricting the number
> of streams, even if we're unlikely to create that many, should there be
> an assert if someone tries to go over that amount?
> 
> We're unlikely to get this high, so it's optional - but you never know
> what some crazy person might do ;-)
> 
> Perhaps it could also catch bugs if someone is adding streams without
> care to remove or some other category of bug though.

I think I'd classify this in the category of bugs that really can never
happen. You can quote me on this in the future ;-)

On 02/03/2021 10:16, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tue, Mar 02, 2021 at 10:12:31AM +0000, Kieran Bingham wrote:
>> On 02/03/2021 00:33, Laurent Pinchart wrote:
>>>>>> I was wondering if it would be better to reference the Stream objects
>>>>>> rather than indexes, but I can see that might become confusing with our
>>>>>> Stream *stream we already have ...
>>>>>
>>>>> I've thought about that, but it would require exposing the Stream class.
>>>>> If this was a public API I'd put more effort in the design :-)
>>>>
>>>> Heh. I think indexes are fine. Due to the masking above I guess we're
>>>> limited to 31 streams?
>>>
>>> I'd be surprised if a converter had enough bandwidth to run more than 32
>>> passes within a single frame interval :-)
>>
>> Given that there is now an internal detail here restricting the number
>> of streams, even if we're unlikely to create that many, should there be
>> an assert if someone tries to go over that amount?
>>
>> We're unlikely to get this high, so it's optional - but you never know
>> what some crazy person might do ;-)
>>
>> Perhaps it could also catch bugs if someone is adding streams without
>> care to remove or some other category of bug though.
> 
> I think I'd classify this in the category of bugs that really can never
> happen. You can quote me on this in the future ;-)

I look forward to that ;-)