[{"id":27755,"web_url":"https://patchwork.libcamera.org/comment/27755/","msgid":"","date":"2023-09-11T15:55:47","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: converter: split\n\tConverterMD (media device) out of Converter","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Andrey\n\nOn Sun, Sep 10, 2023 at 08:50:25PM +0300, Andrey Konovalov via libcamera-devel wrote:\n> Make Converter a bit more generic by not requiring it to use a media device.\n> For this split out ConverterMD from Converter class, and rename\n> ConverterFactoryBase / ConverterFactory to ConverterMDFactoryBase /\n> ConverterMDFactory for consistency.\n>\n> Signed-off-by: Andrey Konovalov \n\nI understand where this is going, but before going into a full review\nI would like to explore the possibility of generalizing the existing\nhierarchy before splitting it in media and non-media versions, which\nwould require re-defining another FactoryBase because as you said, I\ndo expect as well software converter to need a factory too..\n\nAs far as I can see the MediaDevice is mostly used to get the\n->driver() name out and match it against a list of compatibles the\nConverter-derived class has been instantiated with.\n\nIt seems to possible to make the '*media' parameter optional to most\nfunctions that accept it, or even overload the\nConverterFactoryBase::create() and createInstance() functions to\naccept instead a string that identifies the software converter version\n(ie \"linaro-sw-converter\" in your case?).\n\nI would go as far as wondering if insted we shouldn't make the whole\nhierarchy string-based and MediaDevice completely.\n\nThe only thing we would lose for real would be\n\n\tconst std::vector &entities = media->entities();\n\tauto it = std::find_if(entities.begin(), entities.end(),\n\t\t\t [](MediaEntity *entity) {\n\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n\t\t\t });\n\tif (it == entities.end()) {\n\t\tLOG(Converter, Error)\n\t\t\t<< \"No entity suitable for implementing a converter in \"\n\t\t\t<< media->driver() << \" entities list.\";\n\t\treturn;\n\t}\n\nat construction-time. Would this be that bad ? (if we want to keep it\nwe can have 2 overloaded constructors, in example).\n\nHave this options been discussed in the previous version or have you\nconsidered them but then discarded by any chance ?\n\nBelow some drive-by comments\n\n\n> ---\n> include/libcamera/internal/converter.h | 49 +---\n> .../internal/converter/converter_v4l2_m2m.h | 4 +-\n> include/libcamera/internal/converter_media.h | 86 +++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/converter.cpp | 191 +-------------\n> .../converter/converter_v4l2_m2m.cpp | 4 +-\n> src/libcamera/converter_media.cpp | 241 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/pipeline/simple/simple.cpp | 4 +-\n> 9 files changed, 337 insertions(+), 244 deletions(-)\n> create mode 100644 include/libcamera/internal/converter_media.h\n> create mode 100644 src/libcamera/converter_media.cpp\n>\n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 834ec5ab..da1f5d73 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -24,14 +24,13 @@\n> namespace libcamera {\n>\n> class FrameBuffer;\n> -class MediaDevice;\n> class PixelFormat;\n> struct StreamConfiguration;\n>\n> class Converter\n> {\n> public:\n> -\tConverter(MediaDevice *media);\n> +\tConverter();\n> \tvirtual ~Converter();\n>\n> \tvirtual int loadConfiguration(const std::string &filename) = 0;\n> @@ -57,52 +56,6 @@ public:\n>\n> \tSignal inputBufferReady;\n> \tSignal outputBufferReady;\n> -\n> -\tconst std::string &deviceNode() const { return deviceNode_; }\n> -\n> -private:\n> -\tstd::string deviceNode_;\n> };\n>\n> -class ConverterFactoryBase\n> -{\n> -public:\n> -\tConverterFactoryBase(const std::string name, std::initializer_list compatibles);\n> -\tvirtual ~ConverterFactoryBase() = default;\n> -\n> -\tconst std::vector &compatibles() const { return compatibles_; }\n> -\n> -\tstatic std::unique_ptr create(MediaDevice *media);\n> -\tstatic std::vector &factories();\n> -\tstatic std::vector names();\n> -\n> -private:\n> -\tLIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactoryBase)\n> -\n> -\tstatic void registerType(ConverterFactoryBase *factory);\n> -\n> -\tvirtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n> -\n> -\tstd::string name_;\n> -\tstd::vector compatibles_;\n> -};\n> -\n> -template\n> -class ConverterFactory : public ConverterFactoryBase\n> -{\n> -public:\n> -\tConverterFactory(const char *name, std::initializer_list compatibles)\n> -\t\t: ConverterFactoryBase(name, compatibles)\n> -\t{\n> -\t}\n> -\n> -\tstd::unique_ptr createInstance(MediaDevice *media) const override\n> -\t{\n> -\t\treturn std::make_unique<_Converter>(media);\n> -\t}\n> -};\n> -\n> -#define REGISTER_CONVERTER(name, converter, compatibles) \\\n> -\tstatic ConverterFactory global_##converter##Factory(name, compatibles);\n> -\n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 815916d0..aeb8c0e9 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -20,7 +20,7 @@\n>\n> #include \n>\n> -#include \"libcamera/internal/converter.h\"\n> +#include \"libcamera/internal/converter_media.h\"\n>\n> namespace libcamera {\n>\n> @@ -31,7 +31,7 @@ class SizeRange;\n> struct StreamConfiguration;\n> class V4L2M2MDevice;\n>\n> -class V4L2M2MConverter : public Converter\n> +class V4L2M2MConverter : public ConverterMD\n> {\n> public:\n> \tV4L2M2MConverter(MediaDevice *media);\n> diff --git a/include/libcamera/internal/converter_media.h b/include/libcamera/internal/converter_media.h\n> new file mode 100644\n> index 00000000..2b56ebdb\n> --- /dev/null\n> +++ b/include/libcamera/internal/converter_media.h\n> @@ -0,0 +1,86 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Laurent Pinchart\n> + * Copyright 2022 NXP\n> + *\n> + * converter_media.h - Generic media device based format converter interface\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/converter.h\"\n\nA lot of stuff (inclusions an forward declaration) are duplicated in\nthis and in the libcamera/internal/converter.h header. You can include\nconverter.h first and add only what is missing.\n\n> +\n> +namespace libcamera {\n> +\n> +class FrameBuffer;\n> +class MediaDevice;\n> +class PixelFormat;\n> +struct StreamConfiguration;\n> +\n> +class ConverterMD : public Converter\n> +{\n> +public:\n> +\tConverterMD(MediaDevice *media);\n> +\t~ConverterMD();\n> +\n> +\tconst std::string &deviceNode() const { return deviceNode_; }\n> +\n> +private:\n> +\tstd::string deviceNode_;\n> +};\n> +\n> +class ConverterMDFactoryBase\n> +{\n> +public:\n> +\tConverterMDFactoryBase(const std::string name, std::initializer_list compatibles);\n> +\tvirtual ~ConverterMDFactoryBase() = default;\n> +\n> +\tconst std::vector &compatibles() const { return compatibles_; }\n> +\n> +\tstatic std::unique_ptr create(MediaDevice *media);\n> +\tstatic std::vector &factories();\n> +\tstatic std::vector names();\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterMDFactoryBase)\n> +\n> +\tstatic void registerType(ConverterMDFactoryBase *factory);\n> +\n> +\tvirtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n> +\n> +\tstd::string name_;\n> +\tstd::vector compatibles_;\n> +};\n> +\n> +template\n> +class ConverterMDFactory : public ConverterMDFactoryBase\n> +{\n> +public:\n> +\tConverterMDFactory(const char *name, std::initializer_list compatibles)\n> +\t\t: ConverterMDFactoryBase(name, compatibles)\n> +\t{\n> +\t}\n> +\n> +\tstd::unique_ptr createInstance(MediaDevice *media) const override\n> +\t{\n> +\t\treturn std::make_unique<_ConverterMD>(media);\n> +\t}\n> +};\n> +\n> +#define REGISTER_CONVERTER_MD(name, converter, compatibles) \\\n> +\tstatic ConverterMDFactory global_##converter##MDFactory(name, compatibles);\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 7f1f3440..e9c41346 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([\n> 'control_serializer.h',\n> 'control_validator.h',\n> 'converter.h',\n> + 'converter_media.h',\n> 'delayed_controls.h',\n> 'device_enumerator.h',\n> 'device_enumerator_sysfs.h',\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index fa0f1ec8..92dcdc03 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -38,26 +38,9 @@ LOG_DEFINE_CATEGORY(Converter)\n>\n> /**\n> * \\brief Construct a Converter instance\n> - * \\param[in] media The media device implementing the converter\n> - *\n> - * This searches for the entity implementing the data streaming function in the\n> - * media graph entities and use its device node as the converter device node.\n> */\n> -Converter::Converter(MediaDevice *media)\n> +Converter::Converter()\n> {\n> -\tconst std::vector &entities = media->entities();\n> -\tauto it = std::find_if(entities.begin(), entities.end(),\n> -\t\t\t [](MediaEntity *entity) {\n> -\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n> -\t\t\t });\n> -\tif (it == entities.end()) {\n> -\t\tLOG(Converter, Error)\n> -\t\t\t<< \"No entity suitable for implementing a converter in \"\n> -\t\t\t<< media->driver() << \" entities list.\";\n> -\t\treturn;\n> -\t}\n> -\n> -\tdeviceNode_ = (*it)->deviceNode();\n> }\n>\n> Converter::~Converter()\n> @@ -159,176 +142,4 @@ Converter::~Converter()\n> * \\brief A signal emitted on each frame buffer completion of the output queue\n> */\n>\n> -/**\n> - * \\fn Converter::deviceNode()\n> - * \\brief The converter device node attribute accessor\n> - * \\return The converter device node string\n> - */\n> -\n> -/**\n> - * \\class ConverterFactoryBase\n> - * \\brief Base class for converter factories\n> - *\n> - * The ConverterFactoryBase class is the base of all specializations of the\n> - * ConverterFactory class template. It implements the factory registration,\n> - * maintains a registry of factories, and provides access to the registered\n> - * factories.\n> - */\n> -\n> -/**\n> - * \\brief Construct a converter factory base\n> - * \\param[in] name Name of the converter class\n> - * \\param[in] compatibles Name aliases of the converter class\n> - *\n> - * Creating an instance of the factory base registers it with the global list of\n> - * factories, accessible through the factories() function.\n> - *\n> - * The factory \\a name is used as unique identifier. If the converter\n> - * implementation fully relies on a generic framework, the name should be the\n> - * same as the framework. Otherwise, if the implementation is specialized, the\n> - * factory name should match the driver name implementing the function.\n> - *\n> - * The factory \\a compatibles holds a list of driver names implementing a generic\n> - * subsystem without any personalizations.\n> - */\n> -ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list compatibles)\n> -\t: name_(name), compatibles_(compatibles)\n> -{\n> -\tregisterType(this);\n> -}\n> -\n> -/**\n> - * \\fn ConverterFactoryBase::compatibles()\n> - * \\return The names compatibles\n> - */\n> -\n> -/**\n> - * \\brief Create an instance of the converter corresponding to a named factory\n> - * \\param[in] media Name of the factory\n\nThis clearly was wrong already, and if you shuffle code around might\nbe a good occasion to change it.\n\n> - *\n> - * \\return A unique pointer to a new instance of the converter subclass\n> - * corresponding to the named factory or one of its alias. Otherwise a null\n> - * pointer if no such factory exists\n> - */\n> -std::unique_ptr ConverterFactoryBase::create(MediaDevice *media)\n> -{\n> -\tconst std::vector &factories =\n> -\t\tConverterFactoryBase::factories();\n> -\n> -\tfor (const ConverterFactoryBase *factory : factories) {\n> -\t\tconst std::vector &compatibles = factory->compatibles();\n> -\t\tauto it = std::find(compatibles.begin(), compatibles.end(), media->driver());\n> -\n> -\t\tif (it == compatibles.end() && media->driver() != factory->name_)\n> -\t\t\tcontinue;\n> -\n> -\t\tLOG(Converter, Debug)\n> -\t\t\t<< \"Creating converter from \"\n> -\t\t\t<< factory->name_ << \" factory with \"\n> -\t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n> -\n> -\t\tstd::unique_ptr converter = factory->createInstance(media);\n> -\t\tif (converter->isValid())\n> -\t\t\treturn converter;\n> -\t}\n> -\n> -\treturn nullptr;\n> -}\n> -\n> -/**\n> - * \\brief Add a converter class to the registry\n> - * \\param[in] factory Factory to use to construct the converter class\n> - *\n> - * The caller is responsible to guarantee the uniqueness of the converter name.\n> - */\n> -void ConverterFactoryBase::registerType(ConverterFactoryBase *factory)\n> -{\n> -\tstd::vector &factories =\n> -\t\tConverterFactoryBase::factories();\n> -\n> -\tfactories.push_back(factory);\n> -}\n> -\n> -/**\n> - * \\brief Retrieve the list of all converter factory names\n> - * \\return The list of all converter factory names\n> - */\n> -std::vector ConverterFactoryBase::names()\n> -{\n> -\tstd::vector list;\n> -\n> -\tstd::vector &factories =\n> -\t\tConverterFactoryBase::factories();\n> -\n> -\tfor (ConverterFactoryBase *factory : factories) {\n> -\t\tlist.push_back(factory->name_);\n> -\t\tfor (auto alias : factory->compatibles())\n> -\t\t\tlist.push_back(alias);\n> -\t}\n> -\n> -\treturn list;\n> -}\n> -\n> -/**\n> - * \\brief Retrieve the list of all converter factories\n> - * \\return The list of converter factories\n> - */\n> -std::vector &ConverterFactoryBase::factories()\n> -{\n> -\t/*\n> -\t * The static factories map is defined inside the function to ensure\n> -\t * it gets initialized on first use, without any dependency on link\n> -\t * order.\n> -\t */\n> -\tstatic std::vector factories;\n> -\treturn factories;\n> -}\n> -\n> -/**\n> - * \\var ConverterFactoryBase::name_\n> - * \\brief The name of the factory\n> - */\n> -\n> -/**\n> - * \\var ConverterFactoryBase::compatibles_\n> - * \\brief The list holding the factory compatibles\n> - */\n> -\n> -/**\n> - * \\class ConverterFactory\n> - * \\brief Registration of ConverterFactory classes and creation of instances\n> - * \\param _Converter The converter class type for this factory\n> - *\n> - * To facilitate discovery and instantiation of Converter classes, the\n> - * ConverterFactory class implements auto-registration of converter helpers.\n> - * Each Converter subclass shall register itself using the REGISTER_CONVERTER()\n> - * macro, which will create a corresponding instance of a ConverterFactory\n> - * subclass and register it with the static list of factories.\n> - */\n> -\n> -/**\n> - * \\fn ConverterFactory::ConverterFactory(const char *name, std::initializer_list compatibles)\n> - * \\brief Construct a converter factory\n> - * \\details \\copydetails ConverterFactoryBase::ConverterFactoryBase\n> - */\n> -\n> -/**\n> - * \\fn ConverterFactory::createInstance() const\n> - * \\brief Create an instance of the Converter corresponding to the factory\n> - * \\param[in] media Media device pointer\n> - * \\return A unique pointer to a newly constructed instance of the Converter\n> - * subclass corresponding to the factory\n> - */\n> -\n> -/**\n> - * \\def REGISTER_CONVERTER\n> - * \\brief Register a converter with the Converter factory\n> - * \\param[in] name Converter name used to register the class\n> - * \\param[in] converter Class name of Converter derived class to register\n> - * \\param[in] compatibles List of compatible names\n> - *\n> - * Register a Converter subclass with the factory and make it available to try\n> - * and match converters.\n> - */\n> -\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index 2a4d1d99..d0a5e3bf 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -194,7 +194,7 @@ void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n> */\n>\n> V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\n> -\t: Converter(media)\n> +\t: ConverterMD(media)\n> {\n> \tif (deviceNode().empty())\n> \t\treturn;\n> @@ -448,6 +448,6 @@ static std::initializer_list compatibles = {\n> \t\"pxp\",\n> };\n>\n> -REGISTER_CONVERTER(\"v4l2_m2m\", V4L2M2MConverter, compatibles)\n> +REGISTER_CONVERTER_MD(\"v4l2_m2m\", V4L2M2MConverter, compatibles)\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/converter_media.cpp b/src/libcamera/converter_media.cpp\n> new file mode 100644\n> index 00000000..f5024764\n> --- /dev/null\n> +++ b/src/libcamera/converter_media.cpp\n> @@ -0,0 +1,241 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright 2022 NXP\n> + *\n\nYou copyright here and in the header file if you like to\n\n> + * converter.cpp - Generic format converter interface\n> + */\n> +\n> +#include \"libcamera/internal/converter_media.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/media_device.h\"\n> +\n> +#include \"linux/media.h\"\n\nI know it was already like this, but I would have expected to see <>\n\nIt shouldn't make a difference though, even if this is a system-wide\nheader we ship our own copy and adjust the inclusion prefix precendece\nto use our own version (I presume) so both \"\" and <> will do the same\nthing ?\n\nAnd, btw, this is included by media_device.h (again, not your fault)\n\n> +\n> +/**\n> + * \\file internal/converter_media.h\n> + * \\brief Abstract media device based converter\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Converter)\n\nMaybe ConverterMD ?\n\n> +\n> +/**\n> + * \\class ConverterMD\n> + * \\brief Abstract Base Class for media device based converter\n> + *\n> + * The ConverterMD class is an Abstract Base Class defining the interfaces of\n> + * media device based converter implementations.\n> + *\n> + * Converters offer scaling and pixel format conversion services on an input\n> + * stream. The converter can output multiple streams with individual conversion\n> + * parameters from the same input stream.\n> + */\n> +\n> +/**\n> + * \\brief Construct a ConverterMD instance\n> + * \\param[in] media The media device implementing the converter\n> + *\n> + * This searches for the entity implementing the data streaming function in the\n> + * media graph entities and use its device node as the converter device node.\n> + */\n> +ConverterMD::ConverterMD(MediaDevice *media)\n> +{\n> +\tconst std::vector &entities = media->entities();\n> +\tauto it = std::find_if(entities.begin(), entities.end(),\n> +\t\t\t [](MediaEntity *entity) {\n> +\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n> +\t\t\t });\n> +\tif (it == entities.end()) {\n> +\t\tLOG(Converter, Error)\n> +\t\t\t<< \"No entity suitable for implementing a converter in \"\n> +\t\t\t<< media->driver() << \" entities list.\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tdeviceNode_ = (*it)->deviceNode();\n> +}\n> +\n> +ConverterMD::~ConverterMD()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn ConverterMD::deviceNode()\n> + * \\brief The converter device node attribute accessor\n> + * \\return The converter device node string\n> + */\n> +\n> +/**\n> + * \\class ConverterMDFactoryBase\n> + * \\brief Base class for media device based converter factories\n> + *\n> + * The ConverterMDFactoryBase class is the base of all specializations of the\n> + * ConverterMDFactory class template. It implements the factory registration,\n> + * maintains a registry of factories, and provides access to the registered\n> + * factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a media device based converter factory base\n> + * \\param[in] name Name of the converter class\n> + * \\param[in] compatibles Name aliases of the converter class\n> + *\n> + * Creating an instance of the factory base registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used as unique identifier. If the converter\n> + * implementation fully relies on a generic framework, the name should be the\n> + * same as the framework. Otherwise, if the implementation is specialized, the\n> + * factory name should match the driver name implementing the function.\n> + *\n> + * The factory \\a compatibles holds a list of driver names implementing a generic\n> + * subsystem without any personalizations.\n> + */\n> +ConverterMDFactoryBase::ConverterMDFactoryBase(const std::string name, std::initializer_list compatibles)\n> +\t: name_(name), compatibles_(compatibles)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\fn ConverterMDFactoryBase::compatibles()\n> + * \\return The names compatibles\n> + */\n> +\n> +/**\n> + * \\brief Create an instance of the converter corresponding to a named factory\n> + * \\param[in] media Name of the factory\n> + *\n> + * \\return A unique pointer to a new instance of the media device based\n> + * converter subclass corresponding to the named factory or one of its alias.\n> + * Otherwise a null pointer if no such factory exists.\n> + */\n> +std::unique_ptr ConverterMDFactoryBase::create(MediaDevice *media)\n> +{\n> +\tconst std::vector &factories =\n> +\t\tConverterMDFactoryBase::factories();\n> +\n> +\tfor (const ConverterMDFactoryBase *factory : factories) {\n> +\t\tconst std::vector &compatibles = factory->compatibles();\n> +\t\tauto it = std::find(compatibles.begin(), compatibles.end(), media->driver());\n> +\n> +\t\tif (it == compatibles.end() && media->driver() != factory->name_)\n> +\t\t\tcontinue;\n> +\n> +\t\tLOG(Converter, Debug)\n> +\t\t\t<< \"Creating converter from \"\n> +\t\t\t<< factory->name_ << \" factory with \"\n> +\t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n> +\n> +\t\tstd::unique_ptr converter = factory->createInstance(media);\n> +\t\tif (converter->isValid())\n> +\t\t\treturn converter;\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Add a media device based converter class to the registry\n> + * \\param[in] factory Factory to use to construct the media device based\n> + * converter class\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the converter name.\n> + */\n> +void ConverterMDFactoryBase::registerType(ConverterMDFactoryBase *factory)\n> +{\n> +\tstd::vector &factories =\n> +\t\tConverterMDFactoryBase::factories();\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all media device based converter factory names\n> + * \\return The list of all media device based converter factory names\n> + */\n> +std::vector ConverterMDFactoryBase::names()\n> +{\n> +\tstd::vector list;\n> +\n> +\tstd::vector &factories =\n> +\t\tConverterMDFactoryBase::factories();\n> +\n> +\tfor (ConverterMDFactoryBase *factory : factories) {\n> +\t\tlist.push_back(factory->name_);\n> +\t\tfor (auto alias : factory->compatibles())\n> +\t\t\tlist.push_back(alias);\n> +\t}\n> +\n> +\treturn list;\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all media device based converter factories\n> + * \\return The list of media device based converter factories\n> + */\n> +std::vector &ConverterMDFactoryBase::factories()\n> +{\n> +\t/*\n> +\t * The static factories map is defined inside the function to ensure\n> +\t * it gets initialized on first use, without any dependency on link\n> +\t * order.\n> +\t */\n> +\tstatic std::vector factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * \\var ConverterMDFactoryBase::name_\n> + * \\brief The name of the factory\n> + */\n> +\n> +/**\n> + * \\var ConverterMDFactoryBase::compatibles_\n> + * \\brief The list holding the factory compatibles\n> + */\n> +\n> +/**\n> + * \\class ConverterMDFactory\n> + * \\brief Registration of ConverterMDFactory classes and creation of instances\n> + * \\param _Converter The converter class type for this factory\n> + *\n> + * To facilitate discovery and instantiation of ConverterMD classes, the\n> + * ConverterMDFactory class implements auto-registration of converter helpers.\n> + * Each ConverterMD subclass shall register itself using the\n> + * REGISTER_CONVERTER() macro, which will create a corresponding instance of a\n> + * ConverterMDFactory subclass and register it with the static list of\n> + * factories.\n> + */\n> +\n> +/**\n> + * \\fn ConverterMDFactory::ConverterMDFactory(const char *name, std::initializer_list compatibles)\n> + * \\brief Construct a media device converter factory\n> + * \\details \\copydetails ConverterMDFactoryBase::ConverterMDFactoryBase\n> + */\n> +\n> +/**\n> + * \\fn ConverterMDFactory::createInstance() const\n> + * \\brief Create an instance of the ConverterMD corresponding to the factory\n> + * \\param[in] media Media device pointer\n> + * \\return A unique pointer to a newly constructed instance of the ConverterMD\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CONVERTER_MD\n> + * \\brief Register a media device based converter with the ConverterMD factory\n> + * \\param[in] name ConverterMD name used to register the class\n> + * \\param[in] converter Class name of ConverterMD derived class to register\n> + * \\param[in] compatibles List of compatible names\n> + *\n> + * Register a ConverterMD subclass with the factory and make it available to try\n> + * and match converters.\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index b24f8296..af8b1812 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -14,6 +14,7 @@ libcamera_sources = files([\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> 'converter.cpp',\n> + 'converter_media.cpp',\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 05ba76bc..f0622a74 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -30,7 +30,7 @@\n>\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> -#include \"libcamera/internal/converter.h\"\n> +#include \"libcamera/internal/converter_media.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -497,7 +497,7 @@ int SimpleCameraData::init()\n> \t/* Open the converter, if any. */\n> \tMediaDevice *converter = pipe->converter();\n> \tif (converter) {\n> -\t\tconverter_ = ConverterFactoryBase::create(converter);\n> +\t\tconverter_ = ConverterMDFactoryBase::create(converter);\n> \t\tif (!converter_) {\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> --\n> 2.34.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 976C0BD160\n\tfor ;\n\tMon, 11 Sep 2023 15:55:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDB9961DF4;\n\tMon, 11 Sep 2023 17:55:52 +0200 (CEST)","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 843D861DF0\n\tfor ;\n\tMon, 11 Sep 2023 17:55:51 +0200 (CEST)","from ideasonboard.com (mob-5-90-67-213.net.vodafone.it\n\t[5.90.67.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D736EF0;\n\tMon, 11 Sep 2023 17:54:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694447753;\n\tbh=FGvGRDZUWj2VR4n/hEBKYRX25SymxRsXtFOmg02ZwsU=;\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:Cc:\n\tFrom;\n\tb=grFt2AQgodxf1OMx4isG2KPIii8aw64Kj0/A4ITLpmsD3UDzKFoXBKTyEy5+62754\n\tuajlYfkKg/vmUO/LbaPl0v4+YjgvOdaMFQCTdo6hdTvPEb2QWmhk4qKHp4QbWMUYje\n\tOUzaa+C++fMTJoHoF0xXA9ofzLFRDDdWBa/Gospo6fCDZGRvNXLhwRRN80ULfoWjEZ\n\tixhSHaRBQtueRnr1MZit0Z7yNcW3Z8bUPcQjWjmInFexHvuA39bG9eudO1YvwsJwug\n\tKCJcCn/6nK4Y85yZbw+07GSB9EuHJrvpIqIwVWIg7itRptAR8z7x3gd9FnoneAq4qv\n\t6rVTwyGhUGh3Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694447660;\n\tbh=FGvGRDZUWj2VR4n/hEBKYRX25SymxRsXtFOmg02ZwsU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eNp3VInPU64dm45P9WCc9IJtaK8VfpeXdzd97hpSgnaoMXnVniz1pwLp24DBbHlyq\n\thtrHbh3FsrfBiJOB02FLbvUFZBZZ4bCCgjr7jolg+riZW9fkDFY0LiOUXL40l+AJSG\n\thib1A2UM9HSmrw0QxY+9dGrQ46qJ17+mSJcnZr4U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eNp3VInP\"; dkim-atps=neutral","Date":"Mon, 11 Sep 2023 17:55:47 +0200","To":"Andrey Konovalov ","Message-ID":"","References":"<20230910175027.23384-1-andrey.konovalov@linaro.org>\n\t<20230910175027.23384-2-andrey.konovalov@linaro.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230910175027.23384-2-andrey.konovalov@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: converter: split\n\tConverterMD (media device) out of Converter","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"jacopo.mondi@ideasonboard.com, bryan.odonoghue@linaro.org,\n\tlibcamera-devel@lists.libcamera.org, srinivas.kandagatla@linaro.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27756,"web_url":"https://patchwork.libcamera.org/comment/27756/","msgid":"","date":"2023-09-11T18:17:37","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: converter: split\n\tConverterMD (media device) out of Converter","submitter":{"id":25,"url":"https://patchwork.libcamera.org/api/people/25/","name":"Andrey Konovalov","email":"andrey.konovalov@linaro.org"},"content":"Hi Jacopo,\n\nThank you for your review!\n\nOn 11.09.2023 18:55, Jacopo Mondi wrote:\n> Hi Andrey\n> \n> On Sun, Sep 10, 2023 at 08:50:25PM +0300, Andrey Konovalov via libcamera-devel wrote:\n>> Make Converter a bit more generic by not requiring it to use a media device.\n>> For this split out ConverterMD from Converter class, and rename\n>> ConverterFactoryBase / ConverterFactory to ConverterMDFactoryBase /\n>> ConverterMDFactory for consistency.\n>>\n>> Signed-off-by: Andrey Konovalov \n> \n> I understand where this is going, but before going into a full review\n> I would like to explore the possibility of generalizing the existing\n> hierarchy before splitting it in media and non-media versions, which\n> would require re-defining another FactoryBase because as you said, I\n> do expect as well software converter to need a factory too..\n> \n> As far as I can see the MediaDevice is mostly used to get the\n> ->driver() name out and match it against a list of compatibles the\n> Converter-derived class has been instantiated with.\n> \n> It seems to possible to make the '*media' parameter optional to most\n> functions that accept it, or even overload the\n> ConverterFactoryBase::create() and createInstance() functions to\n> accept instead a string that identifies the software converter version\n> (ie \"linaro-sw-converter\" in your case?).\n> \n> I would go as far as wondering if insted we shouldn't make the whole\n> hierarchy string-based and MediaDevice completely.\n\n... remove MediaDevice completely? This might be an option.\nThis would definitely work for software based convertor.\nBut when I tried to follow along the usage of MediaDevice in Converter\nand how Converter is handled in the pipeline handler, I got an\nimpression of MediaDevice dependency going down to DeviceEnumerator which\nis critical for media (now) and non-media USB (when implemented) devices,\nand is irrelevant to \"software based\" \"devices\".\nLet me double check, and experiment with the code a bit.\n\n> The only thing we would lose for real would be\n> \n> \tconst std::vector &entities = media->entities();\n> \tauto it = std::find_if(entities.begin(), entities.end(),\n> \t\t\t [](MediaEntity *entity) {\n> \t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n> \t\t\t });\n> \tif (it == entities.end()) {\n> \t\tLOG(Converter, Error)\n> \t\t\t<< \"No entity suitable for implementing a converter in \"\n> \t\t\t<< media->driver() << \" entities list.\";\n> \t\treturn;\n> \t}\n> \n> at construction-time. Would this be that bad ? (if we want to keep it\n> we can have 2 overloaded constructors, in example).\n\n... I would expect 2 overloaded constructors to be needed.\n\n> Have this options been discussed in the previous version or have you\n> considered them but then discarded by any chance ?\n\nThis hasn't been discussed (the previous version used a quick hack instead of\nthis patch, so there was little to discuss I guess).\n\n\n\n\n> Below some drive-by comments\n> \n> \n>> ---\n>> include/libcamera/internal/converter.h | 49 +---\n>> .../internal/converter/converter_v4l2_m2m.h | 4 +-\n>> include/libcamera/internal/converter_media.h | 86 +++++++\n>> include/libcamera/internal/meson.build | 1 +\n>> src/libcamera/converter.cpp | 191 +-------------\n>> .../converter/converter_v4l2_m2m.cpp | 4 +-\n>> src/libcamera/converter_media.cpp | 241 ++++++++++++++++++\n>> src/libcamera/meson.build | 1 +\n>> src/libcamera/pipeline/simple/simple.cpp | 4 +-\n>> 9 files changed, 337 insertions(+), 244 deletions(-)\n>> create mode 100644 include/libcamera/internal/converter_media.h\n>> create mode 100644 src/libcamera/converter_media.cpp\n>>\n>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>> index 834ec5ab..da1f5d73 100644\n>> --- a/include/libcamera/internal/converter.h\n>> +++ b/include/libcamera/internal/converter.h\n>> @@ -24,14 +24,13 @@\n>> namespace libcamera {\n>>\n>> class FrameBuffer;\n>> -class MediaDevice;\n>> class PixelFormat;\n>> struct StreamConfiguration;\n>>\n>> class Converter\n>> {\n>> public:\n>> -\tConverter(MediaDevice *media);\n>> +\tConverter();\n>> \tvirtual ~Converter();\n>>\n>> \tvirtual int loadConfiguration(const std::string &filename) = 0;\n>> @@ -57,52 +56,6 @@ public:\n>>\n>> \tSignal inputBufferReady;\n>> \tSignal outputBufferReady;\n>> -\n>> -\tconst std::string &deviceNode() const { return deviceNode_; }\n>> -\n>> -private:\n>> -\tstd::string deviceNode_;\n>> };\n>>\n>> -class ConverterFactoryBase\n>> -{\n>> -public:\n>> -\tConverterFactoryBase(const std::string name, std::initializer_list compatibles);\n>> -\tvirtual ~ConverterFactoryBase() = default;\n>> -\n>> -\tconst std::vector &compatibles() const { return compatibles_; }\n>> -\n>> -\tstatic std::unique_ptr create(MediaDevice *media);\n>> -\tstatic std::vector &factories();\n>> -\tstatic std::vector names();\n>> -\n>> -private:\n>> -\tLIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactoryBase)\n>> -\n>> -\tstatic void registerType(ConverterFactoryBase *factory);\n>> -\n>> -\tvirtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n>> -\n>> -\tstd::string name_;\n>> -\tstd::vector compatibles_;\n>> -};\n>> -\n>> -template\n>> -class ConverterFactory : public ConverterFactoryBase\n>> -{\n>> -public:\n>> -\tConverterFactory(const char *name, std::initializer_list compatibles)\n>> -\t\t: ConverterFactoryBase(name, compatibles)\n>> -\t{\n>> -\t}\n>> -\n>> -\tstd::unique_ptr createInstance(MediaDevice *media) const override\n>> -\t{\n>> -\t\treturn std::make_unique<_Converter>(media);\n>> -\t}\n>> -};\n>> -\n>> -#define REGISTER_CONVERTER(name, converter, compatibles) \\\n>> -\tstatic ConverterFactory global_##converter##Factory(name, compatibles);\n>> -\n>> } /* namespace libcamera */\n>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> index 815916d0..aeb8c0e9 100644\n>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> @@ -20,7 +20,7 @@\n>>\n>> #include \n>>\n>> -#include \"libcamera/internal/converter.h\"\n>> +#include \"libcamera/internal/converter_media.h\"\n>>\n>> namespace libcamera {\n>>\n>> @@ -31,7 +31,7 @@ class SizeRange;\n>> struct StreamConfiguration;\n>> class V4L2M2MDevice;\n>>\n>> -class V4L2M2MConverter : public Converter\n>> +class V4L2M2MConverter : public ConverterMD\n>> {\n>> public:\n>> \tV4L2M2MConverter(MediaDevice *media);\n>> diff --git a/include/libcamera/internal/converter_media.h b/include/libcamera/internal/converter_media.h\n>> new file mode 100644\n>> index 00000000..2b56ebdb\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/converter_media.h\n>> @@ -0,0 +1,86 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Laurent Pinchart\n>> + * Copyright 2022 NXP\n>> + *\n>> + * converter_media.h - Generic media device based format converter interface\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/converter.h\"\n> \n> A lot of stuff (inclusions an forward declaration) are duplicated in\n> this and in the libcamera/internal/converter.h header. You can include\n> converter.h first and add only what is missing.\n\nRight. I'll fix that.\n\n>> +\n>> +namespace libcamera {\n>> +\n>> +class FrameBuffer;\n>> +class MediaDevice;\n>> +class PixelFormat;\n>> +struct StreamConfiguration;\n>> +\n>> +class ConverterMD : public Converter\n>> +{\n>> +public:\n>> +\tConverterMD(MediaDevice *media);\n>> +\t~ConverterMD();\n>> +\n>> +\tconst std::string &deviceNode() const { return deviceNode_; }\n>> +\n>> +private:\n>> +\tstd::string deviceNode_;\n>> +};\n>> +\n>> +class ConverterMDFactoryBase\n>> +{\n>> +public:\n>> +\tConverterMDFactoryBase(const std::string name, std::initializer_list compatibles);\n>> +\tvirtual ~ConverterMDFactoryBase() = default;\n>> +\n>> +\tconst std::vector &compatibles() const { return compatibles_; }\n>> +\n>> +\tstatic std::unique_ptr create(MediaDevice *media);\n>> +\tstatic std::vector &factories();\n>> +\tstatic std::vector names();\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterMDFactoryBase)\n>> +\n>> +\tstatic void registerType(ConverterMDFactoryBase *factory);\n>> +\n>> +\tvirtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n>> +\n>> +\tstd::string name_;\n>> +\tstd::vector compatibles_;\n>> +};\n>> +\n>> +template\n>> +class ConverterMDFactory : public ConverterMDFactoryBase\n>> +{\n>> +public:\n>> +\tConverterMDFactory(const char *name, std::initializer_list compatibles)\n>> +\t\t: ConverterMDFactoryBase(name, compatibles)\n>> +\t{\n>> +\t}\n>> +\n>> +\tstd::unique_ptr createInstance(MediaDevice *media) const override\n>> +\t{\n>> +\t\treturn std::make_unique<_ConverterMD>(media);\n>> +\t}\n>> +};\n>> +\n>> +#define REGISTER_CONVERTER_MD(name, converter, compatibles) \\\n>> +\tstatic ConverterMDFactory global_##converter##MDFactory(name, compatibles);\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> index 7f1f3440..e9c41346 100644\n>> --- a/include/libcamera/internal/meson.build\n>> +++ b/include/libcamera/internal/meson.build\n>> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([\n>> 'control_serializer.h',\n>> 'control_validator.h',\n>> 'converter.h',\n>> + 'converter_media.h',\n>> 'delayed_controls.h',\n>> 'device_enumerator.h',\n>> 'device_enumerator_sysfs.h',\n>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n>> index fa0f1ec8..92dcdc03 100644\n>> --- a/src/libcamera/converter.cpp\n>> +++ b/src/libcamera/converter.cpp\n>> @@ -38,26 +38,9 @@ LOG_DEFINE_CATEGORY(Converter)\n>>\n>> /**\n>> * \\brief Construct a Converter instance\n>> - * \\param[in] media The media device implementing the converter\n>> - *\n>> - * This searches for the entity implementing the data streaming function in the\n>> - * media graph entities and use its device node as the converter device node.\n>> */\n>> -Converter::Converter(MediaDevice *media)\n>> +Converter::Converter()\n>> {\n>> -\tconst std::vector &entities = media->entities();\n>> -\tauto it = std::find_if(entities.begin(), entities.end(),\n>> -\t\t\t [](MediaEntity *entity) {\n>> -\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n>> -\t\t\t });\n>> -\tif (it == entities.end()) {\n>> -\t\tLOG(Converter, Error)\n>> -\t\t\t<< \"No entity suitable for implementing a converter in \"\n>> -\t\t\t<< media->driver() << \" entities list.\";\n>> -\t\treturn;\n>> -\t}\n>> -\n>> -\tdeviceNode_ = (*it)->deviceNode();\n>> }\n>>\n>> Converter::~Converter()\n>> @@ -159,176 +142,4 @@ Converter::~Converter()\n>> * \\brief A signal emitted on each frame buffer completion of the output queue\n>> */\n>>\n>> -/**\n>> - * \\fn Converter::deviceNode()\n>> - * \\brief The converter device node attribute accessor\n>> - * \\return The converter device node string\n>> - */\n>> -\n>> -/**\n>> - * \\class ConverterFactoryBase\n>> - * \\brief Base class for converter factories\n>> - *\n>> - * The ConverterFactoryBase class is the base of all specializations of the\n>> - * ConverterFactory class template. It implements the factory registration,\n>> - * maintains a registry of factories, and provides access to the registered\n>> - * factories.\n>> - */\n>> -\n>> -/**\n>> - * \\brief Construct a converter factory base\n>> - * \\param[in] name Name of the converter class\n>> - * \\param[in] compatibles Name aliases of the converter class\n>> - *\n>> - * Creating an instance of the factory base registers it with the global list of\n>> - * factories, accessible through the factories() function.\n>> - *\n>> - * The factory \\a name is used as unique identifier. If the converter\n>> - * implementation fully relies on a generic framework, the name should be the\n>> - * same as the framework. Otherwise, if the implementation is specialized, the\n>> - * factory name should match the driver name implementing the function.\n>> - *\n>> - * The factory \\a compatibles holds a list of driver names implementing a generic\n>> - * subsystem without any personalizations.\n>> - */\n>> -ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list compatibles)\n>> -\t: name_(name), compatibles_(compatibles)\n>> -{\n>> -\tregisterType(this);\n>> -}\n>> -\n>> -/**\n>> - * \\fn ConverterFactoryBase::compatibles()\n>> - * \\return The names compatibles\n>> - */\n>> -\n>> -/**\n>> - * \\brief Create an instance of the converter corresponding to a named factory\n>> - * \\param[in] media Name of the factory\n> \n> This clearly was wrong already, and if you shuffle code around might\n> be a good occasion to change it.\n\nOK\n\n>> - *\n>> - * \\return A unique pointer to a new instance of the converter subclass\n>> - * corresponding to the named factory or one of its alias. Otherwise a null\n>> - * pointer if no such factory exists\n>> - */\n>> -std::unique_ptr ConverterFactoryBase::create(MediaDevice *media)\n>> -{\n>> -\tconst std::vector &factories =\n>> -\t\tConverterFactoryBase::factories();\n>> -\n>> -\tfor (const ConverterFactoryBase *factory : factories) {\n>> -\t\tconst std::vector &compatibles = factory->compatibles();\n>> -\t\tauto it = std::find(compatibles.begin(), compatibles.end(), media->driver());\n>> -\n>> -\t\tif (it == compatibles.end() && media->driver() != factory->name_)\n>> -\t\t\tcontinue;\n>> -\n>> -\t\tLOG(Converter, Debug)\n>> -\t\t\t<< \"Creating converter from \"\n>> -\t\t\t<< factory->name_ << \" factory with \"\n>> -\t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n>> -\n>> -\t\tstd::unique_ptr converter = factory->createInstance(media);\n>> -\t\tif (converter->isValid())\n>> -\t\t\treturn converter;\n>> -\t}\n>> -\n>> -\treturn nullptr;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Add a converter class to the registry\n>> - * \\param[in] factory Factory to use to construct the converter class\n>> - *\n>> - * The caller is responsible to guarantee the uniqueness of the converter name.\n>> - */\n>> -void ConverterFactoryBase::registerType(ConverterFactoryBase *factory)\n>> -{\n>> -\tstd::vector &factories =\n>> -\t\tConverterFactoryBase::factories();\n>> -\n>> -\tfactories.push_back(factory);\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrieve the list of all converter factory names\n>> - * \\return The list of all converter factory names\n>> - */\n>> -std::vector ConverterFactoryBase::names()\n>> -{\n>> -\tstd::vector list;\n>> -\n>> -\tstd::vector &factories =\n>> -\t\tConverterFactoryBase::factories();\n>> -\n>> -\tfor (ConverterFactoryBase *factory : factories) {\n>> -\t\tlist.push_back(factory->name_);\n>> -\t\tfor (auto alias : factory->compatibles())\n>> -\t\t\tlist.push_back(alias);\n>> -\t}\n>> -\n>> -\treturn list;\n>> -}\n>> -\n>> -/**\n>> - * \\brief Retrieve the list of all converter factories\n>> - * \\return The list of converter factories\n>> - */\n>> -std::vector &ConverterFactoryBase::factories()\n>> -{\n>> -\t/*\n>> -\t * The static factories map is defined inside the function to ensure\n>> -\t * it gets initialized on first use, without any dependency on link\n>> -\t * order.\n>> -\t */\n>> -\tstatic std::vector factories;\n>> -\treturn factories;\n>> -}\n>> -\n>> -/**\n>> - * \\var ConverterFactoryBase::name_\n>> - * \\brief The name of the factory\n>> - */\n>> -\n>> -/**\n>> - * \\var ConverterFactoryBase::compatibles_\n>> - * \\brief The list holding the factory compatibles\n>> - */\n>> -\n>> -/**\n>> - * \\class ConverterFactory\n>> - * \\brief Registration of ConverterFactory classes and creation of instances\n>> - * \\param _Converter The converter class type for this factory\n>> - *\n>> - * To facilitate discovery and instantiation of Converter classes, the\n>> - * ConverterFactory class implements auto-registration of converter helpers.\n>> - * Each Converter subclass shall register itself using the REGISTER_CONVERTER()\n>> - * macro, which will create a corresponding instance of a ConverterFactory\n>> - * subclass and register it with the static list of factories.\n>> - */\n>> -\n>> -/**\n>> - * \\fn ConverterFactory::ConverterFactory(const char *name, std::initializer_list compatibles)\n>> - * \\brief Construct a converter factory\n>> - * \\details \\copydetails ConverterFactoryBase::ConverterFactoryBase\n>> - */\n>> -\n>> -/**\n>> - * \\fn ConverterFactory::createInstance() const\n>> - * \\brief Create an instance of the Converter corresponding to the factory\n>> - * \\param[in] media Media device pointer\n>> - * \\return A unique pointer to a newly constructed instance of the Converter\n>> - * subclass corresponding to the factory\n>> - */\n>> -\n>> -/**\n>> - * \\def REGISTER_CONVERTER\n>> - * \\brief Register a converter with the Converter factory\n>> - * \\param[in] name Converter name used to register the class\n>> - * \\param[in] converter Class name of Converter derived class to register\n>> - * \\param[in] compatibles List of compatible names\n>> - *\n>> - * Register a Converter subclass with the factory and make it available to try\n>> - * and match converters.\n>> - */\n>> -\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> index 2a4d1d99..d0a5e3bf 100644\n>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> @@ -194,7 +194,7 @@ void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)\n>> */\n>>\n>> V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\n>> -\t: Converter(media)\n>> +\t: ConverterMD(media)\n>> {\n>> \tif (deviceNode().empty())\n>> \t\treturn;\n>> @@ -448,6 +448,6 @@ static std::initializer_list compatibles = {\n>> \t\"pxp\",\n>> };\n>>\n>> -REGISTER_CONVERTER(\"v4l2_m2m\", V4L2M2MConverter, compatibles)\n>> +REGISTER_CONVERTER_MD(\"v4l2_m2m\", V4L2M2MConverter, compatibles)\n>>\n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/converter_media.cpp b/src/libcamera/converter_media.cpp\n>> new file mode 100644\n>> index 00000000..f5024764\n>> --- /dev/null\n>> +++ b/src/libcamera/converter_media.cpp\n>> @@ -0,0 +1,241 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright 2022 NXP\n>> + *\n> \n> You copyright here and in the header file if you like to\n\nI thought it is not worth it as this is by 99% moving the existing\ncode between two cpp and two header files.\n\n>> + * converter.cpp - Generic format converter interface\n>> + */\n>> +\n>> +#include \"libcamera/internal/converter_media.h\"\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/media_device.h\"\n>> +\n>> +#include \"linux/media.h\"\n> \n> I know it was already like this, but I would have expected to see <>\n> \n> It shouldn't make a difference though, even if this is a system-wide\n> header we ship our own copy and adjust the inclusion prefix precendece\n> to use our own version (I presume) so both \"\" and <> will do the same\n> thing ?\n> \n> And, btw, this is included by media_device.h (again, not your fault)\n\nOK, will sort it out.\n\n>> +\n>> +/**\n>> + * \\file internal/converter_media.h\n>> + * \\brief Abstract media device based converter\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DECLARE_CATEGORY(Converter)\n> \n> Maybe ConverterMD ?\n\nInitially I added the ConverterMD category but then decided that single\nConverter category might be used by both the media device, and software\nconverters? I'll think again.\n\nThanks,\nAndrey\n\n>> +\n>> +/**\n>> + * \\class ConverterMD\n>> + * \\brief Abstract Base Class for media device based converter\n>> + *\n>> + * The ConverterMD class is an Abstract Base Class defining the interfaces of\n>> + * media device based converter implementations.\n>> + *\n>> + * Converters offer scaling and pixel format conversion services on an input\n>> + * stream. The converter can output multiple streams with individual conversion\n>> + * parameters from the same input stream.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a ConverterMD instance\n>> + * \\param[in] media The media device implementing the converter\n>> + *\n>> + * This searches for the entity implementing the data streaming function in the\n>> + * media graph entities and use its device node as the converter device node.\n>> + */\n>> +ConverterMD::ConverterMD(MediaDevice *media)\n>> +{\n>> +\tconst std::vector &entities = media->entities();\n>> +\tauto it = std::find_if(entities.begin(), entities.end(),\n>> +\t\t\t [](MediaEntity *entity) {\n>> +\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n>> +\t\t\t });\n>> +\tif (it == entities.end()) {\n>> +\t\tLOG(Converter, Error)\n>> +\t\t\t<< \"No entity suitable for implementing a converter in \"\n>> +\t\t\t<< media->driver() << \" entities list.\";\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tdeviceNode_ = (*it)->deviceNode();\n>> +}\n>> +\n>> +ConverterMD::~ConverterMD()\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\fn ConverterMD::deviceNode()\n>> + * \\brief The converter device node attribute accessor\n>> + * \\return The converter device node string\n>> + */\n>> +\n>> +/**\n>> + * \\class ConverterMDFactoryBase\n>> + * \\brief Base class for media device based converter factories\n>> + *\n>> + * The ConverterMDFactoryBase class is the base of all specializations of the\n>> + * ConverterMDFactory class template. It implements the factory registration,\n>> + * maintains a registry of factories, and provides access to the registered\n>> + * factories.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a media device based converter factory base\n>> + * \\param[in] name Name of the converter class\n>> + * \\param[in] compatibles Name aliases of the converter class\n>> + *\n>> + * Creating an instance of the factory base registers it with the global list of\n>> + * factories, accessible through the factories() function.\n>> + *\n>> + * The factory \\a name is used as unique identifier. If the converter\n>> + * implementation fully relies on a generic framework, the name should be the\n>> + * same as the framework. Otherwise, if the implementation is specialized, the\n>> + * factory name should match the driver name implementing the function.\n>> + *\n>> + * The factory \\a compatibles holds a list of driver names implementing a generic\n>> + * subsystem without any personalizations.\n>> + */\n>> +ConverterMDFactoryBase::ConverterMDFactoryBase(const std::string name, std::initializer_list compatibles)\n>> +\t: name_(name), compatibles_(compatibles)\n>> +{\n>> +\tregisterType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\fn ConverterMDFactoryBase::compatibles()\n>> + * \\return The names compatibles\n>> + */\n>> +\n>> +/**\n>> + * \\brief Create an instance of the converter corresponding to a named factory\n>> + * \\param[in] media Name of the factory\n>> + *\n>> + * \\return A unique pointer to a new instance of the media device based\n>> + * converter subclass corresponding to the named factory or one of its alias.\n>> + * Otherwise a null pointer if no such factory exists.\n>> + */\n>> +std::unique_ptr ConverterMDFactoryBase::create(MediaDevice *media)\n>> +{\n>> +\tconst std::vector &factories =\n>> +\t\tConverterMDFactoryBase::factories();\n>> +\n>> +\tfor (const ConverterMDFactoryBase *factory : factories) {\n>> +\t\tconst std::vector &compatibles = factory->compatibles();\n>> +\t\tauto it = std::find(compatibles.begin(), compatibles.end(), media->driver());\n>> +\n>> +\t\tif (it == compatibles.end() && media->driver() != factory->name_)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tLOG(Converter, Debug)\n>> +\t\t\t<< \"Creating converter from \"\n>> +\t\t\t<< factory->name_ << \" factory with \"\n>> +\t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n>> +\n>> +\t\tstd::unique_ptr converter = factory->createInstance(media);\n>> +\t\tif (converter->isValid())\n>> +\t\t\treturn converter;\n>> +\t}\n>> +\n>> +\treturn nullptr;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Add a media device based converter class to the registry\n>> + * \\param[in] factory Factory to use to construct the media device based\n>> + * converter class\n>> + *\n>> + * The caller is responsible to guarantee the uniqueness of the converter name.\n>> + */\n>> +void ConverterMDFactoryBase::registerType(ConverterMDFactoryBase *factory)\n>> +{\n>> +\tstd::vector &factories =\n>> +\t\tConverterMDFactoryBase::factories();\n>> +\n>> +\tfactories.push_back(factory);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the list of all media device based converter factory names\n>> + * \\return The list of all media device based converter factory names\n>> + */\n>> +std::vector ConverterMDFactoryBase::names()\n>> +{\n>> +\tstd::vector list;\n>> +\n>> +\tstd::vector &factories =\n>> +\t\tConverterMDFactoryBase::factories();\n>> +\n>> +\tfor (ConverterMDFactoryBase *factory : factories) {\n>> +\t\tlist.push_back(factory->name_);\n>> +\t\tfor (auto alias : factory->compatibles())\n>> +\t\t\tlist.push_back(alias);\n>> +\t}\n>> +\n>> +\treturn list;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the list of all media device based converter factories\n>> + * \\return The list of media device based converter factories\n>> + */\n>> +std::vector &ConverterMDFactoryBase::factories()\n>> +{\n>> +\t/*\n>> +\t * The static factories map is defined inside the function to ensure\n>> +\t * it gets initialized on first use, without any dependency on link\n>> +\t * order.\n>> +\t */\n>> +\tstatic std::vector factories;\n>> +\treturn factories;\n>> +}\n>> +\n>> +/**\n>> + * \\var ConverterMDFactoryBase::name_\n>> + * \\brief The name of the factory\n>> + */\n>> +\n>> +/**\n>> + * \\var ConverterMDFactoryBase::compatibles_\n>> + * \\brief The list holding the factory compatibles\n>> + */\n>> +\n>> +/**\n>> + * \\class ConverterMDFactory\n>> + * \\brief Registration of ConverterMDFactory classes and creation of instances\n>> + * \\param _Converter The converter class type for this factory\n>> + *\n>> + * To facilitate discovery and instantiation of ConverterMD classes, the\n>> + * ConverterMDFactory class implements auto-registration of converter helpers.\n>> + * Each ConverterMD subclass shall register itself using the\n>> + * REGISTER_CONVERTER() macro, which will create a corresponding instance of a\n>> + * ConverterMDFactory subclass and register it with the static list of\n>> + * factories.\n>> + */\n>> +\n>> +/**\n>> + * \\fn ConverterMDFactory::ConverterMDFactory(const char *name, std::initializer_list compatibles)\n>> + * \\brief Construct a media device converter factory\n>> + * \\details \\copydetails ConverterMDFactoryBase::ConverterMDFactoryBase\n>> + */\n>> +\n>> +/**\n>> + * \\fn ConverterMDFactory::createInstance() const\n>> + * \\brief Create an instance of the ConverterMD corresponding to the factory\n>> + * \\param[in] media Media device pointer\n>> + * \\return A unique pointer to a newly constructed instance of the ConverterMD\n>> + * subclass corresponding to the factory\n>> + */\n>> +\n>> +/**\n>> + * \\def REGISTER_CONVERTER_MD\n>> + * \\brief Register a media device based converter with the ConverterMD factory\n>> + * \\param[in] name ConverterMD name used to register the class\n>> + * \\param[in] converter Class name of ConverterMD derived class to register\n>> + * \\param[in] compatibles List of compatible names\n>> + *\n>> + * Register a ConverterMD subclass with the factory and make it available to try\n>> + * and match converters.\n>> + */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index b24f8296..af8b1812 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -14,6 +14,7 @@ libcamera_sources = files([\n>> 'control_serializer.cpp',\n>> 'control_validator.cpp',\n>> 'converter.cpp',\n>> + 'converter_media.cpp',\n>> 'delayed_controls.cpp',\n>> 'device_enumerator.cpp',\n>> 'device_enumerator_sysfs.cpp',\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 05ba76bc..f0622a74 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -30,7 +30,7 @@\n>>\n>> #include \"libcamera/internal/camera.h\"\n>> #include \"libcamera/internal/camera_sensor.h\"\n>> -#include \"libcamera/internal/converter.h\"\n>> +#include \"libcamera/internal/converter_media.h\"\n>> #include \"libcamera/internal/device_enumerator.h\"\n>> #include \"libcamera/internal/media_device.h\"\n>> #include \"libcamera/internal/pipeline_handler.h\"\n>> @@ -497,7 +497,7 @@ int SimpleCameraData::init()\n>> \t/* Open the converter, if any. */\n>> \tMediaDevice *converter = pipe->converter();\n>> \tif (converter) {\n>> -\t\tconverter_ = ConverterFactoryBase::create(converter);\n>> +\t\tconverter_ = ConverterMDFactoryBase::create(converter);\n>> \t\tif (!converter_) {\n>> \t\t\tLOG(SimplePipeline, Warning)\n>> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n>> --\n>> 2.34.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 1642EBD160\n\tfor ;\n\tMon, 11 Sep 2023 18:17:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72AED61DF4;\n\tMon, 11 Sep 2023 20:17:42 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FD9261DF0\n\tfor ;\n\tMon, 11 Sep 2023 20:17:40 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id\n\t4fb4d7f45d1cf-52a23227567so6125276a12.0\n\tfor ;\n\tMon, 11 Sep 2023 11:17:40 -0700 (PDT)","from [192.168.118.20] ([87.116.162.130])\n\tby smtp.gmail.com with ESMTPSA id\n\tc2-20020aa7c982000000b0052e2aa1a0fcsm4945106edt.77.2023.09.11.11.17.38\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 11 Sep 2023 11:17:38 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694456262;\n\tbh=3HsiYYOxlaaexNcmHCUmJI7nV0eoZEzd96EZS5MOe60=;\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:Cc:\n\tFrom;\n\tb=19yh7ej7UyDqgNowXFN6Wz7HP0uekvD8lbP6SBLRAwHxZasdqA2s0qwlzcKOFHlAl\n\t9iJ1glsmH3jr39D64HvHU6U9B2NaKCvEyIeXH1fM0PU71JzfmqePgI049RAY43QO31\n\twk00FMRWxmcvJY+ZdzGo4u79gg4cjheU4XSnGA6+aZ44J0dr4hyxZZa1ozfMvz3qqR\n\tYSYutyB/E+/1aDSRoJebm8IPyZf02nySWh7w0Q6APHQmLxTMkX+dPGzkGWj58OXMNe\n\tuCcyCn9mL7orf3ltbT/Xe6+TeCav+rp5uoGp+gmWbN/+8sU9C0m1QEkNJvjqgtdLI8\n\tN6Cd3ekhA/m5g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=linaro.org; s=google; t=1694456259; x=1695061059;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=uJdiNQMO2VCeNAubYdHJAMJLDTmAea2Rc80H0K2YHl8=;\n\tb=N57IHy/PjgxoRq2Mip7XGnTHqXRjei6L5lJYWBrN9Gs/v82ZYQF7eOzEB0tddqTqes\n\tI7U7zv60pgrQuyjGXFAja8Im42V+h8GJJlnJdGiblTY+oMbl5jrjZ7NErJ+yaHPZ/Lqr\n\tv47DV+z/YJGmaTAKzuZlaDF8Jkj28vHbBs85FIUCM51PQcSFlYx7bg3YxKvozCw4KoMW\n\tdGPwaxggMFXSoExDyn6MCQMqQmB0zletQnoo2lV5mVQVCp1ZmHmKkTL6ut8I28FJ1DVC\n\tZUVOYUCRfCb+Q9RwXzNe4f6A81E7IlBd5GQes1E0eGz33yTwmNR1OKFeGTF5Qd5qY09D\n\tmtbg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=linaro.org\n\theader.i=@linaro.org header.b=\"N57IHy/P\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1694456259; x=1695061059;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=uJdiNQMO2VCeNAubYdHJAMJLDTmAea2Rc80H0K2YHl8=;\n\tb=onhjNrdVBna/0a5GXglqQNpNEqx4iJpE+JbENpAuh/roWjEkFNAVNvoSVu/kLyvV21\n\t3HSeyYHfePn8vzt4ms/SRHR3yIKi8BE1YXiSu97DQhL82vrLPMv+FlHIbQUEOuzdHp4A\n\tBPHwSRFa5HzGl8O5OnK0ag+T6Uo0Mnk+Eux6xc7UA9XjkV5yV7x08yKLys2omR3mh+eL\n\tZ1xADbl4miudX0yDX5LTT0QriYKWr0sna5KzJXavC/aByQ+uRiIT2dhoNi1uHm+Isp/n\n\tFd6ZHB/I9NnY8sChN7DUgXtDgY72l/kYco43ZV6ngQ4ItCkx4llJDIDguqHx3ZtswtLh\n\tnnzg==","X-Gm-Message-State":"AOJu0YyuhuTuTMMPoAj5syXZDJ2iNtp7A657Id95eEHU/wxaw1wb2Tdn\n\txHt/oqVTjsFPhMut2ptdaehgJQ==","X-Google-Smtp-Source":"AGHT+IEqfUeCEta+azI+LT5cgMr0xkE+diuVSwC/UDu1wC7tQTdeqmnTIHhrRn5f7QWFcTA2mMpe7A==","X-Received":"by 2002:aa7:cd6c:0:b0:527:3a95:5bea with SMTP id\n\tca12-20020aa7cd6c000000b005273a955beamr8714391edb.32.1694456259274; \n\tMon, 11 Sep 2023 11:17:39 -0700 (PDT)","Message-ID":"","Date":"Mon, 11 Sep 2023 21:17:37 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.15.0","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20230910175027.23384-1-andrey.konovalov@linaro.org>\n\t<20230910175027.23384-2-andrey.konovalov@linaro.org>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: converter: split\n\tConverterMD (media device) out of Converter","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":"Andrey Konovalov via libcamera-devel\n\t","Reply-To":"Andrey Konovalov ","Cc":"bryan.odonoghue@linaro.org, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]