| Message ID | 20221117185204.11625-2-xavier.roumegue@oss.nxp.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Xavier Roumegue via libcamera-devel (2022-11-17 18:52:04) > Move the simple converter implementation to a generic V4L2 M2M class > derived from the converter interface. This latter could be used by > other pipeline implementations and as base class for customized V4L2 M2M > converters. > I like the sound of this! > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > .../internal/converter/converter_v4l2_m2m.h | 18 +-- > .../libcamera/internal/converter/meson.build | 5 + > include/libcamera/internal/meson.build | 2 + > .../converter_v4l2_m2m.cpp} | 153 +++++++++++------- > src/libcamera/converter/meson.build | 5 + > src/libcamera/meson.build | 1 + > src/libcamera/pipeline/simple/meson.build | 1 - > src/libcamera/pipeline/simple/simple.cpp | 6 +- > 8 files changed, 123 insertions(+), 68 deletions(-) > rename src/libcamera/pipeline/simple/converter.h => include/libcamera/internal/converter/converter_v4l2_m2m.h (83%) > create mode 100644 include/libcamera/internal/converter/meson.build > rename src/libcamera/{pipeline/simple/converter.cpp => converter/converter_v4l2_m2m.cpp} (68%) > create mode 100644 src/libcamera/converter/meson.build > > diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h > similarity index 83% > rename from src/libcamera/pipeline/simple/converter.h > rename to include/libcamera/internal/converter/converter_v4l2_m2m.h > index f0ebe2e0..ef31eeba 100644 > --- a/src/libcamera/pipeline/simple/converter.h > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h > @@ -1,8 +1,9 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > * Copyright (C) 2020, Laurent Pinchart > + * Copyright 2022 NXP > * > - * converter.h - Format converter for simple pipeline handler > + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface > */ > > #pragma once > @@ -19,6 +20,8 @@ > #include <libcamera/base/log.h> > #include <libcamera/base/signal.h> > > +#include "libcamera/internal/converter.h" > + > namespace libcamera { > > class FrameBuffer; > @@ -28,11 +31,12 @@ class SizeRange; > struct StreamConfiguration; > class V4L2M2MDevice; > > -class SimpleConverter > +class V4L2M2MConverter : public Converter > { > public: > - SimpleConverter(MediaDevice *media); > + V4L2M2MConverter(MediaDevice *media); > > + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } > bool isValid() const { return m2m_ != nullptr; } > > std::vector<PixelFormat> formats(PixelFormat input); > @@ -52,14 +56,11 @@ public: > int queueBuffers(FrameBuffer *input, > const std::map<unsigned int, FrameBuffer *> &outputs); > > - Signal<FrameBuffer *> inputBufferReady; > - Signal<FrameBuffer *> outputBufferReady; > - > private: > class Stream : protected Loggable > { > public: > - Stream(SimpleConverter *converter, unsigned int index); > + Stream(V4L2M2MConverter *converter, unsigned int index); > > bool isValid() const { return m2m_ != nullptr; } > > @@ -80,7 +81,7 @@ private: > void captureBufferReady(FrameBuffer *buffer); > void outputBufferReady(FrameBuffer *buffer); > > - SimpleConverter *converter_; > + V4L2M2MConverter *converter_; > unsigned int index_; > std::unique_ptr<V4L2M2MDevice> m2m_; > > @@ -88,7 +89,6 @@ private: > unsigned int outputBufferCount_; > }; > > - std::string deviceNode_; > std::unique_ptr<V4L2M2MDevice> m2m_; > > std::vector<Stream> streams_; > diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build > new file mode 100644 > index 00000000..891e79e7 > --- /dev/null > +++ b/include/libcamera/internal/converter/meson.build > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: CC0-1.0 > + > +libcamera_internal_headers += files([ > + 'converter_v4l2_m2m.h', I'll be interested to see what other convertors end in here too. I could imagine a GPU/OpenGL convertor, a libjpeg convertor ... (I guess that's an encoder?, but I'd imagine the same base interface?) ... or others ... > +]) > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > index 8f50d755..b9db5a8c 100644 > --- a/include/libcamera/internal/meson.build > +++ b/include/libcamera/internal/meson.build > @@ -45,3 +45,5 @@ libcamera_internal_headers = files([ > 'v4l2_videodevice.h', > 'yaml_parser.h', > ]) > + > +subdir('converter') > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp > similarity index 68% > rename from src/libcamera/pipeline/simple/converter.cpp > rename to src/libcamera/converter/converter_v4l2_m2m.cpp > index acaaa64c..31acb048 100644 > --- a/src/libcamera/pipeline/simple/converter.cpp > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp > @@ -1,12 +1,11 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > * Copyright (C) 2020, Laurent Pinchart > + * Copyright 2022 NXP > * > - * converter.cpp - Format converter for simple pipeline handler > + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter > */ > > -#include "converter.h" > - > #include <algorithm> > #include <limits.h> > > @@ -20,19 +19,25 @@ > > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/v4l2_videodevice.h" > +#include "libcamera/internal/converter/converter_v4l2_m2m.h" > + > +/** > + * \file internal/converter/converter_v4l2_m2m.h > + * \brief V4L2 M2M based converter > + */ > > namespace libcamera { > > -LOG_DECLARE_CATEGORY(SimplePipeline) > +LOG_DECLARE_CATEGORY(Converter) Hrm.. no, we normally have one log category per component. I think this should be the V4L2M2MConvertor log category ? But maybe we should have more generic log categories on some occasions? So this isn't a hard reject... > > /* ----------------------------------------------------------------------------- > - * SimpleConverter::Stream > + * V4L2M2MConverter::Stream > */ > > -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) > +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index) > : converter_(converter), index_(index) > { > - m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_); > + m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode()); > > m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady); > m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady); > @@ -42,8 +47,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) > m2m_.reset(); > } > > -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > - const StreamConfiguration &outputCfg) > +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg, > + const StreamConfiguration &outputCfg) > { > V4L2PixelFormat videoFormat = > m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat); > @@ -56,14 +61,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > > int ret = m2m_->output()->setFormat(&format); > if (ret < 0) { > - LOG(SimplePipeline, Error) > + LOG(Converter, 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) > + LOG(Converter, Error) > << "Input format not supported (requested " > << inputCfg.size << "-" << videoFormat > << ", got " << format << ")"; > @@ -78,13 +83,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > > ret = m2m_->capture()->setFormat(&format); > if (ret < 0) { > - LOG(SimplePipeline, Error) > + LOG(Converter, Error) > << "Failed to set output format: " << strerror(-ret); > return ret; > } > > if (format.fourcc != videoFormat || format.size != outputCfg.size) { > - LOG(SimplePipeline, Error) > + LOG(Converter, Error) > << "Output format not supported"; > return -EINVAL; > } > @@ -95,13 +100,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > return 0; > } > > -int SimpleConverter::Stream::exportBuffers(unsigned int count, > - std::vector<std::unique_ptr<FrameBuffer>> *buffers) > +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > return m2m_->capture()->exportBuffers(count, buffers); > } > > -int SimpleConverter::Stream::start() > +int V4L2M2MConverter::Stream::start() > { > int ret = m2m_->output()->importBuffers(inputBufferCount_); > if (ret < 0) > @@ -128,7 +133,7 @@ int SimpleConverter::Stream::start() > return 0; > } > > -void SimpleConverter::Stream::stop() > +void V4L2M2MConverter::Stream::stop() > { > m2m_->capture()->streamOff(); > m2m_->output()->streamOff(); > @@ -136,8 +141,7 @@ void SimpleConverter::Stream::stop() > m2m_->output()->releaseBuffers(); > } > > -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, > - FrameBuffer *output) > +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output) > { > int ret = m2m_->output()->queueBuffer(input); > if (ret < 0) > @@ -150,12 +154,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, > return 0; > } > > -std::string SimpleConverter::Stream::logPrefix() const > +std::string V4L2M2MConverter::Stream::logPrefix() const > { > return "stream" + std::to_string(index_); I'm not yet sure if this will be unique enough if we have multiple convertors. However given the logging infrastructure will print a file and line number, I think it will be clear. So I think this is ok. > } > > -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) > +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer) > { > auto it = converter_->queue_.find(buffer); > if (it == converter_->queue_.end()) > @@ -167,32 +171,34 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) > } > } > > -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer) > +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer) > { > converter_->outputBufferReady.emit(buffer); > } > > /* ----------------------------------------------------------------------------- > - * SimpleConverter > + * V4L2M2MConverter > + */ > + > +/** > + * \class libcamera::V4L2M2MConverter > + * \brief The V4L2 M2M converter implements the converter interface based on > + * V4L2 M2M device. > +*/ > + > +/** > + * \fn V4L2M2MConverter::V4L2M2MConverter > + * \brief Construct a V4L2M2MConverter instance > + * \param[in] media The media device implementing the converter > */ > > -SimpleConverter::SimpleConverter(MediaDevice *media) > +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media) > + : Converter(media) > { > - /* > - * Locate the video node. There's no need to validate the pipeline > - * further, the caller guarantees that this is a V4L2 mem2mem device. > - */ > - const std::vector<MediaEntity *> &entities = media->entities(); > - auto it = std::find_if(entities.begin(), entities.end(), > - [](MediaEntity *entity) { > - return entity->function() == MEDIA_ENT_F_IO_V4L; > - }); > - if (it == entities.end()) > + if (deviceNode().empty()) Do we need any kind of warning here? I presume this means we were unable to construct a suitable convertor... Or will the Converter(media) initialiser have already printed an error in that instance? > return; > > - deviceNode_ = (*it)->deviceNode(); > - > - m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_); > + m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode()); > int ret = m2m_->open(); > if (ret < 0) { > m2m_.reset(); > @@ -200,7 +206,21 @@ SimpleConverter::SimpleConverter(MediaDevice *media) > } > } > > -std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) > +/** > + * \fn libcamera::V4L2M2MConverter::loadConfiguration > + * \details \copydetails libcamera::Converter::loadConfiguration > + */ > + > +/** > + * \fn libcamera::V4L2M2MConverter::isValid > + * \details \copydetails libcamera::Converter::isValid > + */ > + > +/** > + * \fn libcamera::V4L2M2MConverter::formats > + * \details \copydetails libcamera::Converter::formats > + */ > +std::vector<PixelFormat> V4L2M2MConverter::formats(PixelFormat input) > { > if (!m2m_) > return {}; > @@ -215,13 +235,13 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) > > int ret = m2m_->output()->setFormat(&v4l2Format); > if (ret < 0) { > - LOG(SimplePipeline, Error) > + LOG(Converter, Error) > << "Failed to set format: " << strerror(-ret); > return {}; > } > > if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) { > - LOG(SimplePipeline, Debug) > + LOG(Converter, Debug) > << "Input format " << input << " not supported."; > return {}; > } > @@ -237,7 +257,10 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) > return pixelFormats; > } > > -SizeRange SimpleConverter::sizes(const Size &input) > +/** > + * \copydoc libcamera::Converter::sizes > + */ > +SizeRange V4L2M2MConverter::sizes(const Size &input) > { > if (!m2m_) > return {}; > @@ -252,7 +275,7 @@ SizeRange SimpleConverter::sizes(const Size &input) > > int ret = m2m_->output()->setFormat(&format); > if (ret < 0) { > - LOG(SimplePipeline, Error) > + LOG(Converter, Error) > << "Failed to set format: " << strerror(-ret); > return {}; > } > @@ -262,7 +285,7 @@ SizeRange SimpleConverter::sizes(const Size &input) > format.size = { 1, 1 }; > ret = m2m_->capture()->setFormat(&format); > if (ret < 0) { > - LOG(SimplePipeline, Error) > + LOG(Converter, Error) > << "Failed to set format: " << strerror(-ret); > return {}; > } > @@ -272,7 +295,7 @@ SizeRange SimpleConverter::sizes(const Size &input) > format.size = { UINT_MAX, UINT_MAX }; > ret = m2m_->capture()->setFormat(&format); > if (ret < 0) { > - LOG(SimplePipeline, Error) > + LOG(Converter, Error) > << "Failed to set format: " << strerror(-ret); > return {}; > } > @@ -282,9 +305,12 @@ SizeRange SimpleConverter::sizes(const Size &input) > return sizes; > } > > +/** > + * \copydoc libcamera::Converter::strideAndFrameSize > + */ > std::tuple<unsigned int, unsigned int> > -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, > - const Size &size) > +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat, > + const Size &size) > { > V4L2DeviceFormat format; > format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat); > @@ -297,8 +323,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, > return std::make_tuple(format.planes[0].bpl, format.planes[0].size); > } > > -int SimpleConverter::configure(const StreamConfiguration &inputCfg, > - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) > +/** > + * \copydoc libcamera::Converter::configure > + */ > +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, > + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) > { > int ret = 0; > > @@ -309,7 +338,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, > Stream &stream = streams_.emplace_back(this, i); > > if (!stream.isValid()) { > - LOG(SimplePipeline, Error) > + LOG(Converter, Error) > << "Failed to create stream " << i; > ret = -EINVAL; > break; > @@ -328,8 +357,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, > return 0; > } > > -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, > - std::vector<std::unique_ptr<FrameBuffer>> *buffers) > +/** > + * \copydoc libcamera::Converter::exportBuffers > + */ > +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > if (output >= streams_.size()) > return -EINVAL; > @@ -337,7 +369,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, > return streams_[output].exportBuffers(count, buffers); > } > > -int SimpleConverter::start() > +/** > + * \copydoc libcamera::Converter::start > + */ > +int V4L2M2MConverter::start() > { > int ret; > > @@ -352,14 +387,20 @@ int SimpleConverter::start() > return 0; > } > > -void SimpleConverter::stop() > +/** > + * \copydoc libcamera::Converter::stop > + */ > +void V4L2M2MConverter::stop() > { > for (Stream &stream : utils::reverse(streams_)) > stream.stop(); > } > > -int SimpleConverter::queueBuffers(FrameBuffer *input, > - const std::map<unsigned int, FrameBuffer *> &outputs) > +/** > + * \copydoc libcamera::Converter::queueBuffers > + */ > +int V4L2M2MConverter::queueBuffers(FrameBuffer *input, > + const std::map<unsigned int, FrameBuffer *> &outputs) > { > unsigned int mask = 0; > int ret; > @@ -402,4 +443,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input, > return 0; > } > > +REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, "pxp") I think the only part I'd change here is that the "pxp" might be a statically created array or vector of 'compatible' strings, a bit like the kernel would define. Then adding support for a new compatible would just be a new line in a table, rather than modifying this registration. It's not much, but it would feel 'neater' to just have a single line patch to add an entry. And once again, I don't think there are any blockers there. Some things to consider and tidy up for a v4, but I'd be happy to merge at that. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > } /* namespace libcamera */ > diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build > new file mode 100644 > index 00000000..2aa72fe4 > --- /dev/null > +++ b/src/libcamera/converter/meson.build > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: CC0-1.0 > + > +libcamera_sources += files([ > + 'converter_v4l2_m2m.cpp' > +]) > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index e9d0324e..ffc294f3 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false) > libthreads = dependency('threads') > > subdir('base') > +subdir('converter') > subdir('ipa') > subdir('pipeline') > subdir('proxy') > diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build > index 9c99b32f..42b0896d 100644 > --- a/src/libcamera/pipeline/simple/meson.build > +++ b/src/libcamera/pipeline/simple/meson.build > @@ -1,6 +1,5 @@ > # SPDX-License-Identifier: CC0-1.0 > > libcamera_sources += files([ > - 'converter.cpp', > 'simple.cpp', > ]) > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index a32de7f3..24ded4db 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -30,13 +30,13 @@ > > #include "libcamera/internal/camera.h" > #include "libcamera/internal/camera_sensor.h" > +#include "libcamera/internal/converter.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > #include "libcamera/internal/v4l2_subdevice.h" > #include "libcamera/internal/v4l2_videodevice.h" > > -#include "converter.h" > > namespace libcamera { > > @@ -266,7 +266,7 @@ public: > std::vector<Configuration> configs_; > std::map<PixelFormat, std::vector<const Configuration *>> formats_; > > - std::unique_ptr<SimpleConverter> converter_; > + std::unique_ptr<Converter> converter_; > std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; > bool useConverter_; > std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; > @@ -492,7 +492,7 @@ int SimpleCameraData::init() > /* Open the converter, if any. */ > MediaDevice *converter = pipe->converter(); > if (converter) { > - converter_ = std::make_unique<SimpleConverter>(converter); > + converter_ = ConverterFactoryBase::create(converter); > if (!converter_->isValid()) { > LOG(SimplePipeline, Warning) > << "Failed to create converter, disabling format conversion"; > -- > 2.38.1 >
Hi Kieran, Thanks for your review. On 12/7/22 13:55, Kieran Bingham wrote: > Quoting Xavier Roumegue via libcamera-devel (2022-11-17 18:52:04) >> Move the simple converter implementation to a generic V4L2 M2M class >> derived from the converter interface. This latter could be used by >> other pipeline implementations and as base class for customized V4L2 M2M >> converters. >> > > I like the sound of this! > > >> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> .../internal/converter/converter_v4l2_m2m.h | 18 +-- >> .../libcamera/internal/converter/meson.build | 5 + >> include/libcamera/internal/meson.build | 2 + >> .../converter_v4l2_m2m.cpp} | 153 +++++++++++------- >> src/libcamera/converter/meson.build | 5 + >> src/libcamera/meson.build | 1 + >> src/libcamera/pipeline/simple/meson.build | 1 - >> src/libcamera/pipeline/simple/simple.cpp | 6 +- >> 8 files changed, 123 insertions(+), 68 deletions(-) >> rename src/libcamera/pipeline/simple/converter.h => include/libcamera/internal/converter/converter_v4l2_m2m.h (83%) >> create mode 100644 include/libcamera/internal/converter/meson.build >> rename src/libcamera/{pipeline/simple/converter.cpp => converter/converter_v4l2_m2m.cpp} (68%) >> create mode 100644 src/libcamera/converter/meson.build >> >> diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h >> similarity index 83% >> rename from src/libcamera/pipeline/simple/converter.h >> rename to include/libcamera/internal/converter/converter_v4l2_m2m.h >> index f0ebe2e0..ef31eeba 100644 >> --- a/src/libcamera/pipeline/simple/converter.h >> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h >> @@ -1,8 +1,9 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> * Copyright (C) 2020, Laurent Pinchart >> + * Copyright 2022 NXP >> * >> - * converter.h - Format converter for simple pipeline handler >> + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface >> */ >> >> #pragma once >> @@ -19,6 +20,8 @@ >> #include <libcamera/base/log.h> >> #include <libcamera/base/signal.h> >> >> +#include "libcamera/internal/converter.h" >> + >> namespace libcamera { >> >> class FrameBuffer; >> @@ -28,11 +31,12 @@ class SizeRange; >> struct StreamConfiguration; >> class V4L2M2MDevice; >> >> -class SimpleConverter >> +class V4L2M2MConverter : public Converter >> { >> public: >> - SimpleConverter(MediaDevice *media); >> + V4L2M2MConverter(MediaDevice *media); >> >> + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } >> bool isValid() const { return m2m_ != nullptr; } >> >> std::vector<PixelFormat> formats(PixelFormat input); >> @@ -52,14 +56,11 @@ public: >> int queueBuffers(FrameBuffer *input, >> const std::map<unsigned int, FrameBuffer *> &outputs); >> >> - Signal<FrameBuffer *> inputBufferReady; >> - Signal<FrameBuffer *> outputBufferReady; >> - >> private: >> class Stream : protected Loggable >> { >> public: >> - Stream(SimpleConverter *converter, unsigned int index); >> + Stream(V4L2M2MConverter *converter, unsigned int index); >> >> bool isValid() const { return m2m_ != nullptr; } >> >> @@ -80,7 +81,7 @@ private: >> void captureBufferReady(FrameBuffer *buffer); >> void outputBufferReady(FrameBuffer *buffer); >> >> - SimpleConverter *converter_; >> + V4L2M2MConverter *converter_; >> unsigned int index_; >> std::unique_ptr<V4L2M2MDevice> m2m_; >> >> @@ -88,7 +89,6 @@ private: >> unsigned int outputBufferCount_; >> }; >> >> - std::string deviceNode_; >> std::unique_ptr<V4L2M2MDevice> m2m_; >> >> std::vector<Stream> streams_; >> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build >> new file mode 100644 >> index 00000000..891e79e7 >> --- /dev/null >> +++ b/include/libcamera/internal/converter/meson.build >> @@ -0,0 +1,5 @@ >> +# SPDX-License-Identifier: CC0-1.0 >> + >> +libcamera_internal_headers += files([ >> + 'converter_v4l2_m2m.h', > > I'll be interested to see what other convertors end in here too. > > I could imagine a GPU/OpenGL convertor, a libjpeg convertor ... (I guess > that's an encoder?, but I'd imagine the same base interface?) ... or others ... I will likely have a need for adding drm devices as converter. > > >> +]) >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build >> index 8f50d755..b9db5a8c 100644 >> --- a/include/libcamera/internal/meson.build >> +++ b/include/libcamera/internal/meson.build >> @@ -45,3 +45,5 @@ libcamera_internal_headers = files([ >> 'v4l2_videodevice.h', >> 'yaml_parser.h', >> ]) >> + >> +subdir('converter') >> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp >> similarity index 68% >> rename from src/libcamera/pipeline/simple/converter.cpp >> rename to src/libcamera/converter/converter_v4l2_m2m.cpp >> index acaaa64c..31acb048 100644 >> --- a/src/libcamera/pipeline/simple/converter.cpp >> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp >> @@ -1,12 +1,11 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> * Copyright (C) 2020, Laurent Pinchart >> + * Copyright 2022 NXP >> * >> - * converter.cpp - Format converter for simple pipeline handler >> + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter >> */ >> >> -#include "converter.h" >> - >> #include <algorithm> >> #include <limits.h> >> >> @@ -20,19 +19,25 @@ >> >> #include "libcamera/internal/media_device.h" >> #include "libcamera/internal/v4l2_videodevice.h" >> +#include "libcamera/internal/converter/converter_v4l2_m2m.h" >> + >> +/** >> + * \file internal/converter/converter_v4l2_m2m.h >> + * \brief V4L2 M2M based converter >> + */ >> >> namespace libcamera { >> >> -LOG_DECLARE_CATEGORY(SimplePipeline) >> +LOG_DECLARE_CATEGORY(Converter) > > Hrm.. no, we normally have one log category per component. I think this > should be the V4L2M2MConvertor log category ? > > But maybe we should have more generic log categories on some occasions? > So this isn't a hard reject... I initially started with a log category per component, but this was not really convenient as you had to deal with 3 different categories (generic converter, v4l2m2m, specialized m2m for handling dewarp device)...so I finally ended with this proposal, using an unique converter log category. I can revert to the legacy way if you dislike this proposal. > > >> >> /* ----------------------------------------------------------------------------- >> - * SimpleConverter::Stream >> + * V4L2M2MConverter::Stream >> */ >> >> -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) >> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index) >> : converter_(converter), index_(index) >> { >> - m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_); >> + m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode()); >> >> m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady); >> m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady); >> @@ -42,8 +47,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) >> m2m_.reset(); >> } >> >> -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, >> - const StreamConfiguration &outputCfg) >> +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg, >> + const StreamConfiguration &outputCfg) >> { >> V4L2PixelFormat videoFormat = >> m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat); >> @@ -56,14 +61,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, >> >> int ret = m2m_->output()->setFormat(&format); >> if (ret < 0) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, 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) >> + LOG(Converter, Error) >> << "Input format not supported (requested " >> << inputCfg.size << "-" << videoFormat >> << ", got " << format << ")"; >> @@ -78,13 +83,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, >> >> ret = m2m_->capture()->setFormat(&format); >> if (ret < 0) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, Error) >> << "Failed to set output format: " << strerror(-ret); >> return ret; >> } >> >> if (format.fourcc != videoFormat || format.size != outputCfg.size) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, Error) >> << "Output format not supported"; >> return -EINVAL; >> } >> @@ -95,13 +100,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, >> return 0; >> } >> >> -int SimpleConverter::Stream::exportBuffers(unsigned int count, >> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) >> +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count, >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) >> { >> return m2m_->capture()->exportBuffers(count, buffers); >> } >> >> -int SimpleConverter::Stream::start() >> +int V4L2M2MConverter::Stream::start() >> { >> int ret = m2m_->output()->importBuffers(inputBufferCount_); >> if (ret < 0) >> @@ -128,7 +133,7 @@ int SimpleConverter::Stream::start() >> return 0; >> } >> >> -void SimpleConverter::Stream::stop() >> +void V4L2M2MConverter::Stream::stop() >> { >> m2m_->capture()->streamOff(); >> m2m_->output()->streamOff(); >> @@ -136,8 +141,7 @@ void SimpleConverter::Stream::stop() >> m2m_->output()->releaseBuffers(); >> } >> >> -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, >> - FrameBuffer *output) >> +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output) >> { >> int ret = m2m_->output()->queueBuffer(input); >> if (ret < 0) >> @@ -150,12 +154,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, >> return 0; >> } >> >> -std::string SimpleConverter::Stream::logPrefix() const >> +std::string V4L2M2MConverter::Stream::logPrefix() const >> { >> return "stream" + std::to_string(index_); > > I'm not yet sure if this will be unique enough if we have multiple > convertors. However given the logging infrastructure will print a file > and line number, I think it will be clear. > > So I think this is ok. > > >> } >> >> -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) >> +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer) >> { >> auto it = converter_->queue_.find(buffer); >> if (it == converter_->queue_.end()) >> @@ -167,32 +171,34 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) >> } >> } >> >> -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer) >> +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer) >> { >> converter_->outputBufferReady.emit(buffer); >> } >> >> /* ----------------------------------------------------------------------------- >> - * SimpleConverter >> + * V4L2M2MConverter >> + */ >> + >> +/** >> + * \class libcamera::V4L2M2MConverter >> + * \brief The V4L2 M2M converter implements the converter interface based on >> + * V4L2 M2M device. >> +*/ >> + >> +/** >> + * \fn V4L2M2MConverter::V4L2M2MConverter >> + * \brief Construct a V4L2M2MConverter instance >> + * \param[in] media The media device implementing the converter >> */ >> >> -SimpleConverter::SimpleConverter(MediaDevice *media) >> +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media) >> + : Converter(media) >> { >> - /* >> - * Locate the video node. There's no need to validate the pipeline >> - * further, the caller guarantees that this is a V4L2 mem2mem device. >> - */ >> - const std::vector<MediaEntity *> &entities = media->entities(); >> - auto it = std::find_if(entities.begin(), entities.end(), >> - [](MediaEntity *entity) { >> - return entity->function() == MEDIA_ENT_F_IO_V4L; >> - }); >> - if (it == entities.end()) >> + if (deviceNode().empty()) > > Do we need any kind of warning here? I presume this means we were unable > to construct a suitable convertor... Or will the Converter(media) > initialiser have already printed an error in that instance? > >> return; >> >> - deviceNode_ = (*it)->deviceNode(); >> - >> - m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_); >> + m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode()); >> int ret = m2m_->open(); >> if (ret < 0) { >> m2m_.reset(); >> @@ -200,7 +206,21 @@ SimpleConverter::SimpleConverter(MediaDevice *media) >> } >> } >> >> -std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) >> +/** >> + * \fn libcamera::V4L2M2MConverter::loadConfiguration >> + * \details \copydetails libcamera::Converter::loadConfiguration >> + */ >> + >> +/** >> + * \fn libcamera::V4L2M2MConverter::isValid >> + * \details \copydetails libcamera::Converter::isValid >> + */ >> + >> +/** >> + * \fn libcamera::V4L2M2MConverter::formats >> + * \details \copydetails libcamera::Converter::formats >> + */ >> +std::vector<PixelFormat> V4L2M2MConverter::formats(PixelFormat input) >> { >> if (!m2m_) >> return {}; >> @@ -215,13 +235,13 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) >> >> int ret = m2m_->output()->setFormat(&v4l2Format); >> if (ret < 0) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, Error) >> << "Failed to set format: " << strerror(-ret); >> return {}; >> } >> >> if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) { >> - LOG(SimplePipeline, Debug) >> + LOG(Converter, Debug) >> << "Input format " << input << " not supported."; >> return {}; >> } >> @@ -237,7 +257,10 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) >> return pixelFormats; >> } >> >> -SizeRange SimpleConverter::sizes(const Size &input) >> +/** >> + * \copydoc libcamera::Converter::sizes >> + */ >> +SizeRange V4L2M2MConverter::sizes(const Size &input) >> { >> if (!m2m_) >> return {}; >> @@ -252,7 +275,7 @@ SizeRange SimpleConverter::sizes(const Size &input) >> >> int ret = m2m_->output()->setFormat(&format); >> if (ret < 0) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, Error) >> << "Failed to set format: " << strerror(-ret); >> return {}; >> } >> @@ -262,7 +285,7 @@ SizeRange SimpleConverter::sizes(const Size &input) >> format.size = { 1, 1 }; >> ret = m2m_->capture()->setFormat(&format); >> if (ret < 0) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, Error) >> << "Failed to set format: " << strerror(-ret); >> return {}; >> } >> @@ -272,7 +295,7 @@ SizeRange SimpleConverter::sizes(const Size &input) >> format.size = { UINT_MAX, UINT_MAX }; >> ret = m2m_->capture()->setFormat(&format); >> if (ret < 0) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, Error) >> << "Failed to set format: " << strerror(-ret); >> return {}; >> } >> @@ -282,9 +305,12 @@ SizeRange SimpleConverter::sizes(const Size &input) >> return sizes; >> } >> >> +/** >> + * \copydoc libcamera::Converter::strideAndFrameSize >> + */ >> std::tuple<unsigned int, unsigned int> >> -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, >> - const Size &size) >> +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat, >> + const Size &size) >> { >> V4L2DeviceFormat format; >> format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat); >> @@ -297,8 +323,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, >> return std::make_tuple(format.planes[0].bpl, format.planes[0].size); >> } >> >> -int SimpleConverter::configure(const StreamConfiguration &inputCfg, >> - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) >> +/** >> + * \copydoc libcamera::Converter::configure >> + */ >> +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, >> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) >> { >> int ret = 0; >> >> @@ -309,7 +338,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, >> Stream &stream = streams_.emplace_back(this, i); >> >> if (!stream.isValid()) { >> - LOG(SimplePipeline, Error) >> + LOG(Converter, Error) >> << "Failed to create stream " << i; >> ret = -EINVAL; >> break; >> @@ -328,8 +357,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, >> return 0; >> } >> >> -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, >> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) >> +/** >> + * \copydoc libcamera::Converter::exportBuffers >> + */ >> +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count, >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) >> { >> if (output >= streams_.size()) >> return -EINVAL; >> @@ -337,7 +369,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, >> return streams_[output].exportBuffers(count, buffers); >> } >> >> -int SimpleConverter::start() >> +/** >> + * \copydoc libcamera::Converter::start >> + */ >> +int V4L2M2MConverter::start() >> { >> int ret; >> >> @@ -352,14 +387,20 @@ int SimpleConverter::start() >> return 0; >> } >> >> -void SimpleConverter::stop() >> +/** >> + * \copydoc libcamera::Converter::stop >> + */ >> +void V4L2M2MConverter::stop() >> { >> for (Stream &stream : utils::reverse(streams_)) >> stream.stop(); >> } >> >> -int SimpleConverter::queueBuffers(FrameBuffer *input, >> - const std::map<unsigned int, FrameBuffer *> &outputs) >> +/** >> + * \copydoc libcamera::Converter::queueBuffers >> + */ >> +int V4L2M2MConverter::queueBuffers(FrameBuffer *input, >> + const std::map<unsigned int, FrameBuffer *> &outputs) >> { >> unsigned int mask = 0; >> int ret; >> @@ -402,4 +443,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input, >> return 0; >> } >> >> +REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, "pxp") > > I think the only part I'd change here is that the "pxp" might be a > statically created array or vector of 'compatible' strings, a bit like > the kernel would define. > > Then adding support for a new compatible would just be a new line in a > table, rather than modifying this registration. It's not much, but it > would feel 'neater' to just have a single line patch to add an entry. > static std::initializer_list<std::string> aliases = { "pxp" }; REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, aliases) Would it be better ? > > And once again, I don't think there are any blockers there. Some things > to consider and tidy up for a v4, but I'd be happy to merge at that. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> + >> } /* namespace libcamera */ >> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build >> new file mode 100644 >> index 00000000..2aa72fe4 >> --- /dev/null >> +++ b/src/libcamera/converter/meson.build >> @@ -0,0 +1,5 @@ >> +# SPDX-License-Identifier: CC0-1.0 >> + >> +libcamera_sources += files([ >> + 'converter_v4l2_m2m.cpp' >> +]) >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build >> index e9d0324e..ffc294f3 100644 >> --- a/src/libcamera/meson.build >> +++ b/src/libcamera/meson.build >> @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false) >> libthreads = dependency('threads') >> >> subdir('base') >> +subdir('converter') >> subdir('ipa') >> subdir('pipeline') >> subdir('proxy') >> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build >> index 9c99b32f..42b0896d 100644 >> --- a/src/libcamera/pipeline/simple/meson.build >> +++ b/src/libcamera/pipeline/simple/meson.build >> @@ -1,6 +1,5 @@ >> # SPDX-License-Identifier: CC0-1.0 >> >> libcamera_sources += files([ >> - 'converter.cpp', >> 'simple.cpp', >> ]) >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index a32de7f3..24ded4db 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -30,13 +30,13 @@ >> >> #include "libcamera/internal/camera.h" >> #include "libcamera/internal/camera_sensor.h" >> +#include "libcamera/internal/converter.h" >> #include "libcamera/internal/device_enumerator.h" >> #include "libcamera/internal/media_device.h" >> #include "libcamera/internal/pipeline_handler.h" >> #include "libcamera/internal/v4l2_subdevice.h" >> #include "libcamera/internal/v4l2_videodevice.h" >> >> -#include "converter.h" >> >> namespace libcamera { >> >> @@ -266,7 +266,7 @@ public: >> std::vector<Configuration> configs_; >> std::map<PixelFormat, std::vector<const Configuration *>> formats_; >> >> - std::unique_ptr<SimpleConverter> converter_; >> + std::unique_ptr<Converter> converter_; >> std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; >> bool useConverter_; >> std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; >> @@ -492,7 +492,7 @@ int SimpleCameraData::init() >> /* Open the converter, if any. */ >> MediaDevice *converter = pipe->converter(); >> if (converter) { >> - converter_ = std::make_unique<SimpleConverter>(converter); >> + converter_ = ConverterFactoryBase::create(converter); >> if (!converter_->isValid()) { >> LOG(SimplePipeline, Warning) >> << "Failed to create converter, disabling format conversion"; >> -- >> 2.38.1 >>
Hi Xavier, Quoting Xavier Roumegue (OSS) (2022-12-10 15:15:03) > Hi Kieran, > > Thanks for your review. > > On 12/7/22 13:55, Kieran Bingham wrote: > > Quoting Xavier Roumegue via libcamera-devel (2022-11-17 18:52:04) > >> Move the simple converter implementation to a generic V4L2 M2M class > >> derived from the converter interface. This latter could be used by > >> other pipeline implementations and as base class for customized V4L2 M2M > >> converters. > >> > > > > I like the sound of this! > > > > > >> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> .../internal/converter/converter_v4l2_m2m.h | 18 +-- > >> .../libcamera/internal/converter/meson.build | 5 + > >> include/libcamera/internal/meson.build | 2 + > >> .../converter_v4l2_m2m.cpp} | 153 +++++++++++------- > >> src/libcamera/converter/meson.build | 5 + > >> src/libcamera/meson.build | 1 + > >> src/libcamera/pipeline/simple/meson.build | 1 - > >> src/libcamera/pipeline/simple/simple.cpp | 6 +- > >> 8 files changed, 123 insertions(+), 68 deletions(-) > >> rename src/libcamera/pipeline/simple/converter.h => include/libcamera/internal/converter/converter_v4l2_m2m.h (83%) > >> create mode 100644 include/libcamera/internal/converter/meson.build > >> rename src/libcamera/{pipeline/simple/converter.cpp => converter/converter_v4l2_m2m.cpp} (68%) > >> create mode 100644 src/libcamera/converter/meson.build > >> > >> diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h > >> similarity index 83% > >> rename from src/libcamera/pipeline/simple/converter.h > >> rename to include/libcamera/internal/converter/converter_v4l2_m2m.h > >> index f0ebe2e0..ef31eeba 100644 > >> --- a/src/libcamera/pipeline/simple/converter.h > >> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h > >> @@ -1,8 +1,9 @@ > >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> /* > >> * Copyright (C) 2020, Laurent Pinchart > >> + * Copyright 2022 NXP > >> * > >> - * converter.h - Format converter for simple pipeline handler > >> + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface > >> */ > >> > >> #pragma once > >> @@ -19,6 +20,8 @@ > >> #include <libcamera/base/log.h> > >> #include <libcamera/base/signal.h> > >> > >> +#include "libcamera/internal/converter.h" > >> + > >> namespace libcamera { > >> > >> class FrameBuffer; > >> @@ -28,11 +31,12 @@ class SizeRange; > >> struct StreamConfiguration; > >> class V4L2M2MDevice; > >> > >> -class SimpleConverter > >> +class V4L2M2MConverter : public Converter > >> { > >> public: > >> - SimpleConverter(MediaDevice *media); > >> + V4L2M2MConverter(MediaDevice *media); > >> > >> + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } > >> bool isValid() const { return m2m_ != nullptr; } > >> > >> std::vector<PixelFormat> formats(PixelFormat input); > >> @@ -52,14 +56,11 @@ public: > >> int queueBuffers(FrameBuffer *input, > >> const std::map<unsigned int, FrameBuffer *> &outputs); > >> > >> - Signal<FrameBuffer *> inputBufferReady; > >> - Signal<FrameBuffer *> outputBufferReady; > >> - > >> private: > >> class Stream : protected Loggable > >> { > >> public: > >> - Stream(SimpleConverter *converter, unsigned int index); > >> + Stream(V4L2M2MConverter *converter, unsigned int index); > >> > >> bool isValid() const { return m2m_ != nullptr; } > >> > >> @@ -80,7 +81,7 @@ private: > >> void captureBufferReady(FrameBuffer *buffer); > >> void outputBufferReady(FrameBuffer *buffer); > >> > >> - SimpleConverter *converter_; > >> + V4L2M2MConverter *converter_; > >> unsigned int index_; > >> std::unique_ptr<V4L2M2MDevice> m2m_; > >> > >> @@ -88,7 +89,6 @@ private: > >> unsigned int outputBufferCount_; > >> }; > >> > >> - std::string deviceNode_; > >> std::unique_ptr<V4L2M2MDevice> m2m_; > >> > >> std::vector<Stream> streams_; > >> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build > >> new file mode 100644 > >> index 00000000..891e79e7 > >> --- /dev/null > >> +++ b/include/libcamera/internal/converter/meson.build > >> @@ -0,0 +1,5 @@ > >> +# SPDX-License-Identifier: CC0-1.0 > >> + > >> +libcamera_internal_headers += files([ > >> + 'converter_v4l2_m2m.h', > > > > I'll be interested to see what other convertors end in here too. > > > > I could imagine a GPU/OpenGL convertor, a libjpeg convertor ... (I guess > > that's an encoder?, but I'd imagine the same base interface?) ... or others ... > I will likely have a need for adding drm devices as converter. > > > > > >> +]) > >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > >> index 8f50d755..b9db5a8c 100644 > >> --- a/include/libcamera/internal/meson.build > >> +++ b/include/libcamera/internal/meson.build > >> @@ -45,3 +45,5 @@ libcamera_internal_headers = files([ > >> 'v4l2_videodevice.h', > >> 'yaml_parser.h', > >> ]) > >> + > >> +subdir('converter') > >> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp > >> similarity index 68% > >> rename from src/libcamera/pipeline/simple/converter.cpp > >> rename to src/libcamera/converter/converter_v4l2_m2m.cpp > >> index acaaa64c..31acb048 100644 > >> --- a/src/libcamera/pipeline/simple/converter.cpp > >> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp > >> @@ -1,12 +1,11 @@ > >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> /* > >> * Copyright (C) 2020, Laurent Pinchart > >> + * Copyright 2022 NXP > >> * > >> - * converter.cpp - Format converter for simple pipeline handler > >> + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter > >> */ > >> > >> -#include "converter.h" > >> - > >> #include <algorithm> > >> #include <limits.h> > >> > >> @@ -20,19 +19,25 @@ > >> > >> #include "libcamera/internal/media_device.h" > >> #include "libcamera/internal/v4l2_videodevice.h" > >> +#include "libcamera/internal/converter/converter_v4l2_m2m.h" > >> + > >> +/** > >> + * \file internal/converter/converter_v4l2_m2m.h > >> + * \brief V4L2 M2M based converter > >> + */ > >> > >> namespace libcamera { > >> > >> -LOG_DECLARE_CATEGORY(SimplePipeline) > >> +LOG_DECLARE_CATEGORY(Converter) > > > > Hrm.. no, we normally have one log category per component. I think this > > should be the V4L2M2MConvertor log category ? > > > > But maybe we should have more generic log categories on some occasions? > > So this isn't a hard reject... > I initially started with a log category per component, but this was not > really convenient as you had to deal with 3 different categories > (generic converter, v4l2m2m, specialized m2m for handling dewarp > device)...so I finally ended with this proposal, using an unique > converter log category. I can revert to the legacy way if you dislike > this proposal. I think this is fine for the time being. If it becomes troublesome when we have lots of convertors we can make it more specific then... > >> > >> /* ----------------------------------------------------------------------------- > >> - * SimpleConverter::Stream > >> + * V4L2M2MConverter::Stream > >> */ > >> > >> -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) > >> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index) > >> : converter_(converter), index_(index) > >> { > >> - m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_); > >> + m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode()); > >> > >> m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady); > >> m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady); > >> @@ -42,8 +47,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) > >> m2m_.reset(); > >> } > >> > >> -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > >> - const StreamConfiguration &outputCfg) > >> +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg, > >> + const StreamConfiguration &outputCfg) > >> { > >> V4L2PixelFormat videoFormat = > >> m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat); > >> @@ -56,14 +61,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > >> > >> int ret = m2m_->output()->setFormat(&format); > >> if (ret < 0) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, 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) > >> + LOG(Converter, Error) > >> << "Input format not supported (requested " > >> << inputCfg.size << "-" << videoFormat > >> << ", got " << format << ")"; > >> @@ -78,13 +83,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > >> > >> ret = m2m_->capture()->setFormat(&format); > >> if (ret < 0) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, Error) > >> << "Failed to set output format: " << strerror(-ret); > >> return ret; > >> } > >> > >> if (format.fourcc != videoFormat || format.size != outputCfg.size) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, Error) > >> << "Output format not supported"; > >> return -EINVAL; > >> } > >> @@ -95,13 +100,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > >> return 0; > >> } > >> > >> -int SimpleConverter::Stream::exportBuffers(unsigned int count, > >> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) > >> +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count, > >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > >> { > >> return m2m_->capture()->exportBuffers(count, buffers); > >> } > >> > >> -int SimpleConverter::Stream::start() > >> +int V4L2M2MConverter::Stream::start() > >> { > >> int ret = m2m_->output()->importBuffers(inputBufferCount_); > >> if (ret < 0) > >> @@ -128,7 +133,7 @@ int SimpleConverter::Stream::start() > >> return 0; > >> } > >> > >> -void SimpleConverter::Stream::stop() > >> +void V4L2M2MConverter::Stream::stop() > >> { > >> m2m_->capture()->streamOff(); > >> m2m_->output()->streamOff(); > >> @@ -136,8 +141,7 @@ void SimpleConverter::Stream::stop() > >> m2m_->output()->releaseBuffers(); > >> } > >> > >> -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, > >> - FrameBuffer *output) > >> +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output) > >> { > >> int ret = m2m_->output()->queueBuffer(input); > >> if (ret < 0) > >> @@ -150,12 +154,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, > >> return 0; > >> } > >> > >> -std::string SimpleConverter::Stream::logPrefix() const > >> +std::string V4L2M2MConverter::Stream::logPrefix() const > >> { > >> return "stream" + std::to_string(index_); > > > > I'm not yet sure if this will be unique enough if we have multiple > > convertors. However given the logging infrastructure will print a file > > and line number, I think it will be clear. > > > > So I think this is ok. > > > > > >> } > >> > >> -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) > >> +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer) > >> { > >> auto it = converter_->queue_.find(buffer); > >> if (it == converter_->queue_.end()) > >> @@ -167,32 +171,34 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) > >> } > >> } > >> > >> -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer) > >> +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer) > >> { > >> converter_->outputBufferReady.emit(buffer); > >> } > >> > >> /* ----------------------------------------------------------------------------- > >> - * SimpleConverter > >> + * V4L2M2MConverter > >> + */ > >> + > >> +/** > >> + * \class libcamera::V4L2M2MConverter > >> + * \brief The V4L2 M2M converter implements the converter interface based on > >> + * V4L2 M2M device. > >> +*/ > >> + > >> +/** > >> + * \fn V4L2M2MConverter::V4L2M2MConverter > >> + * \brief Construct a V4L2M2MConverter instance > >> + * \param[in] media The media device implementing the converter > >> */ > >> > >> -SimpleConverter::SimpleConverter(MediaDevice *media) > >> +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media) > >> + : Converter(media) > >> { > >> - /* > >> - * Locate the video node. There's no need to validate the pipeline > >> - * further, the caller guarantees that this is a V4L2 mem2mem device. > >> - */ > >> - const std::vector<MediaEntity *> &entities = media->entities(); > >> - auto it = std::find_if(entities.begin(), entities.end(), > >> - [](MediaEntity *entity) { > >> - return entity->function() == MEDIA_ENT_F_IO_V4L; > >> - }); > >> - if (it == entities.end()) > >> + if (deviceNode().empty()) > > > > Do we need any kind of warning here? I presume this means we were unable > > to construct a suitable convertor... Or will the Converter(media) > > initialiser have already printed an error in that instance? > > > >> return; > >> > >> - deviceNode_ = (*it)->deviceNode(); > >> - > >> - m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_); > >> + m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode()); > >> int ret = m2m_->open(); > >> if (ret < 0) { > >> m2m_.reset(); > >> @@ -200,7 +206,21 @@ SimpleConverter::SimpleConverter(MediaDevice *media) > >> } > >> } > >> > >> -std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) > >> +/** > >> + * \fn libcamera::V4L2M2MConverter::loadConfiguration > >> + * \details \copydetails libcamera::Converter::loadConfiguration > >> + */ > >> + > >> +/** > >> + * \fn libcamera::V4L2M2MConverter::isValid > >> + * \details \copydetails libcamera::Converter::isValid > >> + */ > >> + > >> +/** > >> + * \fn libcamera::V4L2M2MConverter::formats > >> + * \details \copydetails libcamera::Converter::formats > >> + */ > >> +std::vector<PixelFormat> V4L2M2MConverter::formats(PixelFormat input) > >> { > >> if (!m2m_) > >> return {}; > >> @@ -215,13 +235,13 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) > >> > >> int ret = m2m_->output()->setFormat(&v4l2Format); > >> if (ret < 0) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, Error) > >> << "Failed to set format: " << strerror(-ret); > >> return {}; > >> } > >> > >> if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) { > >> - LOG(SimplePipeline, Debug) > >> + LOG(Converter, Debug) > >> << "Input format " << input << " not supported."; > >> return {}; > >> } > >> @@ -237,7 +257,10 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) > >> return pixelFormats; > >> } > >> > >> -SizeRange SimpleConverter::sizes(const Size &input) > >> +/** > >> + * \copydoc libcamera::Converter::sizes > >> + */ > >> +SizeRange V4L2M2MConverter::sizes(const Size &input) > >> { > >> if (!m2m_) > >> return {}; > >> @@ -252,7 +275,7 @@ SizeRange SimpleConverter::sizes(const Size &input) > >> > >> int ret = m2m_->output()->setFormat(&format); > >> if (ret < 0) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, Error) > >> << "Failed to set format: " << strerror(-ret); > >> return {}; > >> } > >> @@ -262,7 +285,7 @@ SizeRange SimpleConverter::sizes(const Size &input) > >> format.size = { 1, 1 }; > >> ret = m2m_->capture()->setFormat(&format); > >> if (ret < 0) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, Error) > >> << "Failed to set format: " << strerror(-ret); > >> return {}; > >> } > >> @@ -272,7 +295,7 @@ SizeRange SimpleConverter::sizes(const Size &input) > >> format.size = { UINT_MAX, UINT_MAX }; > >> ret = m2m_->capture()->setFormat(&format); > >> if (ret < 0) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, Error) > >> << "Failed to set format: " << strerror(-ret); > >> return {}; > >> } > >> @@ -282,9 +305,12 @@ SizeRange SimpleConverter::sizes(const Size &input) > >> return sizes; > >> } > >> > >> +/** > >> + * \copydoc libcamera::Converter::strideAndFrameSize > >> + */ > >> std::tuple<unsigned int, unsigned int> > >> -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, > >> - const Size &size) > >> +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat, > >> + const Size &size) > >> { > >> V4L2DeviceFormat format; > >> format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat); > >> @@ -297,8 +323,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, > >> return std::make_tuple(format.planes[0].bpl, format.planes[0].size); > >> } > >> > >> -int SimpleConverter::configure(const StreamConfiguration &inputCfg, > >> - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) > >> +/** > >> + * \copydoc libcamera::Converter::configure > >> + */ > >> +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, > >> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) > >> { > >> int ret = 0; > >> > >> @@ -309,7 +338,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, > >> Stream &stream = streams_.emplace_back(this, i); > >> > >> if (!stream.isValid()) { > >> - LOG(SimplePipeline, Error) > >> + LOG(Converter, Error) > >> << "Failed to create stream " << i; > >> ret = -EINVAL; > >> break; > >> @@ -328,8 +357,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, > >> return 0; > >> } > >> > >> -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, > >> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) > >> +/** > >> + * \copydoc libcamera::Converter::exportBuffers > >> + */ > >> +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count, > >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > >> { > >> if (output >= streams_.size()) > >> return -EINVAL; > >> @@ -337,7 +369,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, > >> return streams_[output].exportBuffers(count, buffers); > >> } > >> > >> -int SimpleConverter::start() > >> +/** > >> + * \copydoc libcamera::Converter::start > >> + */ > >> +int V4L2M2MConverter::start() > >> { > >> int ret; > >> > >> @@ -352,14 +387,20 @@ int SimpleConverter::start() > >> return 0; > >> } > >> > >> -void SimpleConverter::stop() > >> +/** > >> + * \copydoc libcamera::Converter::stop > >> + */ > >> +void V4L2M2MConverter::stop() > >> { > >> for (Stream &stream : utils::reverse(streams_)) > >> stream.stop(); > >> } > >> > >> -int SimpleConverter::queueBuffers(FrameBuffer *input, > >> - const std::map<unsigned int, FrameBuffer *> &outputs) > >> +/** > >> + * \copydoc libcamera::Converter::queueBuffers > >> + */ > >> +int V4L2M2MConverter::queueBuffers(FrameBuffer *input, > >> + const std::map<unsigned int, FrameBuffer *> &outputs) > >> { > >> unsigned int mask = 0; > >> int ret; > >> @@ -402,4 +443,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input, > >> return 0; > >> } > >> > >> +REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, "pxp") > > > > I think the only part I'd change here is that the "pxp" might be a > > statically created array or vector of 'compatible' strings, a bit like > > the kernel would define. > > > > Then adding support for a new compatible would just be a new line in a > > table, rather than modifying this registration. It's not much, but it > > would feel 'neater' to just have a single line patch to add an entry. > > > static std::initializer_list<std::string> aliases = { > "pxp" Can that have a comma at the end? (Or does the compiler complain?) "pxp", It's subtle, but that lets me add the next compatible in a single line. + "other-compatible-m2m-device", > }; > > REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, aliases) > > Would it be better ? Yes, that's what I was thinking of. Although I'd call 'aliases' 'compatibles' ? or 'compatible' ? > > > > And once again, I don't think there are any blockers there. Some things > > to consider and tidy up for a v4, but I'd be happy to merge at that. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > >> + > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build > >> new file mode 100644 > >> index 00000000..2aa72fe4 > >> --- /dev/null > >> +++ b/src/libcamera/converter/meson.build > >> @@ -0,0 +1,5 @@ > >> +# SPDX-License-Identifier: CC0-1.0 > >> + > >> +libcamera_sources += files([ > >> + 'converter_v4l2_m2m.cpp' > >> +]) > >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > >> index e9d0324e..ffc294f3 100644 > >> --- a/src/libcamera/meson.build > >> +++ b/src/libcamera/meson.build > >> @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false) > >> libthreads = dependency('threads') > >> > >> subdir('base') > >> +subdir('converter') > >> subdir('ipa') > >> subdir('pipeline') > >> subdir('proxy') > >> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build > >> index 9c99b32f..42b0896d 100644 > >> --- a/src/libcamera/pipeline/simple/meson.build > >> +++ b/src/libcamera/pipeline/simple/meson.build > >> @@ -1,6 +1,5 @@ > >> # SPDX-License-Identifier: CC0-1.0 > >> > >> libcamera_sources += files([ > >> - 'converter.cpp', > >> 'simple.cpp', > >> ]) > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index a32de7f3..24ded4db 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -30,13 +30,13 @@ > >> > >> #include "libcamera/internal/camera.h" > >> #include "libcamera/internal/camera_sensor.h" > >> +#include "libcamera/internal/converter.h" > >> #include "libcamera/internal/device_enumerator.h" > >> #include "libcamera/internal/media_device.h" > >> #include "libcamera/internal/pipeline_handler.h" > >> #include "libcamera/internal/v4l2_subdevice.h" > >> #include "libcamera/internal/v4l2_videodevice.h" > >> > >> -#include "converter.h" > >> > >> namespace libcamera { > >> > >> @@ -266,7 +266,7 @@ public: > >> std::vector<Configuration> configs_; > >> std::map<PixelFormat, std::vector<const Configuration *>> formats_; > >> > >> - std::unique_ptr<SimpleConverter> converter_; > >> + std::unique_ptr<Converter> converter_; > >> std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; > >> bool useConverter_; > >> std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; > >> @@ -492,7 +492,7 @@ int SimpleCameraData::init() > >> /* Open the converter, if any. */ > >> MediaDevice *converter = pipe->converter(); > >> if (converter) { > >> - converter_ = std::make_unique<SimpleConverter>(converter); > >> + converter_ = ConverterFactoryBase::create(converter); > >> if (!converter_->isValid()) { > >> LOG(SimplePipeline, Warning) > >> << "Failed to create converter, disabling format conversion"; > >> -- > >> 2.38.1 > >>
diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h similarity index 83% rename from src/libcamera/pipeline/simple/converter.h rename to include/libcamera/internal/converter/converter_v4l2_m2m.h index f0ebe2e0..ef31eeba 100644 --- a/src/libcamera/pipeline/simple/converter.h +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h @@ -1,8 +1,9 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* * Copyright (C) 2020, Laurent Pinchart + * Copyright 2022 NXP * - * converter.h - Format converter for simple pipeline handler + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface */ #pragma once @@ -19,6 +20,8 @@ #include <libcamera/base/log.h> #include <libcamera/base/signal.h> +#include "libcamera/internal/converter.h" + namespace libcamera { class FrameBuffer; @@ -28,11 +31,12 @@ class SizeRange; struct StreamConfiguration; class V4L2M2MDevice; -class SimpleConverter +class V4L2M2MConverter : public Converter { public: - SimpleConverter(MediaDevice *media); + V4L2M2MConverter(MediaDevice *media); + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } bool isValid() const { return m2m_ != nullptr; } std::vector<PixelFormat> formats(PixelFormat input); @@ -52,14 +56,11 @@ public: int queueBuffers(FrameBuffer *input, const std::map<unsigned int, FrameBuffer *> &outputs); - Signal<FrameBuffer *> inputBufferReady; - Signal<FrameBuffer *> outputBufferReady; - private: class Stream : protected Loggable { public: - Stream(SimpleConverter *converter, unsigned int index); + Stream(V4L2M2MConverter *converter, unsigned int index); bool isValid() const { return m2m_ != nullptr; } @@ -80,7 +81,7 @@ private: void captureBufferReady(FrameBuffer *buffer); void outputBufferReady(FrameBuffer *buffer); - SimpleConverter *converter_; + V4L2M2MConverter *converter_; unsigned int index_; std::unique_ptr<V4L2M2MDevice> m2m_; @@ -88,7 +89,6 @@ private: unsigned int outputBufferCount_; }; - std::string deviceNode_; std::unique_ptr<V4L2M2MDevice> m2m_; std::vector<Stream> streams_; diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build new file mode 100644 index 00000000..891e79e7 --- /dev/null +++ b/include/libcamera/internal/converter/meson.build @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: CC0-1.0 + +libcamera_internal_headers += files([ + 'converter_v4l2_m2m.h', +]) diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build index 8f50d755..b9db5a8c 100644 --- a/include/libcamera/internal/meson.build +++ b/include/libcamera/internal/meson.build @@ -45,3 +45,5 @@ libcamera_internal_headers = files([ 'v4l2_videodevice.h', 'yaml_parser.h', ]) + +subdir('converter') diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp similarity index 68% rename from src/libcamera/pipeline/simple/converter.cpp rename to src/libcamera/converter/converter_v4l2_m2m.cpp index acaaa64c..31acb048 100644 --- a/src/libcamera/pipeline/simple/converter.cpp +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp @@ -1,12 +1,11 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* * Copyright (C) 2020, Laurent Pinchart + * Copyright 2022 NXP * - * converter.cpp - Format converter for simple pipeline handler + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter */ -#include "converter.h" - #include <algorithm> #include <limits.h> @@ -20,19 +19,25 @@ #include "libcamera/internal/media_device.h" #include "libcamera/internal/v4l2_videodevice.h" +#include "libcamera/internal/converter/converter_v4l2_m2m.h" + +/** + * \file internal/converter/converter_v4l2_m2m.h + * \brief V4L2 M2M based converter + */ namespace libcamera { -LOG_DECLARE_CATEGORY(SimplePipeline) +LOG_DECLARE_CATEGORY(Converter) /* ----------------------------------------------------------------------------- - * SimpleConverter::Stream + * V4L2M2MConverter::Stream */ -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index) : converter_(converter), index_(index) { - m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_); + m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode()); m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady); m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady); @@ -42,8 +47,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index) m2m_.reset(); } -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, - const StreamConfiguration &outputCfg) +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg, + const StreamConfiguration &outputCfg) { V4L2PixelFormat videoFormat = m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat); @@ -56,14 +61,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, int ret = m2m_->output()->setFormat(&format); if (ret < 0) { - LOG(SimplePipeline, Error) + LOG(Converter, 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) + LOG(Converter, Error) << "Input format not supported (requested " << inputCfg.size << "-" << videoFormat << ", got " << format << ")"; @@ -78,13 +83,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, ret = m2m_->capture()->setFormat(&format); if (ret < 0) { - LOG(SimplePipeline, Error) + LOG(Converter, Error) << "Failed to set output format: " << strerror(-ret); return ret; } if (format.fourcc != videoFormat || format.size != outputCfg.size) { - LOG(SimplePipeline, Error) + LOG(Converter, Error) << "Output format not supported"; return -EINVAL; } @@ -95,13 +100,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, return 0; } -int SimpleConverter::Stream::exportBuffers(unsigned int count, - std::vector<std::unique_ptr<FrameBuffer>> *buffers) +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count, + std::vector<std::unique_ptr<FrameBuffer>> *buffers) { return m2m_->capture()->exportBuffers(count, buffers); } -int SimpleConverter::Stream::start() +int V4L2M2MConverter::Stream::start() { int ret = m2m_->output()->importBuffers(inputBufferCount_); if (ret < 0) @@ -128,7 +133,7 @@ int SimpleConverter::Stream::start() return 0; } -void SimpleConverter::Stream::stop() +void V4L2M2MConverter::Stream::stop() { m2m_->capture()->streamOff(); m2m_->output()->streamOff(); @@ -136,8 +141,7 @@ void SimpleConverter::Stream::stop() m2m_->output()->releaseBuffers(); } -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, - FrameBuffer *output) +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output) { int ret = m2m_->output()->queueBuffer(input); if (ret < 0) @@ -150,12 +154,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input, return 0; } -std::string SimpleConverter::Stream::logPrefix() const +std::string V4L2M2MConverter::Stream::logPrefix() const { return "stream" + std::to_string(index_); } -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer) { auto it = converter_->queue_.find(buffer); if (it == converter_->queue_.end()) @@ -167,32 +171,34 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer) } } -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer) +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer) { converter_->outputBufferReady.emit(buffer); } /* ----------------------------------------------------------------------------- - * SimpleConverter + * V4L2M2MConverter + */ + +/** + * \class libcamera::V4L2M2MConverter + * \brief The V4L2 M2M converter implements the converter interface based on + * V4L2 M2M device. +*/ + +/** + * \fn V4L2M2MConverter::V4L2M2MConverter + * \brief Construct a V4L2M2MConverter instance + * \param[in] media The media device implementing the converter */ -SimpleConverter::SimpleConverter(MediaDevice *media) +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media) + : Converter(media) { - /* - * Locate the video node. There's no need to validate the pipeline - * further, the caller guarantees that this is a V4L2 mem2mem device. - */ - const std::vector<MediaEntity *> &entities = media->entities(); - auto it = std::find_if(entities.begin(), entities.end(), - [](MediaEntity *entity) { - return entity->function() == MEDIA_ENT_F_IO_V4L; - }); - if (it == entities.end()) + if (deviceNode().empty()) return; - deviceNode_ = (*it)->deviceNode(); - - m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_); + m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode()); int ret = m2m_->open(); if (ret < 0) { m2m_.reset(); @@ -200,7 +206,21 @@ SimpleConverter::SimpleConverter(MediaDevice *media) } } -std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) +/** + * \fn libcamera::V4L2M2MConverter::loadConfiguration + * \details \copydetails libcamera::Converter::loadConfiguration + */ + +/** + * \fn libcamera::V4L2M2MConverter::isValid + * \details \copydetails libcamera::Converter::isValid + */ + +/** + * \fn libcamera::V4L2M2MConverter::formats + * \details \copydetails libcamera::Converter::formats + */ +std::vector<PixelFormat> V4L2M2MConverter::formats(PixelFormat input) { if (!m2m_) return {}; @@ -215,13 +235,13 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) int ret = m2m_->output()->setFormat(&v4l2Format); if (ret < 0) { - LOG(SimplePipeline, Error) + LOG(Converter, Error) << "Failed to set format: " << strerror(-ret); return {}; } if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) { - LOG(SimplePipeline, Debug) + LOG(Converter, Debug) << "Input format " << input << " not supported."; return {}; } @@ -237,7 +257,10 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input) return pixelFormats; } -SizeRange SimpleConverter::sizes(const Size &input) +/** + * \copydoc libcamera::Converter::sizes + */ +SizeRange V4L2M2MConverter::sizes(const Size &input) { if (!m2m_) return {}; @@ -252,7 +275,7 @@ SizeRange SimpleConverter::sizes(const Size &input) int ret = m2m_->output()->setFormat(&format); if (ret < 0) { - LOG(SimplePipeline, Error) + LOG(Converter, Error) << "Failed to set format: " << strerror(-ret); return {}; } @@ -262,7 +285,7 @@ SizeRange SimpleConverter::sizes(const Size &input) format.size = { 1, 1 }; ret = m2m_->capture()->setFormat(&format); if (ret < 0) { - LOG(SimplePipeline, Error) + LOG(Converter, Error) << "Failed to set format: " << strerror(-ret); return {}; } @@ -272,7 +295,7 @@ SizeRange SimpleConverter::sizes(const Size &input) format.size = { UINT_MAX, UINT_MAX }; ret = m2m_->capture()->setFormat(&format); if (ret < 0) { - LOG(SimplePipeline, Error) + LOG(Converter, Error) << "Failed to set format: " << strerror(-ret); return {}; } @@ -282,9 +305,12 @@ SizeRange SimpleConverter::sizes(const Size &input) return sizes; } +/** + * \copydoc libcamera::Converter::strideAndFrameSize + */ std::tuple<unsigned int, unsigned int> -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, - const Size &size) +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat, + const Size &size) { V4L2DeviceFormat format; format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat); @@ -297,8 +323,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat, return std::make_tuple(format.planes[0].bpl, format.planes[0].size); } -int SimpleConverter::configure(const StreamConfiguration &inputCfg, - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) +/** + * \copydoc libcamera::Converter::configure + */ +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) { int ret = 0; @@ -309,7 +338,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, Stream &stream = streams_.emplace_back(this, i); if (!stream.isValid()) { - LOG(SimplePipeline, Error) + LOG(Converter, Error) << "Failed to create stream " << i; ret = -EINVAL; break; @@ -328,8 +357,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg, return 0; } -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, - std::vector<std::unique_ptr<FrameBuffer>> *buffers) +/** + * \copydoc libcamera::Converter::exportBuffers + */ +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count, + std::vector<std::unique_ptr<FrameBuffer>> *buffers) { if (output >= streams_.size()) return -EINVAL; @@ -337,7 +369,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, return streams_[output].exportBuffers(count, buffers); } -int SimpleConverter::start() +/** + * \copydoc libcamera::Converter::start + */ +int V4L2M2MConverter::start() { int ret; @@ -352,14 +387,20 @@ int SimpleConverter::start() return 0; } -void SimpleConverter::stop() +/** + * \copydoc libcamera::Converter::stop + */ +void V4L2M2MConverter::stop() { for (Stream &stream : utils::reverse(streams_)) stream.stop(); } -int SimpleConverter::queueBuffers(FrameBuffer *input, - const std::map<unsigned int, FrameBuffer *> &outputs) +/** + * \copydoc libcamera::Converter::queueBuffers + */ +int V4L2M2MConverter::queueBuffers(FrameBuffer *input, + const std::map<unsigned int, FrameBuffer *> &outputs) { unsigned int mask = 0; int ret; @@ -402,4 +443,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input, return 0; } +REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, "pxp") + } /* namespace libcamera */ diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build new file mode 100644 index 00000000..2aa72fe4 --- /dev/null +++ b/src/libcamera/converter/meson.build @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: CC0-1.0 + +libcamera_sources += files([ + 'converter_v4l2_m2m.cpp' +]) diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index e9d0324e..ffc294f3 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false) libthreads = dependency('threads') subdir('base') +subdir('converter') subdir('ipa') subdir('pipeline') subdir('proxy') diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build index 9c99b32f..42b0896d 100644 --- a/src/libcamera/pipeline/simple/meson.build +++ b/src/libcamera/pipeline/simple/meson.build @@ -1,6 +1,5 @@ # SPDX-License-Identifier: CC0-1.0 libcamera_sources += files([ - 'converter.cpp', 'simple.cpp', ]) diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index a32de7f3..24ded4db 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -30,13 +30,13 @@ #include "libcamera/internal/camera.h" #include "libcamera/internal/camera_sensor.h" +#include "libcamera/internal/converter.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" #include "libcamera/internal/v4l2_subdevice.h" #include "libcamera/internal/v4l2_videodevice.h" -#include "converter.h" namespace libcamera { @@ -266,7 +266,7 @@ public: std::vector<Configuration> configs_; std::map<PixelFormat, std::vector<const Configuration *>> formats_; - std::unique_ptr<SimpleConverter> converter_; + std::unique_ptr<Converter> converter_; std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; bool useConverter_; std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; @@ -492,7 +492,7 @@ int SimpleCameraData::init() /* Open the converter, if any. */ MediaDevice *converter = pipe->converter(); if (converter) { - converter_ = std::make_unique<SimpleConverter>(converter); + converter_ = ConverterFactoryBase::create(converter); if (!converter_->isValid()) { LOG(SimplePipeline, Warning) << "Failed to create converter, disabling format conversion";