[libcamera-devel] [PATCH 09/20] libcamera: pipeline: simple: converter: Add multi-stream support
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Mar 2 01:11:20 CET 2021
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 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;
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 at 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 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list