[libcamera-devel] [PATCH 1/1] libcamera: enumeration: Generalize DeviceMatch
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon May 15 11:43:09 CEST 2023
Hi Sophie,
a few drive-by cosmetic comments
On Thu, Mar 02, 2023 at 01:54:15AM +0100, Sophie Friedrich via libcamera-devel wrote:
> Currently `DeviceMatch` is very specialized to handle `MediaDevice`s, which
> has strong dependencies to V4L2 internals.
>
> This patch enables `DeviceMatch` to match using key/value pairs instead
> of a simple list of strings and introduces a `DeviceMatchEntityInterface` to
> abstract away the likes of `MediaEntity` in order to search for a
> device.
>
> Signed-off-by: Sophie Friedrich <dev at flowerpot.me>
> ---
> .../libcamera/internal/device_enumerator.h | 16 +--
> include/libcamera/internal/device_match.h | 46 +++++++
> include/libcamera/internal/media_device.h | 1 +
> include/libcamera/internal/media_object.h | 7 +-
> include/libcamera/internal/meson.build | 1 +
> src/libcamera/device_enumerator.cpp | 73 ----------
> src/libcamera/device_match.cpp | 126 ++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 8 files changed, 183 insertions(+), 88 deletions(-)
> create mode 100644 include/libcamera/internal/device_match.h
> create mode 100644 src/libcamera/device_match.cpp
>
> diff --git a/include/libcamera/internal/device_enumerator.h b/include/libcamera/internal/device_enumerator.h
> index 72ec9a60..259a9e46 100644
> --- a/include/libcamera/internal/device_enumerator.h
> +++ b/include/libcamera/internal/device_enumerator.h
> @@ -13,24 +13,12 @@
>
> #include <libcamera/base/signal.h>
>
> +#include "libcamera/internal/device_match.h"
> +
You could forward-declare the DeviceMatch class in the header and move
the inclusion directive to the cpp file to reduce the compile-time
dependencies and improve the build times
> namespace libcamera {
>
> class MediaDevice;
>
> -class DeviceMatch
> -{
> -public:
> - DeviceMatch(const std::string &driver);
> -
> - void add(const std::string &entity);
> -
> - bool match(const MediaDevice *device) const;
> -
> -private:
> - std::string driver_;
> - std::vector<std::string> entities_;
> -};
> -
> class DeviceEnumerator
> {
> public:
> diff --git a/include/libcamera/internal/device_match.h b/include/libcamera/internal/device_match.h
> new file mode 100644
> index 00000000..e4563d3c
> --- /dev/null
> +++ b/include/libcamera/internal/device_match.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Sophie Friedrich
> + *
> + * device_match.h - API to match devices
> + */
> +
> +#pragma once
> +
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +
> +
Multiple blank lines
> +namespace libcamera {
> +
> +class MediaDevice;
> +
> +class DeviceMatchEntityInterface
> +{
> +public:
> + virtual const std::string &match_key() const = 0;
> + virtual const std::string &match_value() const = 0;
libcamera uses camelCase and not snake_case
> + virtual ~DeviceMatchEntityInterface() = default;
> +
> + static const std::string MATCH_ALL_KEY;
> +};
> +
> +class DeviceMatch
> +{
> +public:
> + DeviceMatch(const std::string &driver);
> +
> + void add(const std::string &entity);
> + void add(const std::string &key, const std::string &value);
> +
> + bool match(const MediaDevice *device) const;
> +
> +private:
> + std::string driver_;
> + std::vector<std::pair<std::string, std::string>> map_;
I'm a bit skeptical about this mapping, see below
> +};
> +
> +
multiple blank lines
> +}
> \ No newline at end of file
> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h
> index eb8cfde4..b10bb2eb 100644
> --- a/include/libcamera/internal/media_device.h
> +++ b/include/libcamera/internal/media_device.h
> @@ -19,6 +19,7 @@
> #include <libcamera/base/unique_fd.h>
>
> #include "libcamera/internal/media_object.h"
> +#include "libcamera/internal/device_match.h"
Alphabetical order please
>
> namespace libcamera {
>
> diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h
> index b1572968..d07ed698 100644
> --- a/include/libcamera/internal/media_object.h
> +++ b/include/libcamera/internal/media_object.h
> @@ -14,6 +14,8 @@
>
> #include <libcamera/base/class.h>
>
> +#include "libcamera/internal/device_match.h"
> +
> namespace libcamera {
>
> class MediaDevice;
> @@ -85,7 +87,7 @@ private:
> std::vector<MediaLink *> links_;
> };
>
> -class MediaEntity : public MediaObject
> +class MediaEntity : public MediaObject, public DeviceMatchEntityInterface
> {
> public:
> enum class Type {
> @@ -111,6 +113,9 @@ public:
>
> int setDeviceNode(const std::string &deviceNode);
>
> + const std::string &match_key() const{ return DeviceMatchEntityInterface::MATCH_ALL_KEY; }
> + const std::string &match_value() const { return name_; }
> +
This interface is designed to match the requirement of matching on a
key/value pair. I wonder if we could find a pattern to delegate
matching to the DeviceMatchEntityInterface derived classes. If you
have code already working for your usb camera I would like to see how
it gets to look like
> private:
> LIBCAMERA_DISABLE_COPY_AND_MOVE(MediaEntity)
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index d7508805..1e3c15ed 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -21,6 +21,7 @@ libcamera_internal_headers = files([
> 'control_validator.h',
> 'converter.h',
> 'delayed_controls.h',
> + 'device_match.h',
> 'device_enumerator.h',
> 'device_enumerator_sysfs.h',
> 'device_enumerator_udev.h',
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index f2e055de..49afd783 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -40,79 +40,6 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(DeviceEnumerator)
>
> -/**
> - * \class DeviceMatch
> - * \brief Description of a media device search pattern
> - *
> - * The DeviceMatch class describes a media device using properties from the
> - * Media Controller struct media_device_info, entity names in the media graph
> - * or other properties that can be used to identify a media device.
> - *
> - * The description is meant to be filled by pipeline managers and passed to a
> - * device enumerator to find matching media devices.
> - *
> - * A DeviceMatch is created with a specific Linux device driver in mind,
> - * therefore the name of the driver is a required property. One or more Entity
> - * names can be added as match criteria.
> - *
> - * Pipeline handlers are recommended to add entities to DeviceMatch as
> - * appropriare to ensure that the media device they need can be uniquely
> - * identified. This is useful when the corresponding kernel driver can produce
> - * different graphs, for instance as a result of different driver versions or
> - * hardware configurations, and not all those graphs are suitable for a pipeline
> - * handler.
> - */
> -
> -/**
> - * \brief Construct a media device search pattern
> - * \param[in] driver The Linux device driver name that created the media device
> - */
> -DeviceMatch::DeviceMatch(const std::string &driver)
> - : driver_(driver)
> -{
> -}
> -
> -/**
> - * \brief Add a media entity name to the search pattern
> - * \param[in] entity The name of the entity in the media graph
> - */
> -void DeviceMatch::add(const std::string &entity)
> -{
> - entities_.push_back(entity);
> -}
> -
> -/**
> - * \brief Compare a search pattern with a media device
> - * \param[in] device The media device
> - *
> - * Matching is performed on the Linux device driver name and entity names from
> - * the media graph. A match is found if both the driver name matches and the
> - * media device contains all the entities listed in the search pattern.
> - *
> - * \return true if the media device matches the search pattern, false otherwise
> - */
> -bool DeviceMatch::match(const MediaDevice *device) const
> -{
> - if (driver_ != device->driver())
> - return false;
> -
> - for (const std::string &name : entities_) {
> - bool found = false;
> -
> - for (const MediaEntity *entity : device->entities()) {
> - if (name == entity->name()) {
> - found = true;
> - break;
> - }
> - }
> -
> - if (!found)
> - return false;
> - }
> -
> - return true;
> -}
> -
> /**
> * \class DeviceEnumerator
> * \brief Enumerate, store and search media devices
> diff --git a/src/libcamera/device_match.cpp b/src/libcamera/device_match.cpp
> new file mode 100644
> index 00000000..0c205f93
> --- /dev/null
> +++ b/src/libcamera/device_match.cpp
> @@ -0,0 +1,126 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Sophie Friedrich
> + *
> + * device_match.cpp - API to match devices
> + */
> +
> +#include "libcamera/internal/device_match.h"
> +
> +#include <string.h>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "libcamera/internal/media_device.h"
> +
> +/**
> + * \file device_match.h
> + * \brief Matching of media devices
> + *
> + * Pipelines find compatible devices by matching against known strings or
> + * key/value pairs during device enumeration. MediaDevice are responsible for
> + * finding these entities during their population phase.
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(DeviceMatch)
> +
> +/**
> + * \interface DeviceMatchInterface
> + * \brief Interface which enables matching using DeviceMatch
> + *
> + *
> + *
Multiple blank lines
> + */
> +const std::string DeviceMatchEntityInterface::MATCH_ALL_KEY = "*";
> +
> +/**
> + * \class DeviceMatch
> + * \brief Description of a media device search pattern
> + *
> + * The DeviceMatch class describes a media device using properties from the
> + * Media Controller struct media_device_info, entity names in the media graph
> + * or other properties that can be used to identify a media device.
> + *
> + * The description is meant to be filled by pipeline managers and passed to a
> + * device enumerator to find matching media devices.
> + *
> + * A DeviceMatch is created with a specific Linux device driver in mind,
> + * therefore the name of the driver is a required property. One or more Entity
> + * names can be added as match criteria.
> + *
> + * Pipeline handlers are recommended to add entities to DeviceMatch as
> + * appropriare to ensure that the media device they need can be uniquely
> + * identified. This is useful when the corresponding kernel driver can produce
> + * different graphs, for instance as a result of different driver versions or
> + * hardware configurations, and not all those graphs are suitable for a pipeline
> + * handler.
> + */
> +
> +/**
> + * \brief Construct a media device search pattern
> + * \param[in] driver The Linux device driver name that created the media device
> + */
> +DeviceMatch::DeviceMatch(const std::string &driver)
> + : driver_(driver)
> +{
> +}
> +
> +/**
> + * \brief Add a media entity name to the search pattern
> + * \param[in] entity The name of the entity in the media graph
> + */
> +void DeviceMatch::add(const std::string &entity)
> +{
> + add(DeviceMatchEntityInterface::MATCH_ALL_KEY, entity);
> +}
> +
> +/**
> + * \brief Add key / value pair to search pattern
> + * \param[in] key The key the value is associated to
> + * \param[in] value The value that should be searched for
> + *
> + * There is no uniqueness enforced to key/value. Mentioning a certain
> + * key multiple times (in best case with different values) allows matching
> + * multiple properties on the same key.
Is this just to accommodate the usage of "*" as a key or is a
requirement for your use case too ?
> + */
> +void DeviceMatch::add(const std::string &key, const std::string &value)
> +{
> + map_.push_back(std::pair(key, value));
> +}
> +
> +/**
> + * \brief Compare a search pattern with a media device
> + * \param[in] device The media device
> + *
> + * Matching is performed on the Linux device driver name and entity names from
> + * the media graph. A match is found if both the driver name matches and the
> + * media device contains all the entities listed in the search pattern.
> + *
> + * \return true if the media device matches the search pattern, false otherwise
> + */
> +bool DeviceMatch::match(const MediaDevice *device) const
> +{
> + if (driver_ != device->driver())
> + return false;
> +
> + for (const std::pair<std::string, std::string> &item : map_) {
> + bool found = false;
> +
> + for (const DeviceMatchEntityInterface *entity : device->entities()) {
> + if (item.first == entity->match_key() &&
> + item.second == entity->match_value()) {
Align to the open ( please
If we could if (entity.match(item)) this would be nicer as it would
abstract away the matching criteria from the DeviceMatch class and
delegate it to sub-classes. Let's ponder a bit if that's possible and
how to design it.
Thanks
j
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return false;
> + }
> +
> + return true;
> +}
> +
> +}
> \ No newline at end of file
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 9869bfe7..445eb8e9 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -15,6 +15,7 @@ libcamera_sources = files([
> 'control_validator.cpp',
> 'converter.cpp',
> 'delayed_controls.cpp',
> + 'device_match.cpp',
> 'device_enumerator.cpp',
> 'device_enumerator_sysfs.cpp',
> 'fence.cpp',
> --
> 2.34.1
>
More information about the libcamera-devel
mailing list