[libcamera-devel,1/1] libcamera: enumeration: Generalize DeviceMatch

18329 diff mbox series

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@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

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@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
>