[libcamera-devel] [PATCH 0/3] libcamera: Add new Camera devices property

[Laurent Pinchart]

Hi Kieran,

On Thu, Apr 20, 2023 at 12:42:28PM +0100, Kieran Bingham wrote:
> Cc + George who I think may also be looking at this
> 
> Quoting Laurent Pinchart (2023-04-20 09:08:57)
> > On Thu, Apr 20, 2023 at 08:48:14AM +0200, Jacopo Mondi wrote:
> > > On Thu, Apr 20, 2023 at 05:57:37AM +0300, Laurent Pinchart wrote:
> > > > On Wed, Apr 19, 2023 at 01:11:39PM +0100, Kieran Bingham via libcamera-devel wrote:
> > > > > On 19/04/2023 10:45, Jacopo Mondi wrote:
> > > > > > On Wed, Apr 19, 2023 at 09:58:18AM +0100, Kieran Bingham via libcamera-devel wrote:
> > > > > >> It can be beneficial to allow reporting the devices used by a camera to
> > > > > >> facilitate de-duplication of resources when handling cameras from a
> > > > > >> centralised infrastructure such as PipeWire.
> > > > > >
> > > > > > This series allows to retrieve the list of dev_t a libcamera::Camera
> > > > > > handles by using a camera properties.
> > > > > >
> > > > > > The CameraManager class however maintains a camerasByDevnum_ map that
> > > > > > associates a dev_t with the Camera it is owned by. It would be trivial
> > > > > > to reverse the map. Have you considered that ? Getting dev_t from the
> > > > > > manager seems more natural than doing so from the Camera (and would
> > > > > > avoid the additional template specialization for dev_t you don't like)
> > > > >
> > > > > No I haven't, so that's an interesting point. I'll take a look.
> > > > >
> > > > > Robert/Wim,
> > > > >
> > > > > Do you have any specific requirements here? Do you need to be able to
> > > > > map which devices are used by each camera? Or is a single list sufficient?
> > > > >
> > > > > Or maybe returning a map of camera (id?) to device list is best all round?
> > > > >
> > > > > My only thought on the separation is the list would have to be
> > > > > re-obtained for hotplugged cameras, while it's available at the 'camera'
> > > > > when on the object. Conversely, perhaps hotplug is a reason to
> > > > > centralize the whole list as after a camera is removed it would need to
> > > > > be updated anyway.
> > > > >
> > > > > So - I'll see if I can add in a call on the Camera Manager that returns
> > > > > a map of camera id / dev_t.
> > > >
> > > > The map we expose through the CameraManager is meant for the V4L2
> > > > adaptation layer only. I'm actually thinking about how we could avoid
> > > 
> > > It's for V4L2 layer only because so far we had no other use cases,
> > > isn't it ?
> > 
> > Yes, but I also want to avoid new ue cases :-) V4L2 should be an
> > implementation detail that applications shouldn't care about. There are
> > some transitional use cases that we have to support, and I'd like them
> > to be exceptions, not a norm.
> > 
> > > > exposing it in the public API. A camera property seems a nicer solution
> > > > for the problem at hand.
> > > 
> > > However it will require getting hold of all camera intances, fetch and
> > > read the properties. As Kieran said with hotplug it gets even worse.
> > > Isn't it better to get it from the manager which maintains an overall
> > > view of all resources in the system ?
> > 
> > Hotplug is already supported by libcamera, through a signal that
> > indicates when a camera is plugged in. You can then get the list of
> > dev_t from the camera. If you want to expose that information in a
> > different way, you will need a separate hotplug notification to tell
> > when new dev_t are added or removed.
> 
> I envisaged that upon hotplug, an application can call the camera
> manager to get the revised complete list on both hotplug and hotunplug.
> 
> For a while I thought otherwise, the application would have to iterate
> all the cameras again to re-fresh the list. However - on hotunplug - the
> Camera object is passed in, so the unplug call already reports which
> dev_t is now being 'released' and could be handled by the app.
> 
> So with:
> 
> --- a/src/apps/cam/main.cpp
> +++ b/src/apps/cam/main.cpp
> @@ -187,11 +187,23 @@ int CamApp::parseOptions(int argc, char *argv[])
>  void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
>  {
>         std::cout << "Camera Added: " << cam->id() << std::endl;
> +
> +       std::cout << " Utilising the following devices: " << std::endl;
> +       for (auto d : cam->properties()
> +                             .get(properties::Devices)
> +                             .value_or(std::vector<dev_t>{}))
> +               std::cout << " - " << d << std::endl;
>  }
> 
>  void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>  {
>         std::cout << "Camera Removed: " << cam->id() << std::endl;
> +
> +       std::cout << " Utilising the following devices: " << std::endl;
> +       for (auto d : cam->properties()
> +                             .get(properties::Devices)
> +                             .value_or(std::vector<dev_t>{}))
> +               std::cout << " - " << d << std::endl;
>  }
> 
>  void CamApp::captureDone()
> 
> 
> We (helpfully) get:
> 
> ./build/gcc/src/apps/cam/cam -m
> ...
> Camera Removed: \_SB_.PCI0.GP13.XHC0.RHUB.PRT4-4.1:1.0-0525:a4a2
>  Utilising the following devices:
>  - 20736
> 
> Camera Added: \_SB_.PCI0.GP13.XHC0.RHUB.PRT4-4.1:1.0-0525:a4a2
>  Utilising the following devices:
>  - 20736

That's also what I had in mind :-)

> So that leaves my only concern about how we 'store' the dev_t in a
> ControlInfo.

How about switching internal storage of all existing dev_t to 64-bit
integers ? That will be large enough for all platforms and will avoid
having to add a dev_t type to the control API.

-- 
Regards,

Laurent Pinchart