[libcamera-devel,3/3] libcamera: pipeline: Register device numbers with camera
diff mbox series

Message ID 20230419085821.2682901-4-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Add new Camera devices property
Related show

Commit Message

Kieran Bingham April 19, 2023, 8:58 a.m. UTC
Register the identified device numbers with each camera as the Devices
property.

This facilitates camera daemons or other systems to identify which
devices are being managed by libcamera, and can prevent duplication of
camera resources.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/pipeline_handler.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Barnabás Pőcze May 3, 2023, 1:58 p.m. UTC | #1
Hi


2023. április 19., szerda 10:58 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta:

> Register the identified device numbers with each camera as the Devices
> property.
> 
> This facilitates camera daemons or other systems to identify which
> devices are being managed by libcamera, and can prevent duplication of
> camera resources.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/pipeline_handler.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index f72613b8e515..66e7def51cd1 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -18,6 +18,7 @@
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/framebuffer.h>
> +#include <libcamera/property_ids.h>
> 
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/device_enumerator.h"
> @@ -624,6 +625,13 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
>  		}
>  	}
> 
> +	/*
> +	 * Store the associated devices as a property of the camera to allow
> +	 * systems to identify which devices are managed by libcamera.
> +	 */
> +	Camera::Private *data = camera->_d();
> +	data->properties_.set(properties::Devices, devnums);

I am getting an assertion failure while testing https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1611

  ASSERT(elementSize == ControlValueSize[type]);

in `controls.cpp:ControlValue::set()` fails because `sizeof(dev_t) != 4`.


> +
>  	manager_->addCamera(std::move(camera), devnums);
>  }
> 
> --
> 2.34.1


Regards,
Barnabás Pőcze
Kieran Bingham May 3, 2023, 2:02 p.m. UTC | #2
Quoting Barnabás Pőcze (2023-05-03 14:58:58)
> Hi
> 
> 
> 2023. április 19., szerda 10:58 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta:
> 
> > Register the identified device numbers with each camera as the Devices
> > property.
> > 
> > This facilitates camera daemons or other systems to identify which
> > devices are being managed by libcamera, and can prevent duplication of
> > camera resources.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline_handler.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index f72613b8e515..66e7def51cd1 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -18,6 +18,7 @@
> >  #include <libcamera/camera.h>
> >  #include <libcamera/camera_manager.h>
> >  #include <libcamera/framebuffer.h>
> > +#include <libcamera/property_ids.h>
> > 
> >  #include "libcamera/internal/camera.h"
> >  #include "libcamera/internal/device_enumerator.h"
> > @@ -624,6 +625,13 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> >               }
> >       }
> > 
> > +     /*
> > +      * Store the associated devices as a property of the camera to allow
> > +      * systems to identify which devices are managed by libcamera.
> > +      */
> > +     Camera::Private *data = camera->_d();
> > +     data->properties_.set(properties::Devices, devnums);
> 
> I am getting an assertion failure while testing https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1611
> 
>   ASSERT(elementSize == ControlValueSize[type]);
> 
> in `controls.cpp:ControlValue::set()` fails because `sizeof(dev_t) != 4`.

Yes, this is fixed ready for a V2 ... but the first patch needs to use a
64 bit integer, not a 32 bit.

> 
> 
> > +
> >       manager_->addCamera(std::move(camera), devnums);
> >  }
> > 
> > --
> > 2.34.1
> 
> 
> Regards,
> Barnabás Pőcze
Barnabás Pőcze May 3, 2023, 3:12 p.m. UTC | #3
Hi


2023. május 3., szerda 16:02 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> [...]
> > > +     /*
> > > +      * Store the associated devices as a property of the camera to allow
> > > +      * systems to identify which devices are managed by libcamera.
> > > +      */
> > > +     Camera::Private *data = camera->_d();
> > > +     data->properties_.set(properties::Devices, devnums);
> >
> > I am getting an assertion failure while testing https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1611
> >
> >   ASSERT(elementSize == ControlValueSize[type]);
> >
> > in `controls.cpp:ControlValue::set()` fails because `sizeof(dev_t) != 4`.
> 
> Yes, this is fixed ready for a V2 ... but the first patch needs to use a
> 64 bit integer, not a 32 bit.

I see, thanks for the clarification. These patches were linked in the pipewire MR
and I couldn't find a newer version. I presume it is not posted yet? As far as I
am aware `dev_t` is 64-bit on most(?) Linux platforms, so I don't understand
the remark regarding the first patch.


> [...]


Regards,
Barnabás Pőcze
Kieran Bingham May 3, 2023, 4:41 p.m. UTC | #4
Quoting Barnabás Pőcze (2023-05-03 16:12:30)
> Hi
> 
> 
> 2023. május 3., szerda 16:02 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > [...]
> > > > +     /*
> > > > +      * Store the associated devices as a property of the camera to allow
> > > > +      * systems to identify which devices are managed by libcamera.
> > > > +      */
> > > > +     Camera::Private *data = camera->_d();
> > > > +     data->properties_.set(properties::Devices, devnums);
> > >
> > > I am getting an assertion failure while testing https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1611
> > >
> > >   ASSERT(elementSize == ControlValueSize[type]);
> > >
> > > in `controls.cpp:ControlValue::set()` fails because `sizeof(dev_t) != 4`.
> > 
> > Yes, this is fixed ready for a V2 ... but the first patch needs to use a
> > 64 bit integer, not a 32 bit.
> 
> I see, thanks for the clarification. These patches were linked in the pipewire MR
> and I couldn't find a newer version. I presume it is not posted yet? As far as I
> am aware `dev_t` is 64-bit on most(?) Linux platforms, so I don't understand
> the remark regarding the first patch.

Correct, I have a new version here but haven't posted yet. It's on my
todo as soon as I can get to it!

I was referring to "libcamera: controls: Support dev_t in an Integer32 type."

When I looked up dev_t 'man makedev' I think I was confused by the
references to unsigned int on the parameters of makedev, and return
types of major() and minor() ... but dev_t is separate. Anyway, I
clearly have sent untested patches which I'm sorry for.

I wish we had a system where automated tests ran as soon as things are
pushed/posted that would have highlighted it immediately!

V2 will resolve it.

> 
> 
> > [...]
> 
> 
> Regards,
> Barnabás Pőcze

Patch
diff mbox series

diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index f72613b8e515..66e7def51cd1 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -18,6 +18,7 @@ 
 #include <libcamera/camera.h>
 #include <libcamera/camera_manager.h>
 #include <libcamera/framebuffer.h>
+#include <libcamera/property_ids.h>
 
 #include "libcamera/internal/camera.h"
 #include "libcamera/internal/device_enumerator.h"
@@ -624,6 +625,13 @@  void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
 		}
 	}
 
+	/*
+	 * Store the associated devices as a property of the camera to allow
+	 * systems to identify which devices are managed by libcamera.
+	 */
+	Camera::Private *data = camera->_d();
+	data->properties_.set(properties::Devices, devnums);
+
 	manager_->addCamera(std::move(camera), devnums);
 }