[libcamera-devel] [PATCH 0/1] Generalized DeviceMatch

[Jacopo Mondi]

Hi Harvey

On Mon, May 22, 2023 at 08:22:52PM +0800, Cheng-Hao Yang via libcamera-devel wrote:
> Thanks Jacopo for forwarding this to me!
>
> On Mon, May 15, 2023 at 5:29 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> wrote:
>
> > Hi Sophie,
> >   sorry for the very long delay in reviewing this patch
> >
> > let me cc Harvey which, as you might know, is working on adding a
> > pipeline handler implementation that does not use the media device
> > abstraction.
> >
> > Harvey: Sophie is instead introducing a generalized form of device
> > matching that can be made to use different identifiers than the media
> > entities names. I wonder if this is something that could help with
> > virtual pipeline (see below)
> >
> > On Thu, Mar 02, 2023 at 01:54:14AM +0100, Sophie Friedrich via
> > libcamera-devel wrote:
> > > In preparation for introducing libusb support into libcamera, we need
> > > to generalize the matching functionality. The `MediaEntity` class was
> > > never meant to handle the cases required for e.g. libusb. Additionally
> > > the current way matching is handled is insufficient for (future)
> > > `MediaDevices` in which a specific key/value relationship exists
> > > for properties (e.g. USB vid and pid).
> >
> > Do I get it right that you plan to use the vid as key and the pid as
> > value ?
> >
> >
> I don't really understand the difference of the key and the value in
> `DeviceMatchEntityInterface`: it seems that `DeviceMatch::match`
> needs to compare them both to match an entity, so why not merging
> them into one (say id or key)? For USB devices, we can come up with an
> encoder
> that takes vid and pid into one single id.
>
> Unless there are further plans for the key and the value that I'm not aware
> of :)
>

Let's hear from Sophie how she plans to use them

>
> > >
> > > To solve these two problems I've abstracted away the requirements for
> > > entities that `DeviceMatch` requires in `DeviceMatchEntityInterface`
> > > and extended `DeviceMatch` to handle key/value pairs. Using "*" as a
> > > key is currently a catch all for the presumably keyless property of
> > > V4L2 `MediaEntity`.
> >
> > Harvey: my understanding is that your plan is to define the number of
> > virtual cameras in a configuration file. I wonder if by introducing a
> > device enumerator that parses the configuration file we could create
> > some form of matching for the Virtual pipeline handler.
> >
> >
> Yes, that makes a lot of sense to me. I'll update
> https://patchwork.libcamera.org/patch/18532/ when this patch is merged.
>
>
> > The "ConfigurationFileDeviceEnumerator" would define a number of
> > "virtual instances" and the virtual pipeline handler will be matches
> > against each of them. This would allow to create multiple instances of
> > the virtual pipeline handler to test it, and in future, if this would
> > ever be useful, to match different version of the virtual pipeline
> > handler against different 'tags' specified in the configuration. Do
> > you think it's something that might prove useful ?
> >
> >
> Yes, that sounds very applicable to me. I don't have a clear view of the
> benefit of multiple VirtualPipelineHandlers though. I guess that's why
> we need a DeviceEnumerator though :)
>

The actual benefits of multiple virtual pipeline handlers are not 100%
clear to me neither at this point. Still I would consider enumerating
cameras from config file neat, as it integrates well with how libcamera
works in that regard. It could be considered an overkill though. Maybe
an RFC would help quantify how hard it would be to land it. I can try
have a look


>
> > >
> > > Sophie Friedrich (1):
> > >   libcamera: enumeration: Generalize DeviceMatch
> > >
> > >  .../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
> > >
> > > --
> > > 2.34.1
> > >
> >