[{"id":27822,"web_url":"https://patchwork.libcamera.org/comment/27822/","msgid":"","date":"2023-09-21T08:51:44","subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: converter: make\n\tusing MediaDevice optional for the Converter","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Andrey\n\nOn Wed, Sep 20, 2023 at 06:19:19PM +0300, Andrey Konovalov via libcamera-devel wrote:\n> Make Converter a bit more generic by making pointer to MediaDevice\n> an optional argument for Converter::Converter(),\n> ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),\n> and ConverterFactory::createInstance() functions.\n\n.. to prepare for adding support for coverters not backed by a media\ndevice.\n\n>\n> Instead of the MediaDevice driver name, use a generic string to match\n> against the convertor factory name and its compatibles list. For\n\ns/convertor/converter\n\n> MediaDevice based converters this string will be the MediaDevice driver\n> name as before.\n>\n> Signed-off-by: Andrey Konovalov \n> ---\n> include/libcamera/internal/converter.h | 9 ++--\n> src/libcamera/converter.cpp | 53 ++++++++++++++----------\n> src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----\n> 3 files changed, 60 insertions(+), 35 deletions(-)\n>\n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 834ec5ab..d0c70296 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -2,6 +2,7 @@\n> /*\n> * Copyright (C) 2020, Laurent Pinchart\n> * Copyright 2022 NXP\n> + * Copyright 2023, Linaro Ltd\n> *\n> * converter.h - Generic format converter interface\n> */\n> @@ -31,7 +32,7 @@ struct StreamConfiguration;\n> class Converter\n> {\n> public:\n> -\tConverter(MediaDevice *media);\n> +\tConverter(MediaDevice *media = nullptr);\n> \tvirtual ~Converter();\n>\n> \tvirtual int loadConfiguration(const std::string &filename) = 0;\n> @@ -72,7 +73,7 @@ public:\n>\n> \tconst std::vector &compatibles() const { return compatibles_; }\n>\n> -\tstatic std::unique_ptr create(MediaDevice *media);\n> +\tstatic std::unique_ptr create(std::string name, MediaDevice *media = nullptr);\n\nI'm debated. Should we overload create() instead ?\n\nWhat's others opinion here ?\n\n> \tstatic std::vector &factories();\n> \tstatic std::vector names();\n>\n> @@ -81,7 +82,7 @@ private:\n>\n> \tstatic void registerType(ConverterFactoryBase *factory);\n>\n> -\tvirtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n> +\tvirtual std::unique_ptr createInstance(MediaDevice *media = nullptr) const = 0;\n>\n> \tstd::string name_;\n> \tstd::vector compatibles_;\n> @@ -96,7 +97,7 @@ public:\n> \t{\n> \t}\n>\n> -\tstd::unique_ptr createInstance(MediaDevice *media) const override\n> +\tstd::unique_ptr createInstance(MediaDevice *media = nullptr) const override\n> \t{\n> \t\treturn std::make_unique<_Converter>(media);\n> \t}\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 466f8421..5070eabc 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -1,6 +1,7 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> * Copyright 2022 NXP\n> + * Copyright 2023 Linaro Ltd\n> *\n> * converter.cpp - Generic format converter interface\n> */\n> @@ -13,8 +14,6 @@\n>\n> #include \"libcamera/internal/media_device.h\"\n>\n> -#include \"linux/media.h\"\n> -\n\nunrelated to this change, isn't it ?\n\n> /**\n> * \\file internal/converter.h\n> * \\brief Abstract converter\n> @@ -38,26 +37,28 @@ LOG_DEFINE_CATEGORY(Converter)\n>\n> /**\n> * \\brief Construct a Converter instance\n> - * \\param[in] media The media device implementing the converter\n> + * \\param[in] media The media device implementing the converter (optional)\n> *\n> * This searches for the entity implementing the data streaming function in the\n> * media graph entities and use its device node as the converter device node.\n> */\n> Converter::Converter(MediaDevice *media)\n> {\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> +\tif (media) {\n\nor\n if (!media)\n return;\n\n> +\t\tconst std::vector &entities = media->entities();\n> +\t\tauto it = std::find_if(entities.begin(), entities.end(),\n> +\t\t\t\t [](MediaEntity *entity) {\n> +\t\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n> +\t\t\t\t });\n> +\t\tif (it == entities.end()) {\n> +\t\t\tLOG(Converter, Error)\n> +\t\t\t\t<< \"No entity suitable for implementing a converter in \"\n> +\t\t\t\t<< media->driver() << \" entities list.\";\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tdeviceNode_ = (*it)->deviceNode();\n> \t}\n> -\n> -\tdeviceNode_ = (*it)->deviceNode();\n> }\n>\n> Converter::~Converter()\n> @@ -162,7 +163,8 @@ Converter::~Converter()\n> /**\n> * \\fn Converter::deviceNode()\n> * \\brief The converter device node attribute accessor\n> - * \\return The converter device node string\n> + * \\return The converter device node string. If there is no device node for\n> + * the converter returnes an empty string.\n\ns/returnes/returns/\n\n> */\n>\n> /**\n> @@ -203,8 +205,13 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali\n> */\n>\n> /**\n> - * \\brief Create an instance of the converter corresponding to the media device\n> - * \\param[in] media the media device to create the converter for\n> + * \\brief Create an instance of the converter corresponding to the converter\n> + * name\n> + * \\param[in] name the name of the converter to create\n ^ The\n\n> + * \\param[in] media the media device to create the converter for (optional)\n> + *\n> + * The converter \\a name must match the name of the converter factory, or one\n> + * of its compatibles.\n> *\n> * \\return A unique pointer to a new instance of the converter subclass\n> * corresponding to the media device. The converter is created by the factory\n\nYou should also modify this one\n\n> @@ -212,22 +219,22 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali\n> * If the media device driver name doesn't match anything a null pointer is\n> * returned.\n> */\n> -std::unique_ptr ConverterFactoryBase::create(MediaDevice *media)\n> +std::unique_ptr ConverterFactoryBase::create(std::string name, MediaDevice *media)\n> {\n> \tconst std::vector &factories =\n> \t\tConverterFactoryBase::factories();\n>\n> \tfor (const ConverterFactoryBase *factory : factories) {\n> \t\tconst std::vector &compatibles = factory->compatibles();\n> -\t\tauto it = std::find(compatibles.begin(), compatibles.end(), media->driver());\n> +\t\tauto it = std::find(compatibles.begin(), compatibles.end(), name);\n>\n> -\t\tif (it == compatibles.end() && media->driver() != factory->name_)\n> +\t\tif (it == compatibles.end() && name != factory->name_)\n> \t\t\tcontinue;\n>\n> \t\tLOG(Converter, Debug)\n> \t\t\t<< \"Creating converter from \"\n> \t\t\t<< factory->name_ << \" factory with \"\n> -\t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n> +\t\t\t<< (it == compatibles.end() ? \"no\" : name) << \" alias.\";\n>\n> \t\tstd::unique_ptr converter = factory->createInstance(media);\n> \t\tif (converter->isValid())\n> @@ -318,7 +325,7 @@ std::vector &ConverterFactoryBase::factories()\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> + * \\param[in] media Media device pointer (optional)\n> * \\return A unique pointer to a newly constructed instance of the Converter\n> * subclass corresponding to the factory\n> */\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 05ba76bc..3f1b523a 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -178,20 +178,26 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n>\n> class SimplePipelineHandler;\n>\n> +enum class ConverterFlag {\n> +\tNoFlag = 0,\n> +\tMediaDevice = (1 << 0),\n> +};\n> +using ConverterFlags = Flags;\n> +\n\nDo you expect this to be used as a bitmask or a plain enum would be\nenough ? Not that it makes that much difference after all..\n\n> struct SimplePipelineInfo {\n> \tconst char *driver;\n> \t/*\n> \t * Each converter in the list contains the name\n> \t * and the number of streams it supports.\n> \t */\n> -\tstd::vector> converters;\n> +\tstd::vector> converters;\n> };\n>\n> namespace {\n>\n> static const SimplePipelineInfo supportedDevices[] = {\n> \t{ \"dcmipp\", {} },\n> -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> +\t{ \"imx7-csi\", { { ConverterFlag::MediaDevice, \"pxp\", 1 } } },\n> \t{ \"j721e-csi2rx\", {} },\n> \t{ \"mxc-isi\", {} },\n> \t{ \"qcom-camss\", {} },\n> @@ -330,6 +336,7 @@ public:\n>\n> \tV4L2VideoDevice *video(const MediaEntity *entity);\n> \tV4L2Subdevice *subdev(const MediaEntity *entity);\n> +\tconst char *converterName() { return converterName_; }\n> \tMediaDevice *converter() { return converter_; }\n>\n> protected:\n> @@ -358,6 +365,7 @@ private:\n> \tMediaDevice *media_;\n> \tstd::map entities_;\n>\n> +\tconst char *converterName_;\n> \tMediaDevice *converter_;\n> };\n>\n> @@ -496,8 +504,10 @@ int SimpleCameraData::init()\n>\n> \t/* Open the converter, if any. */\n> \tMediaDevice *converter = pipe->converter();\n\nyou could drop this\n\n> -\tif (converter) {\n> -\t\tconverter_ = ConverterFactoryBase::create(converter);\n> +\tconst char *converterName = pipe->converterName();\n> +\tif (converterName) {\n> +\t\tLOG(SimplePipeline, Info) << \"Creating converter '\" << converterName << \"'\";\n> +\t\tconverter_ = ConverterFactoryBase::create(std::string(converterName), converter);\n\nAnd\n\t\tconverter_ = ConverterFactoryBase::create(std::string(converterName),\n pipe->converter());\n\n> \t\tif (!converter_) {\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> @@ -1409,10 +1419,17 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> \tif (!media_)\n> \t\treturn false;\n>\n> -\tfor (const auto &[name, streams] : info->converters) {\n> -\t\tDeviceMatch converterMatch(name);\n> -\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n> -\t\tif (converter_) {\n> +\tfor (const auto &[flags, name, streams] : info->converters) {\n> +\t\tif (flags & ConverterFlag::MediaDevice) {\n> +\t\t\tDeviceMatch converterMatch(name);\n> +\t\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n> +\t\t\tif (converter_) {\n> +\t\t\t\tconverterName_ = name;\n> +\t\t\t\tnumStreams = streams;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tconverterName_ = name;\n> \t\t\tnumStreams = streams;\n> \t\t\tbreak;\n> \t\t}\n\nIsn't this equivalent to:\n\n\tfor (const auto &[flags, name, streams] : info->converters) {\n\t\tif (flags & ConverterFlag::MediaDevice) {\n\t\t\tDeviceMatch converterMatch(name);\n\t\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n\t\t\tif (!converter_) {\n\t\t\t\tLOG(SimplePipeline, Debug)\n\t\t\t\t\t<< \"Failed to acquire converter media device\";\n\t\t\t\tcontinue;\n\t\t\t}\n\t\t}\n\n\t\tconverterName_ = name;\n\t\tnumStreams = streams;\n\t\tbreak;\n\t}\n?\n\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5538FBE080\n\tfor ;\n\tThu, 21 Sep 2023 08:51:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C0F36291F;\n\tThu, 21 Sep 2023 10:51:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 548FA61DE7\n\tfor ;\n\tThu, 21 Sep 2023 10:51:48 +0200 (CEST)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D622410FE;\n\tThu, 21 Sep 2023 10:50:10 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695286310;\n\tbh=A/igXnbuacIXZJPD2A79o2WiiB8jisdXJhU/3sbg1rk=;\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=HFazuFzx4uC6hqsq6npM+CIUqqlnWRe+nR5nr+BwoD0Z8BuI+uJ4aBP0Qq/Fq4nda\n\t9kd2qaa+Pa0Kp/wrCpPU7amPpZTtPoFlWVex/NaQFXHCUWYQfNnjxqcMuOEHSSPU52\n\ta9WDVw7wGSrDlJjRw7EfTmgcplB/bb4VOhZIlFHpYdHZQ3Rg90TQHORFFVzEMN7WkN\n\tgOSgGHzKnK693lFhJ2lJdVoLh7WtD27VBY3bwxJZmpDaayr+p/FAQRnm0lzpEb7mBT\n\tBN47gHP4E+5MhYWMuuwuQ1hKIi2LOdnwV7P/TUuQiViq+PYWbCKWVwGrABBivY72Ix\n\tB6uVGMzsAF7XQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1695286211;\n\tbh=A/igXnbuacIXZJPD2A79o2WiiB8jisdXJhU/3sbg1rk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZZnT7ExEF4sEOEl6Cw/+s4MfpXBQGn7aM8z703C+SIqfBQDvKN9rd7y6V9LHVIYsg\n\tnDGWmgOteh0vS/kdMkpOF1GCaJoFM6Fo7ZZ9N+cM8G5YmbIpWMvXU7DnCl2QKp2Kta\n\t+qBHZlr+855qVEP6F/C/FO2eYgtkuqMnmcwPfTY0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ZZnT7ExE\"; dkim-atps=neutral","Date":"Thu, 21 Sep 2023 10:51:44 +0200","To":"Andrey Konovalov ","Message-ID":"","References":"<20230920151921.31273-1-andrey.konovalov@linaro.org>\n\t<20230920151921.31273-3-andrey.konovalov@linaro.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230920151921.31273-3-andrey.konovalov@linaro.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: converter: make\n\tusing MediaDevice optional for the Converter","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"jacopo.mondi@ideasonboard.com, bryan.odonoghue@linaro.org,\n\tlibcamera-devel@lists.libcamera.org, srinivas.kandagatla@linaro.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27839,"web_url":"https://patchwork.libcamera.org/comment/27839/","msgid":"","date":"2023-09-21T18:52:13","subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: converter: make\n\tusing MediaDevice optional for the Converter","submitter":{"id":25,"url":"https://patchwork.libcamera.org/api/people/25/","name":"Andrey Konovalov","email":"andrey.konovalov@linaro.org"},"content":"Hi Jacopo,\n\nThank you for the review!\n\nOn 21.09.2023 11:51, Jacopo Mondi wrote:\n> Hi Andrey\n> \n> On Wed, Sep 20, 2023 at 06:19:19PM +0300, Andrey Konovalov via libcamera-devel wrote:\n>> Make Converter a bit more generic by making pointer to MediaDevice\n>> an optional argument for Converter::Converter(),\n>> ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),\n>> and ConverterFactory::createInstance() functions.\n> \n> .. to prepare for adding support for coverters not backed by a media\n> device.\n\nWill add that.\n\n>>\n>> Instead of the MediaDevice driver name, use a generic string to match\n>> against the convertor factory name and its compatibles list. For\n> \n> s/convertor/converter\n\nWill fix.\n\n>> MediaDevice based converters this string will be the MediaDevice driver\n>> name as before.\n>>\n>> Signed-off-by: Andrey Konovalov \n>> ---\n>> include/libcamera/internal/converter.h | 9 ++--\n>> src/libcamera/converter.cpp | 53 ++++++++++++++----------\n>> src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----\n>> 3 files changed, 60 insertions(+), 35 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>> index 834ec5ab..d0c70296 100644\n>> --- a/include/libcamera/internal/converter.h\n>> +++ b/include/libcamera/internal/converter.h\n>> @@ -2,6 +2,7 @@\n>> /*\n>> * Copyright (C) 2020, Laurent Pinchart\n>> * Copyright 2022 NXP\n>> + * Copyright 2023, Linaro Ltd\n>> *\n>> * converter.h - Generic format converter interface\n>> */\n>> @@ -31,7 +32,7 @@ struct StreamConfiguration;\n>> class Converter\n>> {\n>> public:\n>> -\tConverter(MediaDevice *media);\n>> +\tConverter(MediaDevice *media = nullptr);\n>> \tvirtual ~Converter();\n>>\n>> \tvirtual int loadConfiguration(const std::string &filename) = 0;\n>> @@ -72,7 +73,7 @@ public:\n>>\n>> \tconst std::vector &compatibles() const { return compatibles_; }\n>>\n>> -\tstatic std::unique_ptr create(MediaDevice *media);\n>> +\tstatic std::unique_ptr create(std::string name, MediaDevice *media = nullptr);\n> \n> I'm debated. Should we overload create() instead ?\n\nProbably. But currently I am not sure.\n\n> What's others opinion here ?\n\nIf there are no comments from the others I'll send the next version still using the default arg.\nBut we can always get back to this later (even in the next versions of this patch set).\n\n>> \tstatic std::vector &factories();\n>> \tstatic std::vector names();\n>>\n>> @@ -81,7 +82,7 @@ private:\n>>\n>> \tstatic void registerType(ConverterFactoryBase *factory);\n>>\n>> -\tvirtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n>> +\tvirtual std::unique_ptr createInstance(MediaDevice *media = nullptr) const = 0;\n>>\n>> \tstd::string name_;\n>> \tstd::vector compatibles_;\n>> @@ -96,7 +97,7 @@ public:\n>> \t{\n>> \t}\n>>\n>> -\tstd::unique_ptr createInstance(MediaDevice *media) const override\n>> +\tstd::unique_ptr createInstance(MediaDevice *media = nullptr) const override\n>> \t{\n>> \t\treturn std::make_unique<_Converter>(media);\n>> \t}\n>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n>> index 466f8421..5070eabc 100644\n>> --- a/src/libcamera/converter.cpp\n>> +++ b/src/libcamera/converter.cpp\n>> @@ -1,6 +1,7 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> * Copyright 2022 NXP\n>> + * Copyright 2023 Linaro Ltd\n>> *\n>> * converter.cpp - Generic format converter interface\n>> */\n>> @@ -13,8 +14,6 @@\n>>\n>> #include \"libcamera/internal/media_device.h\"\n>>\n>> -#include \"linux/media.h\"\n>> -\n> \n> unrelated to this change, isn't it ?\n\nI'll create a separate patch for this change.\nWhat should I credit you with in the commit message as it was you who spotted this\nissue and suggested to fix it?\n\n>> /**\n>> * \\file internal/converter.h\n>> * \\brief Abstract converter\n>> @@ -38,26 +37,28 @@ LOG_DEFINE_CATEGORY(Converter)\n>>\n>> /**\n>> * \\brief Construct a Converter instance\n>> - * \\param[in] media The media device implementing the converter\n>> + * \\param[in] media The media device implementing the converter (optional)\n>> *\n>> * This searches for the entity implementing the data streaming function in the\n>> * media graph entities and use its device node as the converter device node.\n>> */\n>> Converter::Converter(MediaDevice *media)\n>> {\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>> +\tif (media) {\n> \n> or\n> if (!media)\n> return;\n\nOK\n\n>> +\t\tconst std::vector &entities = media->entities();\n>> +\t\tauto it = std::find_if(entities.begin(), entities.end(),\n>> +\t\t\t\t [](MediaEntity *entity) {\n>> +\t\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n>> +\t\t\t\t });\n>> +\t\tif (it == entities.end()) {\n>> +\t\t\tLOG(Converter, Error)\n>> +\t\t\t\t<< \"No entity suitable for implementing a converter in \"\n>> +\t\t\t\t<< media->driver() << \" entities list.\";\n>> +\t\t\treturn;\n>> +\t\t}\n>> +\n>> +\t\tdeviceNode_ = (*it)->deviceNode();\n>> \t}\n>> -\n>> -\tdeviceNode_ = (*it)->deviceNode();\n>> }\n>>\n>> Converter::~Converter()\n>> @@ -162,7 +163,8 @@ Converter::~Converter()\n>> /**\n>> * \\fn Converter::deviceNode()\n>> * \\brief The converter device node attribute accessor\n>> - * \\return The converter device node string\n>> + * \\return The converter device node string. If there is no device node for\n>> + * the converter returnes an empty string.\n> \n> s/returnes/returns/\n\nWill fix\n\n>> */\n>>\n>> /**\n>> @@ -203,8 +205,13 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali\n>> */\n>>\n>> /**\n>> - * \\brief Create an instance of the converter corresponding to the media device\n>> - * \\param[in] media the media device to create the converter for\n>> + * \\brief Create an instance of the converter corresponding to the converter\n>> + * name\n>> + * \\param[in] name the name of the converter to create\n> ^ The\n\nRight. Will fix that\n\n>> + * \\param[in] media the media device to create the converter for (optional)\n>> + *\n>> + * The converter \\a name must match the name of the converter factory, or one\n>> + * of its compatibles.\n>> *\n>> * \\return A unique pointer to a new instance of the converter subclass\n>> * corresponding to the media device. The converter is created by the factory\n> \n> You should also modify this one\n\nWill do\n\n>> @@ -212,22 +219,22 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali\n>> * If the media device driver name doesn't match anything a null pointer is\n>> * returned.\n>> */\n>> -std::unique_ptr ConverterFactoryBase::create(MediaDevice *media)\n>> +std::unique_ptr ConverterFactoryBase::create(std::string name, MediaDevice *media)\n>> {\n>> \tconst std::vector &factories =\n>> \t\tConverterFactoryBase::factories();\n>>\n>> \tfor (const ConverterFactoryBase *factory : factories) {\n>> \t\tconst std::vector &compatibles = factory->compatibles();\n>> -\t\tauto it = std::find(compatibles.begin(), compatibles.end(), media->driver());\n>> +\t\tauto it = std::find(compatibles.begin(), compatibles.end(), name);\n>>\n>> -\t\tif (it == compatibles.end() && media->driver() != factory->name_)\n>> +\t\tif (it == compatibles.end() && name != factory->name_)\n>> \t\t\tcontinue;\n>>\n>> \t\tLOG(Converter, Debug)\n>> \t\t\t<< \"Creating converter from \"\n>> \t\t\t<< factory->name_ << \" factory with \"\n>> -\t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n>> +\t\t\t<< (it == compatibles.end() ? \"no\" : name) << \" alias.\";\n>>\n>> \t\tstd::unique_ptr converter = factory->createInstance(media);\n>> \t\tif (converter->isValid())\n>> @@ -318,7 +325,7 @@ std::vector &ConverterFactoryBase::factories()\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>> + * \\param[in] media Media device pointer (optional)\n>> * \\return A unique pointer to a newly constructed instance of the Converter\n>> * subclass corresponding to the factory\n>> */\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 05ba76bc..3f1b523a 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -178,20 +178,26 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n>>\n>> class SimplePipelineHandler;\n>>\n>> +enum class ConverterFlag {\n>> +\tNoFlag = 0,\n>> +\tMediaDevice = (1 << 0),\n>> +};\n>> +using ConverterFlags = Flags;\n>> +\n> \n> Do you expect this to be used as a bitmask or a plain enum would be\n> enough ? Not that it makes that much difference after all..\n\nFor now a plain enum (or even bool) would be enough.\nBut having a bitmask might become useful if in future some of different Converter\nsubclasses could share some features. And it doesn't add a lot of complexity, so\nI'd probably keep it.\n\n>> struct SimplePipelineInfo {\n>> \tconst char *driver;\n>> \t/*\n>> \t * Each converter in the list contains the name\n>> \t * and the number of streams it supports.\n>> \t */\n>> -\tstd::vector> converters;\n>> +\tstd::vector> converters;\n>> };\n>>\n>> namespace {\n>>\n>> static const SimplePipelineInfo supportedDevices[] = {\n>> \t{ \"dcmipp\", {} },\n>> -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n>> +\t{ \"imx7-csi\", { { ConverterFlag::MediaDevice, \"pxp\", 1 } } },\n>> \t{ \"j721e-csi2rx\", {} },\n>> \t{ \"mxc-isi\", {} },\n>> \t{ \"qcom-camss\", {} },\n>> @@ -330,6 +336,7 @@ public:\n>>\n>> \tV4L2VideoDevice *video(const MediaEntity *entity);\n>> \tV4L2Subdevice *subdev(const MediaEntity *entity);\n>> +\tconst char *converterName() { return converterName_; }\n>> \tMediaDevice *converter() { return converter_; }\n>>\n>> protected:\n>> @@ -358,6 +365,7 @@ private:\n>> \tMediaDevice *media_;\n>> \tstd::map entities_;\n>>\n>> +\tconst char *converterName_;\n>> \tMediaDevice *converter_;\n>> };\n>>\n>> @@ -496,8 +504,10 @@ int SimpleCameraData::init()\n>>\n>> \t/* Open the converter, if any. */\n>> \tMediaDevice *converter = pipe->converter();\n> \n> you could drop this\n> \n>> -\tif (converter) {\n>> -\t\tconverter_ = ConverterFactoryBase::create(converter);\n>> +\tconst char *converterName = pipe->converterName();\n>> +\tif (converterName) {\n>> +\t\tLOG(SimplePipeline, Info) << \"Creating converter '\" << converterName << \"'\";\n>> +\t\tconverter_ = ConverterFactoryBase::create(std::string(converterName), converter);\n> \n> And\n> \t\tconverter_ = ConverterFactoryBase::create(std::string(converterName),\n> pipe->converter());\n\nOK, sounds good.\n\n>> \t\tif (!converter_) {\n>> \t\t\tLOG(SimplePipeline, Warning)\n>> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n>> @@ -1409,10 +1419,17 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n>> \tif (!media_)\n>> \t\treturn false;\n>>\n>> -\tfor (const auto &[name, streams] : info->converters) {\n>> -\t\tDeviceMatch converterMatch(name);\n>> -\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n>> -\t\tif (converter_) {\n>> +\tfor (const auto &[flags, name, streams] : info->converters) {\n>> +\t\tif (flags & ConverterFlag::MediaDevice) {\n>> +\t\t\tDeviceMatch converterMatch(name);\n>> +\t\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n>> +\t\t\tif (converter_) {\n>> +\t\t\t\tconverterName_ = name;\n>> +\t\t\t\tnumStreams = streams;\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t} else {\n>> +\t\t\tconverterName_ = name;\n>> \t\t\tnumStreams = streams;\n>> \t\t\tbreak;\n>> \t\t}\n> \n> Isn't this equivalent to:\n> \n> \tfor (const auto &[flags, name, streams] : info->converters) {\n> \t\tif (flags & ConverterFlag::MediaDevice) {\n> \t\t\tDeviceMatch converterMatch(name);\n> \t\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n> \t\t\tif (!converter_) {\n> \t\t\t\tLOG(SimplePipeline, Debug)\n> \t\t\t\t\t<< \"Failed to acquire converter media device\";\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n> \n> \t\tconverterName_ = name;\n> \t\tnumStreams = streams;\n> \t\tbreak;\n> \t}\n> ?\n\nYep, it is. Will update the patch.\n\n\nThanks,\nAndrey\n\n>> --\n>> 2.34.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 91D95BE080\n\tfor ;\n\tThu, 21 Sep 2023 18:52:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE1C562931;\n\tThu, 21 Sep 2023 20:52:16 +0200 (CEST)","from mail-wr1-x430.google.com (mail-wr1-x430.google.com\n\t[IPv6:2a00:1450:4864:20::430])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B476628D8\n\tfor ;\n\tThu, 21 Sep 2023 20:52:15 +0200 (CEST)","by mail-wr1-x430.google.com with SMTP id\n\tffacd0b85a97d-307d58b3efbso1257063f8f.0\n\tfor ;\n\tThu, 21 Sep 2023 11:52:15 -0700 (PDT)","from [192.168.118.20] ([87.116.161.81])\n\tby smtp.gmail.com with ESMTPSA id\n\tl5-20020a170906a40500b009ae4ead6c01sm1439303ejz.163.2023.09.21.11.52.13\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tThu, 21 Sep 2023 11:52:14 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695322336;\n\tbh=upGSed4RII6M8dlWjRvTph+Mrqja2PGUIktLoUUXfhQ=;\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=IanmOrcBurDLyq0Ky7sbSo+HGQ4rygl376BNfxRnC0K4os7iHGqP+NSsWRypjD949\n\t0QGDq6jrb9milwpMbvSItvEDuyr/qLXgYpCvwfOhfmNIrko0QZZkZKmw0TcWTLKlZN\n\tm9mfocWMmvRSKkob52ZEtvhT8UmaKIIPcDWfRjMxJ2Yg8WIxSM2lWcORLilaDqI2Kn\n\tdxsJU/wpQJTtpKuUv8KZc2NYrzBXr0madEjKYaPjIy9JZc2ioG5Kv4+tlaUiKPZcPX\n\tngN9hUJCNLgxSgZN6SDYMkBmA+onuf0NOysUEEkuDOqtH+QDkM0IsEyH/7P+Nh9PN5\n\tI7AiHPH01QREg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=linaro.org; s=google; t=1695322335; x=1695927135;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Pk69MoVa1SuxwfAXlUP9NF044QzyzGsIrDs/IG9YTGg=;\n\tb=qcsTsKx6ptsY/j2u0vmotAiP/6ft2T/DcuKUrUHmByTsUcfyk58w/qTlKu/rcmS/ls\n\tGoDq8bD2S10u1GAahlnnfUSdh5KdrJKpzN6hZmlbT9jIXrIHd/KwMSCS2B2x0vg+QLy7\n\tMnQ5L7LiWC1gl7ekWZCJFjDnfW34eDp4nEYqehArBZ9F4b6OT47cENF6CfhGLYnwA5OX\n\toc3RhgCDlAxT1hnIHXKSMeEl4mpJUCRag+yfzuB3inIPktfGduREheVY8kEvnoEVOosx\n\tPauOwfNW3RyO7OsE77yGbAO5EGyD1LVW1Kvj0WgF2zN2m1AYqLtEZAGOsG5WBhHx2gXd\n\tBlOg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=linaro.org\n\theader.i=@linaro.org header.b=\"qcsTsKx6\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1695322335; x=1695927135;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Pk69MoVa1SuxwfAXlUP9NF044QzyzGsIrDs/IG9YTGg=;\n\tb=YZS/+J5QSjgiRUvBPLVIHlqPoRUii50ZAmZmFnHheZCEOGdB/V/32ZVu+br8ZgccJW\n\tw8sdFuzUikEiRljQ3ZFdozXUNf+fd+1ID6CiBZXMjkRi9QbiBQCV7ig3/aJyG+ziq0aH\n\tGVX4nJPn3+1YFy38+I5pS1JabTvD0mW0zhv7AY7KTFMbHcSO7ufNOlSLVZ7OqOUufXih\n\tci1K0SZSrHQ/CW4H5lA82AcJrnNg++am/xkBXT1jWbNQ4mlW3rguGA0h3ODh7/PTzXmt\n\tT18MWPvd91eOoGZpWhmsgTIZHGcIO2KvLOp8wiSJ818fkSXNA1Kg6GSSOPIiAE8CLpDF\n\tBH1g==","X-Gm-Message-State":"AOJu0YzdLPSs6BTOv4b3FTBGISzU5NjgXIwKpDwUllAANg4ZvfgkwDYn\n\thNs4VRg8+/UE93q3V1xkZtk9Ig==","X-Google-Smtp-Source":"AGHT+IEkfuNRGZcimObT0s1alVr1VqDYT5ZGxh5mb93LA7UJ6yDMMPyqIvUiYrZ1oFjWp1vCdYJ3Ag==","X-Received":"by 2002:adf:f741:0:b0:320:77f:a982 with SMTP id\n\tz1-20020adff741000000b00320077fa982mr6026974wrp.45.1695322334552; \n\tThu, 21 Sep 2023 11:52:14 -0700 (PDT)","Message-ID":"","Date":"Thu, 21 Sep 2023 21:52:13 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.15.1","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20230920151921.31273-1-andrey.konovalov@linaro.org>\n\t<20230920151921.31273-3-andrey.konovalov@linaro.org>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: converter: make\n\tusing MediaDevice optional for the Converter","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Andrey Konovalov via libcamera-devel\n\t","Reply-To":"Andrey Konovalov ","Cc":"bryan.odonoghue@linaro.org, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27841,"web_url":"https://patchwork.libcamera.org/comment/27841/","msgid":"","date":"2023-09-22T07:25:12","subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: converter: make\n\tusing MediaDevice optional for the Converter","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"HI Andrey\n\nOn Thu, Sep 21, 2023 at 09:52:13PM +0300, Andrey Konovalov wrote:\n> Hi Jacopo,\n>\n> Thank you for the review!\n>\n> On 21.09.2023 11:51, Jacopo Mondi wrote:\n> > Hi Andrey\n> >\n> > On Wed, Sep 20, 2023 at 06:19:19PM +0300, Andrey Konovalov via libcamera-devel wrote:\n> > > Make Converter a bit more generic by making pointer to MediaDevice\n> > > an optional argument for Converter::Converter(),\n> > > ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),\n> > > and ConverterFactory::createInstance() functions.\n> >\n> > .. to prepare for adding support for coverters not backed by a media\n> > device.\n>\n> Will add that.\n>\n> > >\n> > > Instead of the MediaDevice driver name, use a generic string to match\n> > > against the convertor factory name and its compatibles list. For\n> >\n> > s/convertor/converter\n>\n> Will fix.\n>\n> > > MediaDevice based converters this string will be the MediaDevice driver\n> > > name as before.\n> > >\n> > > Signed-off-by: Andrey Konovalov \n> > > ---\n> > > include/libcamera/internal/converter.h | 9 ++--\n> > > src/libcamera/converter.cpp | 53 ++++++++++++++----------\n> > > src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----\n> > > 3 files changed, 60 insertions(+), 35 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> > > index 834ec5ab..d0c70296 100644\n> > > --- a/include/libcamera/internal/converter.h\n> > > +++ b/include/libcamera/internal/converter.h\n> > > @@ -2,6 +2,7 @@\n> > > /*\n> > > * Copyright (C) 2020, Laurent Pinchart\n> > > * Copyright 2022 NXP\n> > > + * Copyright 2023, Linaro Ltd\n> > > *\n> > > * converter.h - Generic format converter interface\n> > > */\n> > > @@ -31,7 +32,7 @@ struct StreamConfiguration;\n> > > class Converter\n> > > {\n> > > public:\n> > > -\tConverter(MediaDevice *media);\n> > > +\tConverter(MediaDevice *media = nullptr);\n> > > \tvirtual ~Converter();\n> > >\n> > > \tvirtual int loadConfiguration(const std::string &filename) = 0;\n> > > @@ -72,7 +73,7 @@ public:\n> > >\n> > > \tconst std::vector &compatibles() const { return compatibles_; }\n> > >\n> > > -\tstatic std::unique_ptr create(MediaDevice *media);\n> > > +\tstatic std::unique_ptr create(std::string name, MediaDevice *media = nullptr);\n> >\n> > I'm debated. Should we overload create() instead ?\n>\n> Probably. But currently I am not sure.\n>\n> > What's others opinion here ?\n>\n> If there are no comments from the others I'll send the next version still using the default arg.\n> But we can always get back to this later (even in the next versions of this patch set).\n>\n> > > \tstatic std::vector &factories();\n> > > \tstatic std::vector names();\n> > >\n> > > @@ -81,7 +82,7 @@ private:\n> > >\n> > > \tstatic void registerType(ConverterFactoryBase *factory);\n> > >\n> > > -\tvirtual std::unique_ptr createInstance(MediaDevice *media) const = 0;\n> > > +\tvirtual std::unique_ptr createInstance(MediaDevice *media = nullptr) const = 0;\n> > >\n> > > \tstd::string name_;\n> > > \tstd::vector compatibles_;\n> > > @@ -96,7 +97,7 @@ public:\n> > > \t{\n> > > \t}\n> > >\n> > > -\tstd::unique_ptr createInstance(MediaDevice *media) const override\n> > > +\tstd::unique_ptr createInstance(MediaDevice *media = nullptr) const override\n> > > \t{\n> > > \t\treturn std::make_unique<_Converter>(media);\n> > > \t}\n> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > index 466f8421..5070eabc 100644\n> > > --- a/src/libcamera/converter.cpp\n> > > +++ b/src/libcamera/converter.cpp\n> > > @@ -1,6 +1,7 @@\n> > > /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > /*\n> > > * Copyright 2022 NXP\n> > > + * Copyright 2023 Linaro Ltd\n> > > *\n> > > * converter.cpp - Generic format converter interface\n> > > */\n> > > @@ -13,8 +14,6 @@\n> > >\n> > > #include \"libcamera/internal/media_device.h\"\n> > >\n> > > -#include \"linux/media.h\"\n> > > -\n> >\n> > unrelated to this change, isn't it ?\n>\n> I'll create a separate patch for this change.\n> What should I credit you with in the commit message as it was you who spotted this\n> issue and suggested to fix it?\n>\n\nThat would be a Suggested-by: tag probably, but it's not that\nnecessary in this case. Up to you :)\n\nThanks\n j\n\n> > > /**\n> > > * \\file internal/converter.h\n> > > * \\brief Abstract converter\n> > > @@ -38,26 +37,28 @@ LOG_DEFINE_CATEGORY(Converter)\n> > >\n> > > /**\n> > > * \\brief Construct a Converter instance\n> > > - * \\param[in] media The media device implementing the converter\n> > > + * \\param[in] media The media device implementing the converter (optional)\n> > > *\n> > > * This searches for the entity implementing the data streaming function in the\n> > > * media graph entities and use its device node as the converter device node.\n> > > */\n> > > Converter::Converter(MediaDevice *media)\n> > > {\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> > > +\tif (media) {\n> >\n> > or\n> > if (!media)\n> > return;\n>\n> OK\n>\n> > > +\t\tconst std::vector &entities = media->entities();\n> > > +\t\tauto it = std::find_if(entities.begin(), entities.end(),\n> > > +\t\t\t\t [](MediaEntity *entity) {\n> > > +\t\t\t\t\t return entity->function() == MEDIA_ENT_F_IO_V4L;\n> > > +\t\t\t\t });\n> > > +\t\tif (it == entities.end()) {\n> > > +\t\t\tLOG(Converter, Error)\n> > > +\t\t\t\t<< \"No entity suitable for implementing a converter in \"\n> > > +\t\t\t\t<< media->driver() << \" entities list.\";\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +\t\tdeviceNode_ = (*it)->deviceNode();\n> > > \t}\n> > > -\n> > > -\tdeviceNode_ = (*it)->deviceNode();\n> > > }\n> > >\n> > > Converter::~Converter()\n> > > @@ -162,7 +163,8 @@ Converter::~Converter()\n> > > /**\n> > > * \\fn Converter::deviceNode()\n> > > * \\brief The converter device node attribute accessor\n> > > - * \\return The converter device node string\n> > > + * \\return The converter device node string. If there is no device node for\n> > > + * the converter returnes an empty string.\n> >\n> > s/returnes/returns/\n>\n> Will fix\n>\n> > > */\n> > >\n> > > /**\n> > > @@ -203,8 +205,13 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali\n> > > */\n> > >\n> > > /**\n> > > - * \\brief Create an instance of the converter corresponding to the media device\n> > > - * \\param[in] media the media device to create the converter for\n> > > + * \\brief Create an instance of the converter corresponding to the converter\n> > > + * name\n> > > + * \\param[in] name the name of the converter to create\n> > ^ The\n>\n> Right. Will fix that\n>\n> > > + * \\param[in] media the media device to create the converter for (optional)\n> > > + *\n> > > + * The converter \\a name must match the name of the converter factory, or one\n> > > + * of its compatibles.\n> > > *\n> > > * \\return A unique pointer to a new instance of the converter subclass\n> > > * corresponding to the media device. The converter is created by the factory\n> >\n> > You should also modify this one\n>\n> Will do\n>\n> > > @@ -212,22 +219,22 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali\n> > > * If the media device driver name doesn't match anything a null pointer is\n> > > * returned.\n> > > */\n> > > -std::unique_ptr ConverterFactoryBase::create(MediaDevice *media)\n> > > +std::unique_ptr ConverterFactoryBase::create(std::string name, MediaDevice *media)\n> > > {\n> > > \tconst std::vector &factories =\n> > > \t\tConverterFactoryBase::factories();\n> > >\n> > > \tfor (const ConverterFactoryBase *factory : factories) {\n> > > \t\tconst std::vector &compatibles = factory->compatibles();\n> > > -\t\tauto it = std::find(compatibles.begin(), compatibles.end(), media->driver());\n> > > +\t\tauto it = std::find(compatibles.begin(), compatibles.end(), name);\n> > >\n> > > -\t\tif (it == compatibles.end() && media->driver() != factory->name_)\n> > > +\t\tif (it == compatibles.end() && name != factory->name_)\n> > > \t\t\tcontinue;\n> > >\n> > > \t\tLOG(Converter, Debug)\n> > > \t\t\t<< \"Creating converter from \"\n> > > \t\t\t<< factory->name_ << \" factory with \"\n> > > -\t\t\t<< (it == compatibles.end() ? \"no\" : media->driver()) << \" alias.\";\n> > > +\t\t\t<< (it == compatibles.end() ? \"no\" : name) << \" alias.\";\n> > >\n> > > \t\tstd::unique_ptr converter = factory->createInstance(media);\n> > > \t\tif (converter->isValid())\n> > > @@ -318,7 +325,7 @@ std::vector &ConverterFactoryBase::factories()\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> > > + * \\param[in] media Media device pointer (optional)\n> > > * \\return A unique pointer to a newly constructed instance of the Converter\n> > > * subclass corresponding to the factory\n> > > */\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 05ba76bc..3f1b523a 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -178,20 +178,26 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> > >\n> > > class SimplePipelineHandler;\n> > >\n> > > +enum class ConverterFlag {\n> > > +\tNoFlag = 0,\n> > > +\tMediaDevice = (1 << 0),\n> > > +};\n> > > +using ConverterFlags = Flags;\n> > > +\n> >\n> > Do you expect this to be used as a bitmask or a plain enum would be\n> > enough ? Not that it makes that much difference after all..\n>\n> For now a plain enum (or even bool) would be enough.\n> But having a bitmask might become useful if in future some of different Converter\n> subclasses could share some features. And it doesn't add a lot of complexity, so\n> I'd probably keep it.\n>\n> > > struct SimplePipelineInfo {\n> > > \tconst char *driver;\n> > > \t/*\n> > > \t * Each converter in the list contains the name\n> > > \t * and the number of streams it supports.\n> > > \t */\n> > > -\tstd::vector> converters;\n> > > +\tstd::vector> converters;\n> > > };\n> > >\n> > > namespace {\n> > >\n> > > static const SimplePipelineInfo supportedDevices[] = {\n> > > \t{ \"dcmipp\", {} },\n> > > -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> > > +\t{ \"imx7-csi\", { { ConverterFlag::MediaDevice, \"pxp\", 1 } } },\n> > > \t{ \"j721e-csi2rx\", {} },\n> > > \t{ \"mxc-isi\", {} },\n> > > \t{ \"qcom-camss\", {} },\n> > > @@ -330,6 +336,7 @@ public:\n> > >\n> > > \tV4L2VideoDevice *video(const MediaEntity *entity);\n> > > \tV4L2Subdevice *subdev(const MediaEntity *entity);\n> > > +\tconst char *converterName() { return converterName_; }\n> > > \tMediaDevice *converter() { return converter_; }\n> > >\n> > > protected:\n> > > @@ -358,6 +365,7 @@ private:\n> > > \tMediaDevice *media_;\n> > > \tstd::map entities_;\n> > >\n> > > +\tconst char *converterName_;\n> > > \tMediaDevice *converter_;\n> > > };\n> > >\n> > > @@ -496,8 +504,10 @@ int SimpleCameraData::init()\n> > >\n> > > \t/* Open the converter, if any. */\n> > > \tMediaDevice *converter = pipe->converter();\n> >\n> > you could drop this\n> >\n> > > -\tif (converter) {\n> > > -\t\tconverter_ = ConverterFactoryBase::create(converter);\n> > > +\tconst char *converterName = pipe->converterName();\n> > > +\tif (converterName) {\n> > > +\t\tLOG(SimplePipeline, Info) << \"Creating converter '\" << converterName << \"'\";\n> > > +\t\tconverter_ = ConverterFactoryBase::create(std::string(converterName), converter);\n> >\n> > And\n> > \t\tconverter_ = ConverterFactoryBase::create(std::string(converterName),\n> > pipe->converter());\n>\n> OK, sounds good.\n>\n> > > \t\tif (!converter_) {\n> > > \t\t\tLOG(SimplePipeline, Warning)\n> > > \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> > > @@ -1409,10 +1419,17 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > > \tif (!media_)\n> > > \t\treturn false;\n> > >\n> > > -\tfor (const auto &[name, streams] : info->converters) {\n> > > -\t\tDeviceMatch converterMatch(name);\n> > > -\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n> > > -\t\tif (converter_) {\n> > > +\tfor (const auto &[flags, name, streams] : info->converters) {\n> > > +\t\tif (flags & ConverterFlag::MediaDevice) {\n> > > +\t\t\tDeviceMatch converterMatch(name);\n> > > +\t\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n> > > +\t\t\tif (converter_) {\n> > > +\t\t\t\tconverterName_ = name;\n> > > +\t\t\t\tnumStreams = streams;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > > +\t\t} else {\n> > > +\t\t\tconverterName_ = name;\n> > > \t\t\tnumStreams = streams;\n> > > \t\t\tbreak;\n> > > \t\t}\n> >\n> > Isn't this equivalent to:\n> >\n> > \tfor (const auto &[flags, name, streams] : info->converters) {\n> > \t\tif (flags & ConverterFlag::MediaDevice) {\n> > \t\t\tDeviceMatch converterMatch(name);\n> > \t\t\tconverter_ = acquireMediaDevice(enumerator, converterMatch);\n> > \t\t\tif (!converter_) {\n> > \t\t\t\tLOG(SimplePipeline, Debug)\n> > \t\t\t\t\t<< \"Failed to acquire converter media device\";\n> > \t\t\t\tcontinue;\n> > \t\t\t}\n> > \t\t}\n> >\n> > \t\tconverterName_ = name;\n> > \t\tnumStreams = streams;\n> > \t\tbreak;\n> > \t}\n> > ?\n>\n> Yep, it is. Will update the patch.\n>\n>\n> Thanks,\n> Andrey\n>\n> > > --\n> > > 2.34.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CB968C326B\n\tfor ;\n\tFri, 22 Sep 2023 07:25:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E431A62944;\n\tFri, 22 Sep 2023 09:25:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBDD562916\n\tfor ;\n\tFri, 22 Sep 2023 09:25:15 +0200 (CEST)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C77C18D0;\n\tFri, 22 Sep 2023 09:23:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1695367518;\n\tbh=QVfBISbVyjC9+eI00/KZWvl9PCsjV0k5LmzfX/i6kRE=;\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=2zKXi//zcJ3bgaV8J3GM/yRCHhVoAFg+w1pyLGnffZ2SoQzAGz31jThcxAa/p9ypr\n\t8Q0hmY8CIczNV/OQ3xTCTh5BjKyTPwkO0NfhT6pPEd1vate1Za+el+yIXLA8cdNc6L\n\tXYJCyzkZ0jNn3qWH0m8MOfgzL18Jwzg0kozhwOMmH2YGXY7n2lII/YsTS+DOfzv6GZ\n\tXuy1uiackWME16PBU/MrU2r3q3AOEVcmfvQdX6RZDR2+43y3ULws7Lfpz71jeS/gL+\n\tTmfZpkX6V7vrfGZAoMvRMyha1lWQCLPcI9BM2kIwTiP5e6KITZ3lRV5TD1Q17h3XhO\n\tqTRa9SNC48dCQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1695367417;\n\tbh=QVfBISbVyjC9+eI00/KZWvl9PCsjV0k5LmzfX/i6kRE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UhCs3gz9SB1+tA6JUrGFx77NNmt37rTPFlGH5EkI8VkUsK7A1ZlZ69+VR7jLk/5qJ\n\tLn47qkxNovbcRV2/ZYUNb68K5mOMsHxlmNDrcLhTc/QpcCDDIis0Ku5dv1dJwUBb0T\n\tu15JUGxHkJ+oDOq3QfvKHHUbw/pmZVzDHRZuHgaY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UhCs3gz9\"; dkim-atps=neutral","Date":"Fri, 22 Sep 2023 09:25:12 +0200","To":"Andrey Konovalov ","Message-ID":"","References":"<20230920151921.31273-1-andrey.konovalov@linaro.org>\n\t<20230920151921.31273-3-andrey.konovalov@linaro.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] libcamera: converter: make\n\tusing MediaDevice optional for the Converter","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi , bryan.odonoghue@linaro.org,\n\tlibcamera-devel@lists.libcamera.org, srinivas.kandagatla@linaro.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]