[libcamera-devel] [PATCH v3 2/2] libcamera: pipeline: simple: converter: Use generic converter interface

Kieran Bingham kieran.bingham at ideasonboard.com
Mon Dec 12 18:25:37 CET 2022


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 at oss.nxp.com>
> >> Reviewed-by: Jacopo Mondi <jacopo at 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 at 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
> >>


More information about the libcamera-devel mailing list