[libcamera-devel] [PATCH 09/20] libcamera: pipeline: simple: converter: Add multi-stream support
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Feb 22 14:08:12 CET 2021
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 at 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 at 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
--
Kieran
More information about the libcamera-devel
mailing list