[{"id":27097,"web_url":"https://patchwork.libcamera.org/comment/27097/","msgid":"<20230515094309.3z3q4mwbjhuh4r5x@lati>","date":"2023-05-15T09:43:09","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: enumeration:\n\tGeneralize DeviceMatch","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Sophie,\n\n a few drive-by cosmetic comments\n\nOn Thu, Mar 02, 2023 at 01:54:15AM +0100, Sophie Friedrich via libcamera-devel wrote:\n> Currently `DeviceMatch` is very specialized to handle `MediaDevice`s, which\n> has strong dependencies to V4L2 internals.\n>\n> This patch enables `DeviceMatch` to match using key/value pairs instead\n> of a simple list of strings and introduces a `DeviceMatchEntityInterface` to\n> abstract away the likes of `MediaEntity` in order to search for a\n> device.\n>\n> Signed-off-by: Sophie Friedrich <dev@flowerpot.me>\n> ---\n>  .../libcamera/internal/device_enumerator.h    |  16 +--\n>  include/libcamera/internal/device_match.h     |  46 +++++++\n>  include/libcamera/internal/media_device.h     |   1 +\n>  include/libcamera/internal/media_object.h     |   7 +-\n>  include/libcamera/internal/meson.build        |   1 +\n>  src/libcamera/device_enumerator.cpp           |  73 ----------\n>  src/libcamera/device_match.cpp                | 126 ++++++++++++++++++\n>  src/libcamera/meson.build                     |   1 +\n>  8 files changed, 183 insertions(+), 88 deletions(-)\n>  create mode 100644 include/libcamera/internal/device_match.h\n>  create mode 100644 src/libcamera/device_match.cpp\n>\n> diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h\n> index 72ec9a60..259a9e46 100644\n> --- a/include/libcamera/internal/device_enumerator.h\n> +++ b/include/libcamera/internal/device_enumerator.h\n> @@ -13,24 +13,12 @@\n>\n>  #include <libcamera/base/signal.h>\n>\n> +#include \"libcamera/internal/device_match.h\"\n> +\n\nYou could forward-declare the DeviceMatch class in the header and move\nthe inclusion directive to the cpp file to reduce the compile-time\ndependencies and improve the build times\n\n>  namespace libcamera {\n>\n>  class MediaDevice;\n>\n> -class DeviceMatch\n> -{\n> -public:\n> -\tDeviceMatch(const std::string &driver);\n> -\n> -\tvoid add(const std::string &entity);\n> -\n> -\tbool match(const MediaDevice *device) const;\n> -\n> -private:\n> -\tstd::string driver_;\n> -\tstd::vector<std::string> entities_;\n> -};\n> -\n>  class DeviceEnumerator\n>  {\n>  public:\n> diff --git a/include/libcamera/internal/device_match.h b/include/libcamera/internal/device_match.h\n> new file mode 100644\n> index 00000000..e4563d3c\n> --- /dev/null\n> +++ b/include/libcamera/internal/device_match.h\n> @@ -0,0 +1,46 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Sophie Friedrich\n> + *\n> + * device_match.h - API to match devices\n> + */\n> +\n> +#pragma once\n> +\n> +#include <string>\n> +#include <vector>\n> +\n> +#include <libcamera/base/class.h>\n> +\n> +\n\nMultiple blank lines\n\n> +namespace libcamera {\n> +\n> +class MediaDevice;\n> +\n> +class DeviceMatchEntityInterface\n> +{\n> +public:\n> +\tvirtual const std::string &match_key() const = 0;\n> +\tvirtual const std::string &match_value() const = 0;\n\nlibcamera uses camelCase and not snake_case\n\n> +\tvirtual ~DeviceMatchEntityInterface() = default;\n> +\n> +\tstatic const std::string MATCH_ALL_KEY;\n> +};\n> +\n> +class DeviceMatch\n> +{\n> +public:\n> +\tDeviceMatch(const std::string &driver);\n> +\n> +\tvoid add(const std::string &entity);\n> +\tvoid add(const std::string &key, const std::string &value);\n> +\n> +\tbool match(const MediaDevice *device) const;\n> +\n> +private:\n> +\tstd::string driver_;\n> +\tstd::vector<std::pair<std::string, std::string>> map_;\n\nI'm a bit skeptical about this mapping, see below\n\n> +};\n> +\n> +\n\nmultiple blank lines\n\n> +}\n> \\ No newline at end of file\n> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h\n> index eb8cfde4..b10bb2eb 100644\n> --- a/include/libcamera/internal/media_device.h\n> +++ b/include/libcamera/internal/media_device.h\n> @@ -19,6 +19,7 @@\n>  #include <libcamera/base/unique_fd.h>\n>\n>  #include \"libcamera/internal/media_object.h\"\n> +#include \"libcamera/internal/device_match.h\"\n\nAlphabetical order please\n\n>\n>  namespace libcamera {\n>\n> diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h\n> index b1572968..d07ed698 100644\n> --- a/include/libcamera/internal/media_object.h\n> +++ b/include/libcamera/internal/media_object.h\n> @@ -14,6 +14,8 @@\n>\n>  #include <libcamera/base/class.h>\n>\n> +#include \"libcamera/internal/device_match.h\"\n> +\n>  namespace libcamera {\n>\n>  class MediaDevice;\n> @@ -85,7 +87,7 @@ private:\n>  \tstd::vector<MediaLink *> links_;\n>  };\n>\n> -class MediaEntity : public MediaObject\n> +class MediaEntity : public MediaObject, public DeviceMatchEntityInterface\n>  {\n>  public:\n>  \tenum class Type {\n> @@ -111,6 +113,9 @@ public:\n>\n>  \tint setDeviceNode(const std::string &deviceNode);\n>\n> +\tconst std::string &match_key() const{ return DeviceMatchEntityInterface::MATCH_ALL_KEY; }\n> +\tconst std::string &match_value() const { return name_; }\n> +\n\nThis interface is designed to match the requirement of matching on a\nkey/value pair. I wonder if we could find a pattern to delegate\nmatching to the DeviceMatchEntityInterface derived classes. If you\nhave code already working for your usb camera I would like to see how\nit gets to look like\n\n>  private:\n>  \tLIBCAMERA_DISABLE_COPY_AND_MOVE(MediaEntity)\n>\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index d7508805..1e3c15ed 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([\n>      'control_validator.h',\n>      'converter.h',\n>      'delayed_controls.h',\n> +    'device_match.h',\n>      'device_enumerator.h',\n>      'device_enumerator_sysfs.h',\n>      'device_enumerator_udev.h',\n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index f2e055de..49afd783 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -40,79 +40,6 @@ namespace libcamera {\n>\n>  LOG_DEFINE_CATEGORY(DeviceEnumerator)\n>\n> -/**\n> - * \\class DeviceMatch\n> - * \\brief Description of a media device search pattern\n> - *\n> - * The DeviceMatch class describes a media device using properties from the\n> - * Media Controller struct media_device_info, entity names in the media graph\n> - * or other properties that can be used to identify a media device.\n> - *\n> - * The description is meant to be filled by pipeline managers and passed to a\n> - * device enumerator to find matching media devices.\n> - *\n> - * A DeviceMatch is created with a specific Linux device driver in mind,\n> - * therefore the name of the driver is a required property. One or more Entity\n> - * names can be added as match criteria.\n> - *\n> - * Pipeline handlers are recommended to add entities to DeviceMatch as\n> - * appropriare to ensure that the media device they need can be uniquely\n> - * identified. This is useful when the corresponding kernel driver can produce\n> - * different graphs, for instance as a result of different driver versions or\n> - * hardware configurations, and not all those graphs are suitable for a pipeline\n> - * handler.\n> - */\n> -\n> -/**\n> - * \\brief Construct a media device search pattern\n> - * \\param[in] driver The Linux device driver name that created the media device\n> - */\n> -DeviceMatch::DeviceMatch(const std::string &driver)\n> -\t: driver_(driver)\n> -{\n> -}\n> -\n> -/**\n> - * \\brief Add a media entity name to the search pattern\n> - * \\param[in] entity The name of the entity in the media graph\n> - */\n> -void DeviceMatch::add(const std::string &entity)\n> -{\n> -\tentities_.push_back(entity);\n> -}\n> -\n> -/**\n> - * \\brief Compare a search pattern with a media device\n> - * \\param[in] device The media device\n> - *\n> - * Matching is performed on the Linux device driver name and entity names from\n> - * the media graph. A match is found if both the driver name matches and the\n> - * media device contains all the entities listed in the search pattern.\n> - *\n> - * \\return true if the media device matches the search pattern, false otherwise\n> - */\n> -bool DeviceMatch::match(const MediaDevice *device) const\n> -{\n> -\tif (driver_ != device->driver())\n> -\t\treturn false;\n> -\n> -\tfor (const std::string &name : entities_) {\n> -\t\tbool found = false;\n> -\n> -\t\tfor (const MediaEntity *entity : device->entities()) {\n> -\t\t\tif (name == entity->name()) {\n> -\t\t\t\tfound = true;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\t}\n> -\n> -\t\tif (!found)\n> -\t\t\treturn false;\n> -\t}\n> -\n> -\treturn true;\n> -}\n> -\n>  /**\n>   * \\class DeviceEnumerator\n>   * \\brief Enumerate, store and search media devices\n> diff --git a/src/libcamera/device_match.cpp b/src/libcamera/device_match.cpp\n> new file mode 100644\n> index 00000000..0c205f93\n> --- /dev/null\n> +++ b/src/libcamera/device_match.cpp\n> @@ -0,0 +1,126 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Sophie Friedrich\n> + *\n> + * device_match.cpp - API to match devices\n> + */\n> +\n> +#include \"libcamera/internal/device_match.h\"\n> +\n> +#include <string.h>\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +#include \"libcamera/internal/media_device.h\"\n> +\n> +/**\n> + * \\file device_match.h\n> + * \\brief Matching of media devices\n> + *\n> + * Pipelines find compatible devices by matching against known strings or\n> + * key/value pairs during device enumeration. MediaDevice are responsible for\n> + * finding these entities during their population phase.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(DeviceMatch)\n> +\n> +/**\n> + * \\interface DeviceMatchInterface\n> + * \\brief Interface which enables matching using DeviceMatch\n> + *\n> + *\n> + *\n\nMultiple blank lines\n\n> + */\n> +const std::string DeviceMatchEntityInterface::MATCH_ALL_KEY = \"*\";\n> +\n> +/**\n> + * \\class DeviceMatch\n> + * \\brief Description of a media device search pattern\n> + *\n> + * The DeviceMatch class describes a media device using properties from the\n> + * Media Controller struct media_device_info, entity names in the media graph\n> + * or other properties that can be used to identify a media device.\n> + *\n> + * The description is meant to be filled by pipeline managers and passed to a\n> + * device enumerator to find matching media devices.\n> + *\n> + * A DeviceMatch is created with a specific Linux device driver in mind,\n> + * therefore the name of the driver is a required property. One or more Entity\n> + * names can be added as match criteria.\n> + *\n> + * Pipeline handlers are recommended to add entities to DeviceMatch as\n> + * appropriare to ensure that the media device they need can be uniquely\n> + * identified. This is useful when the corresponding kernel driver can produce\n> + * different graphs, for instance as a result of different driver versions or\n> + * hardware configurations, and not all those graphs are suitable for a pipeline\n> + * handler.\n> + */\n> +\n> +/**\n> + * \\brief Construct a media device search pattern\n> + * \\param[in] driver The Linux device driver name that created the media device\n> + */\n> +DeviceMatch::DeviceMatch(const std::string &driver)\n> +\t: driver_(driver)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Add a media entity name to the search pattern\n> + * \\param[in] entity The name of the entity in the media graph\n> + */\n> +void DeviceMatch::add(const std::string &entity)\n> +{\n> +\tadd(DeviceMatchEntityInterface::MATCH_ALL_KEY, entity);\n> +}\n> +\n> +/**\n> + * \\brief Add key / value pair to search pattern\n> + * \\param[in] key The key the value is associated to\n> + * \\param[in] value The value that should be searched for\n> + *\n> + * There is no uniqueness enforced to key/value. Mentioning a certain\n> + * key multiple times (in best case with different values) allows matching\n> + * multiple properties on the same key.\n\nIs this just to accommodate the usage of \"*\" as a key or is a\nrequirement for your use case too ?\n\n> + */\n> +void DeviceMatch::add(const std::string &key, const std::string &value)\n> +{\n> +\tmap_.push_back(std::pair(key, value));\n> +}\n> +\n> +/**\n> + * \\brief Compare a search pattern with a media device\n> + * \\param[in] device The media device\n> + *\n> + * Matching is performed on the Linux device driver name and entity names from\n> + * the media graph. A match is found if both the driver name matches and the\n> + * media device contains all the entities listed in the search pattern.\n> + *\n> + * \\return true if the media device matches the search pattern, false otherwise\n> + */\n> +bool DeviceMatch::match(const MediaDevice *device) const\n> +{\n> +\tif (driver_ != device->driver())\n> +\t\treturn false;\n> +\n> +\tfor (const std::pair<std::string, std::string> &item : map_) {\n> +\t\tbool found = false;\n> +\n> +\t\tfor (const DeviceMatchEntityInterface *entity : device->entities()) {\n> +\t\t\tif (item.first == entity->match_key() &&\n> +\t\t\t\titem.second == entity->match_value()) {\n\nAlign to the open ( please\n\nIf we could if (entity.match(item)) this would be nicer as it would\nabstract away the matching criteria from the DeviceMatch class and\ndelegate it to sub-classes. Let's ponder a bit if that's possible and\nhow to design it.\n\nThanks\n  j\n\n> +\t\t\t\tfound = true;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!found)\n> +\t\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +}\n> \\ No newline at end of file\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 9869bfe7..445eb8e9 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -15,6 +15,7 @@ libcamera_sources = files([\n>      'control_validator.cpp',\n>      'converter.cpp',\n>      'delayed_controls.cpp',\n> +    'device_match.cpp',\n>      'device_enumerator.cpp',\n>      'device_enumerator_sysfs.cpp',\n>      'fence.cpp',\n> --\n> 2.34.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 55467BDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 15 May 2023 09:43:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A94D8627D4;\n\tMon, 15 May 2023 11:43:14 +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 A69116039A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 15 May 2023 11:43:13 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF5086CF;\n\tMon, 15 May 2023 11:43:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684143794;\n\tbh=7EE3kOYZOi4GXIKsCYOjYqYVauSGAfNRcKA5HQskhV0=;\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=hW/C+n8Kf9OAVAyy9bhZzOEWS4a2zgMMawmaDdstwy1AN1PW8FFbXJxuM/XTo8JBy\n\t04ThcfQBxcMNNc+9mK084A1nGVW6NZLfry54rKU0ethixB4/0PkNwDGmj5f0CSUla8\n\tvHSkENQkrtpDsvlT/Wy26FAkXk990xajK89Fkfy5fuJWceb60AB07c4AMY0XnbQNB6\n\tuTSRxQaeWLseE6Ot5Rg519D/vWHIFRA9QjoP4GUs1JT5YuQwmPcdaIZQSvfUy5t1yl\n\teWqBVQQYwHHwa1Xk0h19OO6+346+xlnDNVyAz3FNGU5ILjTR38915hb7TQaUipRoxj\n\tYFNGh/xYBG6DQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1684143783;\n\tbh=7EE3kOYZOi4GXIKsCYOjYqYVauSGAfNRcKA5HQskhV0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GGadWewuc5+a60I0Wrzv4c1BPA9LR1NHGmDufULLSg3XKISx+idCXpx3EdUyBMdZl\n\tDa3ujsZx8WbzIEIBLwH5TfAYLku4YQmSMjZ16P0jUD8gZ7NQOFa9x1jyyxw5M78fsE\n\t1eM4kiC79emDzndpSd4Yfhz19n07FxI+Vmtje088="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GGadWewu\"; dkim-atps=neutral","Date":"Mon, 15 May 2023 11:43:09 +0200","To":"Sophie Friedrich <dev@flowerpot.me>","Message-ID":"<20230515094309.3z3q4mwbjhuh4r5x@lati>","References":"<20230302005415.1789544-1-dev@flowerpot.me>\n\t<20230302005415.1789544-2-dev@flowerpot.me>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230302005415.1789544-2-dev@flowerpot.me>","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: enumeration:\n\tGeneralize DeviceMatch","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]