From c3fec9769ce4089571afb450d1b181f2334fa3e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Barnab=C3=A1s=20P=C5=91cze?= Date: Sat, 12 Jul 2025 19:38:24 +0200 Subject: [PATCH] spa: libcamera: manager: fix id allocation There is an issue in the id allocation mechanism which can result in the different devices having the same id. Specifically, consider the scenario where there are only two cameras, which have just been added. In this case `impl::devices` looks like this: (0, camA) | (1, camB) | (?, nullptr) | ... Now assume that `camA` is removed, after which the array appears as follows: (1, camB) | (1, nullptr) | (?, nullptr) | ... Then assume that a new camera appears. When `get_free_id()` runs, when `i == 1`, it will observe that `devices[i].camera == nullptr`, so it selects `1` as the id. Leading to the following: (1, camB) | (1, camC) | (?, nullptr) | ... This is of course incorrect. The set of ids must be unique. When wireplumber is faced with this situation it destroys the device object for `camB` when `camC` is emitted. Fix this by simply not moving elements in the `devices` array, leaving everything where it is. In which case the array looks like this: (nullptr) | (camB) | (nullptr) | ... // after `camA` removal (camC) | (camB) | (nullptr) | ... // after `camC` appearance Note that `device::id` is removed, and the id is now derived from the position in `impl::devices`. (cherry picked from commit 2c2808fab1ae27284b51cfabe644836f5b4a93f9) --- spa/plugins/libcamera/libcamera-manager.cpp | 80 ++++++++++----------- 1 file changed, 37 insertions(+), 43 deletions(-) diff --git a/spa/plugins/libcamera/libcamera-manager.cpp b/spa/plugins/libcamera/libcamera-manager.cpp index 4cac76fb2..eda1442ee 100644 --- a/spa/plugins/libcamera/libcamera-manager.cpp +++ b/spa/plugins/libcamera/libcamera-manager.cpp @@ -3,6 +3,7 @@ /* SPDX-License-Identifier: MIT */ #include +#include #include #include #include @@ -27,16 +28,15 @@ using namespace libcamera; #include "libcamera.h" #include "libcamera-manager.hpp" -#define MAX_DEVICES 64 - namespace { struct device { - uint32_t id; std::shared_ptr camera; }; struct impl { + static constexpr std::size_t max_devices = 64; + struct spa_handle handle; struct spa_device device = {}; @@ -52,8 +52,7 @@ struct impl { void addCamera(std::shared_ptr camera); void removeCamera(std::shared_ptr camera); - struct device devices[MAX_DEVICES]; - uint32_t n_devices = 0; + struct device devices[max_devices]; struct hotplug_event { enum class type { add, remove } type; @@ -70,59 +69,50 @@ struct impl { { spa_loop_utils_destroy_source(loop_utils, hotplug_event_source); } -}; -uint32_t get_free_id(struct impl *impl) -{ - for (std::size_t i = 0; i < MAX_DEVICES; i++) - if (impl->devices[i].camera == nullptr) - return i; - return 0; -} + std::uint32_t id_of(const struct device& d) const + { + spa_assert(std::begin(devices) <= &d && &d < std::end(devices)); + return &d - std::begin(devices); + } +}; struct device *add_device(struct impl *impl, std::shared_ptr camera) { - struct device *device; - uint32_t id; + for (auto& d : impl->devices) { + if (!d.camera) { + d.camera = std::move(camera); + return &d; + } + } - if (impl->n_devices >= MAX_DEVICES) - return nullptr; - id = get_free_id(impl); - device = &impl->devices[id]; - device->id = id; - device->camera = std::move(camera); - impl->n_devices++; - return device; + return nullptr; } struct device *find_device(struct impl *impl, const Camera *camera) { - uint32_t i; - for (i = 0; i < impl->n_devices; i++) { - if (impl->devices[i].camera.get() == camera) - return &impl->devices[i]; + for (auto& d : impl->devices) { + if (d.camera.get() == camera) + return &d; } + return nullptr; } void remove_device(struct impl *impl, struct device *device) { - uint32_t old = --impl->n_devices; - device->camera.reset(); - *device = std::move(impl->devices[old]); - impl->devices[old].camera = nullptr; + *device = {}; } void clear_devices(struct impl *impl) { - while (impl->n_devices > 0) - impl->devices[--impl->n_devices].camera.reset(); + for (auto& d : impl->devices) + d = {}; } -int emit_object_info(struct impl *impl, struct device *device) +int emit_object_info(struct impl *impl, const struct device *device) { struct spa_device_object_info info; - uint32_t id = device->id; struct spa_dict_item items[20]; struct spa_dict dict; uint32_t n_items = 0; @@ -144,7 +134,7 @@ int emit_object_info(struct impl *impl, struct device *device) dict = SPA_DICT_INIT(items, n_items); info.props = &dict; - spa_device_emit_object_info(&impl->hooks, id, &info); + spa_device_emit_object_info(&impl->hooks, impl->id_of(*device), &info); return 1; } @@ -159,8 +149,8 @@ void try_add_camera(struct impl *impl, std::shared_ptr camera) if ((device = add_device(impl, std::move(camera))) == nullptr) return; - spa_log_info(impl->log, "camera added: id:%d %s", device->id, - device->camera->id().c_str()); + spa_log_info(impl->log, "camera added: id:%" PRIu32 " %s", + impl->id_of(*device), device->camera->id().c_str()); emit_object_info(impl, device); } @@ -171,9 +161,11 @@ void try_remove_camera(struct impl *impl, const Camera *camera) if ((device = find_device(impl, camera)) == nullptr) return; - spa_log_info(impl->log, "camera removed: id:%d %s", device->id, - device->camera->id().c_str()); - spa_device_emit_object_info(&impl->hooks, device->id, nullptr); + auto id = impl->id_of(*device); + + spa_log_info(impl->log, "camera removed: id:%" PRIu32 " %s", + id, device->camera->id().c_str()); + spa_device_emit_object_info(&impl->hooks, id, nullptr); remove_device(impl, device); } @@ -308,8 +300,10 @@ impl_device_add_listener(void *object, struct spa_hook *listener, emit_device_info(impl, true); if (had_manager) { - for (std::size_t i = 0; i < impl->n_devices; i++) - emit_object_info(impl, &impl->devices[i]); + for (const auto& d : impl->devices) { + if (d.camera) + emit_object_info(impl, &d); + } } else { collect_existing_devices(impl);