[{"id":25746,"web_url":"https://patchwork.libcamera.org/comment/25746/","msgid":"<20221109074317.z72yxczzzui26ol2@uno.localdomain>","date":"2022-11-09T07:43:17","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Declare generic\n\tconverter interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Xavier,\n sorry for the delay!\n\nOn Mon, Oct 10, 2022 at 03:17:43PM +0200, Xavier Roumegue (OSS) wrote:\n> From: Xavier Roumegue \n>\n> Declare a converter Abstract Base Class intended to provide generic\n> interfaces to hardware offering size and format conversion services on\n> streams. This is mainly based on the public interfaces of the current\n> converter class implementation found in the simple pipeline handler.\n>\n> The main change is the introduction of loadConfiguration() function\n> which can be used by the concrete implementation to load hardware\n> specific runtime parameters defined by the application.\n>\n> Signed-off-by: Xavier Roumegue \n> ---\n> include/libcamera/internal/converter.h | 108 ++++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/converter.cpp | 337 +++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 447 insertions(+)\n> create mode 100644 include/libcamera/internal/converter.h\n> create mode 100644 src/libcamera/converter.cpp\n>\n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> new file mode 100644\n> index 00000000..38c05ee9\n> --- /dev/null\n> +++ b/include/libcamera/internal/converter.h\n> @@ -0,0 +1,108 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Laurent Pinchart\n> + * Copyright 2022 NXP\n> + *\n> + * converter.h - Generic stream converter infrastructure\n\nPlease align this with the .cpp one. Pick the one that you prefer\n\n * converter.h - Generic stream converter infrastructure\n * converter.cpp - Generic Format converter interface\n\n\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n\n#include \n\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class FrameBuffer;\n> +class MediaDevice;\n> +class Size;\n> +class SizeRange;\n> +struct StreamConfiguration;\n> +\n> +class Converter\n> +{\n> +public:\n> +\tConverter(MediaDevice *media);\n> +\tvirtual ~Converter();\n> +\n> +\tvirtual int loadConfiguration(const std::string &filename) = 0;\n> +\n> +\tvirtual bool isValid() const = 0;\n> +\n> +\tvirtual std::vector formats(PixelFormat input) = 0;\n> +\tvirtual SizeRange sizes(const Size &input) = 0;\n> +\n> +\tvirtual std::tuple\n> +\tstrideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;\n> +\n> +\tvirtual int configure(const StreamConfiguration &inputCfg,\n> +\t\t\t const std::vector> &outputCfg) = 0;\n\nI wonder if using would be easier than\nreference_wrapper, but I'll find out in the next patch I presume\n\n> +\tvirtual int exportBuffers(unsigned int ouput, unsigned int count,\n> +\t\t\t\t std::vector> *buffers) = 0;\n> +\n> +\tvirtual int start() = 0;\n> +\tvirtual void stop() = 0;\n> +\n> +\tvirtual int queueBuffers(FrameBuffer *input,\n> +\t\t\t\t const std::map &outputs) = 0;\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 aliases);\n> +\tvirtual ~ConverterFactoryBase() = default;\n> +\n> +\tconst std::vector &aliases() const { return aliases_; }\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 aliases_;\n> +};\n> +\n> +template\n> +class ConverterFactory : public ConverterFactoryBase\n> +{\n> +public:\n> +\tConverterFactory(const char *name, std::initializer_list aliases)\n> +\t\t: ConverterFactoryBase(name, aliases)\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, ...) \\\n> +\tstatic ConverterFactory global_##converter##Factory(name, { __VA_ARGS__ });\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 7a780d48..8f50d755 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -19,6 +19,7 @@ libcamera_internal_headers = files([\n> 'camera_sensor_properties.h',\n> 'control_serializer.h',\n> 'control_validator.h',\n> + 'converter.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> new file mode 100644\n> index 00000000..5e444f5f\n> --- /dev/null\n> +++ b/src/libcamera/converter.cpp\n\nI get quite a few Doxygen warnings on this class\n\nsrc/libcamera/converter.cpp:328: warning: argument 'aliases' of command @param is not found in the argument list of REGISTER_CONVERTER(name, converter,...)\nsrc/libcamera/converter.cpp:113: warning: argument 'output' of command @param is not found in the argument list of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0\ninclude/libcamera/internal/converter.h:49: warning: The following parameter of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0 is not documented:\n parameter 'ouput'\nsrc/libcamera/converter.cpp:139: warning: argument 'buffers' of command @param is not found in the argument list of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0\ninclude/libcamera/internal/converter.h:55: warning: The following parameter of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0 is not documented:\n parameter 'outputs'\ninclude/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\ninclude/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\ninclude/libcamera/internal/converter.h:70: warning: The following parameter of libcamera::ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list< std::string > aliases) is not documented:\n parameter 'aliases'\nsrc/libcamera/converter.cpp:199: warning: argument 'name' of command @param is not found in the argument list of libcamera::ConverterFactoryBase::create(MediaDevice *media)\ninclude/libcamera/internal/converter.h:75: warning: The following parameter of libcamera::ConverterFactoryBase::create(MediaDevice *media) is not documented:\n parameter 'media'\n\n\nWith the next patch applied the list increases:\n\n\nsrc/libcamera/converter.cpp:328: warning: argument 'aliases' of command @param is not found in the argument list of REGISTER_CONVERTER(name, converter,...)\nsrc/libcamera/converter.cpp:113: warning: argument 'output' of command @param is not found in the argument list of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0\ninclude/libcamera/internal/converter.h:49: warning: The following parameter of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0 is not documented:\n parameter 'ouput'\nsrc/libcamera/converter.cpp:139: warning: argument 'buffers' of command @param is not found in the argument list of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0\ninclude/libcamera/internal/converter.h:55: warning: The following parameter of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0 is not documented:\n parameter 'outputs'\ninclude/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\ninclude/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\ninclude/libcamera/internal/converter.h:70: warning: The following parameter of libcamera::ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list< std::string > aliases) is not documented:\n parameter 'aliases'\nsrc/libcamera/converter.cpp:199: warning: argument 'name' of command @param is not found in the argument list of libcamera::ConverterFactoryBase::create(MediaDevice *media)\ninclude/libcamera/internal/converter.h:75: warning: The following parameter of libcamera::ConverterFactoryBase::create(MediaDevice *media) is not documented:\n parameter 'media'\nsrc/libcamera/converter/converter_v4l2_m2m.cpp:323: warning: argument 'outputCfg' of command @param is not found in the argument list of libcamera::V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, const std::vector< std::reference_wrapper< StreamConfiguration >> &outputCfgs)\ninclude/libcamera/internal/converter/converter_v4l2_m2m.h:48: warning: The following parameter of libcamera::V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, const std::vector< std::reference_wrapper< StreamConfiguration >> &outputCfgs) is not documented:\n parameter 'outputCfgs'\nsrc/libcamera/converter/converter_v4l2_m2m.cpp:217: warning: argument 'filename' of command @param is not found in the argument list of libcamera::V4L2M2MConverter::formats(PixelFormat input)\nsrc/libcamera/converter/converter_v4l2_m2m.cpp:396: warning: argument 'buffers' of command @param is not found in the argument list of libcamera::V4L2M2MConverter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)\ninclude/libcamera/internal/converter/converter_v4l2_m2m.h:56: warning: The following parameter of libcamera::V4L2M2MConverter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs) is not documented: parameter 'outputs'\n\nCould you make sure Doxygen doesn't report any warning ?\n\n> @@ -0,0 +1,337 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright 2022 NXP\n> + *\n> + * converter.cpp - Generic Format converter interface\n> + */\n> +\n> +#include \"libcamera/internal/converter.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/media_device.h\"\n> +\n> +/**\n> + * \\file internal/converter.h\n> + * \\brief Abstract converter\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Converter)\n> +\n> +/**\n> + * \\class Converter\n> + * \\brief Abstract Base Class for converter\n> + *\n> + * The Converter class is an Abstract Base Class defining the interfaces of\n> + * converter implementations.\n> + *\n> + * Converters offers scaling and pixel formats conversion services on a input\n\nConverters -> offer\n\nI would also use \"pixel format\" in singular form, but I can't tell if\nthat's more correct or not.\n\n\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 Converter instance\n> + * \\param[in] media The media device implementing the converter\n> + *\n> + * This seeks for the entity implementing data streaming function in the media\n> + * graph entities and use its device node as converter device node.\n> + */\n> +Converter::Converter(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> +Converter::~Converter()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn Converter::loadConfiguration()\n> + * \\brief Load converter configuration from file\n> + * \\param[in] filename The file name path\n> + *\n> + * Load converter dependent configuration parameters to apply on the hardware.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Converter::isValid()\n> + * \\brief Check if the converter configuration is valid\n> + * \\return True is the converter is valid, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn Converter::formats()\n> + * \\brief Retrieve the list of supported pixel formats for an input pixel format\n> + * \\param[in] input Input pixel format to retrieve output pixel format list for\n> + * \\return The list of output supported pixel formats\n\nI would swap \"output supported\" to \"supported output\"\n\n> + */\n> +\n> +/**\n> + * \\fn Converter::sizes()\n> + * \\brief Retrieve the output range of minimum and maximum sizes for an input size\n\nHere too: \"range of minimum and maximum output sizes\"\n\n> + * \\param[in] input Input stream size to retrieve range for\n> + * \\return A range of output image sizes\n> + */\n> +\n> +/**\n> + * \\fn Converter::strideAndFrameSize()\n> + * \\brief Retrieve the output stride and frame size for an input configutation\n> + * \\param[in] pixelFormat Input stream pixel format\n> + * \\param[in] size Input stream size\n> + * \\return A tuple indicating the stride and frame size or an empty tuple on error\n> + */\n> +\n> +/**\n> + * \\fn Converter::configure()\n> + * \\brief Configure a set of output stream conversion from an input stream\n> + * \\param[in] inputCfg Input stream configuration\n> + * \\param[in] outputCfg A list of output stream configurations\n\nIs this an [out] parameter ?\n\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Converter::exportBuffers()\n> + * \\brief Export buffers from the converter device\n> + * \\param[in] output Output stream index exporting the buffers\n> + * \\param[in] count Number of buffers to allocate\n> + * \\param[out] buffers Vector to store allocated buffers\n> + *\n> + * This function operates similarly as V4L2VideoDevice::exportBuffers() on the\n> + * output stream indicated by the \\a output index.\n> + *\n> + * \\return The number of allocated buffers on success or a negative error code\n> + * otherwise\n> + */\n> +\n> +/**\n> + * \\fn Converter::start()\n> + * \\brief Start the converter streaming operation\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Converter::stop()\n> + * \\brief Stop the converter streaming operation\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Converter::queueBuffers()\n> + * \\brief Queue buffers to converter device\n> + * \\param[in] input The frame buffer to apply the conversion\n> + * \\param[out] buffers The container holding the output stream indexes and\n> + * their respective frame buffer outputs.\n> + *\n> + * This function queues the \\a input frame buffer on the output streams of the\n> + * \\a buffers map key and retrieve the output frame buffer indicated by the\n> + * \\a buffer map value.\n\n\"buffer\" is not an argument. Maybe \"buffers\".\n\nI'll better understand how the output map is populated in the next\npatch\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\var Converter::inputBufferReady\n> + * \\brief A signal emitted when the input frame buffer completes\n> + */\n> +\n> +/**\n> + * \\var Converter::outputBufferReady\n> + * \\brief A signal emitted when the output frame buffer completes\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> + *\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.\n\nPlease don't break lines in paragraphs.\n\nEither\n One line.\n\n With a line break in between.\n\nOr\n One sentence. And the paragraph continues without breaking.\n\n> + * If the converter implemententation fully relies on a generic framework, the\n> + * name should be the same as the framework.\n> + * Otherwise, if the implementation is specialized, the factory name should match\n> + * the driver name implementing the function.\n> + * The factory \\a aliases 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 aliases)\n> +\t: name_(name), aliases_(aliases)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the converter corresponding to a named factory\n> + * \\param[in] name Name of the factory\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\tstd::vector aliases = factory->aliases();\n\n &aliases\n\n> +\t\tauto it = std::find(aliases.begin(), aliases.end(), media->driver());\n> +\n> +\t\tif (it == aliases.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 == aliases.end() ? \"no\" : media->driver()) << \" alias.\";\n> +\n> +\t\treturn factory->createInstance(media);\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\nWhat is this function for ? Return the names of all converters\nregistered in the system ? Am I missing where it is used ?\n\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->aliases())\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::aliases_\n> + * \\brief The list holding the factory aliases\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 aliases)\n> + * \\brief Construct a converter factory\n> + * \\param[in] name Name of the converter class\n> + * \\param[in] aliases Aliases of the converter class\n> + *\n> + * Creating an instance of the factory 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.\n> + * If the converter implemententation fully relies on a generic framework, the\n> + * name should be the same as the framework.\n> + * Otherwise, if the implementation is specialized, the factory name should match\n> + * the driver name implementing the function.\n> + * The factory \\a aliases holds a list of driver names implementing a generic\n> + * subsystem without any personalizations.\n\nSame comment as above.\nAlso, you can use \\copydoc\n\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] aliases Optional list of alias 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/meson.build b/src/libcamera/meson.build\n> index 7fcbb2dd..3a9fc31f 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -13,6 +13,7 @@ libcamera_sources = files([\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> + 'converter.cpp',\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> --\n> 2.37.3\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 09C8EBD16B\n\tfor ;\n\tWed, 9 Nov 2022 07:43:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52E216307B;\n\tWed, 9 Nov 2022 08:43:22 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 491D461F39\n\tfor ;\n\tWed, 9 Nov 2022 08:43:20 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 128691BF20D;\n\tWed, 9 Nov 2022 07:43:18 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667979802;\n\tbh=5V66e1G6Utx4nVPmaIraK+zwnkYZ3JwDEzbyOiFPXh0=;\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=gSahDqbtUyUZ7BHu8yejSIwOwNYIPE21ZeRSeQjQMslOv+jRC0bMQvBb5x02cy5dJ\n\tZAM9TL6lexqRnMuEAgHDE42d1Zdh0ldNl+GEeZ2vwxCwkVLdRKv41c4EBuYe8NZUgr\n\tpbDvhLxZ5j+IRoT1Tr8wdGalxYj4F/TMO185kSm1+20/2R1eTDYKCpsEWW/FImwAqo\n\tkig6SkA5YfikbSGbQHswb6JZTdZUG62k++F4nEdkkoGXpP6bbJIbmhw837/gBhcCT5\n\td9jYdsZx+TFwsdKYPNWuw5F7zNfAyotwmq7dwbhueEy0QI2FsEjP4dX6XAVBTdQ0qL\n\trB4mAM2pGvZ9A==","Date":"Wed, 9 Nov 2022 08:43:17 +0100","To":"\"Xavier Roumegue (OSS)\" ","Message-ID":"<20221109074317.z72yxczzzui26ol2@uno.localdomain>","References":"<20221010131744.513261-1-xavier.roumegue@oss.nxp.com>\n\t<20221010131744.513261-2-xavier.roumegue@oss.nxp.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221010131744.513261-2-xavier.roumegue@oss.nxp.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Declare generic\n\tconverter 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25796,"web_url":"https://patchwork.libcamera.org/comment/25796/","msgid":"<33df6e7d-6a3f-89b7-f0de-19eafb55c847@oss.nxp.com>","date":"2022-11-15T09:51:36","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Declare generic\n\tconverter interface","submitter":{"id":107,"url":"https://patchwork.libcamera.org/api/people/107/","name":"Xavier Roumegue","email":"xavier.roumegue@oss.nxp.com"},"content":"Hi Jacopo,\n\nOn 11/9/22 08:43, Jacopo Mondi wrote:\n> HI Xavier,\n> sorry for the delay!\n> \n> On Mon, Oct 10, 2022 at 03:17:43PM +0200, Xavier Roumegue (OSS) wrote:\n>> From: Xavier Roumegue \n>>\n>> Declare a converter Abstract Base Class intended to provide generic\n>> interfaces to hardware offering size and format conversion services on\n>> streams. This is mainly based on the public interfaces of the current\n>> converter class implementation found in the simple pipeline handler.\n>>\n>> The main change is the introduction of loadConfiguration() function\n>> which can be used by the concrete implementation to load hardware\n>> specific runtime parameters defined by the application.\n>>\n>> Signed-off-by: Xavier Roumegue \n>> ---\n>> include/libcamera/internal/converter.h | 108 ++++++++\n>> include/libcamera/internal/meson.build | 1 +\n>> src/libcamera/converter.cpp | 337 +++++++++++++++++++++++++\n>> src/libcamera/meson.build | 1 +\n>> 4 files changed, 447 insertions(+)\n>> create mode 100644 include/libcamera/internal/converter.h\n>> create mode 100644 src/libcamera/converter.cpp\n>>\n>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>> new file mode 100644\n>> index 00000000..38c05ee9\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/converter.h\n>> @@ -0,0 +1,108 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Laurent Pinchart\n>> + * Copyright 2022 NXP\n>> + *\n>> + * converter.h - Generic stream converter infrastructure\n> \n> Please align this with the .cpp one. Pick the one that you prefer\n> \n> * converter.h - Generic stream converter infrastructure\n> * converter.cpp - Generic Format converter interface\n> \n> \n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +#include \n> \n> #include \n> \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +class FrameBuffer;\n>> +class MediaDevice;\n>> +class Size;\n>> +class SizeRange;\n>> +struct StreamConfiguration;\n>> +\n>> +class Converter\n>> +{\n>> +public:\n>> +\tConverter(MediaDevice *media);\n>> +\tvirtual ~Converter();\n>> +\n>> +\tvirtual int loadConfiguration(const std::string &filename) = 0;\n>> +\n>> +\tvirtual bool isValid() const = 0;\n>> +\n>> +\tvirtual std::vector formats(PixelFormat input) = 0;\n>> +\tvirtual SizeRange sizes(const Size &input) = 0;\n>> +\n>> +\tvirtual std::tuple\n>> +\tstrideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;\n>> +\n>> +\tvirtual int configure(const StreamConfiguration &inputCfg,\n>> +\t\t\t const std::vector> &outputCfg) = 0;\n> \n> I wonder if using would be easier than\n> reference_wrapper, but I'll find out in the next patch I presume\n> \n>> +\tvirtual int exportBuffers(unsigned int ouput, unsigned int count,\n>> +\t\t\t\t std::vector> *buffers) = 0;\n>> +\n>> +\tvirtual int start() = 0;\n>> +\tvirtual void stop() = 0;\n>> +\n>> +\tvirtual int queueBuffers(FrameBuffer *input,\n>> +\t\t\t\t const std::map &outputs) = 0;\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 aliases);\n>> +\tvirtual ~ConverterFactoryBase() = default;\n>> +\n>> +\tconst std::vector &aliases() const { return aliases_; }\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 aliases_;\n>> +};\n>> +\n>> +template\n>> +class ConverterFactory : public ConverterFactoryBase\n>> +{\n>> +public:\n>> +\tConverterFactory(const char *name, std::initializer_list aliases)\n>> +\t\t: ConverterFactoryBase(name, aliases)\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, ...) \\\n>> +\tstatic ConverterFactory global_##converter##Factory(name, { __VA_ARGS__ });\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> index 7a780d48..8f50d755 100644\n>> --- a/include/libcamera/internal/meson.build\n>> +++ b/include/libcamera/internal/meson.build\n>> @@ -19,6 +19,7 @@ libcamera_internal_headers = files([\n>> 'camera_sensor_properties.h',\n>> 'control_serializer.h',\n>> 'control_validator.h',\n>> + 'converter.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>> new file mode 100644\n>> index 00000000..5e444f5f\n>> --- /dev/null\n>> +++ b/src/libcamera/converter.cpp\n> \n> I get quite a few Doxygen warnings on this class\n> \n> src/libcamera/converter.cpp:328: warning: argument 'aliases' of command @param is not found in the argument list of REGISTER_CONVERTER(name, converter,...)\n> src/libcamera/converter.cpp:113: warning: argument 'output' of command @param is not found in the argument list of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0\n> include/libcamera/internal/converter.h:49: warning: The following parameter of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0 is not documented:\n> parameter 'ouput'\n> src/libcamera/converter.cpp:139: warning: argument 'buffers' of command @param is not found in the argument list of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0\n> include/libcamera/internal/converter.h:55: warning: The following parameter of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0 is not documented:\n> parameter 'outputs'\n> include/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\n> include/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\n> include/libcamera/internal/converter.h:70: warning: The following parameter of libcamera::ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list< std::string > aliases) is not documented:\n> parameter 'aliases'\n> src/libcamera/converter.cpp:199: warning: argument 'name' of command @param is not found in the argument list of libcamera::ConverterFactoryBase::create(MediaDevice *media)\n> include/libcamera/internal/converter.h:75: warning: The following parameter of libcamera::ConverterFactoryBase::create(MediaDevice *media) is not documented:\n> parameter 'media'\n> \n> \n> With the next patch applied the list increases:\n> \n> \n> src/libcamera/converter.cpp:328: warning: argument 'aliases' of command @param is not found in the argument list of REGISTER_CONVERTER(name, converter,...)\n> src/libcamera/converter.cpp:113: warning: argument 'output' of command @param is not found in the argument list of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0\n> include/libcamera/internal/converter.h:49: warning: The following parameter of libcamera::Converter::exportBuffers(unsigned int ouput, unsigned int count, std::vector< std::unique_ptr< FrameBuffer >> *buffers)=0 is not documented:\n> parameter 'ouput'\n> src/libcamera/converter.cpp:139: warning: argument 'buffers' of command @param is not found in the argument list of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0\n> include/libcamera/internal/converter.h:55: warning: The following parameter of libcamera::Converter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)=0 is not documented:\n> parameter 'outputs'\n> include/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\n> include/libcamera/internal/converter.h:73: warning: Member aliases() const (function) of class libcamera::ConverterFactoryBase is not documented.\n> include/libcamera/internal/converter.h:70: warning: The following parameter of libcamera::ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list< std::string > aliases) is not documented:\n> parameter 'aliases'\n> src/libcamera/converter.cpp:199: warning: argument 'name' of command @param is not found in the argument list of libcamera::ConverterFactoryBase::create(MediaDevice *media)\n> include/libcamera/internal/converter.h:75: warning: The following parameter of libcamera::ConverterFactoryBase::create(MediaDevice *media) is not documented:\n> parameter 'media'\n> src/libcamera/converter/converter_v4l2_m2m.cpp:323: warning: argument 'outputCfg' of command @param is not found in the argument list of libcamera::V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, const std::vector< std::reference_wrapper< StreamConfiguration >> &outputCfgs)\n> include/libcamera/internal/converter/converter_v4l2_m2m.h:48: warning: The following parameter of libcamera::V4L2M2MConverter::configure(const StreamConfiguration &inputCfg, const std::vector< std::reference_wrapper< StreamConfiguration >> &outputCfgs) is not documented:\n> parameter 'outputCfgs'\n> src/libcamera/converter/converter_v4l2_m2m.cpp:217: warning: argument 'filename' of command @param is not found in the argument list of libcamera::V4L2M2MConverter::formats(PixelFormat input)\n> src/libcamera/converter/converter_v4l2_m2m.cpp:396: warning: argument 'buffers' of command @param is not found in the argument list of libcamera::V4L2M2MConverter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs)\n> include/libcamera/internal/converter/converter_v4l2_m2m.h:56: warning: The following parameter of libcamera::V4L2M2MConverter::queueBuffers(FrameBuffer *input, const std::map< unsigned int, FrameBuffer * > &outputs) is not documented: parameter 'outputs'\n> \n> Could you make sure Doxygen doesn't report any warning ?\nFixed in v2.\n> \n>> @@ -0,0 +1,337 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright 2022 NXP\n>> + *\n>> + * converter.cpp - Generic Format converter interface\n>> + */\n>> +\n>> +#include \"libcamera/internal/converter.h\"\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/media_device.h\"\n>> +\n>> +/**\n>> + * \\file internal/converter.h\n>> + * \\brief Abstract converter\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(Converter)\n>> +\n>> +/**\n>> + * \\class Converter\n>> + * \\brief Abstract Base Class for converter\n>> + *\n>> + * The Converter class is an Abstract Base Class defining the interfaces of\n>> + * converter implementations.\n>> + *\n>> + * Converters offers scaling and pixel formats conversion services on a input\n> \n> Converters -> offer\n> \n> I would also use \"pixel format\" in singular form, but I can't tell if\n> that's more correct or not.\n> \n> \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 Converter instance\n>> + * \\param[in] media The media device implementing the converter\n>> + *\n>> + * This seeks for the entity implementing data streaming function in the media\n>> + * graph entities and use its device node as converter device node.\n>> + */\n>> +Converter::Converter(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>> +Converter::~Converter()\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\fn Converter::loadConfiguration()\n>> + * \\brief Load converter configuration from file\n>> + * \\param[in] filename The file name path\n>> + *\n>> + * Load converter dependent configuration parameters to apply on the hardware.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::isValid()\n>> + * \\brief Check if the converter configuration is valid\n>> + * \\return True is the converter is valid, false otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::formats()\n>> + * \\brief Retrieve the list of supported pixel formats for an input pixel format\n>> + * \\param[in] input Input pixel format to retrieve output pixel format list for\n>> + * \\return The list of output supported pixel formats\n> \n> I would swap \"output supported\" to \"supported output\"\n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::sizes()\n>> + * \\brief Retrieve the output range of minimum and maximum sizes for an input size\n> \n> Here too: \"range of minimum and maximum output sizes\"\n> \n>> + * \\param[in] input Input stream size to retrieve range for\n>> + * \\return A range of output image sizes\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::strideAndFrameSize()\n>> + * \\brief Retrieve the output stride and frame size for an input configutation\n>> + * \\param[in] pixelFormat Input stream pixel format\n>> + * \\param[in] size Input stream size\n>> + * \\return A tuple indicating the stride and frame size or an empty tuple on error\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::configure()\n>> + * \\brief Configure a set of output stream conversion from an input stream\n>> + * \\param[in] inputCfg Input stream configuration\n>> + * \\param[in] outputCfg A list of output stream configurations\n> \n> Is this an [out] parameter ?\nYes, changed in v2\n> \n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::exportBuffers()\n>> + * \\brief Export buffers from the converter device\n>> + * \\param[in] output Output stream index exporting the buffers\n>> + * \\param[in] count Number of buffers to allocate\n>> + * \\param[out] buffers Vector to store allocated buffers\n>> + *\n>> + * This function operates similarly as V4L2VideoDevice::exportBuffers() on the\n>> + * output stream indicated by the \\a output index.\n>> + *\n>> + * \\return The number of allocated buffers on success or a negative error code\n>> + * otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::start()\n>> + * \\brief Start the converter streaming operation\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::stop()\n>> + * \\brief Stop the converter streaming operation\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\fn Converter::queueBuffers()\n>> + * \\brief Queue buffers to converter device\n>> + * \\param[in] input The frame buffer to apply the conversion\n>> + * \\param[out] buffers The container holding the output stream indexes and\n>> + * their respective frame buffer outputs.\n>> + *\n>> + * This function queues the \\a input frame buffer on the output streams of the\n>> + * \\a buffers map key and retrieve the output frame buffer indicated by the\n>> + * \\a buffer map value.\n> \n> \"buffer\" is not an argument. Maybe \"buffers\".\n> \n> I'll better understand how the output map is populated in the next\n> patch\n> \n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +\n>> +/**\n>> + * \\var Converter::inputBufferReady\n>> + * \\brief A signal emitted when the input frame buffer completes\n>> + */\n>> +\n>> +/**\n>> + * \\var Converter::outputBufferReady\n>> + * \\brief A signal emitted when the output frame buffer completes\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>> + *\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.\n> \n> Please don't break lines in paragraphs.\n> \n> Either\n> One line.\n> \n> With a line break in between.\n> \n> Or\n> One sentence. And the paragraph continues without breaking.\n> \n>> + * If the converter implemententation fully relies on a generic framework, the\n>> + * name should be the same as the framework.\n>> + * Otherwise, if the implementation is specialized, the factory name should match\n>> + * the driver name implementing the function.\n>> + * The factory \\a aliases 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 aliases)\n>> +\t: name_(name), aliases_(aliases)\n>> +{\n>> +\tregisterType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create an instance of the converter corresponding to a named factory\n>> + * \\param[in] name Name of the factory\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\tstd::vector aliases = factory->aliases();\n> \n> &aliases\n> \n>> +\t\tauto it = std::find(aliases.begin(), aliases.end(), media->driver());\n>> +\n>> +\t\tif (it == aliases.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 == aliases.end() ? \"no\" : media->driver()) << \" alias.\";\n>> +\n>> +\t\treturn factory->createInstance(media);\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> What is this function for ? Return the names of all converters\n> registered in the system ?\nYes.\n\n Am I missing where it is used ?\nCurrently not used but will be used in next series involving rkisp1 pipeline.\nPipeline handlers can retrieve a converter available in the system without \nhardcoding its name.\n> \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->aliases())\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::aliases_\n>> + * \\brief The list holding the factory aliases\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 aliases)\n>> + * \\brief Construct a converter factory\n>> + * \\param[in] name Name of the converter class\n>> + * \\param[in] aliases Aliases of the converter class\n>> + *\n>> + * Creating an instance of the factory 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.\n>> + * If the converter implemententation fully relies on a generic framework, the\n>> + * name should be the same as the framework.\n>> + * Otherwise, if the implementation is specialized, the factory name should match\n>> + * the driver name implementing the function.\n>> + * The factory \\a aliases holds a list of driver names implementing a generic\n>> + * subsystem without any personalizations.\n> \n> Same comment as above.\n> Also, you can use \\copydoc\n> \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] aliases Optional list of alias 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/meson.build b/src/libcamera/meson.build\n>> index 7fcbb2dd..3a9fc31f 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -13,6 +13,7 @@ libcamera_sources = files([\n>> 'controls.cpp',\n>> 'control_serializer.cpp',\n>> 'control_validator.cpp',\n>> + 'converter.cpp',\n>> 'delayed_controls.cpp',\n>> 'device_enumerator.cpp',\n>> 'device_enumerator_sysfs.cpp',\n>> --\n>> 2.37.3\n>>\nRegards,\n Xavier","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 501C2BD16B\n\tfor ;\n\tTue, 15 Nov 2022 09:51:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B51BC6307F;\n\tTue, 15 Nov 2022 10:51:53 +0100 (CET)","from EUR01-HE1-obe.outbound.protection.outlook.com\n\t(mail-he1eur01on0606.outbound.protection.outlook.com\n\t[IPv6:2a01:111:f400:fe1e::606])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A38B563079\n\tfor ;\n\tTue, 15 Nov 2022 10:51:51 +0100 (CET)","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)\n\tby DB8PR04MB7161.eurprd04.prod.outlook.com (2603:10a6:10:124::7) with\n\tMicrosoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5813.13;\n\tTue, 15 Nov 2022 09:51:47 +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%3]) with mapi id 15.20.5813.018;\n\tTue, 15 Nov 2022 09:51:47 +0000"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668505913;\n\tbh=XTzwgGZyrSb38rhSrPd4SmWBqePYZ9sA4fAEQZ1jjgc=;\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=FKE+g+LYFKZwg4U3cIwEzcl0ljVdQ5N//fyoeFPEebOBnlo4lu6TKyntGCfoNSKHa\n\tU7MDvqSdyaRiQbFw/J9YFuq97+t+UqR8/sHRmWs4ZbZsKXj9zzhKwx4/Pj2yob/I7J\n\tuDdQITrbXgpeUdJNKV2APN1OF7ct9JRrXmm00hMAKl7ps5nj5JjjKVBtdYLf9yDAa6\n\tqbSqXsbs4FYEDaMosMt3/OzIBWG1/BjCpneru7wTLFrW2yo7WPtYoQcgodV+y+wfNQ\n\tnM6JhW0fL8z7BeXUKTxTTLeYTGKjYb10Ym+WQqTCrKVuZSTtUESpKbdEwOwJaxPfEG\n\tofTlwFd7WATFA==","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=UfeXDax3vP3UoswjHXNG9bf0DjqK/at8/fmM7kY6yiM=;\n\tb=S1l8431Z/8oE35qd2bvKzfKPWnk42AcGpD8aj7FotfBJSHfwAiKlt5O3MHDpSx/IEtxJmQZcY6EfAgJZbJCdWsHiIGRhEGgcVFaML/TB9uCgu2QsGwJ8CeTvlfr4C/J8TLtjj82u0uiayOcWhooSs+VlVov73TWRxBRdi9dFIqM="],"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=\"S1l8431Z\"; 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=CkQHAI1Nn1fMAS+IQCDoR4ijE479Dd5AEha+Ys1N6bFHdmx+74PUytWUKpi+iSJBKKVTJSnYeTVs6HN5SQqDDxHoZwzG63Hzp9HNpY/4/63jBFJCbz77OMgyHMOobtjcMLiNXO1Ay7hvB4xNnRONeO/18DtlMW3EAXWd0YVTLjDml0/4/njswRpHmLeD72v812lc93MPLCcbKP5u16YMDt+QRmy5aFagmO7tY4kQNFNV+DJH102ybTBcNwK6JeSef1fMfqALoZ0dvdPujQYwkoUCYhkmaqm27vJkJcPJwqUqfkK8pHzAyO/2VEaIMo+ndVyHhxDvXcWJ9dzDGwylxg==","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=UfeXDax3vP3UoswjHXNG9bf0DjqK/at8/fmM7kY6yiM=;\n\tb=LlpgIyMi0VQDUgiR2kPdFTGWk55Dmqt28j/nm7qHxwUFuq7Zox5EgmcqsusD7tcKXx/ArbhT+H1NFGSGAXA33HfZ2xBcjopsiqO++b2VMXcHCavS3Vk7W/QD1jNxs2OxSRppj72w9Zc1G4/JdIOfRlG6ct3wWcDzWaK/RsrRgZPpIJ6Lu8svwi2lye1RqtNNs9DjXdkO6K19qk2LgaNTS6WZzo6P8oqpP1neIbB23iCFHO3Um4Gu8D/aMTvT0QkwIMW2FoUhDsaHDLpDCdYZalEZLDuTrD6/f1cvxO2pMDiQU0PnMDol4TeviOvnyB1hDnzq4m7O8z+Lg285suDi+Q==","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":"<33df6e7d-6a3f-89b7-f0de-19eafb55c847@oss.nxp.com>","Date":"Tue, 15 Nov 2022 10:51:36 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.4.1","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20221010131744.513261-1-xavier.roumegue@oss.nxp.com>\n\t<20221010131744.513261-2-xavier.roumegue@oss.nxp.com>\n\t<20221109074317.z72yxczzzui26ol2@uno.localdomain>","In-Reply-To":"<20221109074317.z72yxczzzui26ol2@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-ClientProxiedBy":"SJ0PR03CA0199.namprd03.prod.outlook.com\n\t(2603:10b6:a03:2ef::24) 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_|DB8PR04MB7161:EE_","X-MS-Office365-Filtering-Correlation-Id":"a557b055-403c-4565-7b64-08dac6ef0272","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":"q2q0ijTJz8q/HhZMUoCZo8W7kWkRvvrF4DXW70b1MliBjfL06oGoCi8UWCUeJETgcGqZiXPh3g0se4Cxw3+7t0rfvf4U9ppSJM+D13B0WJErfk6H24E2/wqlQUV0CuQjZRyQeZdybIwdybkwHWJPow+2FiDWrqN9qwIOWNcQHJMPTX6/npgCz34nLxO/AimHHKQbKHC4TZ0u5fvN0Qt0SY60TOZ+kBv8qthiLxsK6lbREFy1lQglpxnJWzn5AKqmYMdNf2QpWeB6RVRSuYbuPryacVXAb7nCir9DwiMknD3P3E4u13Nr465gBGOU7EjDNywzTDTgg/R1T9PJrQqSCAdF2bWMUlT1Mz1XiZxN3Mctji1PbgGgLSk/xTM5cezIvd6A6Z+vkAxWmS1VIA+Z+l5owFLY7EJ1KVdP6MdQvRsap3IwnzOIigIznogTl/JUvtkwkjGb99IM3QB7hDHUHnAvXD7sisVEK+rEnpPtADWIts5XKAixYqG3LOgKvxobBb+2fpuTdn81Kyvirs69HnKMvQQeYvwsOuSyxSdTLeXSwUhAd6PDo4tQrVqkjiid7F434MeSpLn8yeS1s0Vwwm7gKVXJZKZ7PXWx6IuzrajVkbSZfQU32b7ifXuqZ35IJQDrcSan3k5lEcKugKxV0XzgZWcRvnBHI2iJNgIu6mB+7vItl8fF2n2tYRh0a5ZJJud5y00pHo3shIwP20IMsh/97InK2oRILf4tJa0aFoZSi4yY7mT2kV+TFHppCt8zTrK6DNFsXkGCAp0AFgHXvzb3mBWCDck3ErFf5DKxdkg58lY90TNWz7rLE/c+txZ2eiaa487vO2XFft1aKCY+F2olj8+XVRG3rRqgZl9A+eM=","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)(136003)(396003)(39860400002)(366004)(376002)(346002)(451199015)(83380400001)(38100700002)(38350700002)(86362001)(31696002)(30864003)(5660300002)(8936002)(2906002)(66476007)(8676002)(66946007)(66556008)(4326008)(41300700001)(6512007)(6506007)(2616005)(26005)(52116002)(53546011)(186003)(55236004)(316002)(6666004)(6916009)(478600001)(31686004)(66899015)(6486002)(41533002)(43740500002)(45980500001);\n\tDIR:OUT; SFP:1101; ","X-MS-Exchange-AntiSpam-MessageData-ChunkCount":"1","X-MS-Exchange-AntiSpam-MessageData-0":"=?utf-8?q?p6IVlQHONCBd+wnXb7Fd+Aq0E?=\n\t=?utf-8?q?2U6E9FTskhDX9s/cyRthlK9DmPCQol4M5j7EIa24Au9cFk2aSe3UAqes?=\n\t=?utf-8?q?CnGKw+48qi3bjqaCry01uPqrb1JhSPepcfVaGiCfTiX6OEl+ENiISnWQ?=\n\t=?utf-8?q?QL/GWYSqnsx55f+lVXhr+TF55H6QK9AtNEMfLDw3RL5d0UIXLv7P5HcR?=\n\t=?utf-8?q?yZ8Q148q8r+g5Sz6gbipw9T8GlewvLC3GJIECm0jMi2KRSW2DZYQ1QXm?=\n\t=?utf-8?q?Pici1Dfl09fkeNgiV4ZAzZsA87ATb6lWMRWJoKQ8bXLXnorpGXwp+gU0?=\n\t=?utf-8?q?YRjAA4YcXeyVZATCsGMPi+GQuswT74rrKQeMsNkXy5f6NRyeNpw5lhCn?=\n\t=?utf-8?q?By2xhv+ygmvP2DntrQ5YY5ahJaK4i42AqStZCWc5SZ3NA57DD/4zhdV+?=\n\t=?utf-8?q?JVYhvp8W+O6oyufTLbHIqfdvJyRiP+6v+teF8DZFLieH4pe8SlVTNHeH?=\n\t=?utf-8?q?APqOJshTFYAbarMlGgEXkj3ficz4s3adRTepOWSC1erVuxdxqWPdA1jv?=\n\t=?utf-8?q?aZcF6E22+1EDjE91Nw+p6CP++KUsEyC/5CrhMijnpDq+SvftGCvNMj0t?=\n\t=?utf-8?q?IR5pZQyVYiDHO0bnhgTc0P0w7fdLUec8JA5MgQIQLGv0b8+r94a6cBUO?=\n\t=?utf-8?q?tp+LWszIxEyZANINrtHjZtihvC3UKgEYxqqQt67c7EL9WIbvKowzWo47?=\n\t=?utf-8?q?bZpfhzBJrm/GU1lQgYjqHg0TPU6I6UiD7dDBrRFcwwIyEoEW2SwWZNch?=\n\t=?utf-8?q?SBfvQYh2B4LkjDkaEST6SwNrHjmZstoVQ70JcE561XRy7u20b2YUoxL0?=\n\t=?utf-8?q?as/yNLxAWCxXkEFxCXnwmGI2T5NufrVhoPoDisuSIgTsZYFttumdtasv?=\n\t=?utf-8?q?xA3BucLH9D/yJ1E9mjose7CHalyO1E07u7eDhxRHlc+8xDtpcOhS7QMM?=\n\t=?utf-8?q?hFazZHscn43WHHjVGAsN547P1xDP3YNNl5GnybXIbPcWBc4wLK4C4XWW?=\n\t=?utf-8?q?KO9+USNDHhVXNhad8T0awpxdx8VGn8h3xLwtW9PpIIJlWZ2zhQjmvKXr?=\n\t=?utf-8?q?m54yTU/rZ2udii8WUeR7XEKUs8Hc7iYimTYNDbhj3dWP2yeqdItKyrJQ?=\n\t=?utf-8?q?u3afteMQY6kqqMm2iowfSUN1rD6lgLDB0jB2gQzqy7uipIiqXqzTawGY?=\n\t=?utf-8?q?vasWjhW7pmz/1gL8oq5002FIbfv4191NmpoICbTtJAOm9MIRp2b4TdB1?=\n\t=?utf-8?q?UwyBO4cLrqL1nNuhP9pift2m++FHx2i7OlWmKR293XVjXmalDvfHpXFC?=\n\t=?utf-8?q?8MTqaEjI/e08omaQAC0mrBz5Z1UHzRXboG8ahddvSy8U/m0hwaL4zVCW?=\n\t=?utf-8?q?ETSwQWICPyhtNRp1iZN2WA0Q4INXUQdYjSQRKE+4IQ22Mlq0gWM3ywmE?=\n\t=?utf-8?q?Bjkcpl7sriHuUAOtBp6fUSvAt8qROYUVpMyi4yM7IA7EP9+9zccLCI/V?=\n\t=?utf-8?q?SnG6LMUHEOB3W90DXXJDxNisyJcmDZKfA9GYRFGi0QSuoA2FFxOF2a4e?=\n\t=?utf-8?q?CbssW7dZz7xrH7QHt8VkF5t55FRXPieclesXh3/BbfWs/Zy4p4D/AUtL?=\n\t=?utf-8?q?ghwFQKgT35aKe/pUWxYs/0IHn/yppZld7toaqvH+D/FG6nyeERhpwbUI?=\n\t=?utf-8?q?ReRSuoJ4K0fi8aQNz7O5kmjSNMDXg=3D=3D?=","X-OriginatorOrg":"oss.nxp.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"a557b055-403c-4565-7b64-08dac6ef0272","X-MS-Exchange-CrossTenant-AuthSource":"PAXPR04MB8703.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-OriginalArrivalTime":"15 Nov 2022 09:51:47.6117\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":"9dqehilheln7rqr8u46J72fKYVuPBprRNDUt1pORhJBR5Xlmx+Ht664QPDvnqcZOwkFdAhG9NqzPtSHIegR+uQ==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"DB8PR04MB7161","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: Declare generic\n\tconverter 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\\)\" ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]