[{"id":27912,"web_url":"https://patchwork.libcamera.org/comment/27912/","msgid":"<874jjc2fzx.fsf@baylibre.com>","date":"2023-09-30T08:59:46","subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: converter: make\n\tusing MediaDevice optional for the Converter","submitter":{"id":153,"url":"https://patchwork.libcamera.org/api/people/153/","name":"Mattijs Korpershoek","email":"mkorpershoek@baylibre.com"},"content":"Hi Andrey,\n\nThank you for the patch\n\nOn jeu., sept. 28, 2023 at 21:55, Andrey Konovalov wrote:\n\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 to prepare for\n> adding support for coverters not backed by a media device.\n\nnit: s/coverters/converters\n\n>\n> Instead of the MediaDevice driver name, use a generic string to match\n> against the converter factory name and its compatibles list. For\n> MediaDevice based converters this string will be the MediaDevice driver\n> name as before.\n>\n> Signed-off-by: Andrey Konovalov \n\nReviewed-by: Mattijs Korpershoek \n\n> ---\n> include/libcamera/internal/converter.h | 9 +++---\n> src/libcamera/converter.cpp | 40 +++++++++++++++---------\n> src/libcamera/pipeline/simple/simple.cpp | 39 ++++++++++++++++-------\n> 3 files changed, 58 insertions(+), 30 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> \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 aca4fbc7..fe073bf2 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> @@ -36,13 +37,16 @@ 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> +\tif (!media)\n> +\t\treturn;\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> @@ -160,7 +164,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 returns an empty string.\n> */\n> \n> /**\n> @@ -201,32 +206,37 @@ 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> + * \\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. For media device based converters the converter \\a name\n> + * is the media device driver name.\n> + *\n> + * \\return A unique pointer to a new instance of the converter subclass.\n> + * The converter is created by matching the factory name or any of its\n> + * compatible aliases with the converter name.\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 matching\n> - * the factory name or any of its compatible aliases with the media device\n> - * driver name.\n> - * If the media device driver name doesn't match anything a null pointer is\n> - * returned.\n> + * If the converter name doesn't match anything a null pointer is 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> @@ -317,7 +327,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..c7da700b 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> 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> @@ -495,9 +503,10 @@ int SimpleCameraData::init()\n> \tint ret;\n> \n> \t/* Open the converter, if any. */\n> -\tMediaDevice *converter = pipe->converter();\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), pipe->converter());\n> \t\tif (!converter_) {\n> \t\t\tLOG(SimplePipeline, Warning)\n> \t\t\t\t<< \"Failed to create converter, disabling format conversion\";\n> @@ -1409,13 +1418,21 @@ 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> -\t\t\tnumStreams = streams;\n> -\t\t\tbreak;\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\t\t<< name << \"'\";\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> \t/* Locate the sensors. */\n> -- \n> 2.34.1","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 074C6C0F2A\n\tfor ;\n\tSat, 30 Sep 2023 08:59:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7686462960;\n\tSat, 30 Sep 2023 10:59:57 +0200 (CEST)","from mail-wm1-x330.google.com (mail-wm1-x330.google.com\n\t[IPv6:2a00:1450:4864:20::330])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5980C6295F\n\tfor ;\n\tSat, 30 Sep 2023 10:59:55 +0200 (CEST)","by mail-wm1-x330.google.com with SMTP id\n\t5b1f17b1804b1-40566f89f6eso121979545e9.3\n\tfor ;\n\tSat, 30 Sep 2023 01:59:55 -0700 (PDT)","from localhost\n\t(2a01cb06b0237dd3c01dd287ba46c248.ipv6.abo.wanadoo.fr.\n\t[2a01:cb06:b023:7dd3:c01d:d287:ba46:c248])\n\tby smtp.gmail.com with ESMTPSA id\n\tx11-20020a1c7c0b000000b003fe1c332810sm2954434wmc.33.2023.09.30.01.59.53\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 30 Sep 2023 01:59:54 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1696064397;\n\tbh=zN8dZotNtHKOKCltgSuPhzEtyf4X73p3qfZ4xeHWfho=;\n\th=To:In-Reply-To:References:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kwiBKjExpUyQT2ZtqT0OuS50EDz+h5am8ZUMIaGiNot9WODBC6OnuUFrgJp5E/hgh\n\t59MGYP/17IC3geaUQX/suCqfxsqVoEOPu/g+f85uNkZxXq7g/AWv6pmjE1hO6s5diP\n\t9nVINHH+XeZafjjkuYMQlda5OBWFvGNH7NjRrOuLEwCFV9xSuohJxNBv72LPE/KTis\n\tGlp+d8dvoo0WaUqhg0BLorhuVcj6Cj2/i+xutU9R4UGQ1p7ut2s0T3G4ibjRjqhce8\n\t3bYNZyK5z9YfcZburM6xd6fZ9A4kY7eGTh0fGw/S/doZgYu8j+7pKtYO36g94H4PS1\n\tPMoR3YweufpSQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1696064395;\n\tx=1696669195; darn=lists.libcamera.org; \n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:from:to:cc:subject:date:message-id:reply-to;\n\tbh=nBb9t3sZq7eQ8sbZ7nTZnIhZXCB1z7GNMMvj31RzpMM=;\n\tb=YUiQZVI+KYJyqUACMNtL4p68ywA8Jiuaqfavs3hwP5z6gT/B9l0njjd+SQEBUm1r8c\n\tSrbtyq3x9NuzY0RsWR/H5N5gF6mJ0wTTXZQ52TXAlHA+CZAA0tHBN1OhRkjNX5QHY4i4\n\tItAabJF4PvBrrpFDnyK0/494/7W1K72SMnGNyBlxIOjnBseQKe4c/lToRY/bu81GWQS2\n\ti3M3tLP1dkRqFquHmwYWz9zCDUaLlIRPHNRLTuiL0OlSpx74l/2cc7FWU9l93x+q1IgP\n\thrtUk0xul6kuyhVGFiF8d/zvVkzqIRg8glrDQvtLHQ0KJAAWeWq1GUh8S+kyPIPX9ToB\n\tZ1rQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=baylibre-com.20230601.gappssmtp.com\n\theader.i=@baylibre-com.20230601.gappssmtp.com header.b=\"YUiQZVI+\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1696064395; x=1696669195;\n\th=mime-version:message-id:date:references:in-reply-to:subject:cc:to\n\t:from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; \n\tbh=nBb9t3sZq7eQ8sbZ7nTZnIhZXCB1z7GNMMvj31RzpMM=;\n\tb=twGSVn36JdXQvbCEty7ZTT147y2y/atP+ti7AaUDrgWUD0Q5O4oqco1/gL1gAH5fU5\n\tgHJvTPi+dxsfJMBySOdPk/EEHB665agLYl8yrUG7OiFJrLm21AinG3qj+HI3Cmt54RlN\n\tVjrsNh7ZiHLBvQuDihtKne4IyYjiE6G+6YzkAXqqmj7FU8915ulcnngRtXwKNvzKblFC\n\ti7l8Ophg/4sMyXC2lioaNbdBC+C0S3/iYgvZIXGQFM6iltG+KuMQXX8N3DlTbouCUMw/\n\tSwa/CSicC5WOHqdxgQCthf8HeEg38VadQO+ggKunKfEFNv0zyVVKkU+iUXN3YV8apf/G\n\tcIbw==","X-Gm-Message-State":"AOJu0YwO9s1+eDd/s79FBCDb5PgYu4GkhlMn2fBGqDbAiF5yDcHKrsnH\n\txeXsPoFhofsDDPELttFZ9Y3BVQ==","X-Google-Smtp-Source":"AGHT+IG5McPQBS4JsdbY5P75kJwrNeT1nImeDM/WpMQxRkEGFkuNmjA9j5yL1BaP3pSplSz8+9qV0w==","X-Received":"by 2002:a05:600c:3b0e:b0:406:53ab:a9af with SMTP id\n\tm14-20020a05600c3b0e00b0040653aba9afmr5501200wms.10.1696064394746; \n\tSat, 30 Sep 2023 01:59:54 -0700 (PDT)","To":"Andrey Konovalov ,\n\tlibcamera-devel@lists.libcamera.org","In-Reply-To":"<20230928185537.20178-4-andrey.konovalov@linaro.org>","References":"<20230928185537.20178-1-andrey.konovalov@linaro.org>\n\t<20230928185537.20178-4-andrey.konovalov@linaro.org>","Date":"Sat, 30 Sep 2023 10:59:46 +0200","Message-ID":"<874jjc2fzx.fsf@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] 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":"Mattijs Korpershoek via libcamera-devel\n\t","Reply-To":"Mattijs Korpershoek ","Cc":"jacopo.mondi@ideasonboard.com, bryan.odonoghue@linaro.org,\n\tsrinivas.kandagatla@linaro.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]