| Message ID | 20230615225133.622612-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Jun 15, 2023 at 11:51:32PM +0100, Kieran Bingham via libcamera-devel wrote: > The CameraManager->get(dev_t) helper was implemented only to support the > V4L2 Adaptation layer, and has been deprecated now that a new camera > property - SystemDevices has been introduced. > > Rework the implementation of getCameraIndex() to use the SystemDevices > property and remove reliance on the now deprecated call. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > index 0f7575c54b22..c49124290b42 100644 > --- a/src/v4l2/v4l2_compat_manager.cpp > +++ b/src/v4l2/v4l2_compat_manager.cpp > @@ -24,6 +24,7 @@ > > #include <libcamera/camera.h> > #include <libcamera/camera_manager.h> > +#include <libcamera/property_ids.h> > > #include "v4l2_camera_file.h" > > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd) > if (ret < 0) > return -1; > > - std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev); > - if (!target) > - return -1; > + const dev_t devnum = statbuf.st_rdev; const int64_t devnum = static_cast<int64_t>(statbuf.st_rdev); will save you a static_cast<> in the loop below. Up to you. > > + /* > + * Iterate each known camera and identify if it reports this nodes > + * device number in its list of SystemDevices. > + */ > auto cameras = cm_->cameras(); > for (auto [index, camera] : utils::enumerate(cameras)) { > - if (camera == target) > - return index; > + const auto devices = camera->properties() > + .get(properties::SystemDevices) > + .value_or(std::vector<int64_t>{}); Can we be explicit ? const std::vector<int64_t> devices = camera->properties() .get(properties::SystemDevices) .value_or(std::vector<int64_t>{}); > + > + /* > + * While there may be multiple Cameras that could reference the > + * same device node, we take a first match as a best effort for > + * now. > + * > + * \todo Consider reworking the V4L2 adaptation layer to stop > + * trying to map video nodes directly to a camera and instead > + * hide all devices that may be used by libcamera and expose a > + * consistent 1:1 mapping with each Camera instance. I'm not sure what you mean here, could you elaborate ? > + */ > + for (const int64_t dev : devices) > + if (dev == static_cast<int64_t>(devnum)) > + return index; for (const int64_t dev : devices) { if (dev == static_cast<int64_t>(devnum)) return index; } > } > > return -1;
Hi 2023. június 16., péntek 0:51 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta: > The CameraManager->get(dev_t) helper was implemented only to support the > V4L2 Adaptation layer, and has been deprecated now that a new camera > property - SystemDevices has been introduced. > > Rework the implementation of getCameraIndex() to use the SystemDevices > property and remove reliance on the now deprecated call. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > index 0f7575c54b22..c49124290b42 100644 > --- a/src/v4l2/v4l2_compat_manager.cpp > +++ b/src/v4l2/v4l2_compat_manager.cpp > @@ -24,6 +24,7 @@ > > #include <libcamera/camera.h> > #include <libcamera/camera_manager.h> > +#include <libcamera/property_ids.h> > > #include "v4l2_camera_file.h" > > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd) > if (ret < 0) > return -1; > > - std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev); > - if (!target) > - return -1; > + const dev_t devnum = statbuf.st_rdev; > > + /* > + * Iterate each known camera and identify if it reports this nodes > + * device number in its list of SystemDevices. > + */ > auto cameras = cm_->cameras(); > for (auto [index, camera] : utils::enumerate(cameras)) { > - if (camera == target) > - return index; > + const auto devices = camera->properties() > + .get(properties::SystemDevices) > + .value_or(std::vector<int64_t>{}); Why a vector? Why not `Span<int64_t>{}`? > + > + /* > + * While there may be multiple Cameras that could reference the > + * same device node, we take a first match as a best effort for > + * now. > + * > + * \todo Consider reworking the V4L2 adaptation layer to stop > + * trying to map video nodes directly to a camera and instead > + * hide all devices that may be used by libcamera and expose a > + * consistent 1:1 mapping with each Camera instance. > + */ > + for (const int64_t dev : devices) > + if (dev == static_cast<int64_t>(devnum)) > + return index; > } > > return -1; > -- > 2.34.1 > Regards, Barnabás Pőcze
Quoting Barnabás Pőcze (2023-06-16 15:27:27) > Hi > > > 2023. június 16., péntek 0:51 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta: > > > The CameraManager->get(dev_t) helper was implemented only to support the > > V4L2 Adaptation layer, and has been deprecated now that a new camera > > property - SystemDevices has been introduced. > > > > Rework the implementation of getCameraIndex() to use the SystemDevices > > property and remove reliance on the now deprecated call. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++----- > > 1 file changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > index 0f7575c54b22..c49124290b42 100644 > > --- a/src/v4l2/v4l2_compat_manager.cpp > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > @@ -24,6 +24,7 @@ > > > > #include <libcamera/camera.h> > > #include <libcamera/camera_manager.h> > > +#include <libcamera/property_ids.h> > > > > #include "v4l2_camera_file.h" > > > > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd) > > if (ret < 0) > > return -1; > > > > - std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev); > > - if (!target) > > - return -1; > > + const dev_t devnum = statbuf.st_rdev; > > > > + /* > > + * Iterate each known camera and identify if it reports this nodes > > + * device number in its list of SystemDevices. > > + */ > > auto cameras = cm_->cameras(); > > for (auto [index, camera] : utils::enumerate(cameras)) { > > - if (camera == target) > > - return index; > > + const auto devices = camera->properties() > > + .get(properties::SystemDevices) > > + .value_or(std::vector<int64_t>{}); > > Why a vector? Why not `Span<int64_t>{}`? I think I just used vector because that's the control type. But I think you're right, a Span could potentially work here. I'll try it and if it works I'll use that. -- Kieran > > > > + > > + /* > > + * While there may be multiple Cameras that could reference the > > + * same device node, we take a first match as a best effort for > > + * now. > > + * > > + * \todo Consider reworking the V4L2 adaptation layer to stop > > + * trying to map video nodes directly to a camera and instead > > + * hide all devices that may be used by libcamera and expose a > > + * consistent 1:1 mapping with each Camera instance. > > + */ > > + for (const int64_t dev : devices) > > + if (dev == static_cast<int64_t>(devnum)) > > + return index; > > } > > > > return -1; > > -- > > 2.34.1 > > > > > Regards, > Barnabás Pőcze
Quoting Laurent Pinchart (2023-06-16 15:09:16) > Hi Kieran, > > Thank you for the patch. > > On Thu, Jun 15, 2023 at 11:51:32PM +0100, Kieran Bingham via libcamera-devel wrote: > > The CameraManager->get(dev_t) helper was implemented only to support the > > V4L2 Adaptation layer, and has been deprecated now that a new camera > > property - SystemDevices has been introduced. > > > > Rework the implementation of getCameraIndex() to use the SystemDevices > > property and remove reliance on the now deprecated call. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++----- > > 1 file changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > index 0f7575c54b22..c49124290b42 100644 > > --- a/src/v4l2/v4l2_compat_manager.cpp > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > @@ -24,6 +24,7 @@ > > > > #include <libcamera/camera.h> > > #include <libcamera/camera_manager.h> > > +#include <libcamera/property_ids.h> > > > > #include "v4l2_camera_file.h" > > > > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd) > > if (ret < 0) > > return -1; > > > > - std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev); > > - if (!target) > > - return -1; > > + const dev_t devnum = statbuf.st_rdev; > > const int64_t devnum = static_cast<int64_t>(statbuf.st_rdev); > > will save you a static_cast<> in the loop below. Up to you. > > > > > + /* > > + * Iterate each known camera and identify if it reports this nodes > > + * device number in its list of SystemDevices. > > + */ > > auto cameras = cm_->cameras(); > > for (auto [index, camera] : utils::enumerate(cameras)) { > > - if (camera == target) > > - return index; > > + const auto devices = camera->properties() > > + .get(properties::SystemDevices) > > + .value_or(std::vector<int64_t>{}); > > Can we be explicit ? > > const std::vector<int64_t> devices = camera->properties() > .get(properties::SystemDevices) > .value_or(std::vector<int64_t>{}); > > > + > > + /* > > + * While there may be multiple Cameras that could reference the > > + * same device node, we take a first match as a best effort for > > + * now. > > + * > > + * \todo Consider reworking the V4L2 adaptation layer to stop > > + * trying to map video nodes directly to a camera and instead > > + * hide all devices that may be used by libcamera and expose a > > + * consistent 1:1 mapping with each Camera instance. > > I'm not sure what you mean here, could you elaborate ? What I mean is I think this could all be improved. In particular to stop the issue where if mulitple cameras reference the same video node (unicam for instance) they would only ever work for the first one. If there were time on this I would investigate making this layer provides a better mapping where each camera is /dev/videoN directly. > > + */ > > + for (const int64_t dev : devices) > > + if (dev == static_cast<int64_t>(devnum)) > > + return index; > > for (const int64_t dev : devices) { > if (dev == static_cast<int64_t>(devnum)) > return index; > } > > > } > > > > return -1; > > -- > Regards, > > Laurent Pinchart
Hi 2023. június 18., vasárnap 0:10 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Barnabás Pőcze (2023-06-16 15:27:27) > > Hi > > > > > > 2023. június 16., péntek 0:51 keltezéssel, Kieran Bingham via libcamera-devel <libcamera-devel@lists.libcamera.org> írta: > > > > > The CameraManager->get(dev_t) helper was implemented only to support the > > > V4L2 Adaptation layer, and has been deprecated now that a new camera > > > property - SystemDevices has been introduced. > > > > > > Rework the implementation of getCameraIndex() to use the SystemDevices > > > property and remove reliance on the now deprecated call. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++----- > > > 1 file changed, 23 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > > index 0f7575c54b22..c49124290b42 100644 > > > --- a/src/v4l2/v4l2_compat_manager.cpp > > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > > @@ -24,6 +24,7 @@ > > > > > > #include <libcamera/camera.h> > > > #include <libcamera/camera_manager.h> > > > +#include <libcamera/property_ids.h> > > > > > > #include "v4l2_camera_file.h" > > > > > > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd) > > > if (ret < 0) > > > return -1; > > > > > > - std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev); > > > - if (!target) > > > - return -1; > > > + const dev_t devnum = statbuf.st_rdev; > > > > > > + /* > > > + * Iterate each known camera and identify if it reports this nodes > > > + * device number in its list of SystemDevices. > > > + */ > > > auto cameras = cm_->cameras(); > > > for (auto [index, camera] : utils::enumerate(cameras)) { > > > - if (camera == target) > > > - return index; > > > + const auto devices = camera->properties() > > > + .get(properties::SystemDevices) > > > + .value_or(std::vector<int64_t>{}); > > > > Why a vector? Why not `Span<int64_t>{}`? > > I think I just used vector because that's the control type. But I think > you're right, a Span could potentially work here. > > I'll try it and if it works I'll use that. > [...] As far as I can see, array-like controls have type `Control<Span<T>>`, so `.get(...)` returns a span. That's the reason I asked, it seemed a bit odd that you used a vector there. Regards, Barnabás Pőcze
Quoting Kieran Bingham (2023-06-17 23:16:47) > Quoting Laurent Pinchart (2023-06-16 15:09:16) > > Hi Kieran, > > > > Thank you for the patch. > > > > On Thu, Jun 15, 2023 at 11:51:32PM +0100, Kieran Bingham via libcamera-devel wrote: > > > The CameraManager->get(dev_t) helper was implemented only to support the > > > V4L2 Adaptation layer, and has been deprecated now that a new camera > > > property - SystemDevices has been introduced. > > > > > > Rework the implementation of getCameraIndex() to use the SystemDevices > > > property and remove reliance on the now deprecated call. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++----- > > > 1 file changed, 23 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > > index 0f7575c54b22..c49124290b42 100644 > > > --- a/src/v4l2/v4l2_compat_manager.cpp > > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > > @@ -24,6 +24,7 @@ > > > > > > #include <libcamera/camera.h> > > > #include <libcamera/camera_manager.h> > > > +#include <libcamera/property_ids.h> > > > > > > #include "v4l2_camera_file.h" > > > > > > @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd) > > > if (ret < 0) > > > return -1; > > > > > > - std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev); > > > - if (!target) > > > - return -1; > > > + const dev_t devnum = statbuf.st_rdev; > > > > const int64_t devnum = static_cast<int64_t>(statbuf.st_rdev); > > > > will save you a static_cast<> in the loop below. Up to you. I'd rather keep it clear that statbuf.st_rdev is a dev_t. There's not a great deal of difference in a cast here or a cast below otherwise. > > > > > > > > + /* > > > + * Iterate each known camera and identify if it reports this nodes > > > + * device number in its list of SystemDevices. > > > + */ > > > auto cameras = cm_->cameras(); > > > for (auto [index, camera] : utils::enumerate(cameras)) { > > > - if (camera == target) > > > - return index; > > > + const auto devices = camera->properties() > > > + .get(properties::SystemDevices) > > > + .value_or(std::vector<int64_t>{}); > > > > Can we be explicit ? > > > > const std::vector<int64_t> devices = camera->properties() > > .get(properties::SystemDevices) > > .value_or(std::vector<int64_t>{}); I'll use the following (Thanks to Barnabás) diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index c49124290b42..fa3cb01bb798 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -122,9 +122,9 @@ int V4L2CompatManager::getCameraIndex(int fd) */ auto cameras = cm_->cameras(); for (auto [index, camera] : utils::enumerate(cameras)) { - const auto devices = camera->properties() + Span<const int64_t> devices = camera->properties() .get(properties::SystemDevices) - .value_or(std::vector<int64_t>{}); + .value_or(Span<int64_t>{}); /* * While there may be multiple Cameras that could reference the @@ -136,9 +136,10 @@ int V4L2CompatManager::getCameraIndex(int fd) * hide all devices that may be used by libcamera and expose a * consistent 1:1 mapping with each Camera instance. */ - for (const int64_t dev : devices) + for (const int64_t dev : devices) { if (dev == static_cast<int64_t>(devnum)) return index; + } } return -1; > > > > > + > > > + /* > > > + * While there may be multiple Cameras that could reference the > > > + * same device node, we take a first match as a best effort for > > > + * now. > > > + * > > > + * \todo Consider reworking the V4L2 adaptation layer to stop > > > + * trying to map video nodes directly to a camera and instead > > > + * hide all devices that may be used by libcamera and expose a > > > + * consistent 1:1 mapping with each Camera instance. > > > > I'm not sure what you mean here, could you elaborate ? > > What I mean is I think this could all be improved. In particular to stop > the issue where if mulitple cameras reference the same video node > (unicam for instance) they would only ever work for the first one. > > If there were time on this I would investigate making this layer > provides a better mapping where each camera is /dev/videoN directly. > > > > > > + */ > > > + for (const int64_t dev : devices) > > > + if (dev == static_cast<int64_t>(devnum)) > > > + return index; > > > > for (const int64_t dev : devices) { > > if (dev == static_cast<int64_t>(devnum)) > > return index; > > } > > > > > } > > > > > > return -1; > > > > -- > > Regards, > > > > Laurent Pinchart
diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index 0f7575c54b22..c49124290b42 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -24,6 +24,7 @@ #include <libcamera/camera.h> #include <libcamera/camera_manager.h> +#include <libcamera/property_ids.h> #include "v4l2_camera_file.h" @@ -113,14 +114,31 @@ int V4L2CompatManager::getCameraIndex(int fd) if (ret < 0) return -1; - std::shared_ptr<Camera> target = cm_->get(statbuf.st_rdev); - if (!target) - return -1; + const dev_t devnum = statbuf.st_rdev; + /* + * Iterate each known camera and identify if it reports this nodes + * device number in its list of SystemDevices. + */ auto cameras = cm_->cameras(); for (auto [index, camera] : utils::enumerate(cameras)) { - if (camera == target) - return index; + const auto devices = camera->properties() + .get(properties::SystemDevices) + .value_or(std::vector<int64_t>{}); + + /* + * While there may be multiple Cameras that could reference the + * same device node, we take a first match as a best effort for + * now. + * + * \todo Consider reworking the V4L2 adaptation layer to stop + * trying to map video nodes directly to a camera and instead + * hide all devices that may be used by libcamera and expose a + * consistent 1:1 mapping with each Camera instance. + */ + for (const int64_t dev : devices) + if (dev == static_cast<int64_t>(devnum)) + return index; } return -1;
The CameraManager->get(dev_t) helper was implemented only to support the V4L2 Adaptation layer, and has been deprecated now that a new camera property - SystemDevices has been introduced. Rework the implementation of getCameraIndex() to use the SystemDevices property and remove reliance on the now deprecated call. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/v4l2/v4l2_compat_manager.cpp | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)