[libcamera-devel] [PATCH RFC v2] libcamera: enumeration: Generalize matching

[Sophie Friedrich]

The issue at hand is, that the current implementation of how devices are 
matched
to their pipeline handlers is insufficient for future requirements as it 
doesn't
generalize well to pipeline handlers which would directly find USB 
devices or
initialize virtual test devices using e.g. a config file.

After initial discussions in the accompanying IRC it has become clear 
that the
original idea proposed by my previous patch (see [1]) regarding this 
topic is
insufficient as it replaced a makeshift implementation with an only 
marginally
better solution. In accordance to that this RFC should better outline the
problem and help in finding a good and future-proof solution.


## How does device matching work right now

The terminology and internal procedures has become quite confusing, so 
this is a
small reminder on how it (normally) works rn on main.

1. `CameraManager::Private::init` creates a `DeviceEnumerator`
   - the enumerator is responsible for discovering devices (includes 
hot-plugging)
   - devices are of instance `MediaDevice`

2. `::createPipelineHandlers` is called on a list of devices and pipelines
   - creates a pipeline by calling 
`PipelineHandler::match(DeviceEnumerator*)`

3. `PipelineHandler::match` is a virtual function, but the most common
    implementation does two steps relevant to matching
   1. it initializes and populates a `DeviceMatch` class using a driver 
and list
      of entities. The latter is done using `DeviceMatch::add(std::string*)`
   2. it (indirectly) calls `DeviceEnumerator::search` using the 
`DeviceMatch`
      instance and the previously enumerated devices

4. `DeviceEnumerator::search` filters the list of devices by calling
    `DeviceMatch::match(MediaDevice*)` for each device

The restrictions on filtering/matching are currently encoded in the
implementation of `DeviceMatch::match` and `MediaDevice`. The former 
function
first string compares `DeviceMatch::driver_` against 
`MediaDevice::driver()` and
afterwards string compares all `DeviceMatch::entities_` against all
`MediaDevice::entities()[:].name()`. Only if for each `DeviceMatch` entity a
corresponding `MediaDevice` entity could be found, the given device is 
accepted.


## Proposed changes

My previous proposal (see [1]) was to expand `DeviceMatch` by using a 
map with
pairs of (`std::string` property name, `std::string` value). In order to not
disrupt the current pipeline code a catch-all property name ('*') was
introduced, with which `DeviceMatch` could be used as before, but 
allowed more
advanced filtering if required.

One major critique has been, that the specific filter criteria were still
defined by how `DeviceMatch::match()` worked. Instead of comparing 
string entity
names it now compared string entity (key/value) pairs. (see comment by 
J. Mondi
in [2])

In the IRC two alternative ideas were proposed by J. Mondy to solve this 
issue:

1. `DeviceMatch::add()` could accept differing types of parameters with each
    pipeline handler choosing what they want. Matching would still happen in
    `DeviceMatch::match`

2. Have `DeviceMatch` an abstract class with derived `MediaDeviceMatch`,
    `USBDeviceMatch`, … classes that implement their own `::add` and 
`::match`
    functions. The `DeviceEnumerator` would then create the correct
    `MediaDevice`, `USBDevice` classes and pass them as `Device*` to the 
virtual
    `DeviceMatch::match` function.


Additionally, I would like to clear up the current terminology. I think 
renaming
`DeviceMatch` to e.g. `DeviceFilter` would more sharply explain its 
function as
it basically just describes filter criteria for matching. Currently, the 
word
`match` seems to me a bit overused.


### My own opinion

I personally prefer idea (2) over idea (1), as it allows restricting and 
naming
the possible filter criteria in code, while at the same time being abstract
enough to allow a wide range of matching. For e.g. `USBDeviceMatch` this 
could
mean that the possible matchable property names are very limited on 
purpose. See
the Linux kernel internal `usb_match_id` struct for example [3], which is
responsible for defining the filtering criteria for USB drivers.

As (1) in its initial idea doesn't have this property, it seems to be 
lacking in
similar regards as my initial proposal.

One issue with both proposals that I see, is that C++ `dynamic_cast` (or
similar) would need to be used internally using a naive approach. 
Although I'm
unsure if this even really is a problem. The performance penalty could be
irrelevant and maybe RTTI support is already assumed.


Best regards
   Sophie

[1] https://patchwork.libcamera.org/patch/18328/
[2] https://patchwork.libcamera.org/patch/18329/
[3] 
https://elixir.bootlin.com/linux/v6.4/source/include/linux/mod_devicetable.h#L128

---
v1 -> RFC: moved from possible implementation back to discussion stage