[{"id":24985,"web_url":"https://patchwork.libcamera.org/comment/24985/","msgid":"<20220914104604.qbggiupgdvlrwc2q@uno.localdomain>","date":"2022-09-14T10:46:04","subject":"Re: [libcamera-devel] [PATCH 04/14] libcamera: Declare generic size\n\tand format converter interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Xavier\n I would drop \"size and format\" and use \"Converter\" in the subject\ntitle.\n\nOn Thu, Sep 08, 2022 at 08:48:40PM +0200, Xavier Roumegue via libcamera-devel wrote:\n> Declare a converter Abstract Base Class intented 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 load_configuration() method which\n> can be used by the concrete implementation to load hardware specific\n> runtime parameters defined by the application.\n>\n> Signed-off-by: Xavier Roumegue \n> ---\n> include/libcamera/internal/converter.h | 101 ++++++++++++++++++++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/converter.cpp | 102 +++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 205 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..e2237c57\n> --- /dev/null\n> +++ b/include/libcamera/internal/converter.h\n> @@ -0,0 +1,101 @@\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> +\n> +#pragma once\n> +\n> +#include \n\nI might have missed what is functional included for\n\n> +#include \n> +#include \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\nEmpty line maybe ?\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> +\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> +\tstd::string deviceNode_;\n> +\tSignal inputBufferReady;\n> +\tSignal outputBufferReady;\n> +};\n> +\n> +class ConverterFactory\n> +{\n> +public:\n> +\tConverterFactory(const std::string name);\n> +\tvirtual ~ConverterFactory() = default;\n> +\n> +\tstatic std::unique_ptr create(MediaDevice *media);\n> +\n> +\tstatic void registerType(ConverterFactory *factory);\n> +\tstatic std::vector &factories();\n> +\tstatic std::vector names();\n> +\n> +protected:\n> +\tvirtual Converter *createInstance(MediaDevice *media) = 0;\n> +\tvirtual const std::vector aliases() const = 0;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactory)\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CONVERTER(name, converter, ...) \\\n> +\tclass converter##Factory final : public ConverterFactory \\\n> +\t{ \\\n> +\tpublic: \\\n> +\t\tconverter##Factory() : ConverterFactory(name) {} \\\n> + \\\n> +\tprivate: \\\n> +\t\tConverter *createInstance(MediaDevice *media) \\\n> +\t\t{ \\\n> +\t\t\treturn new converter(media); \\\n> +\t\t} \\\n> +\t\tstd::vector aliases_ = { __VA_ARGS__ }; \\\n> +\t\tconst std::vector aliases() const { return aliases_; } \\\n> +\t}; \\\n> +\tstatic converter##Factory global_##converter##Factory;\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..89a594d1\n> --- /dev/null\n> +++ b/src/libcamera/converter.cpp\n> @@ -0,0 +1,102 @@\n> +\n\nRogue empty line\n\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 \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/converter.h\"\n> +#include \"libcamera/internal/media_device.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Converter)\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\treturn;\n\nShould we complain loud and set some state variable that can be\ninspected by the isValid() function already ?\n\n> +\n> +\tdeviceNode_ = (*it)->deviceNode();\n\nI'm not sure storing a string here is any useful if not fo debugging\npurposes. What if you store a pointer to the entity instead, so\nderived classes can use the static fromMediaEntity() function (which\nprobably have to be added to the M2M device) to open the device node ?\n\n> +}\n> +\n> +Converter::~Converter()\n> +{\n> +}\n> +\n> +ConverterFactory::ConverterFactory(const std::string name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +std::unique_ptr ConverterFactory::create(MediaDevice *media)\n> +{\n> +\tstd::vector &factories =\n> +\t\tConverterFactory::factories();\n\nOh, this is different from the other favtories we have.\nThe usage here is:\n\n ConverterFavtory::create()\n for (factory: factories())\n factory->create();\n\nWhile other factories (looking at the pipeline handler factory) have a\nstatic method to retrieve factories, and then call create() on them.\n\nMy understanding is that this shouldn't change much in regard to the\nlink order protection realized by declaring factories_ inside the\nfactories() function, just pointing it out to see if someone else see\nan issue here.\n\n> +\n> +\tfor (ConverterFactory *factory : factories) {\n> +\t\tstd::vector aliases = factory->aliases();\n> +\t\tauto it = std::find(aliases.begin(), aliases.end(), media->driver());\n\nHow do you envision aliases to be used ? I mean, do you expect a\nConverter instance to handle different devices ? Probably yes, as a\ngeneric M2M converter will follow in the series and could handle\nmultiple devices...\n\n> +\n> +\t\tif (it == aliases.end() && media->driver() != factory->name_)\n\nwhat is \"media->driver() != factory->name_)\" for ? Shouldn't the media\ndriver name be part of aliases only ?\n\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\nHow long is this message ? Can it be shortened or the factory name\nhere is relevant ?\n\n> +\n> +\t\tConverter *converter = factory->createInstance(media);\n> +\t\treturn std::unique_ptr(converter);\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +void ConverterFactory::registerType(ConverterFactory *factory)\n> +{\n> +\tstd::vector &factories =\n> +\t\tConverterFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +std::vector ConverterFactory::names()\n> +{\n> +\tstd::vector list;\n> +\n> +\tstd::vector &factories =\n> +\t\tConverterFactory::factories();\n> +\n> +\tfor (ConverterFactory *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> +std::vector &ConverterFactory::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> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 63b47b17..a261d4b4 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 AE72AC0DA4\n\tfor ;\n\tWed, 14 Sep 2022 10:46:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A57F61FA5;\n\tWed, 14 Sep 2022 12:46:10 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::226])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EFCBE6099F\n\tfor ;\n\tWed, 14 Sep 2022 12:46:07 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2CC14C0012;\n\tWed, 14 Sep 2022 10:46:06 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663152370;\n\tbh=4CfwUsilLxmevaJV0f2DGYuaI7rALG0pOBqI6M4+9TQ=;\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=fIhmm7W4U9gODyKoZmSycjaWSDnj3EoUYBNtqA+6WKQE3QerhTtblhTS3RGBTbMao\n\tiW5tKvrA3gnt+qHb87frBcR5mIrm6HkjexH/rLykHxGuDWRHONJMCbzDuL2lrk9sIc\n\tbqpwQKCXyDo5LyPbGMzHm/8QB48kvjeGZNo8favCL370Auo4vTybTGwb3d7gJvKoLp\n\tGlAYs/gRJtMdvwqEa0GQyWPPJZqk0tdoPj7Y6/anzVRgbbcG7AZsaoA7LqVAEhS9hJ\n\tr84W4CZYgjCIDlkrYfFb0FrAU2HWz9KB5mqUQcMnwzfTFjbZKUslxAWIZqPGWnSVa6\n\t4mJ2IyHjCCqIw==","Date":"Wed, 14 Sep 2022 12:46:04 +0200","To":"Xavier Roumegue ","Message-ID":"<20220914104604.qbggiupgdvlrwc2q@uno.localdomain>","References":"<20220908184850.1874303-1-xavier.roumegue@oss.nxp.com>\n\t<20220908184850.1874303-5-xavier.roumegue@oss.nxp.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220908184850.1874303-5-xavier.roumegue@oss.nxp.com>","Subject":"Re: [libcamera-devel] [PATCH 04/14] libcamera: Declare generic size\n\tand format converter interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"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":24996,"web_url":"https://patchwork.libcamera.org/comment/24996/","msgid":"<63051502-e2f7-d568-b8dd-0dd8f0602478@oss.nxp.com>","date":"2022-09-18T08:39:32","subject":"Re: [libcamera-devel] [PATCH 04/14] libcamera: Declare generic size\n\tand format converter 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 9/14/22 12:46, Jacopo Mondi wrote:\n> Hi Xavier\n> I would drop \"size and format\" and use \"Converter\" in the subject\n> title.\n> \n> On Thu, Sep 08, 2022 at 08:48:40PM +0200, Xavier Roumegue via libcamera-devel wrote:\n>> Declare a converter Abstract Base Class intented 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 load_configuration() method which\n>> can be used by the concrete implementation to load hardware specific\n>> runtime parameters defined by the application.\n>>\n>> Signed-off-by: Xavier Roumegue \n>> ---\n>> include/libcamera/internal/converter.h | 101 ++++++++++++++++++++++++\n>> include/libcamera/internal/meson.build | 1 +\n>> src/libcamera/converter.cpp | 102 +++++++++++++++++++++++++\n>> src/libcamera/meson.build | 1 +\n>> 4 files changed, 205 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..e2237c57\n>> --- /dev/null\n>> +++ b/include/libcamera/internal/converter.h\n>> @@ -0,0 +1,101 @@\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>> +\n>> +#pragma once\n>> +\n>> +#include \n> \n> I might have missed what is functional included for\nRight, I will remove it.\n> \n>> +#include \n>> +#include \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> Empty line maybe ?\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>> +\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>> +\tstd::string deviceNode_;\n>> +\tSignal inputBufferReady;\n>> +\tSignal outputBufferReady;\n>> +};\n>> +\n>> +class ConverterFactory\n>> +{\n>> +public:\n>> +\tConverterFactory(const std::string name);\n>> +\tvirtual ~ConverterFactory() = default;\n>> +\n>> +\tstatic std::unique_ptr create(MediaDevice *media);\n>> +\n>> +\tstatic void registerType(ConverterFactory *factory);\n>> +\tstatic std::vector &factories();\n>> +\tstatic std::vector names();\n>> +\n>> +protected:\n>> +\tvirtual Converter *createInstance(MediaDevice *media) = 0;\n>> +\tvirtual const std::vector aliases() const = 0;\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactory)\n>> +\n>> +\tstd::string name_;\n>> +};\n>> +\n>> +#define REGISTER_CONVERTER(name, converter, ...) \\\n>> +\tclass converter##Factory final : public ConverterFactory \\\n>> +\t{ \\\n>> +\tpublic: \\\n>> +\t\tconverter##Factory() : ConverterFactory(name) {} \\\n>> + \\\n>> +\tprivate: \\\n>> +\t\tConverter *createInstance(MediaDevice *media) \\\n>> +\t\t{ \\\n>> +\t\t\treturn new converter(media); \\\n>> +\t\t} \\\n>> +\t\tstd::vector aliases_ = { __VA_ARGS__ }; \\\n>> +\t\tconst std::vector aliases() const { return aliases_; } \\\n>> +\t}; \\\n>> +\tstatic converter##Factory global_##converter##Factory;\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..89a594d1\n>> --- /dev/null\n>> +++ b/src/libcamera/converter.cpp\n>> @@ -0,0 +1,102 @@\n>> +\n> \n> Rogue empty line\n> \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 \n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/converter.h\"\n>> +#include \"libcamera/internal/media_device.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(Converter)\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\treturn;\n> \n> Should we complain loud and set some state variable that can be\n> inspected by the isValid() function already ?\nComplaining loud can not hurt. An empty deviceNode_ string can be used \nto indicate the instance is invalid in case the concrete class relies on \na device node. Adding a kind of isValid boolean attribute would likely \nbe redundant with isValid().\n> \n>> +\n>> +\tdeviceNode_ = (*it)->deviceNode();\n> \n> I'm not sure storing a string here is any useful if not fo debugging\n> purposes. What if you store a pointer to the entity instead, so\n> derived classes can use the static fromMediaEntity() function (which\n> probably have to be added to the M2M device) to open the device node ?\nThe ambition of this abstract class is to be framework/hardware \nagnostic. For instance, the converter could sit on top on a DRM device, \nor any hardware compute backend (dsp, cpu, etc..) to implement the \ninterface.\nHence, the design choice to not embed specialized framework in the \nabstract class except in the constructor.\n> \n>> +}\n>> +\n>> +Converter::~Converter()\n>> +{\n>> +}\n>> +\n>> +ConverterFactory::ConverterFactory(const std::string name)\n>> +\t: name_(name)\n>> +{\n>> +\tregisterType(this);\n>> +}\n>> +\n>> +std::unique_ptr ConverterFactory::create(MediaDevice *media)\n>> +{\n>> +\tstd::vector &factories =\n>> +\t\tConverterFactory::factories();\n> \n> Oh, this is different from the other favtories we have.\n> The usage here is:\n> \n> ConverterFavtory::create()\n> for (factory: factories())\n> factory->create();\n> \n> While other factories (looking at the pipeline handler factory) have a\n> static method to retrieve factories, and then call create() on them.\n> \n> My understanding is that this shouldn't change much in regard to the\n> link order protection realized by declaring factories_ inside the\n> factories() function, just pointing it out to see if someone else see\n> an issue here.\n> \n>> +\n>> +\tfor (ConverterFactory *factory : factories) {\n>> +\t\tstd::vector aliases = factory->aliases();\n>> +\t\tauto it = std::find(aliases.begin(), aliases.end(), media->driver());\n> \n> How do you envision aliases to be used ? I mean, do you expect a\n> Converter instance to handle different devices ? Probably yes, as a\n> generic M2M converter will follow in the series and could handle\n> multiple devices...\nYes, the factory name would be a framework name in this case and the \naliases would point to driver names libcamera could use as converters.\n> \n>> +src/libcamera/pipeline_handler.cpp\n>> +\t\tif (it == aliases.end() && media->driver() != factory->name_)\n> \n> what is \"media->driver() != factory->name_)\" for ? Shouldn't the media\n> driver name be part of aliases only ?\nIn case the media driver name should directly be used as binding string.\nYes, the driver name should be part of the aliases, but that's a way to \nenforce it.\n\n> \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> How long is this message ? Can it be shortened or the factory name\n> here is relevant ?\n\"Creating converter from dw100 with no alias\"\nThat's not so long but can be removed as this just serve debug purpose.\n> \n>> +\n>> +\t\tConverter *converter = factory->createInstance(media);\n>> +\t\treturn std::unique_ptr(converter);\n>> +\t}\n>> +\n>> +\treturn nullptr;\n>> +}\n>> +\n>> +void ConverterFactory::registerType(ConverterFactory *factory)\n>> +{\n>> +\tstd::vector &factories =\n>> +\t\tConverterFactory::factories();\n>> +\n>> +\tfactories.push_back(factory);\n>> +}\n>> +\n>> +std::vector ConverterFactory::names()\n>> +{\n>> +\tstd::vector list;\n>> +\n>> +\tstd::vector &factories =\n>> +\t\tConverterFactory::factories();\n>> +\n>> +\tfor (ConverterFactory *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>> +std::vector &ConverterFactory::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>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 63b47b17..a261d4b4 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 0E16EC3272\n\tfor ;\n\tSun, 18 Sep 2022 08:40:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11D9F6218D;\n\tSun, 18 Sep 2022 10:40:05 +0200 (CEST)","from EUR04-DB3-obe.outbound.protection.outlook.com\n\t(mail-eopbgr60055.outbound.protection.outlook.com [40.107.6.55])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B94C603DF\n\tfor ;\n\tSun, 18 Sep 2022 10:40:02 +0200 (CEST)","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t(2603:10a6:102:21e::22)\n\tby AS4PR04MB9624.eurprd04.prod.outlook.com (2603:10a6:20b:4ce::9)\n\twith Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5632.18;\n\tSun, 18 Sep 2022 08:40:00 +0000","from PAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::4f72:a35a:8c60:63f1]) by\n\tPAXPR04MB8703.eurprd04.prod.outlook.com\n\t([fe80::4f72:a35a:8c60:63f1%5]) with mapi id 15.20.5632.019;\n\tSun, 18 Sep 2022 08:40:00 +0000"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663490405;\n\tbh=TOeygJvrUM5StVPZbhhSvG1zW/WANz+zS6fxE7ysegg=;\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=P0JUfGU0DDdVUclN9EWvjOT2hwY3rXm4qrGHSl7nJYJXfGn2V+41nNbRms6oPhF6g\n\tR2shApbGjkgcg97PaXrc/6VgsYq0DAJIRu3qySoL7ardHeyDw/l77hgQ3u4IVjJQrO\n\tZALcpKtfFSgEdet7MrDlaKC6HDfgzqCIJvSE1cP5UHXSbrnDYFLLvV/RI/r5vuqToW\n\tiDuwDA30yMWoeJKT/dQ6Cne9LullhAOU8MtKoxhpy5oKiyZd24IQaxbbijz2zbsSIb\n\tLN+KJXNZjnaooAxOEpxrWdlopam779HhJYbk78vB0bOpC38WL6T+GBN1KwIFz29NW8\n\tq0Scwhqz4GDSQ==","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=ZdyJOn5qogwy18w5+T9Dp5lzQcHth5uZTq0QlKtitdE=;\n\tb=ka89vPaVddvKmh1ErDmSsKXdEQ3+QvfpKIGu4eeMc4D0vVdNc4LCqWyROG4YyvZSUMxhCIsAjVFelCBE/+XHpVcvV9Spz/xyuby/M3wGNxSn7d4D9NJ3YZZEHbOMcIswcPGyJcBDU1XVR3ypj3miuOONXNyUHCZCBc2DcorFgT8="],"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=\"ka89vPaV\"; 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=KKALn8Kzt7+2HdjzPP/gg+d+sJZnKXEW9VeKyDwpycGlg7jaCsR6572YipNb17MU1YtnAZUVDmUWzaZ+/9vr9Nzg96KOA4SKHE9TCJ4PJ4r4OwiFMCVvSVUpGhRyMOfyK17ocMQ0Ka1XQ6yrSFobdLhczabQCVzaaV7CP8slFNvlRUntL50NEnxGyk+vJNq7KPrXaHOh8POmBob5aH4lnu4NZ+xo+2Q0+JIo1uLzl7z1kto+PLpimsd4YDbWjCFB+B7rIp8YlOZyWucPWNKBQkWxU9xTgswdH74kkZmE8q32SL2nGwjv49YOY9s4N3HFEOdsADm6Kvkul7g8XjRIeQ==","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=ZdyJOn5qogwy18w5+T9Dp5lzQcHth5uZTq0QlKtitdE=;\n\tb=XvSmTsrFMtoZ9GCvn4uBIk8wLkm645sPp/6JDlcrd/nGds4nihryCRLkhXTeJ4yZRBBEm4in1G31GBRYYqrSmXhUD2oZLhP8iSacmul75UDH8OdzmkOJFWVH26DlbYS/MELHFNvIpOS92o3CaSBcu8Vmf73L8is/66s9USLowm9HdhQxn1BEh5lvLy75OtMd6+OlXfVHjc8NT1V4bUgRWKEqnwUYgknjN/aWhMO5DeB40+60z1k56RLAYJcogfvZDBplY8/Q5H0+7F7ZQGylDUXIRD3YfyuajOlaBCX+C3iqaUTX0YjmFwCVVBwWYnGOo9qntnPlxjkBGJtx/2zUXA==","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":"<63051502-e2f7-d568-b8dd-0dd8f0602478@oss.nxp.com>","Date":"Sun, 18 Sep 2022 10:39:32 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","To":"Jacopo Mondi ","References":"<20220908184850.1874303-1-xavier.roumegue@oss.nxp.com>\n\t<20220908184850.1874303-5-xavier.roumegue@oss.nxp.com>\n\t<20220914104604.qbggiupgdvlrwc2q@uno.localdomain>","Content-Language":"en-US","In-Reply-To":"<20220914104604.qbggiupgdvlrwc2q@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-ClientProxiedBy":"PAZP264CA0146.FRAP264.PROD.OUTLOOK.COM\n\t(2603:10a6:102:1f9::6) 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_|AS4PR04MB9624:EE_","X-MS-Office365-Filtering-Correlation-Id":"c68e02df-bef8-4e2d-0d28-08da99515f6b","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":"LreA7BCXXd+m0/pj3IiK9wLiZS7MJRvG/qJuInduXkyxGS4d1vjaOBQ3BYnH3HgWrYARtrRLL+avaRkCA/IiXoF9GkB5mZqiMHGJs2QXoh1ko5jVwDglqDcEsddbRgJiRF/hirdAOHQTSyDIA4KvCX3oaeR5chLY0piqNFw1yppJPDy8cOJS/m4tnY2Od4veVqmDvS4BvunW61OServdeZIVPZsIhrMDLX37zGuNIEIWG5Hop69Dh0ximcbIOUTW80C9SlZeDgkGQM975A1yUD0ryWM/Zwadx5Tx3/p+mCK0Bgf5S1Zwl2bMoiGP2pAgsir5zVBDMFnf/N1X95jllVnrqm6KKyS1L41jhpyGbIVvuoaI5f24zCWqnHxdaWgd39VpN9kl3xRrlgNx6QVkg9qbSvwV8ouqyo9ppKmuSMoo5yr7IYTXzp+/4ZjQ/zEEfeeKXFwkyabXwN7SbxDcezy8AKc8sUUfQOh37IQXp4ubMIs8dryrqzDgZ0mxSvla945/hYRyl4ZvK81N3dnkNHK1gvq18z69oZ0zb5pr50v+hy2c7XE0RH42F/YL6kaZHBO+Aypj1qXLnstNNDdP295ZmLsJk+WnrqzaTz3zo6qZ+2YZyrojQKXgL07upPyi1bR9htnBHVg+WVxq2EBNke0TWx8xiIj1//X1DhQ82+2J39wRAlikSj9/Y5Z0dZWok80wERsqptC5MKlQnFT3VjTYNWYIUvNS9rYp+bkVuthYGfFDy3USHKym4dy3SZ5RH6vdlI4S6ySn8DzW8AbPEiHYr5fq7dWJx+/mYpsSVyUGTBashZ5D4MZtwRRknHab","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)(39860400002)(376002)(346002)(396003)(366004)(136003)(451199015)(6512007)(2616005)(186003)(478600001)(6486002)(52116002)(53546011)(41300700001)(6666004)(6506007)(38100700002)(31696002)(86362001)(83380400001)(8936002)(66946007)(66476007)(2906002)(66556008)(5660300002)(316002)(8676002)(30864003)(6916009)(31686004)(4326008)(41533002)(45980500001)(43740500002);\n\tDIR:OUT; SFP:1101; ","X-MS-Exchange-AntiSpam-MessageData-ChunkCount":"1","X-MS-Exchange-AntiSpam-MessageData-0":"=?utf-8?q?E5IxcZ43Axw4iHZtJ0zfQqRIT?=\n\t=?utf-8?q?IT5kXQo3biaYQYejLA/dozDWtKfEkWED4eBzMB4EXL/GDdi1PYDNgwqN?=\n\t=?utf-8?q?wE7x7lOsLMM1ui0v2YiLdEtCiKDVjWHyAsQEpW1ECjYM169cUbTOOgbL?=\n\t=?utf-8?q?ePcVN3RL/Ct/Ha/t5dhYqYtHY5ZenLBMnXkKX4S+sf2atxvPGNWgyP4P?=\n\t=?utf-8?q?I14QfmSwdYxeI7r+OtEGGwuVo0XhAWBdepDNfxvSyz8baZmJLm9hFISI?=\n\t=?utf-8?q?UdXLmDBw8WOCXhhfGGtiWZb7cwXggwtctOYovEjvEtJoqlqD22k4cAsu?=\n\t=?utf-8?q?FTODe5dOG9L2dRFXphrmoqFIyiIDKazgKUvZf08ALrMGC3iG10J3f64i?=\n\t=?utf-8?q?LxAiLmtN+sWBjZ9sp22TDf/YCDFqwptWtPdE4Nx6l2TrqFptlurmU/bQ?=\n\t=?utf-8?q?LisxwhOMHvrtDrk+y/LS1LGXRPe5u2itBxX6ABv1AwDRrdtpdJikFzOD?=\n\t=?utf-8?q?DcqVP7cxoiC3Snv+oE+4QAgkBL57yjM4OrN4ncYt1q7em0osldZoWK8C?=\n\t=?utf-8?q?IMsbf1amYrGirafSxQSphxV71H8FEyZywIzgYY9wfX+MMu5rfTaeqBMt?=\n\t=?utf-8?q?gQYdXSAdamYmLFa/etVsKatQbqaSaQX0ellgGQpnM8+NBruqoCXa1ZoA?=\n\t=?utf-8?q?xTmWrKE+bAaN5fi5ZqFgbgdKPg5H//DW9XjO7UoYVhK8TKerAzdwIh16?=\n\t=?utf-8?q?EbQWR85mZQF54ROJcow/heg+inA4uKU3BJ272HMjCf+tWGPoOYKzWOha?=\n\t=?utf-8?q?S/F49lGSfna3yicMM2I0Kp8Xddrjs19vK2Q0wC7Mvz2bzSzKenrd1OF6?=\n\t=?utf-8?q?xeUKVojvmw/EPy4aP49CvuHQ3Hds4pIeaU0NvbXGiNiTRRcM5wUjY8rJ?=\n\t=?utf-8?q?fRzVH+z/KN8pa4yaNTLqJ45nujdTG82DWcVwYfm2t4jmcalZDkP7IGJN?=\n\t=?utf-8?q?SltO0AW7OfCpdqATQgE0kaEl+H9JG5nfGp4hFqOoK3m5EEwnTu39F596?=\n\t=?utf-8?q?kzpVMEOo4BnVweR21SRZ1cUYhR+AqpDWk+NBJmglNGwafgDfiwoJiWQR?=\n\t=?utf-8?q?uDa4jYvu+TF16lpC1/vAm/+WG84Lzir2oXy5/DUpeDaqrGAp+SgX2LUe?=\n\t=?utf-8?q?BLgAV3Yxm/jFU5UChwiWwxCH00FHmCSljMuludpeBWW/MIFjWE5Te79P?=\n\t=?utf-8?q?SNmzAEMmJ/mjvbyy1IDOh7hR3GpJzAj4scWD7Wk6xJV9c3t540rxesG1?=\n\t=?utf-8?q?iemtPksXT74cEXEa7F8/0LJNui9efueITdMiqpDW1eR5Y4lCt/5dQiDY?=\n\t=?utf-8?q?S04pwgcWncnKHBSV95WYQZZZi//+RrYfOt3ytn+VvEvI0VWATKKgK3ZL?=\n\t=?utf-8?q?hwnGXfbdcp9vbmihZawsRYB6szOC8pgB1DwTlDIHycbRn+gN8fERLaEQ?=\n\t=?utf-8?q?lr7Y5rlKHbAVTznw5sIkzKy2BvFkReuXRF276uZ6zm6I7dzzU2rUd5hG?=\n\t=?utf-8?q?B+bdoxz7jtFsvCOde0VWmKA/FQZFPF6j/HaZ+fb6XIcSoyJ/AIptbYAi?=\n\t=?utf-8?q?gI24UL1dMsbBVmrmOMbhtJajF2rU8CA/w40EnAMm/oNMDjYSiD51qgoE?=\n\t=?utf-8?q?Wc7YvwaWB2v2B+DZ27pMvPMwFOqwfKRWxm77SyDfX95tdUMUJj+y+zx4?=\n\t=?utf-8?q?Gaz25Ue2PShhQqCz1DgKbe9mf7j+EwlgJzESXqrl1sshhgpJQL+5oE8G?=\n\t=?utf-8?q?CFNwWPQpOsWS5xzhCb6HWc7bDP7lkisVu5qog=3D=3D?=","X-OriginatorOrg":"oss.nxp.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"c68e02df-bef8-4e2d-0d28-08da99515f6b","X-MS-Exchange-CrossTenant-AuthSource":"PAXPR04MB8703.eurprd04.prod.outlook.com","X-MS-Exchange-CrossTenant-AuthAs":"Internal","X-MS-Exchange-CrossTenant-OriginalArrivalTime":"18 Sep 2022 08:40:00.3141\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":"k0DUN/ZmGJCPJnT1bddNDYmWphppDaV3BQDvGUzwp+naiHTt84rA6bGryyVViXAUCNvRpsU8zTon2vJ8UEEoxQ==","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"AS4PR04MB9624","Subject":"Re: [libcamera-devel] [PATCH 04/14] libcamera: Declare generic size\n\tand format converter interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"\"Xavier Roumegue \\(OSS\\) via libcamera-devel\"\n\t","Reply-To":"\"Xavier Roumegue \\(OSS\\)\" ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25243,"web_url":"https://patchwork.libcamera.org/comment/25243/","msgid":"","date":"2022-10-03T22:08:38","subject":"Re: [libcamera-devel] [PATCH 04/14] libcamera: Declare generic size\n\tand format converter interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Xavier,\n\nOn Sun, Sep 18, 2022 at 10:39:32AM +0200, Xavier Roumegue (OSS) via libcamera-devel wrote:\n> On 9/14/22 12:46, Jacopo Mondi wrote:\n> > Hi Xavier\n> > I would drop \"size and format\" and use \"Converter\" in the subject\n> > title.\n> > \n> > On Thu, Sep 08, 2022 at 08:48:40PM +0200, Xavier Roumegue via libcamera-devel wrote:\n> >> Declare a converter Abstract Base Class intented to provide generic\n\ns/intented/intended/\n\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 load_configuration() method which\n\ns/load_configuration() method/loadConfigure() function/\n\n\"method\" is a term from Java, it's not used by the C++ specification\n(which uses \"member function\" instead).\n\n> >> can be used by the concrete implementation to load hardware specific\n> >> runtime parameters defined by the application.\n> >>\n> >> Signed-off-by: Xavier Roumegue \n> >> ---\n> >> include/libcamera/internal/converter.h | 101 ++++++++++++++++++++++++\n> >> include/libcamera/internal/meson.build | 1 +\n> >> src/libcamera/converter.cpp | 102 +++++++++++++++++++++++++\n> >> src/libcamera/meson.build | 1 +\n> >> 4 files changed, 205 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..e2237c57\n> >> --- /dev/null\n> >> +++ b/include/libcamera/internal/converter.h\n> >> @@ -0,0 +1,101 @@\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> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> > \n> > I might have missed what is functional included for\n> \n> Right, I will remove it.\n\nIt's for std::reference_wrapper.\n\n> >> +#include \n> >> +#include \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> > Empty line maybe ?\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\nThis was a bit of an ad-hoc API for the simple pipeline handler. We can\nkeep it as-is for now, and rework it on top to improve the converter\nAPI.\n\n> >> +\n> >> +\tvirtual int configure(const StreamConfiguration &inputCfg,\n> >> +\t\t\t const std::vector> &outputCfg) = 0;\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> >> +\tstd::string deviceNode_;\n\nThis should be private, with an accessor function.\n\n> >> +\tSignal inputBufferReady;\n> >> +\tSignal outputBufferReady;\n> >> +};\n> >> +\n> >> +class ConverterFactory\n> >> +{\n> >> +public:\n> >> +\tConverterFactory(const std::string name);\n> >> +\tvirtual ~ConverterFactory() = default;\n> >> +\n> >> +\tstatic std::unique_ptr create(MediaDevice *media);\n> >> +\n> >> +\tstatic void registerType(ConverterFactory *factory);\n> >> +\tstatic std::vector &factories();\n> >> +\tstatic std::vector names();\n> >> +\n> >> +protected:\n> >> +\tvirtual Converter *createInstance(MediaDevice *media) = 0;\n> >> +\tvirtual const std::vector aliases() const = 0;\n> >> +\n> >> +private:\n> >> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactory)\n> >> +\n> >> +\tstd::string name_;\n> >> +};\n> >> +\n> >> +#define REGISTER_CONVERTER(name, converter, ...) \\\n> >> +\tclass converter##Factory final : public ConverterFactory \\\n> >> +\t{ \\\n> >> +\tpublic: \\\n> >> +\t\tconverter##Factory() : ConverterFactory(name) {} \\\n> >> + \\\n> >> +\tprivate: \\\n> >> +\t\tConverter *createInstance(MediaDevice *media) \\\n> >> +\t\t{ \\\n> >> +\t\t\treturn new converter(media); \\\n> >> +\t\t} \\\n> >> +\t\tstd::vector aliases_ = { __VA_ARGS__ }; \\\n> >> +\t\tconst std::vector aliases() const { return aliases_; } \\\n\nThis should return a reference.\n\n> >> +\t}; \\\n> >> +\tstatic converter##Factory global_##converter##Factory;\n\nI've sent a patch series that moved the CameraSensorHelper and\nPipelineHandler factories to using a class template for registration\n(\"[PATCH 0/8] libcamera: Use class templates for auto-registration\").\nCould you base this code on that design ? It would result in\n\nclass ConverterFactoryBase\n{\npublic:\n\tConverterFactoryBase(const char *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\nprivate:\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\ntemplate\nclass ConverterFactory : public ConverterFactoryBase\n{\npublic:\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, ...) \\\nstatic ConverterFactory global_##converter##Factory(name, { __VA_ARGS__ });\n\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..89a594d1\n> >> --- /dev/null\n> >> +++ b/src/libcamera/converter.cpp\n> >> @@ -0,0 +1,102 @@\n> >> +\n> > \n> > Rogue empty line\n> > \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 \n> >> +\n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/converter.h\"\n> >> +#include \"libcamera/internal/media_device.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +LOG_DEFINE_CATEGORY(Converter)\n> >> +\n\nDocumentation will be needed ;-)\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\treturn;\n> > \n> > Should we complain loud and set some state variable that can be\n> > inspected by the isValid() function already ?\n> \n> Complaining loud can not hurt. An empty deviceNode_ string can be used \n> to indicate the instance is invalid in case the concrete class relies on \n> a device node. Adding a kind of isValid boolean attribute would likely \n> be redundant with isValid().\n> \n> >> +\n> >> +\tdeviceNode_ = (*it)->deviceNode();\n> > \n> > I'm not sure storing a string here is any useful if not fo debugging\n> > purposes. What if you store a pointer to the entity instead, so\n> > derived classes can use the static fromMediaEntity() function (which\n> > probably have to be added to the M2M device) to open the device node ?\n> \n> The ambition of this abstract class is to be framework/hardware \n> agnostic. For instance, the converter could sit on top on a DRM device, \n> or any hardware compute backend (dsp, cpu, etc..) to implement the \n> interface.\n> Hence, the design choice to not embed specialized framework in the \n> abstract class except in the constructor.\n\nI agree that's the way to go, but there will be no media device in that\ncase. We can specialize the class for now if that brings any\nimprovement, and rework it later.\n\n> >> +}\n> >> +\n> >> +Converter::~Converter()\n> >> +{\n> >> +}\n> >> +\n> >> +ConverterFactory::ConverterFactory(const std::string name)\n> >> +\t: name_(name)\n> >> +{\n> >> +\tregisterType(this);\n> >> +}\n> >> +\n> >> +std::unique_ptr ConverterFactory::create(MediaDevice *media)\n> >> +{\n> >> +\tstd::vector &factories =\n\nYou can make this const, as well as the factory pointer below.\n\n> >> +\t\tConverterFactory::factories();\n> > \n> > Oh, this is different from the other favtories we have.\n> > The usage here is:\n> > \n> > ConverterFavtory::create()\n> > for (factory: factories())\n> > factory->create();\n> > \n> > While other factories (looking at the pipeline handler factory) have a\n> > static method to retrieve factories, and then call create() on them.\n> > \n> > My understanding is that this shouldn't change much in regard to the\n> > link order protection realized by declaring factories_ inside the\n> > factories() function, just pointing it out to see if someone else see\n> > an issue here.\n> > \n> >> +\n> >> +\tfor (ConverterFactory *factory : factories) {\n> >> +\t\tstd::vector aliases = factory->aliases();\n> >> +\t\tauto it = std::find(aliases.begin(), aliases.end(), media->driver());\n> > \n> > How do you envision aliases to be used ? I mean, do you expect a\n> > Converter instance to handle different devices ? Probably yes, as a\n> > generic M2M converter will follow in the series and could handle\n> > multiple devices...\n> \n> Yes, the factory name would be a framework name in this case and the \n> aliases would point to driver names libcamera could use as converters.\n> \n> >> +src/libcamera/pipeline_handler.cpp\n> >> +\t\tif (it == aliases.end() && media->driver() != factory->name_)\n> > \n> > what is \"media->driver() != factory->name_)\" for ? Shouldn't the media\n> > driver name be part of aliases only ?\n> \n> In case the media driver name should directly be used as binding string.\n> Yes, the driver name should be part of the aliases, but that's a way to \n> enforce it.\n\nI'm not too fond of the alias mechanism, especially with the separate\nname, but that can be handled later (or maybe I'll make a proposal with\nfixup patches on top after reviewing the whole series, I don't know yet\nhow I would like the code to be structured).\n\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> > How long is this message ? Can it be shortened or the factory name\n> > here is relevant ?\n> \n> \"Creating converter from dw100 with no alias\"\n> That's not so long but can be removed as this just serve debug purpose.\n> \n> >> +\n> >> +\t\tConverter *converter = factory->createInstance(media);\n> >> +\t\treturn std::unique_ptr(converter);\n> >> +\t}\n> >> +\n> >> +\treturn nullptr;\n> >> +}\n> >> +\n> >> +void ConverterFactory::registerType(ConverterFactory *factory)\n> >> +{\n> >> +\tstd::vector &factories =\n> >> +\t\tConverterFactory::factories();\n> >> +\n> >> +\tfactories.push_back(factory);\n> >> +}\n> >> +\n> >> +std::vector ConverterFactory::names()\n> >> +{\n> >> +\tstd::vector list;\n> >> +\n> >> +\tstd::vector &factories =\n> >> +\t\tConverterFactory::factories();\n> >> +\n> >> +\tfor (ConverterFactory *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\nThis I'm not very fond of either, I'll check how it's used.\n\n> >> +\n> >> +std::vector &ConverterFactory::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> >> +} /* namespace libcamera */\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index 63b47b17..a261d4b4 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',","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 2014CC0DA4\n\tfor ;\n\tMon, 3 Oct 2022 22:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 352B8601CF;\n\tTue, 4 Oct 2022 00:08:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 112B4601C7\n\tfor ;\n\tTue, 4 Oct 2022 00:08:42 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 212BC9DE;\n\tTue, 4 Oct 2022 00:08:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664834924;\n\tbh=ElwebYR6jyjifrLbZBcfMVWtfK7JlzyuQhDxioYCYA0=;\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=N/lXRgKx92A0SOwESrAhC98Zepr27T4EbafpkVVMAb1K92DsVC+6TYdKFi19G+kSO\n\tP5+BHD4JSyNGM3+VmzhU4zvQvHIG7i+rrObTIbaWsknJ0xCFEf74SYDB5ZQqr3hbL/\n\tdZSGvYQAPlku5WrU/4I1EapdLd3NQ5x5tiLyiNp/wqJl0XWoNbAhSSWotbiyOuZY50\n\tMJGTJ8L87e+vmBfjMSGw4vrJQXHUI1daM55urkKcBM3q9/q0PSaCzapKBzrHyvmNlm\n\tuKtGxUdc629Fbk43Bei5cnOuQICf+wze4w4DdK6lVzauPD2EbMZX4ahmP4SY1P3BtY\n\tkQ5kelmy6eI5Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664834921;\n\tbh=ElwebYR6jyjifrLbZBcfMVWtfK7JlzyuQhDxioYCYA0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AhNwtEIM8iIZvINYsMrDAjnSzOEK1PtYDOL4N/aAYNwEvnaIjU27dnhJfUpOffVtk\n\tPLUirz9jsPoXLMPd4P0nGOPxzPWafD8HQkmmoMiaMXxD7F4csOuGZNME6RvZhSc0Ae\n\tkj/XS9bZOZ4P/QMvCdMaOhxEqHvnyN3v19fnO7kQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AhNwtEIM\"; dkim-atps=neutral","Date":"Tue, 4 Oct 2022 01:08:38 +0300","To":"\"Xavier Roumegue (OSS)\" ","Message-ID":"","References":"<20220908184850.1874303-1-xavier.roumegue@oss.nxp.com>\n\t<20220908184850.1874303-5-xavier.roumegue@oss.nxp.com>\n\t<20220914104604.qbggiupgdvlrwc2q@uno.localdomain>\n\t<63051502-e2f7-d568-b8dd-0dd8f0602478@oss.nxp.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<63051502-e2f7-d568-b8dd-0dd8f0602478@oss.nxp.com>","Subject":"Re: [libcamera-devel] [PATCH 04/14] libcamera: Declare generic size\n\tand format converter interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]