[libcamera-devel] [PATCH 09/20] libcamera: pipeline: simple: converter: Add multi-stream support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 1 23:19:02 CET 2021
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 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?
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 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list