| Message ID | 20230302005415.1789544-1-dev@flowerpot.me |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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 ? > > 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. 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 ? > > 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 >
Thanks Jacopo for forwarding this to me! On Mon, May 15, 2023 at 5:29 PM Jacopo Mondi <jacopo.mondi@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 :) > > > > 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 :) > > > > 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 > > >
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@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 > > > > >
Hi Harvey and Jacopo, On 22/05/2023 15:37, Jacopo Mondi wrote: > 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@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. Thank you this sounds like a good idea. >>> 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 Maybe using VID and PID as an example was a bit misleading. I wanted to generalize the interface in a way where we won't run into issues with potential requirements for other device matching in the future. In the case of matching USB devices this would be more than just VID and PID. The Linux kernel finds the drivers for a new USB device by using the `usb_device_id` struct which can contain more than just VID or PID and is even able to look at these separately. Just using VID together with PID could proof to be to limiting. See the Linux kernel docs: https://www.kernel.org/doc/html/v6.1/driver-api/basics.html#c.usb_device_id For example, I imagine it to be used with the following keys (for e.g. USB) | key | value | |------------------|-------| | usb_id_product | 1d6b | | usb_id_vendor | 0003 | | usb_bDeviceClass | 3 | >>>> >>>> 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 >>>> >>>