[{"id":26021,"web_url":"https://patchwork.libcamera.org/comment/26021/","msgid":"<167041771955.9133.3362055462627412916@Monstersaurus>","date":"2022-12-07T12:55:19","subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: pipeline: simple:\n\tconverter: Use generic converter interface","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Xavier Roumegue via libcamera-devel (2022-11-17 18:52:04)\n> Move the simple converter implementation to a generic V4L2 M2M class\n> derived from the converter interface. This latter could be used by\n> other pipeline implementations and as base class for customized V4L2 M2M\n> converters.\n> \n\nI like the sound of this!\n\n\n> Signed-off-by: Xavier Roumegue \n> Reviewed-by: Jacopo Mondi \n> ---\n> .../internal/converter/converter_v4l2_m2m.h | 18 +--\n> .../libcamera/internal/converter/meson.build | 5 +\n> include/libcamera/internal/meson.build | 2 +\n> .../converter_v4l2_m2m.cpp} | 153 +++++++++++-------\n> src/libcamera/converter/meson.build | 5 +\n> src/libcamera/meson.build | 1 +\n> src/libcamera/pipeline/simple/meson.build | 1 -\n> src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> 8 files changed, 123 insertions(+), 68 deletions(-)\n> rename src/libcamera/pipeline/simple/converter.h => include/libcamera/internal/converter/converter_v4l2_m2m.h (83%)\n> create mode 100644 include/libcamera/internal/converter/meson.build\n> rename src/libcamera/{pipeline/simple/converter.cpp => converter/converter_v4l2_m2m.cpp} (68%)\n> create mode 100644 src/libcamera/converter/meson.build\n> \n> diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> similarity index 83%\n> rename from src/libcamera/pipeline/simple/converter.h\n> rename to include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index f0ebe2e0..ef31eeba 100644\n> --- a/src/libcamera/pipeline/simple/converter.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -1,8 +1,9 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> * Copyright (C) 2020, Laurent Pinchart\n> + * Copyright 2022 NXP\n> *\n> - * converter.h - Format converter for simple pipeline handler\n> + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface\n> */\n> \n> #pragma once\n> @@ -19,6 +20,8 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/converter.h\"\n> +\n> namespace libcamera {\n> \n> class FrameBuffer;\n> @@ -28,11 +31,12 @@ class SizeRange;\n> struct StreamConfiguration;\n> class V4L2M2MDevice;\n> \n> -class SimpleConverter\n> +class V4L2M2MConverter : public Converter\n> {\n> public:\n> - SimpleConverter(MediaDevice *media);\n> + V4L2M2MConverter(MediaDevice *media);\n> \n> + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n> bool isValid() const { return m2m_ != nullptr; }\n> \n> std::vector formats(PixelFormat input);\n> @@ -52,14 +56,11 @@ public:\n> int queueBuffers(FrameBuffer *input,\n> const std::map &outputs);\n> \n> - Signal inputBufferReady;\n> - Signal outputBufferReady;\n> -\n> private:\n> class Stream : protected Loggable\n> {\n> public:\n> - Stream(SimpleConverter *converter, unsigned int index);\n> + Stream(V4L2M2MConverter *converter, unsigned int index);\n> \n> bool isValid() const { return m2m_ != nullptr; }\n> \n> @@ -80,7 +81,7 @@ private:\n> void captureBufferReady(FrameBuffer *buffer);\n> void outputBufferReady(FrameBuffer *buffer);\n> \n> - SimpleConverter *converter_;\n> + V4L2M2MConverter *converter_;\n> unsigned int index_;\n> std::unique_ptr m2m_;\n> \n> @@ -88,7 +89,6 @@ private:\n> unsigned int outputBufferCount_;\n> };\n> \n> - std::string deviceNode_;\n> std::unique_ptr m2m_;\n> \n> std::vector streams_;\n> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build\n> new file mode 100644\n> index 00000000..891e79e7\n> --- /dev/null\n> +++ b/include/libcamera/internal/converter/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_internal_headers += files([\n> + 'converter_v4l2_m2m.h',\n\nI'll be interested to see what other convertors end in here too.\n\nI could imagine a GPU/OpenGL convertor, a libjpeg convertor ... (I guess\nthat's an encoder?, but I'd imagine the same base interface?) ... or others ...\n\n\n> +])\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 8f50d755..b9db5a8c 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -45,3 +45,5 @@ libcamera_internal_headers = files([\n> 'v4l2_videodevice.h',\n> 'yaml_parser.h',\n> ])\n> +\n> +subdir('converter')\n> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> similarity index 68%\n> rename from src/libcamera/pipeline/simple/converter.cpp\n> rename to src/libcamera/converter/converter_v4l2_m2m.cpp\n> index acaaa64c..31acb048 100644\n> --- a/src/libcamera/pipeline/simple/converter.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -1,12 +1,11 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> * Copyright (C) 2020, Laurent Pinchart\n> + * Copyright 2022 NXP\n> *\n> - * converter.cpp - Format converter for simple pipeline handler\n> + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter\n> */\n> \n> -#include \"converter.h\"\n> -\n> #include \n> #include \n> \n> @@ -20,19 +19,25 @@\n> \n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> +#include \"libcamera/internal/converter/converter_v4l2_m2m.h\"\n> +\n> +/**\n> + * \\file internal/converter/converter_v4l2_m2m.h\n> + * \\brief V4L2 M2M based converter\n> + */\n> \n> namespace libcamera {\n> \n> -LOG_DECLARE_CATEGORY(SimplePipeline)\n> +LOG_DECLARE_CATEGORY(Converter)\n\nHrm.. no, we normally have one log category per component. I think this\nshould be the V4L2M2MConvertor log category ?\n\nBut maybe we should have more generic log categories on some occasions?\nSo this isn't a hard reject...\n\n\n> \n> /* -----------------------------------------------------------------------------\n> - * SimpleConverter::Stream\n> + * V4L2M2MConverter::Stream\n> */\n> \n> -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)\n> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index)\n> : converter_(converter), index_(index)\n> {\n> - m2m_ = std::make_unique(converter->deviceNode_);\n> + m2m_ = std::make_unique(converter->deviceNode());\n> \n> m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);\n> m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);\n> @@ -42,8 +47,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)\n> m2m_.reset();\n> }\n> \n> -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> - const StreamConfiguration &outputCfg)\n> +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> + const StreamConfiguration &outputCfg)\n> {\n> V4L2PixelFormat videoFormat =\n> m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);\n> @@ -56,14 +61,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> \n> int ret = m2m_->output()->setFormat(&format);\n> if (ret < 0) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Failed to set input format: \" << strerror(-ret);\n> return ret;\n> }\n> \n> if (format.fourcc != videoFormat || format.size != inputCfg.size ||\n> format.planes[0].bpl != inputCfg.stride) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Input format not supported (requested \"\n> << inputCfg.size << \"-\" << videoFormat\n> << \", got \" << format << \")\";\n> @@ -78,13 +83,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> \n> ret = m2m_->capture()->setFormat(&format);\n> if (ret < 0) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Failed to set output format: \" << strerror(-ret);\n> return ret;\n> }\n> \n> if (format.fourcc != videoFormat || format.size != outputCfg.size) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Output format not supported\";\n> return -EINVAL;\n> }\n> @@ -95,13 +100,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> return 0;\n> }\n> \n> -int SimpleConverter::Stream::exportBuffers(unsigned int count,\n> - std::vector> *buffers)\n> +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,\n> + std::vector> *buffers)\n> {\n> return m2m_->capture()->exportBuffers(count, buffers);\n> }\n> \n> -int SimpleConverter::Stream::start()\n> +int V4L2M2MConverter::Stream::start()\n> {\n> int ret = m2m_->output()->importBuffers(inputBufferCount_);\n> if (ret < 0)\n> @@ -128,7 +133,7 @@ int SimpleConverter::Stream::start()\n> return 0;\n> }\n> \n> -void SimpleConverter::Stream::stop()\n> +void V4L2M2MConverter::Stream::stop()\n> {\n> m2m_->capture()->streamOff();\n> m2m_->output()->streamOff();\n> @@ -136,8 +141,7 @@ void SimpleConverter::Stream::stop()\n> m2m_->output()->releaseBuffers();\n> }\n> \n> -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,\n> - FrameBuffer *output)\n> +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output)\n> {\n> int ret = m2m_->output()->queueBuffer(input);\n> if (ret < 0)\n> @@ -150,12 +154,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,\n> return 0;\n> }\n> \n> -std::string SimpleConverter::Stream::logPrefix() const\n> +std::string V4L2M2MConverter::Stream::logPrefix() const\n> {\n> return \"stream\" + std::to_string(index_);\n\nI'm not yet sure if this will be unique enough if we have multiple\nconvertors. However given the logging infrastructure will print a file\nand line number, I think it will be clear.\n\nSo I think this is ok.\n\n\n> }\n> \n> -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n> +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n> {\n> auto it = converter_->queue_.find(buffer);\n> if (it == converter_->queue_.end())\n> @@ -167,32 +171,34 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n> }\n> }\n> \n> -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n> +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n> {\n> converter_->outputBufferReady.emit(buffer);\n> }\n> \n> /* -----------------------------------------------------------------------------\n> - * SimpleConverter\n> + * V4L2M2MConverter\n> + */\n> +\n> +/**\n> + * \\class libcamera::V4L2M2MConverter\n> + * \\brief The V4L2 M2M converter implements the converter interface based on\n> + * V4L2 M2M device.\n> +*/\n> +\n> +/**\n> + * \\fn V4L2M2MConverter::V4L2M2MConverter\n> + * \\brief Construct a V4L2M2MConverter instance\n> + * \\param[in] media The media device implementing the converter\n> */\n> \n> -SimpleConverter::SimpleConverter(MediaDevice *media)\n> +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\n> + : Converter(media)\n> {\n> - /*\n> - * Locate the video node. There's no need to validate the pipeline\n> - * further, the caller guarantees that this is a V4L2 mem2mem device.\n> - */\n> - const std::vector &entities = media->entities();\n> - auto it = std::find_if(entities.begin(), entities.end(),\n> - [](MediaEntity *entity) {\n> - return entity->function() == MEDIA_ENT_F_IO_V4L;\n> - });\n> - if (it == entities.end())\n> + if (deviceNode().empty())\n\nDo we need any kind of warning here? I presume this means we were unable\nto construct a suitable convertor... Or will the Converter(media)\ninitialiser have already printed an error in that instance?\n\n> return;\n> \n> - deviceNode_ = (*it)->deviceNode();\n> -\n> - m2m_ = std::make_unique(deviceNode_);\n> + m2m_ = std::make_unique(deviceNode());\n> int ret = m2m_->open();\n> if (ret < 0) {\n> m2m_.reset();\n> @@ -200,7 +206,21 @@ SimpleConverter::SimpleConverter(MediaDevice *media)\n> }\n> }\n> \n> -std::vector SimpleConverter::formats(PixelFormat input)\n> +/**\n> + * \\fn libcamera::V4L2M2MConverter::loadConfiguration\n> + * \\details \\copydetails libcamera::Converter::loadConfiguration\n> + */\n> +\n> +/**\n> + * \\fn libcamera::V4L2M2MConverter::isValid\n> + * \\details \\copydetails libcamera::Converter::isValid\n> + */\n> +\n> +/**\n> + * \\fn libcamera::V4L2M2MConverter::formats\n> + * \\details \\copydetails libcamera::Converter::formats\n> + */\n> +std::vector V4L2M2MConverter::formats(PixelFormat input)\n> {\n> if (!m2m_)\n> return {};\n> @@ -215,13 +235,13 @@ std::vector SimpleConverter::formats(PixelFormat input)\n> \n> int ret = m2m_->output()->setFormat(&v4l2Format);\n> if (ret < 0) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Failed to set format: \" << strerror(-ret);\n> return {};\n> }\n> \n> if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) {\n> - LOG(SimplePipeline, Debug)\n> + LOG(Converter, Debug)\n> << \"Input format \" << input << \" not supported.\";\n> return {};\n> }\n> @@ -237,7 +257,10 @@ std::vector SimpleConverter::formats(PixelFormat input)\n> return pixelFormats;\n> }\n> \n> -SizeRange SimpleConverter::sizes(const Size &input)\n> +/**\n> + * \\copydoc libcamera::Converter::sizes\n> + */\n> +SizeRange V4L2M2MConverter::sizes(const Size &input)\n> {\n> if (!m2m_)\n> return {};\n> @@ -252,7 +275,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> \n> int ret = m2m_->output()->setFormat(&format);\n> if (ret < 0) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Failed to set format: \" << strerror(-ret);\n> return {};\n> }\n> @@ -262,7 +285,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> format.size = { 1, 1 };\n> ret = m2m_->capture()->setFormat(&format);\n> if (ret < 0) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Failed to set format: \" << strerror(-ret);\n> return {};\n> }\n> @@ -272,7 +295,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> format.size = { UINT_MAX, UINT_MAX };\n> ret = m2m_->capture()->setFormat(&format);\n> if (ret < 0) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Failed to set format: \" << strerror(-ret);\n> return {};\n> }\n> @@ -282,9 +305,12 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> return sizes;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::Converter::strideAndFrameSize\n> + */\n> std::tuple\n> -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n> - const Size &size)\n> +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n> + const Size &size)\n> {\n> V4L2DeviceFormat format;\n> format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);\n> @@ -297,8 +323,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n> return std::make_tuple(format.planes[0].bpl, format.planes[0].size);\n> }\n> \n> -int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n> - const std::vector> &outputCfgs)\n> +/**\n> + * \\copydoc libcamera::Converter::configure\n> + */\n> +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> + const std::vector> &outputCfgs)\n> {\n> int ret = 0;\n> \n> @@ -309,7 +338,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n> Stream &stream = streams_.emplace_back(this, i);\n> \n> if (!stream.isValid()) {\n> - LOG(SimplePipeline, Error)\n> + LOG(Converter, Error)\n> << \"Failed to create stream \" << i;\n> ret = -EINVAL;\n> break;\n> @@ -328,8 +357,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n> return 0;\n> }\n> \n> -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n> - std::vector> *buffers)\n> +/**\n> + * \\copydoc libcamera::Converter::exportBuffers\n> + */\n> +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,\n> + std::vector> *buffers)\n> {\n> if (output >= streams_.size())\n> return -EINVAL;\n> @@ -337,7 +369,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n> return streams_[output].exportBuffers(count, buffers);\n> }\n> \n> -int SimpleConverter::start()\n> +/**\n> + * \\copydoc libcamera::Converter::start\n> + */\n> +int V4L2M2MConverter::start()\n> {\n> int ret;\n> \n> @@ -352,14 +387,20 @@ int SimpleConverter::start()\n> return 0;\n> }\n> \n> -void SimpleConverter::stop()\n> +/**\n> + * \\copydoc libcamera::Converter::stop\n> + */\n> +void V4L2M2MConverter::stop()\n> {\n> for (Stream &stream : utils::reverse(streams_))\n> stream.stop();\n> }\n> \n> -int SimpleConverter::queueBuffers(FrameBuffer *input,\n> - const std::map &outputs)\n> +/**\n> + * \\copydoc libcamera::Converter::queueBuffers\n> + */\n> +int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> + const std::map &outputs)\n> {\n> unsigned int mask = 0;\n> int ret;\n> @@ -402,4 +443,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input,\n> return 0;\n> }\n> \n> +REGISTER_CONVERTER(\"v4l2_m2m\", V4L2M2MConverter, \"pxp\")\n\nI think the only part I'd change here is that the \"pxp\" might be a\nstatically created array or vector of 'compatible' strings, a bit like\nthe kernel would define.\n\nThen adding support for a new compatible would just be a new line in a\ntable, rather than modifying this registration. It's not much, but it\nwould feel 'neater' to just have a single line patch to add an entry.\n\n\nAnd once again, I don't think there are any blockers there. Some things\nto consider and tidy up for a v4, but I'd be happy to merge at that.\n\n\nReviewed-by: Kieran Bingham \n\n\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build\n> new file mode 100644\n> index 00000000..2aa72fe4\n> --- /dev/null\n> +++ b/src/libcamera/converter/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_sources += files([\n> + 'converter_v4l2_m2m.cpp'\n> +])\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index e9d0324e..ffc294f3 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false)\n> libthreads = dependency('threads')\n> \n> subdir('base')\n> +subdir('converter')\n> subdir('ipa')\n> subdir('pipeline')\n> subdir('proxy')\n> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build\n> index 9c99b32f..42b0896d 100644\n> --- a/src/libcamera/pipeline/simple/meson.build\n> +++ b/src/libcamera/pipeline/simple/meson.build\n> @@ -1,6 +1,5 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libcamera_sources += files([\n> - 'converter.cpp',\n> 'simple.cpp',\n> ])\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index a32de7f3..24ded4db 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -30,13 +30,13 @@\n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/converter.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> #include \"libcamera/internal/v4l2_subdevice.h\"\n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> \n> -#include \"converter.h\"\n> \n> namespace libcamera {\n> \n> @@ -266,7 +266,7 @@ public:\n> std::vector configs_;\n> std::map> formats_;\n> \n> - std::unique_ptr converter_;\n> + std::unique_ptr converter_;\n> std::vector> converterBuffers_;\n> bool useConverter_;\n> std::queue> converterQueue_;\n> @@ -492,7 +492,7 @@ int SimpleCameraData::init()\n> /* Open the converter, if any. */\n> MediaDevice *converter = pipe->converter();\n> if (converter) {\n> - converter_ = std::make_unique(converter);\n> + converter_ = ConverterFactoryBase::create(converter);\n> if (!converter_->isValid()) {\n> LOG(SimplePipeline, Warning)\n> << \"Failed to create converter, disabling format conversion\";\n> -- \n> 2.38.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D82BBDE6B\n\tfor ;\n\tWed, 7 Dec 2022 12:55:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C03746333F;\n\tWed, 7 Dec 2022 13:55:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5EC763335\n\tfor ;\n\tWed, 7 Dec 2022 13:55:21 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 50D1787B;\n\tWed, 7 Dec 2022 13:55:21 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670417723;\n\tbh=cyO64R9tt3beakWdX++zt0N9De2Ws0CLoiAPNkMrum4=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZYGqCkxuW5SFp9yV0JWHxDCAvfGLe42NeZDsXGOG/5GFeJUsI2OgYfDPE8o3oMipH\n\tNfDV+4bNF8b8n8MBqZq6EAmRGRZfgU3ILvAT0xo8i3IET50xXg13QltYUJijrJwrfX\n\tpoouT/lG753WJH7ZaB87lleug5IQ4U4TWmNNmJ60xHHvgr2/aVnfkj97AfNQEaK1q+\n\tco6A77AVusCxtyN/DXQDlX+HC5UzaA7VhgxIr73htetFo7akWEgtRyGCnFd2eWWg7o\n\tRULLX/BHECDbpm3rBzhLNBcZEBRvxczzTWQ23uvmKoVoyQ6IaQXq+9BDSpgDUFI9z5\n\tUUGuKweAigJew==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670417721;\n\tbh=cyO64R9tt3beakWdX++zt0N9De2Ws0CLoiAPNkMrum4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=KGbH0SGwSowSWxgfD571GEM3yXvrZPvDLOZzLVlZXrwm9dpdUD1WR2NkxHaDengOn\n\trW+Av7k/dfuO89l49uUfcB8WvxDmhbq+hcRDJy4QS6ACnHthXlGTQybRT4fquIWgV0\n\tu2SsWjUYu+yVLNv0qcJ+LdjhdeHeh6UJnQh5f6Vs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KGbH0SGw\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221117185204.11625-2-xavier.roumegue@oss.nxp.com>","References":"<20221117185204.11625-1-xavier.roumegue@oss.nxp.com>\n\t<20221117185204.11625-2-xavier.roumegue@oss.nxp.com>","To":"Xavier Roumegue ,\n\tXavier Roumegue via libcamera-devel\n\t, \n\tjacopo@jmondi.org, laurent.pinchart@ideasonboard.com","Date":"Wed, 07 Dec 2022 12:55:19 +0000","Message-ID":"<167041771955.9133.3362055462627412916@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: pipeline: simple:\n\tconverter: Use generic converter interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26037,"web_url":"https://patchwork.libcamera.org/comment/26037/","msgid":"<28798aa5-26f6-6d13-93c7-ed33cd4a97a2@oss.nxp.com>","date":"2022-12-10T15:15:03","subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: pipeline: simple:\n\tconverter: Use generic converter interface","submitter":{"id":107,"url":"https://patchwork.libcamera.org/api/people/107/","name":"Xavier Roumegue","email":"xavier.roumegue@oss.nxp.com"},"content":"Hi Kieran,\n\nThanks for your review.\n\nOn 12/7/22 13:55, Kieran Bingham wrote:\n> Quoting Xavier Roumegue via libcamera-devel (2022-11-17 18:52:04)\n>> Move the simple converter implementation to a generic V4L2 M2M class\n>> derived from the converter interface. This latter could be used by\n>> other pipeline implementations and as base class for customized V4L2 M2M\n>> converters.\n>>\n> \n> I like the sound of this!\n> \n> \n>> Signed-off-by: Xavier Roumegue \n>> Reviewed-by: Jacopo Mondi \n>> ---\n>> .../internal/converter/converter_v4l2_m2m.h | 18 +--\n>> .../libcamera/internal/converter/meson.build | 5 +\n>> include/libcamera/internal/meson.build | 2 +\n>> .../converter_v4l2_m2m.cpp} | 153 +++++++++++-------\n>> src/libcamera/converter/meson.build | 5 +\n>> src/libcamera/meson.build | 1 +\n>> src/libcamera/pipeline/simple/meson.build | 1 -\n>> src/libcamera/pipeline/simple/simple.cpp | 6 +-\n>> 8 files changed, 123 insertions(+), 68 deletions(-)\n>> rename src/libcamera/pipeline/simple/converter.h => include/libcamera/internal/converter/converter_v4l2_m2m.h (83%)\n>> create mode 100644 include/libcamera/internal/converter/meson.build\n>> rename src/libcamera/{pipeline/simple/converter.cpp => converter/converter_v4l2_m2m.cpp} (68%)\n>> create mode 100644 src/libcamera/converter/meson.build\n>>\n>> diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> similarity index 83%\n>> rename from src/libcamera/pipeline/simple/converter.h\n>> rename to include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> index f0ebe2e0..ef31eeba 100644\n>> --- a/src/libcamera/pipeline/simple/converter.h\n>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> @@ -1,8 +1,9 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> * Copyright (C) 2020, Laurent Pinchart\n>> + * Copyright 2022 NXP\n>> *\n>> - * converter.h - Format converter for simple pipeline handler\n>> + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface\n>> */\n>> \n>> #pragma once\n>> @@ -19,6 +20,8 @@\n>> #include \n>> #include \n>> \n>> +#include \"libcamera/internal/converter.h\"\n>> +\n>> namespace libcamera {\n>> \n>> class FrameBuffer;\n>> @@ -28,11 +31,12 @@ class SizeRange;\n>> struct StreamConfiguration;\n>> class V4L2M2MDevice;\n>> \n>> -class SimpleConverter\n>> +class V4L2M2MConverter : public Converter\n>> {\n>> public:\n>> - SimpleConverter(MediaDevice *media);\n>> + V4L2M2MConverter(MediaDevice *media);\n>> \n>> + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n>> bool isValid() const { return m2m_ != nullptr; }\n>> \n>> std::vector formats(PixelFormat input);\n>> @@ -52,14 +56,11 @@ public:\n>> int queueBuffers(FrameBuffer *input,\n>> const std::map &outputs);\n>> \n>> - Signal inputBufferReady;\n>> - Signal outputBufferReady;\n>> -\n>> private:\n>> class Stream : protected Loggable\n>> {\n>> public:\n>> - Stream(SimpleConverter *converter, unsigned int index);\n>> + Stream(V4L2M2MConverter *converter, unsigned int index);\n>> \n>> bool isValid() const { return m2m_ != nullptr; }\n>> \n>> @@ -80,7 +81,7 @@ private:\n>> void captureBufferReady(FrameBuffer *buffer);\n>> void outputBufferReady(FrameBuffer *buffer);\n>> \n>> - SimpleConverter *converter_;\n>> + V4L2M2MConverter *converter_;\n>> unsigned int index_;\n>> std::unique_ptr m2m_;\n>> \n>> @@ -88,7 +89,6 @@ private:\n>> unsigned int outputBufferCount_;\n>> };\n>> \n>> - std::string deviceNode_;\n>> std::unique_ptr m2m_;\n>> \n>> std::vector streams_;\n>> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build\n>> new file mode 100644\n>> index 00000000..891e79e7\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/converter/meson.build\n>> @@ -0,0 +1,5 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +libcamera_internal_headers += files([\n>> + 'converter_v4l2_m2m.h',\n> \n> I'll be interested to see what other convertors end in here too.\n> \n> I could imagine a GPU/OpenGL convertor, a libjpeg convertor ... (I guess\n> that's an encoder?, but I'd imagine the same base interface?) ... or others ...\nI will likely have a need for adding drm devices as converter.\n> \n> \n>> +])\n>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> index 8f50d755..b9db5a8c 100644\n>> --- a/include/libcamera/internal/meson.build\n>> +++ b/include/libcamera/internal/meson.build\n>> @@ -45,3 +45,5 @@ libcamera_internal_headers = files([\n>> 'v4l2_videodevice.h',\n>> 'yaml_parser.h',\n>> ])\n>> +\n>> +subdir('converter')\n>> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> similarity index 68%\n>> rename from src/libcamera/pipeline/simple/converter.cpp\n>> rename to src/libcamera/converter/converter_v4l2_m2m.cpp\n>> index acaaa64c..31acb048 100644\n>> --- a/src/libcamera/pipeline/simple/converter.cpp\n>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> @@ -1,12 +1,11 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> * Copyright (C) 2020, Laurent Pinchart\n>> + * Copyright 2022 NXP\n>> *\n>> - * converter.cpp - Format converter for simple pipeline handler\n>> + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter\n>> */\n>> \n>> -#include \"converter.h\"\n>> -\n>> #include \n>> #include \n>> \n>> @@ -20,19 +19,25 @@\n>> \n>> #include \"libcamera/internal/media_device.h\"\n>> #include \"libcamera/internal/v4l2_videodevice.h\"\n>> +#include \"libcamera/internal/converter/converter_v4l2_m2m.h\"\n>> +\n>> +/**\n>> + * \\file internal/converter/converter_v4l2_m2m.h\n>> + * \\brief V4L2 M2M based converter\n>> + */\n>> \n>> namespace libcamera {\n>> \n>> -LOG_DECLARE_CATEGORY(SimplePipeline)\n>> +LOG_DECLARE_CATEGORY(Converter)\n> \n> Hrm.. no, we normally have one log category per component. I think this\n> should be the V4L2M2MConvertor log category ?\n> \n> But maybe we should have more generic log categories on some occasions?\n> So this isn't a hard reject...\nI initially started with a log category per component, but this was not \nreally convenient as you had to deal with 3 different categories \n(generic converter, v4l2m2m, specialized m2m for handling dewarp \ndevice)...so I finally ended with this proposal, using an unique \nconverter log category. I can revert to the legacy way if you dislike \nthis proposal.\n\n> \n> \n>> \n>> /* -----------------------------------------------------------------------------\n>> - * SimpleConverter::Stream\n>> + * V4L2M2MConverter::Stream\n>> */\n>> \n>> -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)\n>> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index)\n>> : converter_(converter), index_(index)\n>> {\n>> - m2m_ = std::make_unique(converter->deviceNode_);\n>> + m2m_ = std::make_unique(converter->deviceNode());\n>> \n>> m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);\n>> m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);\n>> @@ -42,8 +47,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)\n>> m2m_.reset();\n>> }\n>> \n>> -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n>> - const StreamConfiguration &outputCfg)\n>> +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg,\n>> + const StreamConfiguration &outputCfg)\n>> {\n>> V4L2PixelFormat videoFormat =\n>> m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);\n>> @@ -56,14 +61,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n>> \n>> int ret = m2m_->output()->setFormat(&format);\n>> if (ret < 0) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Failed to set input format: \" << strerror(-ret);\n>> return ret;\n>> }\n>> \n>> if (format.fourcc != videoFormat || format.size != inputCfg.size ||\n>> format.planes[0].bpl != inputCfg.stride) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Input format not supported (requested \"\n>> << inputCfg.size << \"-\" << videoFormat\n>> << \", got \" << format << \")\";\n>> @@ -78,13 +83,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n>> \n>> ret = m2m_->capture()->setFormat(&format);\n>> if (ret < 0) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Failed to set output format: \" << strerror(-ret);\n>> return ret;\n>> }\n>> \n>> if (format.fourcc != videoFormat || format.size != outputCfg.size) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Output format not supported\";\n>> return -EINVAL;\n>> }\n>> @@ -95,13 +100,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n>> return 0;\n>> }\n>> \n>> -int SimpleConverter::Stream::exportBuffers(unsigned int count,\n>> - std::vector> *buffers)\n>> +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,\n>> + std::vector> *buffers)\n>> {\n>> return m2m_->capture()->exportBuffers(count, buffers);\n>> }\n>> \n>> -int SimpleConverter::Stream::start()\n>> +int V4L2M2MConverter::Stream::start()\n>> {\n>> int ret = m2m_->output()->importBuffers(inputBufferCount_);\n>> if (ret < 0)\n>> @@ -128,7 +133,7 @@ int SimpleConverter::Stream::start()\n>> return 0;\n>> }\n>> \n>> -void SimpleConverter::Stream::stop()\n>> +void V4L2M2MConverter::Stream::stop()\n>> {\n>> m2m_->capture()->streamOff();\n>> m2m_->output()->streamOff();\n>> @@ -136,8 +141,7 @@ void SimpleConverter::Stream::stop()\n>> m2m_->output()->releaseBuffers();\n>> }\n>> \n>> -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,\n>> - FrameBuffer *output)\n>> +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output)\n>> {\n>> int ret = m2m_->output()->queueBuffer(input);\n>> if (ret < 0)\n>> @@ -150,12 +154,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,\n>> return 0;\n>> }\n>> \n>> -std::string SimpleConverter::Stream::logPrefix() const\n>> +std::string V4L2M2MConverter::Stream::logPrefix() const\n>> {\n>> return \"stream\" + std::to_string(index_);\n> \n> I'm not yet sure if this will be unique enough if we have multiple\n> convertors. However given the logging infrastructure will print a file\n> and line number, I think it will be clear.\n> \n> So I think this is ok.\n> \n> \n>> }\n>> \n>> -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n>> +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n>> {\n>> auto it = converter_->queue_.find(buffer);\n>> if (it == converter_->queue_.end())\n>> @@ -167,32 +171,34 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n>> }\n>> }\n>> \n>> -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n>> +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n>> {\n>> converter_->outputBufferReady.emit(buffer);\n>> }\n>> \n>> /* -----------------------------------------------------------------------------\n>> - * SimpleConverter\n>> + * V4L2M2MConverter\n>> + */\n>> +\n>> +/**\n>> + * \\class libcamera::V4L2M2MConverter\n>> + * \\brief The V4L2 M2M converter implements the converter interface based on\n>> + * V4L2 M2M device.\n>> +*/\n>> +\n>> +/**\n>> + * \\fn V4L2M2MConverter::V4L2M2MConverter\n>> + * \\brief Construct a V4L2M2MConverter instance\n>> + * \\param[in] media The media device implementing the converter\n>> */\n>> \n>> -SimpleConverter::SimpleConverter(MediaDevice *media)\n>> +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\n>> + : Converter(media)\n>> {\n>> - /*\n>> - * Locate the video node. There's no need to validate the pipeline\n>> - * further, the caller guarantees that this is a V4L2 mem2mem device.\n>> - */\n>> - const std::vector &entities = media->entities();\n>> - auto it = std::find_if(entities.begin(), entities.end(),\n>> - [](MediaEntity *entity) {\n>> - return entity->function() == MEDIA_ENT_F_IO_V4L;\n>> - });\n>> - if (it == entities.end())\n>> + if (deviceNode().empty())\n> \n> Do we need any kind of warning here? I presume this means we were unable\n> to construct a suitable convertor... Or will the Converter(media)\n> initialiser have already printed an error in that instance?\n> \n>> return;\n>> \n>> - deviceNode_ = (*it)->deviceNode();\n>> -\n>> - m2m_ = std::make_unique(deviceNode_);\n>> + m2m_ = std::make_unique(deviceNode());\n>> int ret = m2m_->open();\n>> if (ret < 0) {\n>> m2m_.reset();\n>> @@ -200,7 +206,21 @@ SimpleConverter::SimpleConverter(MediaDevice *media)\n>> }\n>> }\n>> \n>> -std::vector SimpleConverter::formats(PixelFormat input)\n>> +/**\n>> + * \\fn libcamera::V4L2M2MConverter::loadConfiguration\n>> + * \\details \\copydetails libcamera::Converter::loadConfiguration\n>> + */\n>> +\n>> +/**\n>> + * \\fn libcamera::V4L2M2MConverter::isValid\n>> + * \\details \\copydetails libcamera::Converter::isValid\n>> + */\n>> +\n>> +/**\n>> + * \\fn libcamera::V4L2M2MConverter::formats\n>> + * \\details \\copydetails libcamera::Converter::formats\n>> + */\n>> +std::vector V4L2M2MConverter::formats(PixelFormat input)\n>> {\n>> if (!m2m_)\n>> return {};\n>> @@ -215,13 +235,13 @@ std::vector SimpleConverter::formats(PixelFormat input)\n>> \n>> int ret = m2m_->output()->setFormat(&v4l2Format);\n>> if (ret < 0) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Failed to set format: \" << strerror(-ret);\n>> return {};\n>> }\n>> \n>> if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) {\n>> - LOG(SimplePipeline, Debug)\n>> + LOG(Converter, Debug)\n>> << \"Input format \" << input << \" not supported.\";\n>> return {};\n>> }\n>> @@ -237,7 +257,10 @@ std::vector SimpleConverter::formats(PixelFormat input)\n>> return pixelFormats;\n>> }\n>> \n>> -SizeRange SimpleConverter::sizes(const Size &input)\n>> +/**\n>> + * \\copydoc libcamera::Converter::sizes\n>> + */\n>> +SizeRange V4L2M2MConverter::sizes(const Size &input)\n>> {\n>> if (!m2m_)\n>> return {};\n>> @@ -252,7 +275,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n>> \n>> int ret = m2m_->output()->setFormat(&format);\n>> if (ret < 0) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Failed to set format: \" << strerror(-ret);\n>> return {};\n>> }\n>> @@ -262,7 +285,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n>> format.size = { 1, 1 };\n>> ret = m2m_->capture()->setFormat(&format);\n>> if (ret < 0) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Failed to set format: \" << strerror(-ret);\n>> return {};\n>> }\n>> @@ -272,7 +295,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n>> format.size = { UINT_MAX, UINT_MAX };\n>> ret = m2m_->capture()->setFormat(&format);\n>> if (ret < 0) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Failed to set format: \" << strerror(-ret);\n>> return {};\n>> }\n>> @@ -282,9 +305,12 @@ SizeRange SimpleConverter::sizes(const Size &input)\n>> return sizes;\n>> }\n>> \n>> +/**\n>> + * \\copydoc libcamera::Converter::strideAndFrameSize\n>> + */\n>> std::tuple\n>> -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n>> - const Size &size)\n>> +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n>> + const Size &size)\n>> {\n>> V4L2DeviceFormat format;\n>> format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);\n>> @@ -297,8 +323,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n>> return std::make_tuple(format.planes[0].bpl, format.planes[0].size);\n>> }\n>> \n>> -int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n>> - const std::vector> &outputCfgs)\n>> +/**\n>> + * \\copydoc libcamera::Converter::configure\n>> + */\n>> +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n>> + const std::vector> &outputCfgs)\n>> {\n>> int ret = 0;\n>> \n>> @@ -309,7 +338,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n>> Stream &stream = streams_.emplace_back(this, i);\n>> \n>> if (!stream.isValid()) {\n>> - LOG(SimplePipeline, Error)\n>> + LOG(Converter, Error)\n>> << \"Failed to create stream \" << i;\n>> ret = -EINVAL;\n>> break;\n>> @@ -328,8 +357,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n>> return 0;\n>> }\n>> \n>> -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n>> - std::vector> *buffers)\n>> +/**\n>> + * \\copydoc libcamera::Converter::exportBuffers\n>> + */\n>> +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,\n>> + std::vector> *buffers)\n>> {\n>> if (output >= streams_.size())\n>> return -EINVAL;\n>> @@ -337,7 +369,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n>> return streams_[output].exportBuffers(count, buffers);\n>> }\n>> \n>> -int SimpleConverter::start()\n>> +/**\n>> + * \\copydoc libcamera::Converter::start\n>> + */\n>> +int V4L2M2MConverter::start()\n>> {\n>> int ret;\n>> \n>> @@ -352,14 +387,20 @@ int SimpleConverter::start()\n>> return 0;\n>> }\n>> \n>> -void SimpleConverter::stop()\n>> +/**\n>> + * \\copydoc libcamera::Converter::stop\n>> + */\n>> +void V4L2M2MConverter::stop()\n>> {\n>> for (Stream &stream : utils::reverse(streams_))\n>> stream.stop();\n>> }\n>> \n>> -int SimpleConverter::queueBuffers(FrameBuffer *input,\n>> - const std::map &outputs)\n>> +/**\n>> + * \\copydoc libcamera::Converter::queueBuffers\n>> + */\n>> +int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>> + const std::map &outputs)\n>> {\n>> unsigned int mask = 0;\n>> int ret;\n>> @@ -402,4 +443,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input,\n>> return 0;\n>> }\n>> \n>> +REGISTER_CONVERTER(\"v4l2_m2m\", V4L2M2MConverter, \"pxp\")\n> \n> I think the only part I'd change here is that the \"pxp\" might be a\n> statically created array or vector of 'compatible' strings, a bit like\n> the kernel would define.\n> \n> Then adding support for a new compatible would just be a new line in a\n> table, rather than modifying this registration. It's not much, but it\n> would feel 'neater' to just have a single line patch to add an entry.\n> \nstatic std::initializer_list aliases = {\n\t\"pxp\"\n};\n\nREGISTER_CONVERTER(\"v4l2_m2m\", V4L2M2MConverter, aliases)\n\nWould it be better ?\n> \n> And once again, I don't think there are any blockers there. Some things\n> to consider and tidy up for a v4, but I'd be happy to merge at that.\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> \n>> +\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build\n>> new file mode 100644\n>> index 00000000..2aa72fe4\n>> --- /dev/null\n>> +++ b/src/libcamera/converter/meson.build\n>> @@ -0,0 +1,5 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +libcamera_sources += files([\n>> + 'converter_v4l2_m2m.cpp'\n>> +])\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index e9d0324e..ffc294f3 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false)\n>> libthreads = dependency('threads')\n>> \n>> subdir('base')\n>> +subdir('converter')\n>> subdir('ipa')\n>> subdir('pipeline')\n>> subdir('proxy')\n>> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build\n>> index 9c99b32f..42b0896d 100644\n>> --- a/src/libcamera/pipeline/simple/meson.build\n>> +++ b/src/libcamera/pipeline/simple/meson.build\n>> @@ -1,6 +1,5 @@\n>> # SPDX-License-Identifier: CC0-1.0\n>> \n>> libcamera_sources += files([\n>> - 'converter.cpp',\n>> 'simple.cpp',\n>> ])\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index a32de7f3..24ded4db 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -30,13 +30,13 @@\n>> \n>> #include \"libcamera/internal/camera.h\"\n>> #include \"libcamera/internal/camera_sensor.h\"\n>> +#include \"libcamera/internal/converter.h\"\n>> #include \"libcamera/internal/device_enumerator.h\"\n>> #include \"libcamera/internal/media_device.h\"\n>> #include \"libcamera/internal/pipeline_handler.h\"\n>> #include \"libcamera/internal/v4l2_subdevice.h\"\n>> #include \"libcamera/internal/v4l2_videodevice.h\"\n>> \n>> -#include \"converter.h\"\n>> \n>> namespace libcamera {\n>> \n>> @@ -266,7 +266,7 @@ public:\n>> std::vector configs_;\n>> std::map> formats_;\n>> \n>> - std::unique_ptr converter_;\n>> + std::unique_ptr converter_;\n>> std::vector> converterBuffers_;\n>> bool useConverter_;\n>> std::queue> converterQueue_;\n>> @@ -492,7 +492,7 @@ int SimpleCameraData::init()\n>> /* Open the converter, if any. */\n>> MediaDevice *converter = pipe->converter();\n>> if (converter) {\n>> - converter_ = std::make_unique(converter);\n>> + converter_ = ConverterFactoryBase::create(converter);\n>> if (!converter_->isValid()) {\n>> LOG(SimplePipeline, Warning)\n>> << \"Failed to create converter, disabling format conversion\";\n>> -- \n>> 2.38.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F3BB6BE08B\n\tfor ;\n\tSat, 10 Dec 2022 15:15:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 537D66335E;\n\tSat, 10 Dec 2022 16:15:12 +0100 (CET)","from EUR04-HE1-obe.outbound.protection.outlook.com\n\t(mail-he1eur04on2075.outbound.protection.outlook.com [40.107.7.75])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4FCFD603CC\n\tfor ;\n\tSat, 10 Dec 2022 16:15:10 +0100 (CET)","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)\n\tby AS8PR04MB8435.eurprd04.prod.outlook.com (2603:10a6:20b:346::7)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5880.10;\n\tSat, 10 Dec 2022 15:15:06 +0000","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::14d3:8e4:cf07:810d]) by\n\tPAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::14d3:8e4:cf07:810d%4]) with mapi id 15.20.5880.019;\n\tSat, 10 Dec 2022 15:15:06 +0000"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670685312;\n\tbh=ooV9DB4SzeBOSi5sAK92L+vklCSgCiUaNhLbFJRi320=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=VXVRVrK0UpclHz2bgFn3TzUK/zsj+/nDLZoAUhB7PdOMo7UjUvnyiQtUAX574vdVs\n\t9NNeu9A5lEa7hlcMUqhRcOuAjSpJfFWxSkDqLUi4HrttSBrs5nYP/4v6LYqa12K5UB\n\tkn/F6x8WlwDK3I+D0MUl92CyNHYG4h92QdmWMAC2CMFURla68Le0Sfp0LhXppxtVvs\n\tCUF02F2T0c2RbAfKM5yZE8Uoe2CGD9Hsbh2ET8eo78KxGrDmVzwVJ9FVDB6T006DNr\n\tmp+Nh+BYvHy0id0Fm/Z2Y/FqVfh6HvFx5/AUBXmiA7/Vd7h131WMOm07q8WZMa7Z6F\n\tJjXrcOI3p7oew==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=NXP1.onmicrosoft.com;\n\ts=selector2-NXP1-onmicrosoft-com;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;\n\tbh=a34RYSKmt1ytYLs105eFf90dVcw4bgcZKstiDb2IWhk=;\n\tb=momHecVg6zTOOIX1tQ2e+c67XND1+4vHrwRoSiIZjDnQxAUiEN+pb7xhwWM/rC5Xfcovb2Cj4v6rJoxlpJixkdDgEOB62jjjnqlzLEEJxwqieNPZSbOUzk625QnkHIxZj2yGxL9yEmLyAwpCA/GTi/kV+z+OgelbQG7QLVb5V/k="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=NXP1.onmicrosoft.com\n\theader.i=@NXP1.onmicrosoft.com\n\theader.b=\"momHecVg\"; dkim-atps=neutral","dkim=none (message not signed)\n\theader.d=none;dmarc=none action=none header.from=oss.nxp.com;"],"ARC-Seal":"i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;\n\tb=cGOXHdmfKHUfL6iU1mjJl8i3c908MaT1s5q+eYSdphWH50URcbjSXZWPO0My2k7x2zl07wqHkotcdI0lBnSQLR3kan/FTO3PzFiKsERhjgAFavxHapecu+j7YbgskIDEbaI0JZx6Uucb8zYH7evZoCbwaFXZwcoe7d3aM8SnzFm1CJuPNAvrjFcVmDGBH0v3PQyhnLi7Ik4TGsVZ4vX9nkeA1QuquuuQZ6NaoAJzqWTCIyxe/ajd5FgBup4KBNX2Fz44uESqlxxOEDTGlE/lVIr/AgHJnQtJmjNpCBpQ8ygcwR5dDFLHhZgiBs2Tg8VCg9vlvwvozW+1S6VYv7v73g==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com;\n\ts=arcselector9901;\n\th=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1;\n\tbh=a34RYSKmt1ytYLs105eFf90dVcw4bgcZKstiDb2IWhk=;\n\tb=GL8JfjU4vPo0OEOjuTLp/nHNlCUCReXhToUlCmi9X78hNPdJxv0SobCgkqRIfIpmigCVCH9Iy4k4Y8uqfya94qYffFsJHLz0XKzEtH07Onm4+Tgem4v8SyNYNIHuN2zwWtFJR62lEc0SZhH6P9rW4yeRNPMV28TZSJ9okgUYznIjct94Grpbc1mzdDNb0nUl1A+9xca+Chf4kEFgtOwc5mCFmFVWgmaJyBAOET3Lc0+mVMPpAqemll8/iCxv9AQPXtDcpFF01h2ehGMePK+a5100RCAX0MVDD+1V9fycMRiuuLLP39Qpy9FF/n1QbkHz+m9i93P/ytCNa3vtenkqRA==","ARC-Authentication-Results":"i=1; mx.microsoft.com 1; spf=pass\n\tsmtp.mailfrom=oss.nxp.com;\n\tdmarc=pass action=none header.from=oss.nxp.com; \n\tdkim=pass header.d=oss.nxp.com; arc=none","Message-ID":"<28798aa5-26f6-6d13-93c7-ed33cd4a97a2@oss.nxp.com>","Date":"Sat, 10 Dec 2022 16:15:03 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.1","To":"Kieran Bingham ,\n\tXavier Roumegue via libcamera-devel\n\t, \n\tjacopo@jmondi.org, laurent.pinchart@ideasonboard.com","References":"<20221117185204.11625-1-xavier.roumegue@oss.nxp.com>\n\t<20221117185204.11625-2-xavier.roumegue@oss.nxp.com>\n\t<167041771955.9133.3362055462627412916@Monstersaurus>","Content-Language":"en-US, fr","In-Reply-To":"<167041771955.9133.3362055462627412916@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-ClientProxiedBy":"PR2P264CA0046.FRAP264.PROD.OUTLOOK.COM\n\t(2603:10a6:101:1::34) To PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)","MIME-Version":"1.0","X-MS-Exchange-MessageSentRepresentingType":"1","X-MS-PublicTrafficType":"Email","X-MS-TrafficTypeDiagnostic":"PAXPR04MB8703:EE_|AS8PR04MB8435:EE_","X-MS-Office365-Filtering-Correlation-Id":"ca68f8b0-bbcd-43e9-a734-08dadac1508f","X-MS-Exchange-SharedMailbox-RoutingAgent-Processed":"True","X-MS-Exchange-SenderADCheck":"1","X-MS-Exchange-AntiSpam-Relay":"0","X-Microsoft-Antispam":"BCL:0;","X-Microsoft-Antispam-Message-Info":"soVu7gViy8bEjarf7HNnyHVgF8nzKAy3YU/apd5wRZ19kWhrPxcfEvz3NPk1ea9J9TseYB1oRwrhtZgcFhTMqALfxdKBN4LbVlz9wxqgqf1n5z7CgZ1mqPuQoAg2J41hfkOzaIwQJ+YnfOMl8PX76PcUZpHqe+tCt5bZt9MDk38NgAG1IUs+QPynHyNAjUEf1n9HoICCZ7BXPqjEJHNRE+xCl12Vf9NhWCaDVa0h2ttFyOCMQFasiUMX/ziFRYbbi3Bz0Ab9cu3xQVqBGakS7eWKMy+rIMublSKxY8YHoVn2Z5t0YOQY92+S/9aThQQTzYBlf+PzqPqV4Ojw1eFMMqcv7wQayeoS55Xzx0lccR4ROI2VavAeOYYeK196u4CC4tuyM5eL1/s5H3RPWBAM8OggDoTHN2CDZpingurX1ta1d28gjE4Qm46rqriic0jX91YVZqRbLWSRhJJ+A0IPGfATPjO62FOUzxuAwr88dTzJURC614C+WuU5eKXaZQHDYOBoUQIQ6a1jQQ1wu0Ah5BT+LYoBNjrG4FApck8+vhY5zDojzy5PbiOuZo5V+/mmNBdTNUQ+G7eZN+fKXKccXBzxDWtLBSqlUVJNIoHuebQJqE/NigACKT04bv0BZUvHwvkck1QymFTAn+KAshmejoS2E+N0uE+F3W40EieQWHqrHoQsun8pzsb6bqzXqDkdziuT4umKvylxirn76FwQ5BoM4ADEYeAbkXslWmbch8QV1gjCBbDe/OxqfEURK8p3","X-Forefront-Antispam-Report":"CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;\n\tIPV:NLI; SFV:NSPM; H:PAXPR04MB8703.eurprd04.prod.outlook.com; PTR:;\n\tCAT:NONE; \n\tSFS:(13230022)(4636009)(396003)(39860400002)(366004)(376002)(346002)(136003)(451199015)(38100700002)(86362001)(31696002)(6486002)(478600001)(41300700001)(6666004)(8936002)(8676002)(66556008)(66476007)(110136005)(66946007)(316002)(2906002)(83380400001)(4001150100001)(186003)(6512007)(6506007)(5660300002)(53546011)(2616005)(30864003)(31686004)(66899015)(41533002)(45980500001)(43740500002);\n\tDIR:OUT; SFP:1101; ","X-MS-Exchange-AntiSpam-MessageData-ChunkCount":"1","X-MS-Exchange-AntiSpam-MessageData-0":"=?utf-8?q?NE3cjhxggvI28TUoKZVp8HNDD?=\n\t=?utf-8?q?X5sxmq5pd1FdmmblVPdo9EZiVouS+gF0VrRKXcpnw1yLhjRpme9lio1v?=\n\t=?utf-8?q?H1rSP5w1dYJmmwTfPI6A7i9PpTaHnvDDJOvxm2OswQM2uwqSPBRzJ1lX?=\n\t=?utf-8?q?RBQ3t64gomnmWtpLZ/w39bl+ayyyqwhEeOxMzkPMU8JqKf95vwn9CdTr?=\n\t=?utf-8?q?EStKraKSAT+irHh86Ya2VSLJY7/F6slfu2o/u2ccf/st6Ksq8BeJKk26?=\n\t=?utf-8?q?cAB4Ah0TlCssLduk1m1tSQWUexqjiBbLjS+PYIUJfSv3MiDEsxujy09D?=\n\t=?utf-8?q?Dw6cI5N4uXcibSXBWn6rk/DVG64wLblEZx2b0usJGNP55wwuoZLahi5Y?=\n\t=?utf-8?q?vu96yE5I3YZl//N6tCQb4OFye071S6c/OwDYAwn1XosDAK3D+6ApD+fC?=\n\t=?utf-8?q?fzkA/r0BIvRGO/6ti9GWyn9ttyg4dzLqpCqZHLINooSy52PulX+IhPjz?=\n\t=?utf-8?q?6KJlqMwNT9aT/fV4GI7z4IC1gWNkbKuMWxscq3udgAdBvIQNMZ5vNhiB?=\n\t=?utf-8?q?ZSsLfzbUtGUC8Bkek+SKtkuVw1cxSgiADSge93gdkhjpIO8W45+8nPi2?=\n\t=?utf-8?q?0/EA5mVc1GCe2AeOnrTvhzNY7JZkt9LNZ9xQ7u7sw2XRsksab9A+bw2T?=\n\t=?utf-8?q?s38ah2MEw3uSVe+pvvM2pEeEvmG7cqEZv8SVL5fTEO6rUYLsnqGntsbE?=\n\t=?utf-8?q?oJuV1DB9D8s7Zhu7SIQW6GP4IV8I98BGWQ4jrmOIm+KacpwOkVD4/zvR?=\n\t=?utf-8?q?Y7BQ8JnRJkAwlkcslCrY72akxlpxKr1PCHzhuAsIifGjvCYfX5oxeqD4?=\n\t=?utf-8?q?QG0apUxprjHVLzuIapE2vZZjnnUAwn0arQimDPjoJjjH3BsOhKAkIXlA?=\n\t=?utf-8?q?vOZCRIplYoj6r9GApRRmDG+ZzDldma2/8+XYvtb9GlUhGUmMsar0Yt+e?=\n\t=?utf-8?q?+F46SDgbUmPViismdMOJs7PY7Z8NkP+0L9oS5ULD3ULzITrGBfpbQNzz?=\n\t=?utf-8?q?lnuMmWhsSdCFFs1ZwCI1z6zQWxx+PXV0cOPf7uB7p6zlc3WvmmogOCQW?=\n\t=?utf-8?q?w4G7s5cn7I5G1wNRd5t6rEBlGGxDBT3W1BOYghvjpRNCYBwlV0Y7Szl7?=\n\t=?utf-8?q?2dpwsohqitY/+ayxyiyJOCBPP/d3SQ63cJO242USrvAMoY7Z0X6kwcBx?=\n\t=?utf-8?q?76GaQwX26SHQsR4CbJo8hiR4ich0L1mQMDTqgafkd9AApbIce9xzUqND?=\n\t=?utf-8?q?tMnn8dFw/wvw55SRytgW21cMAWTQhDjyLeAcv9gi948Orr7pxMZ1w66I?=\n\t=?utf-8?q?QU3BlcvZDnqBoDoT4S5pJpIkL1f0ZocDf1YHRvmmZ3zOj3NDkqTId0Eg?=\n\t=?utf-8?q?f7sHo3ab/GsxvdHn2F7GrG8JOAmcAuX+npQPoGfeDtgA/J1pih1ZhcoD?=\n\t=?utf-8?q?gHs+Vp2S8VngEmuuiZETM/Q+fsbQTOUwO0X1ZbttJG0vR4B/eLHnBwyd?=\n\t=?utf-8?q?nFexFcVtSTb8pkXccB6IQNo5ZVsYkJjJm377XzkBEinWN/9OLDr1mtHO?=\n\t=?utf-8?q?aIEAEzHJ6tC1EJEkGOBtOXAKV2R/yPPUiDQGIx9S7H2xlJJRda1LCSDV?=\n\t=?utf-8?q?OLY/wNzaEyAqZrruyMdZM/Z+hiEl/x8pzB4MnVXBG9aTUI3DyQLsScNI?=\n\t=?utf-8?q?whgwdu95g3Pc6y5C/+4LUSBM2UoJ48Has7if8Gnq3cH48S31o84+3ZAC?=\n\t=?utf-8?q?/ob4Srp3R9ze6zda8DdZZ6NQ006SfqMM3dEMw=3D=3D?=","X-OriginatorOrg":"oss.nxp.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"ca68f8b0-bbcd-43e9-a734-08dadac1508f","X-MS-Exchange-CrossTenant-AuthSource":"PAXPR04MB8703.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-OriginalArrivalTime":"10 Dec 2022 15:15:06.4472\n\t(UTC)","X-MS-Exchange-CrossTenant-FromEntityHeader":"Hosted","X-MS-Exchange-CrossTenant-Id":"686ea1d3-bc2b-4c6f-a92c-d99c5c301635","X-MS-Exchange-CrossTenant-MailboxType":"HOSTED","X-MS-Exchange-CrossTenant-UserPrincipalName":"0z8qOcjnulrGR3Fjk62y4mlWdp7vqpDiovfXbUorhpnHWLURh37M1SR7w5XSNnSHEuMidIP9mvEN4WLUjN4RYA==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"AS8PR04MB8435","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: pipeline: simple:\n\tconverter: Use generic converter interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"\"Xavier Roumegue \\(OSS\\) via libcamera-devel\"\n\t","Reply-To":"\"Xavier Roumegue \\(OSS\\)\" ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26050,"web_url":"https://patchwork.libcamera.org/comment/26050/","msgid":"<167086593709.9133.14674853388530085140@Monstersaurus>","date":"2022-12-12T17:25:37","subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: pipeline: simple:\n\tconverter: Use generic converter interface","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Xavier,\n\nQuoting Xavier Roumegue (OSS) (2022-12-10 15:15:03)\n> Hi Kieran,\n> \n> Thanks for your review.\n> \n> On 12/7/22 13:55, Kieran Bingham wrote:\n> > Quoting Xavier Roumegue via libcamera-devel (2022-11-17 18:52:04)\n> >> Move the simple converter implementation to a generic V4L2 M2M class\n> >> derived from the converter interface. This latter could be used by\n> >> other pipeline implementations and as base class for customized V4L2 M2M\n> >> converters.\n> >>\n> > \n> > I like the sound of this!\n> > \n> > \n> >> Signed-off-by: Xavier Roumegue \n> >> Reviewed-by: Jacopo Mondi \n> >> ---\n> >> .../internal/converter/converter_v4l2_m2m.h | 18 +--\n> >> .../libcamera/internal/converter/meson.build | 5 +\n> >> include/libcamera/internal/meson.build | 2 +\n> >> .../converter_v4l2_m2m.cpp} | 153 +++++++++++-------\n> >> src/libcamera/converter/meson.build | 5 +\n> >> src/libcamera/meson.build | 1 +\n> >> src/libcamera/pipeline/simple/meson.build | 1 -\n> >> src/libcamera/pipeline/simple/simple.cpp | 6 +-\n> >> 8 files changed, 123 insertions(+), 68 deletions(-)\n> >> rename src/libcamera/pipeline/simple/converter.h => include/libcamera/internal/converter/converter_v4l2_m2m.h (83%)\n> >> create mode 100644 include/libcamera/internal/converter/meson.build\n> >> rename src/libcamera/{pipeline/simple/converter.cpp => converter/converter_v4l2_m2m.cpp} (68%)\n> >> create mode 100644 src/libcamera/converter/meson.build\n> >>\n> >> diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> >> similarity index 83%\n> >> rename from src/libcamera/pipeline/simple/converter.h\n> >> rename to include/libcamera/internal/converter/converter_v4l2_m2m.h\n> >> index f0ebe2e0..ef31eeba 100644\n> >> --- a/src/libcamera/pipeline/simple/converter.h\n> >> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> >> @@ -1,8 +1,9 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> * Copyright (C) 2020, Laurent Pinchart\n> >> + * Copyright 2022 NXP\n> >> *\n> >> - * converter.h - Format converter for simple pipeline handler\n> >> + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface\n> >> */\n> >> \n> >> #pragma once\n> >> @@ -19,6 +20,8 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \"libcamera/internal/converter.h\"\n> >> +\n> >> namespace libcamera {\n> >> \n> >> class FrameBuffer;\n> >> @@ -28,11 +31,12 @@ class SizeRange;\n> >> struct StreamConfiguration;\n> >> class V4L2M2MDevice;\n> >> \n> >> -class SimpleConverter\n> >> +class V4L2M2MConverter : public Converter\n> >> {\n> >> public:\n> >> - SimpleConverter(MediaDevice *media);\n> >> + V4L2M2MConverter(MediaDevice *media);\n> >> \n> >> + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n> >> bool isValid() const { return m2m_ != nullptr; }\n> >> \n> >> std::vector formats(PixelFormat input);\n> >> @@ -52,14 +56,11 @@ public:\n> >> int queueBuffers(FrameBuffer *input,\n> >> const std::map &outputs);\n> >> \n> >> - Signal inputBufferReady;\n> >> - Signal outputBufferReady;\n> >> -\n> >> private:\n> >> class Stream : protected Loggable\n> >> {\n> >> public:\n> >> - Stream(SimpleConverter *converter, unsigned int index);\n> >> + Stream(V4L2M2MConverter *converter, unsigned int index);\n> >> \n> >> bool isValid() const { return m2m_ != nullptr; }\n> >> \n> >> @@ -80,7 +81,7 @@ private:\n> >> void captureBufferReady(FrameBuffer *buffer);\n> >> void outputBufferReady(FrameBuffer *buffer);\n> >> \n> >> - SimpleConverter *converter_;\n> >> + V4L2M2MConverter *converter_;\n> >> unsigned int index_;\n> >> std::unique_ptr m2m_;\n> >> \n> >> @@ -88,7 +89,6 @@ private:\n> >> unsigned int outputBufferCount_;\n> >> };\n> >> \n> >> - std::string deviceNode_;\n> >> std::unique_ptr m2m_;\n> >> \n> >> std::vector streams_;\n> >> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build\n> >> new file mode 100644\n> >> index 00000000..891e79e7\n> >> --- /dev/null\n> >> +++ b/include/libcamera/internal/converter/meson.build\n> >> @@ -0,0 +1,5 @@\n> >> +# SPDX-License-Identifier: CC0-1.0\n> >> +\n> >> +libcamera_internal_headers += files([\n> >> + 'converter_v4l2_m2m.h',\n> > \n> > I'll be interested to see what other convertors end in here too.\n> > \n> > I could imagine a GPU/OpenGL convertor, a libjpeg convertor ... (I guess\n> > that's an encoder?, but I'd imagine the same base interface?) ... or others ...\n> I will likely have a need for adding drm devices as converter.\n> > \n> > \n> >> +])\n> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> >> index 8f50d755..b9db5a8c 100644\n> >> --- a/include/libcamera/internal/meson.build\n> >> +++ b/include/libcamera/internal/meson.build\n> >> @@ -45,3 +45,5 @@ libcamera_internal_headers = files([\n> >> 'v4l2_videodevice.h',\n> >> 'yaml_parser.h',\n> >> ])\n> >> +\n> >> +subdir('converter')\n> >> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> >> similarity index 68%\n> >> rename from src/libcamera/pipeline/simple/converter.cpp\n> >> rename to src/libcamera/converter/converter_v4l2_m2m.cpp\n> >> index acaaa64c..31acb048 100644\n> >> --- a/src/libcamera/pipeline/simple/converter.cpp\n> >> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> >> @@ -1,12 +1,11 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> * Copyright (C) 2020, Laurent Pinchart\n> >> + * Copyright 2022 NXP\n> >> *\n> >> - * converter.cpp - Format converter for simple pipeline handler\n> >> + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter\n> >> */\n> >> \n> >> -#include \"converter.h\"\n> >> -\n> >> #include \n> >> #include \n> >> \n> >> @@ -20,19 +19,25 @@\n> >> \n> >> #include \"libcamera/internal/media_device.h\"\n> >> #include \"libcamera/internal/v4l2_videodevice.h\"\n> >> +#include \"libcamera/internal/converter/converter_v4l2_m2m.h\"\n> >> +\n> >> +/**\n> >> + * \\file internal/converter/converter_v4l2_m2m.h\n> >> + * \\brief V4L2 M2M based converter\n> >> + */\n> >> \n> >> namespace libcamera {\n> >> \n> >> -LOG_DECLARE_CATEGORY(SimplePipeline)\n> >> +LOG_DECLARE_CATEGORY(Converter)\n> > \n> > Hrm.. no, we normally have one log category per component. I think this\n> > should be the V4L2M2MConvertor log category ?\n> > \n> > But maybe we should have more generic log categories on some occasions?\n> > So this isn't a hard reject...\n\n> I initially started with a log category per component, but this was not \n> really convenient as you had to deal with 3 different categories \n> (generic converter, v4l2m2m, specialized m2m for handling dewarp \n> device)...so I finally ended with this proposal, using an unique \n> converter log category. I can revert to the legacy way if you dislike \n> this proposal.\n\nI think this is fine for the time being. If it becomes troublesome when\nwe have lots of convertors we can make it more specific then...\n\n\n> >> \n> >> /* -----------------------------------------------------------------------------\n> >> - * SimpleConverter::Stream\n> >> + * V4L2M2MConverter::Stream\n> >> */\n> >> \n> >> -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)\n> >> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index)\n> >> : converter_(converter), index_(index)\n> >> {\n> >> - m2m_ = std::make_unique(converter->deviceNode_);\n> >> + m2m_ = std::make_unique(converter->deviceNode());\n> >> \n> >> m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);\n> >> m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);\n> >> @@ -42,8 +47,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)\n> >> m2m_.reset();\n> >> }\n> >> \n> >> -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> >> - const StreamConfiguration &outputCfg)\n> >> +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> >> + const StreamConfiguration &outputCfg)\n> >> {\n> >> V4L2PixelFormat videoFormat =\n> >> m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);\n> >> @@ -56,14 +61,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> >> \n> >> int ret = m2m_->output()->setFormat(&format);\n> >> if (ret < 0) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Failed to set input format: \" << strerror(-ret);\n> >> return ret;\n> >> }\n> >> \n> >> if (format.fourcc != videoFormat || format.size != inputCfg.size ||\n> >> format.planes[0].bpl != inputCfg.stride) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Input format not supported (requested \"\n> >> << inputCfg.size << \"-\" << videoFormat\n> >> << \", got \" << format << \")\";\n> >> @@ -78,13 +83,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> >> \n> >> ret = m2m_->capture()->setFormat(&format);\n> >> if (ret < 0) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Failed to set output format: \" << strerror(-ret);\n> >> return ret;\n> >> }\n> >> \n> >> if (format.fourcc != videoFormat || format.size != outputCfg.size) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Output format not supported\";\n> >> return -EINVAL;\n> >> }\n> >> @@ -95,13 +100,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> >> return 0;\n> >> }\n> >> \n> >> -int SimpleConverter::Stream::exportBuffers(unsigned int count,\n> >> - std::vector> *buffers)\n> >> +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,\n> >> + std::vector> *buffers)\n> >> {\n> >> return m2m_->capture()->exportBuffers(count, buffers);\n> >> }\n> >> \n> >> -int SimpleConverter::Stream::start()\n> >> +int V4L2M2MConverter::Stream::start()\n> >> {\n> >> int ret = m2m_->output()->importBuffers(inputBufferCount_);\n> >> if (ret < 0)\n> >> @@ -128,7 +133,7 @@ int SimpleConverter::Stream::start()\n> >> return 0;\n> >> }\n> >> \n> >> -void SimpleConverter::Stream::stop()\n> >> +void V4L2M2MConverter::Stream::stop()\n> >> {\n> >> m2m_->capture()->streamOff();\n> >> m2m_->output()->streamOff();\n> >> @@ -136,8 +141,7 @@ void SimpleConverter::Stream::stop()\n> >> m2m_->output()->releaseBuffers();\n> >> }\n> >> \n> >> -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,\n> >> - FrameBuffer *output)\n> >> +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output)\n> >> {\n> >> int ret = m2m_->output()->queueBuffer(input);\n> >> if (ret < 0)\n> >> @@ -150,12 +154,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,\n> >> return 0;\n> >> }\n> >> \n> >> -std::string SimpleConverter::Stream::logPrefix() const\n> >> +std::string V4L2M2MConverter::Stream::logPrefix() const\n> >> {\n> >> return \"stream\" + std::to_string(index_);\n> > \n> > I'm not yet sure if this will be unique enough if we have multiple\n> > convertors. However given the logging infrastructure will print a file\n> > and line number, I think it will be clear.\n> > \n> > So I think this is ok.\n> > \n> > \n> >> }\n> >> \n> >> -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n> >> +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n> >> {\n> >> auto it = converter_->queue_.find(buffer);\n> >> if (it == converter_->queue_.end())\n> >> @@ -167,32 +171,34 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n> >> }\n> >> }\n> >> \n> >> -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n> >> +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n> >> {\n> >> converter_->outputBufferReady.emit(buffer);\n> >> }\n> >> \n> >> /* -----------------------------------------------------------------------------\n> >> - * SimpleConverter\n> >> + * V4L2M2MConverter\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\class libcamera::V4L2M2MConverter\n> >> + * \\brief The V4L2 M2M converter implements the converter interface based on\n> >> + * V4L2 M2M device.\n> >> +*/\n> >> +\n> >> +/**\n> >> + * \\fn V4L2M2MConverter::V4L2M2MConverter\n> >> + * \\brief Construct a V4L2M2MConverter instance\n> >> + * \\param[in] media The media device implementing the converter\n> >> */\n> >> \n> >> -SimpleConverter::SimpleConverter(MediaDevice *media)\n> >> +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\n> >> + : Converter(media)\n> >> {\n> >> - /*\n> >> - * Locate the video node. There's no need to validate the pipeline\n> >> - * further, the caller guarantees that this is a V4L2 mem2mem device.\n> >> - */\n> >> - const std::vector &entities = media->entities();\n> >> - auto it = std::find_if(entities.begin(), entities.end(),\n> >> - [](MediaEntity *entity) {\n> >> - return entity->function() == MEDIA_ENT_F_IO_V4L;\n> >> - });\n> >> - if (it == entities.end())\n> >> + if (deviceNode().empty())\n> > \n> > Do we need any kind of warning here? I presume this means we were unable\n> > to construct a suitable convertor... Or will the Converter(media)\n> > initialiser have already printed an error in that instance?\n> > \n> >> return;\n> >> \n> >> - deviceNode_ = (*it)->deviceNode();\n> >> -\n> >> - m2m_ = std::make_unique(deviceNode_);\n> >> + m2m_ = std::make_unique(deviceNode());\n> >> int ret = m2m_->open();\n> >> if (ret < 0) {\n> >> m2m_.reset();\n> >> @@ -200,7 +206,21 @@ SimpleConverter::SimpleConverter(MediaDevice *media)\n> >> }\n> >> }\n> >> \n> >> -std::vector SimpleConverter::formats(PixelFormat input)\n> >> +/**\n> >> + * \\fn libcamera::V4L2M2MConverter::loadConfiguration\n> >> + * \\details \\copydetails libcamera::Converter::loadConfiguration\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn libcamera::V4L2M2MConverter::isValid\n> >> + * \\details \\copydetails libcamera::Converter::isValid\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn libcamera::V4L2M2MConverter::formats\n> >> + * \\details \\copydetails libcamera::Converter::formats\n> >> + */\n> >> +std::vector V4L2M2MConverter::formats(PixelFormat input)\n> >> {\n> >> if (!m2m_)\n> >> return {};\n> >> @@ -215,13 +235,13 @@ std::vector SimpleConverter::formats(PixelFormat input)\n> >> \n> >> int ret = m2m_->output()->setFormat(&v4l2Format);\n> >> if (ret < 0) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Failed to set format: \" << strerror(-ret);\n> >> return {};\n> >> }\n> >> \n> >> if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) {\n> >> - LOG(SimplePipeline, Debug)\n> >> + LOG(Converter, Debug)\n> >> << \"Input format \" << input << \" not supported.\";\n> >> return {};\n> >> }\n> >> @@ -237,7 +257,10 @@ std::vector SimpleConverter::formats(PixelFormat input)\n> >> return pixelFormats;\n> >> }\n> >> \n> >> -SizeRange SimpleConverter::sizes(const Size &input)\n> >> +/**\n> >> + * \\copydoc libcamera::Converter::sizes\n> >> + */\n> >> +SizeRange V4L2M2MConverter::sizes(const Size &input)\n> >> {\n> >> if (!m2m_)\n> >> return {};\n> >> @@ -252,7 +275,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> >> \n> >> int ret = m2m_->output()->setFormat(&format);\n> >> if (ret < 0) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Failed to set format: \" << strerror(-ret);\n> >> return {};\n> >> }\n> >> @@ -262,7 +285,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> >> format.size = { 1, 1 };\n> >> ret = m2m_->capture()->setFormat(&format);\n> >> if (ret < 0) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Failed to set format: \" << strerror(-ret);\n> >> return {};\n> >> }\n> >> @@ -272,7 +295,7 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> >> format.size = { UINT_MAX, UINT_MAX };\n> >> ret = m2m_->capture()->setFormat(&format);\n> >> if (ret < 0) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Failed to set format: \" << strerror(-ret);\n> >> return {};\n> >> }\n> >> @@ -282,9 +305,12 @@ SizeRange SimpleConverter::sizes(const Size &input)\n> >> return sizes;\n> >> }\n> >> \n> >> +/**\n> >> + * \\copydoc libcamera::Converter::strideAndFrameSize\n> >> + */\n> >> std::tuple\n> >> -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n> >> - const Size &size)\n> >> +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n> >> + const Size &size)\n> >> {\n> >> V4L2DeviceFormat format;\n> >> format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);\n> >> @@ -297,8 +323,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,\n> >> return std::make_tuple(format.planes[0].bpl, format.planes[0].size);\n> >> }\n> >> \n> >> -int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n> >> - const std::vector> &outputCfgs)\n> >> +/**\n> >> + * \\copydoc libcamera::Converter::configure\n> >> + */\n> >> +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> >> + const std::vector> &outputCfgs)\n> >> {\n> >> int ret = 0;\n> >> \n> >> @@ -309,7 +338,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n> >> Stream &stream = streams_.emplace_back(this, i);\n> >> \n> >> if (!stream.isValid()) {\n> >> - LOG(SimplePipeline, Error)\n> >> + LOG(Converter, Error)\n> >> << \"Failed to create stream \" << i;\n> >> ret = -EINVAL;\n> >> break;\n> >> @@ -328,8 +357,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,\n> >> return 0;\n> >> }\n> >> \n> >> -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n> >> - std::vector> *buffers)\n> >> +/**\n> >> + * \\copydoc libcamera::Converter::exportBuffers\n> >> + */\n> >> +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,\n> >> + std::vector> *buffers)\n> >> {\n> >> if (output >= streams_.size())\n> >> return -EINVAL;\n> >> @@ -337,7 +369,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n> >> return streams_[output].exportBuffers(count, buffers);\n> >> }\n> >> \n> >> -int SimpleConverter::start()\n> >> +/**\n> >> + * \\copydoc libcamera::Converter::start\n> >> + */\n> >> +int V4L2M2MConverter::start()\n> >> {\n> >> int ret;\n> >> \n> >> @@ -352,14 +387,20 @@ int SimpleConverter::start()\n> >> return 0;\n> >> }\n> >> \n> >> -void SimpleConverter::stop()\n> >> +/**\n> >> + * \\copydoc libcamera::Converter::stop\n> >> + */\n> >> +void V4L2M2MConverter::stop()\n> >> {\n> >> for (Stream &stream : utils::reverse(streams_))\n> >> stream.stop();\n> >> }\n> >> \n> >> -int SimpleConverter::queueBuffers(FrameBuffer *input,\n> >> - const std::map &outputs)\n> >> +/**\n> >> + * \\copydoc libcamera::Converter::queueBuffers\n> >> + */\n> >> +int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> >> + const std::map &outputs)\n> >> {\n> >> unsigned int mask = 0;\n> >> int ret;\n> >> @@ -402,4 +443,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input,\n> >> return 0;\n> >> }\n> >> \n> >> +REGISTER_CONVERTER(\"v4l2_m2m\", V4L2M2MConverter, \"pxp\")\n> > \n> > I think the only part I'd change here is that the \"pxp\" might be a\n> > statically created array or vector of 'compatible' strings, a bit like\n> > the kernel would define.\n> > \n> > Then adding support for a new compatible would just be a new line in a\n> > table, rather than modifying this registration. It's not much, but it\n> > would feel 'neater' to just have a single line patch to add an entry.\n> > \n> static std::initializer_list aliases = {\n> \"pxp\"\n\nCan that have a comma at the end? (Or does the compiler complain?)\n\n \"pxp\",\n\nIt's subtle, but that lets me add the next compatible in a single line.\n\n+\t\"other-compatible-m2m-device\",\n\n> };\n> \n> REGISTER_CONVERTER(\"v4l2_m2m\", V4L2M2MConverter, aliases)\n> \n> Would it be better ?\n\nYes, that's what I was thinking of. Although I'd call 'aliases'\n'compatibles' ? or 'compatible' ?\n\n> > \n> > And once again, I don't think there are any blockers there. Some things\n> > to consider and tidy up for a v4, but I'd be happy to merge at that.\n> > \n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > \n> >> +\n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build\n> >> new file mode 100644\n> >> index 00000000..2aa72fe4\n> >> --- /dev/null\n> >> +++ b/src/libcamera/converter/meson.build\n> >> @@ -0,0 +1,5 @@\n> >> +# SPDX-License-Identifier: CC0-1.0\n> >> +\n> >> +libcamera_sources += files([\n> >> + 'converter_v4l2_m2m.cpp'\n> >> +])\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index e9d0324e..ffc294f3 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false)\n> >> libthreads = dependency('threads')\n> >> \n> >> subdir('base')\n> >> +subdir('converter')\n> >> subdir('ipa')\n> >> subdir('pipeline')\n> >> subdir('proxy')\n> >> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build\n> >> index 9c99b32f..42b0896d 100644\n> >> --- a/src/libcamera/pipeline/simple/meson.build\n> >> +++ b/src/libcamera/pipeline/simple/meson.build\n> >> @@ -1,6 +1,5 @@\n> >> # SPDX-License-Identifier: CC0-1.0\n> >> \n> >> libcamera_sources += files([\n> >> - 'converter.cpp',\n> >> 'simple.cpp',\n> >> ])\n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index a32de7f3..24ded4db 100644\n> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >> @@ -30,13 +30,13 @@\n> >> \n> >> #include \"libcamera/internal/camera.h\"\n> >> #include \"libcamera/internal/camera_sensor.h\"\n> >> +#include \"libcamera/internal/converter.h\"\n> >> #include \"libcamera/internal/device_enumerator.h\"\n> >> #include \"libcamera/internal/media_device.h\"\n> >> #include \"libcamera/internal/pipeline_handler.h\"\n> >> #include \"libcamera/internal/v4l2_subdevice.h\"\n> >> #include \"libcamera/internal/v4l2_videodevice.h\"\n> >> \n> >> -#include \"converter.h\"\n> >> \n> >> namespace libcamera {\n> >> \n> >> @@ -266,7 +266,7 @@ public:\n> >> std::vector configs_;\n> >> std::map> formats_;\n> >> \n> >> - std::unique_ptr converter_;\n> >> + std::unique_ptr converter_;\n> >> std::vector> converterBuffers_;\n> >> bool useConverter_;\n> >> std::queue> converterQueue_;\n> >> @@ -492,7 +492,7 @@ int SimpleCameraData::init()\n> >> /* Open the converter, if any. */\n> >> MediaDevice *converter = pipe->converter();\n> >> if (converter) {\n> >> - converter_ = std::make_unique(converter);\n> >> + converter_ = ConverterFactoryBase::create(converter);\n> >> if (!converter_->isValid()) {\n> >> LOG(SimplePipeline, Warning)\n> >> << \"Failed to create converter, disabling format conversion\";\n> >> -- \n> >> 2.38.1\n> >>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F34B7BDE6B\n\tfor ;\n\tMon, 12 Dec 2022 17:25:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CF9363362;\n\tMon, 12 Dec 2022 18:25:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CF1FC6333F\n\tfor ;\n\tMon, 12 Dec 2022 18:25:40 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2FB7A6CF;\n\tMon, 12 Dec 2022 18:25:40 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670865942;\n\tbh=nXhJHEiXSufjiT86y2a/G3tDF+ND4+Jh0AYaN2puEGk=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=0sbCQu9IMh5F64vBDN31HHaNnfOEpuPg2+SAQsGpN8xcPBl0FqFDnwrCgTyQhE7Bg\n\tU1TqI5erWZNCJyXT3jW9XWkZI5pn/rWIkPemEt4v8mMaKvNreB9MNIx5P1t1mrNCJh\n\t12+jDpGvKnF4vPgutifK0JFF0/rTkPYM4jL6HQLmPgZnwhpj9OKwe2BoxHY2aT5dZ2\n\t4Ob/JVWi2GiCRonbeM3G2piKQ2sD9M3lDXhBFws4Edh8e6kZPZ4SnCal0GHg6shsxq\n\tBm19E9F7SlzFJSTZJOtXBJjSnRDwaFtQaMh/LDV38fbQw9FN1EqjsySXZPuvJfUlRC\n\ttLMr4KDlKVaHw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670865940;\n\tbh=nXhJHEiXSufjiT86y2a/G3tDF+ND4+Jh0AYaN2puEGk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=uqlnZGgaCN4F9dPynTiU9WNf8VI72u9r9Qq6jPsIF6VzKDrccur3+kQ6zud6iDdIh\n\tW6w8eIbFrlKaWpZOsZYyzzfx9zB39oA5nYB3LiNUiiu44di1ar68U3Y6HWBVmM0XkM\n\txxAflgXRjOgEwl+2rXHb1xViVYPn3UAOkuRjslYM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uqlnZGga\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<28798aa5-26f6-6d13-93c7-ed33cd4a97a2@oss.nxp.com>","References":"<20221117185204.11625-1-xavier.roumegue@oss.nxp.com>\n\t<20221117185204.11625-2-xavier.roumegue@oss.nxp.com>\n\t<167041771955.9133.3362055462627412916@Monstersaurus>\n\t<28798aa5-26f6-6d13-93c7-ed33cd4a97a2@oss.nxp.com>","To":"Xavier Roumegue (OSS) ,\n\tXavier Roumegue via libcamera-devel\n\t, \n\tjacopo@jmondi.org, laurent.pinchart@ideasonboard.com","Date":"Mon, 12 Dec 2022 17:25:37 +0000","Message-ID":"<167086593709.9133.14674853388530085140@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] libcamera: pipeline: simple:\n\tconverter: Use generic converter interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]