[{"id":26020,"web_url":"https://patchwork.libcamera.org/comment/26020/","msgid":"<167041666275.9133.11513981163425506214@Monstersaurus>","date":"2022-12-07T12:37:42","subject":"Re: [libcamera-devel] [PATCH v3 1/2] libcamera: Declare generic\n\tconverter interface","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Xavier Roumegue via libcamera-devel (2022-11-17 18:52:03)\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> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/converter.h | 109 ++++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/converter.cpp | 331 +++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 442 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..c02911c4\n> --- /dev/null\n> +++ b/include/libcamera/internal/converter.h\n> @@ -0,0 +1,109 @@\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 format converter interface\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n\nI think you need this for std::reference_wrapper\n\n#include \n\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n\nI think you can drop this include and just add class PixelFormat; below\nwith your other forward declarations.\n\n> +\n> +namespace libcamera {\n> +\n> +class FrameBuffer;\n> +class MediaDevice;\n> +class Size;\n> +class SizeRange;\n\nYou don't need to declare Size and SizeRange if you have included\ngeometry.\n\n\n> +struct StreamConfiguration;\n> +\n> +class Converter\n> +{\n> +public:\n> + Converter(MediaDevice *media);\n> + virtual ~Converter();\n> +\n> + virtual int loadConfiguration(const std::string &filename) = 0;\n> +\n> + virtual bool isValid() const = 0;\n> +\n> + virtual std::vector formats(PixelFormat input) = 0;\n> + virtual SizeRange sizes(const Size &input) = 0;\n> +\n> + virtual std::tuple\n> + strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;\n> +\n> + virtual int configure(const StreamConfiguration &inputCfg,\n> + const std::vector> &outputCfgs) = 0;\n> + virtual int exportBuffers(unsigned int output, unsigned int count,\n> + std::vector> *buffers) = 0;\n> +\n> + virtual int start() = 0;\n> + virtual void stop() = 0;\n> +\n> + virtual int queueBuffers(FrameBuffer *input,\n> + const std::map &outputs) = 0;\n> +\n> + Signal inputBufferReady;\n> + Signal outputBufferReady;\n> +\n> + const std::string &deviceNode() const { return deviceNode_; };\n> +\n> +private:\n> + std::string deviceNode_;\n> +};\n> +\n> +class ConverterFactoryBase\n> +{\n> +public:\n> + ConverterFactoryBase(const std::string name, std::initializer_list aliases);\n> + virtual ~ConverterFactoryBase() = default;\n> +\n> + const std::vector &aliases() const { return aliases_; }\n> +\n> + static std::unique_ptr create(MediaDevice *media);\n> + static std::vector &factories();\n> + static std::vector names();\n> +\n> +private:\n> + LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactoryBase)\n> +\n> + static void registerType(ConverterFactoryBase *factory);\n> +\n> + virtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n> +\n> + std::string name_;\n> + std::vector aliases_;\n> +};\n> +\n> +template\n> +class ConverterFactory : public ConverterFactoryBase\n> +{\n> +public:\n> + ConverterFactory(const char *name, std::initializer_list aliases)\n> + : ConverterFactoryBase(name, aliases)\n> + {\n> + }\n> +\n> + std::unique_ptr createInstance(MediaDevice *media) const override\n> + {\n> + return std::make_unique<_Converter>(media);\n\nOh ... I was thrown off by the _ prefix there. But it's to make the\ntypename unique. I'm not sure we've used this style elsewhere, perhaps\nwe've just used unique names. But I'm not opposed to it...\n\n\n\n> + }\n> +};\n> +\n> +#define REGISTER_CONVERTER(name, converter, ...) \\\n> + static 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..1ab3c9f6\n> --- /dev/null\n> +++ b/src/libcamera/converter.cpp\n> @@ -0,0 +1,331 @@\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 offer scaling and pixel format conversion services on a input\n\n\"on an input stream\"\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\n/seeks/searches for/\n\nSemantically 'seeks' is probably correct, but I don't think we'd ever\nuse the phrase like that in English.\n\n/implementing data streaming/implementing the data streaming/ ?\n\n> + * graph entities and use its device node as converter device node.\n\n/node as converter/node as the converter/\n\n> + */\n> +Converter::Converter(MediaDevice *media)\n> +{\n> + const std::vector &entities = media->entities();\n> + auto it = std::find_if(entities.begin(), entities.end(),\n> + [](MediaEntity *entity) {\n> + return entity->function() == MEDIA_ENT_F_IO_V4L;\n\nIWYU (include what you use) tells me you need to #include\n\"linux/media.h\" for the MEDIA_ENT_F_IO_V4L define.\n\n> + });\n> + if (it == entities.end()) {\n> + LOG(Converter, Error)\n> + << \"No entity suitable for implementing a converter in \"\n> + << media->driver() << \" entities list.\";\n> + return;\n> + }\n> +\n> + deviceNode_ = (*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 supported output pixel formats\n> + */\n> +\n> +/**\n> + * \\fn Converter::sizes()\n> + * \\brief Retrieve the range of minimum and maximum output sizes for an input size\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[out] outputCfgs A list of output stream configurations\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\n'similarly to'\n\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> + */\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] outputs The container holding the output stream indexes and\n> + * their respective frame buffer outputs.\n\nI'm curious how multiple streams will function independently in the\nfuture, but I think it's probably reasonable for the time being that at\nthe point buffers are queued, we should know all the buffers for that\noperation.\n\nIt's internal, so I'm not overly concerned here though. We can make\nadaptations if we find we need to in the future.\n\n\n> + *\n> + * This function queues the \\a input frame buffer on the output streams of the\n> + * \\a outputs map key and retrieve the output frame buffer indicated by the\n> + * buffer map value.\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\nWhich output buffer? Does each output buffer have it's own completion\nsignal?\n\nCan this be explained here either way?\n\n\n> + */\n> +\n> +/**\n> + * \\fn Converter::deviceNode()\n> + * \\brief The converter device node attribute accessor\n> + * \\return The converter device node string\n> + */\n> +\n> +/**\n> + * \\class ConverterFactoryBase\n> + * \\brief Base class for converter factories\n> + *\n> + * The ConverterFactoryBase class is the base of all specializations of the\n> + * ConverterFactory class template. It implements the factory registration,\n> + * maintains a registry of factories, and provides access to the registered\n> + * factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a converter factory base\n> + * \\param[in] name Name of the converter class\n> + * \\param[in] aliases Name aliases of the converter class\n> + *\n> + * Creating an instance of the factory base registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used as unique identifier. If the converter\n> + * implementation fully relies on a generic framework, the name should be the\n> + * same as the framework. Otherwise, if the implementation is specialized, the\n> + * factory name should match the driver name implementing the function.\n\nPerhaps I'll see later, but does this mean we can have 'generic'\nConverter instances that support multiple MediaDevices, if they\nimplement the same functionality on top of a V4L2M2M device for\ninstance?\n\nSort of like a set of 'compatible' strings ?\n\n\n\n> + *\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> + : name_(name), aliases_(aliases)\n> +{\n> + registerType(this);\n> +}\n\nDo you envisage that all convertors will be identified and 'constructed'\neven if they're not used?\n\nIs there any cost to having all convertor objects created? (I.e. any\nfile handles kept open when not in use or such ?)\n\n> +\n> +/**\n> + * \\fn ConverterFactoryBase::aliases()\n> + * \\return The names aliases\n> + */\n> +\n> +/**\n> + * \\brief Create an instance of the converter corresponding to a named factory\n> + * \\param[in] media Name of the factory\n\nI'm not sure a MediaDevice is the Name of the factory?\n\nSo this is consructing the convertor object directly when a media device\nis found - so I think that answers my concerns above - the convertors\nwill only be constructed if a pipeline handler has a device to construct\nand wants to make it available for use.\n\n\n> + *\n> + * \\return A unique pointer to a new instance of the converter subclass\n> + * corresponding to the named factory or one of its alias. Otherwise a null\n> + * pointer if no such factory exists\n> + */\n> +std::unique_ptr ConverterFactoryBase::create(MediaDevice *media)\n> +{\n> + const std::vector &factories =\n> + ConverterFactoryBase::factories();\n> +\n> + for (const ConverterFactoryBase *factory : factories) {\n> + const std::vector &aliases = factory->aliases();\n> + auto it = std::find(aliases.begin(), aliases.end(), media->driver());\n> +\n> + if (it == aliases.end() && media->driver() != factory->name_)\n> + continue;\n> +\n> + LOG(Converter, Debug)\n> + << \"Creating converter from \"\n> + << factory->name_ << \" factory with \"\n> + << (it == aliases.end() ? \"no\" : media->driver()) << \" alias.\";\n> +\n> + return factory->createInstance(media);\n> + }\n> +\n> + return 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> + std::vector &factories =\n> + ConverterFactoryBase::factories();\n> +\n> + factories.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\nIs this just for debug? Is it used ? (I like having it, I'm just not\nsure what would use it yet).\n\n> +{\n> + std::vector list;\n> +\n> + std::vector &factories =\n> + ConverterFactoryBase::factories();\n> +\n> + for (ConverterFactoryBase *factory : factories) {\n> + list.push_back(factory->name_);\n> + for (auto alias : factory->aliases())\n> + list.push_back(alias);\n> + }\n> +\n> + return 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> + /*\n> + * The static factories map is defined inside the function to ensure\n> + * it gets initialized on first use, without any dependency on link\n> + * order.\n> + */\n> + static std::vector factories;\n> + return 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> + * \\details \\copydetails ConverterFactoryBase::ConverterFactoryBase\n> + *\n\nno need for that blank line.\n\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] ... Optional list of alias names\n\nAha, that answers my earlier question too. There will be 'compatible'\nstrings with aliases.\n\nNothing major above so with items considered or resolved:\n\n\nReviewed-by: Kieran Bingham \n\n\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 0494e808..e9d0324e 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.38.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 93BACBE08B\n\tfor ;\n\tWed, 7 Dec 2022 12:37:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17DEB63336;\n\tWed, 7 Dec 2022 13:37:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEE0063335\n\tfor ;\n\tWed, 7 Dec 2022 13:37:45 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 61E6687B;\n\tWed, 7 Dec 2022 13:37:45 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670416668;\n\tbh=qeSjrfu30LNDcmbahmSkY9mYExHGgQwTeIp80bjRErw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zhZvEYBzj/bpqLdNjiL5qF4ZLuCAFibNvZDO3y/aLJZqwLTgey4Ym2nUPZZ4gfzlQ\n\tnQyIycqRwQHMIwEeZX4OkeoWThPfS5oqJi3w3Rk+9DN1sTdlTh1am01/2gDhWhxb9q\n\tjhHjcWDlq0ZyhZmzGgr1Y5WXUuGcNt8B2+OVtXi8zMVdpnEUbf28BjniYx4TUXyYAB\n\tV2aXGMKFQ1QYoKYFRjxvAmUyj7LMzUT9otAYNwWpq+7nZAsZkr0dodq3HFsEktD/3r\n\tRRaZxsnK5XMsNCN2tTfvjvFFFrHw7aDKF/5wXzTnuoQiIyAy+kiJOaU3c80q6myq/N\n\t3vM5JT7I4fKlg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670416665;\n\tbh=qeSjrfu30LNDcmbahmSkY9mYExHGgQwTeIp80bjRErw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=AkMAM2AL/RWJlIsIwpOYMPnkWx0YPhUf6duxr4gAko8KjGX4CILAFXzGcqpFkkq8M\n\t4Kg2tkRlvKRuzcQffk8u1myXvAI325m/Ll0gQzl6HPWZvFCYUaGShM9bBsukrwmy8K\n\tFR7MOE5rzYXDiCMMUWHHM007tpxKFF/MxWL+/nU0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AkMAM2AL\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20221117185204.11625-1-xavier.roumegue@oss.nxp.com>","References":"<20221117185204.11625-1-xavier.roumegue@oss.nxp.com>","To":"Xavier Roumegue ,\n\tXavier Roumegue via libcamera-devel\n\t, \n\tjacopo@jmondi.org, laurent.pinchart@ideasonboard.com","Date":"Wed, 07 Dec 2022 12:37:42 +0000","Message-ID":"<167041666275.9133.11513981163425506214@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]