From cc229d4b05315ff4d7fe8ab6b335cccdbf79a278 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Barnab=C3=A1s=20P=C5=91cze?= Date: Sat, 3 Sep 2022 19:55:49 +0200 Subject: [PATCH] spa: libcamera: properly construct/deconstruct libcamera device impl Previously, the "impl" object was never properly constructed or destructed, but it more or less worked out since the memory was initialized to zero bytes and each member had trivial constructors. Except std::shared_ptr, but an all zero storage happened to be equivalent to a default constructed shared_ptr. However, there was the still the problem that the shared_ptr was never destructed, so it kept the referenced `Camera` object alive, which lead to memory leaks. An additional, somewhat unrelated change is that the "props" struct is removed, and the device identifier is now stored in an `std::string`. The reason is that `CameraManager::get()` already takes a const std::string reference, so an std::string must be constructed in any case, so we might as well take advantage of that and use `std::string` in the "impl" object as well. Furthermore, wrap the `impl` struct in an anonymous namespace to avoid name resolution problems. --- spa/plugins/libcamera/libcamera-device.cpp | 94 ++++++++++++---------- 1 file changed, 51 insertions(+), 43 deletions(-) diff --git a/spa/plugins/libcamera/libcamera-device.cpp b/spa/plugins/libcamera/libcamera-device.cpp index 9eec30cc8..c531254aa 100644 --- a/spa/plugins/libcamera/libcamera-device.cpp +++ b/spa/plugins/libcamera/libcamera-device.cpp @@ -54,30 +54,29 @@ using namespace libcamera; -struct props { - char device[128]; - char device_name[128]; -}; - -static void reset_props(struct props *props) -{ - spa_zero(*props); -} +namespace { struct impl { struct spa_handle handle; - struct spa_device device; + struct spa_device device = {}; struct spa_log *log; - struct props props; + std::string device_id; struct spa_hook_list hooks; CameraManager *manager; std::shared_ptr camera; + + impl(spa_log *log, + CameraManager *manager, + std::shared_ptr camera, + std::string device_id); }; +} + static std::string cameraModel(const Camera *camera) { const ControlList &props = camera->properties(); @@ -120,11 +119,11 @@ static int emit_info(struct impl *impl, bool full) info.change_mask = SPA_DEVICE_CHANGE_MASK_PROPS; #define ADD_ITEM(key, value) items[n_items++] = SPA_DICT_ITEM_INIT(key, value) - snprintf(path, sizeof(path), "libcamera:%s", impl->props.device); + snprintf(path, sizeof(path), "libcamera:%s", impl->device_id.c_str()); ADD_ITEM(SPA_KEY_OBJECT_PATH, path); ADD_ITEM(SPA_KEY_DEVICE_API, "libcamera"); ADD_ITEM(SPA_KEY_MEDIA_CLASS, "Video/Device"); - ADD_ITEM(SPA_KEY_API_LIBCAMERA_PATH, impl->props.device); + ADD_ITEM(SPA_KEY_API_LIBCAMERA_PATH, impl->device_id.c_str()); if (auto location = cameraLoc(impl->camera.get())) ADD_ITEM(SPA_KEY_API_LIBCAMERA_LOCATION, location); @@ -132,7 +131,7 @@ static int emit_info(struct impl *impl, bool full) snprintf(model, sizeof(model), "%s", cameraModel(impl->camera.get()).c_str()); ADD_ITEM(SPA_KEY_DEVICE_PRODUCT_NAME, model); ADD_ITEM(SPA_KEY_DEVICE_DESCRIPTION, model); - snprintf(name, sizeof(name), "libcamera_device.%s", impl->props.device); + snprintf(name, sizeof(name), "libcamera_device.%s", impl->device_id.c_str()); ADD_ITEM(SPA_KEY_DEVICE_NAME, name); #undef ADD_ITEM @@ -235,13 +234,32 @@ static int impl_get_interface(struct spa_handle *handle, const char *type, void static int impl_clear(struct spa_handle *handle) { - struct impl *impl = (struct impl *) handle; - if (impl->manager) - libcamera_manager_release(impl->manager); - impl->manager = NULL; + auto impl = reinterpret_cast(handle); + auto manager = impl->manager; + + std::destroy_at(impl); + libcamera_manager_release(manager); + return 0; } +impl::impl(spa_log *log, CameraManager *manager, std::shared_ptr camera, std::string device_id) + : handle({ SPA_VERSION_HANDLE, impl_get_interface, impl_clear }), + log(log), + device_id(std::move(device_id)), + manager(manager), + camera(std::move(camera)) +{ + libcamera_log_topic_init(log); + + spa_hook_list_init(&hooks); + + device.iface = SPA_INTERFACE_INIT( + SPA_TYPE_INTERFACE_Device, + SPA_VERSION_DEVICE, + &impl_device, this); +} + static size_t impl_get_size(const struct spa_handle_factory *factory, const struct spa_dict *params) @@ -256,44 +274,34 @@ impl_init(const struct spa_handle_factory *factory, const struct spa_support *support, uint32_t n_support) { - struct impl *impl; const char *str; int res; spa_return_val_if_fail(factory != NULL, -EINVAL); spa_return_val_if_fail(handle != NULL, -EINVAL); - handle->get_interface = impl_get_interface; - handle->clear = impl_clear, impl = (struct impl *) handle; + auto log = static_cast(spa_support_find(support, n_support, SPA_TYPE_INTERFACE_Log)); - impl->log = (struct spa_log*) spa_support_find(support, n_support, SPA_TYPE_INTERFACE_Log); - libcamera_log_topic_init(impl->log); - - spa_hook_list_init(&impl->hooks); - - impl->device.iface = SPA_INTERFACE_INIT( - SPA_TYPE_INTERFACE_Device, - SPA_VERSION_DEVICE, - &impl_device, impl); - - reset_props(&impl->props); - - if (info && (str = spa_dict_lookup(info, SPA_KEY_API_LIBCAMERA_PATH))) - strncpy(impl->props.device, str, sizeof(impl->props.device)); - - impl->manager = libcamera_manager_acquire(); - if (impl->manager == NULL) { + auto manager = libcamera_manager_acquire(); + if (!manager) { res = -errno; - spa_log_error(impl->log, "can't start camera manager: %s", spa_strerror(res)); + spa_log_error(log, "can't start camera manager: %s", spa_strerror(res)); return res; } - impl->camera = impl->manager->get(impl->props.device); - if (impl->camera == NULL) { - spa_log_error(impl->log, "unknown camera id %s", impl->props.device); - libcamera_manager_release(impl->manager); + std::string device_id; + if (info && (str = spa_dict_lookup(info, SPA_KEY_API_LIBCAMERA_PATH))) + device_id = str; + + auto camera = manager->get(device_id); + if (!camera) { + spa_log_error(log, "unknown camera id %s", device_id.c_str()); + libcamera_manager_release(manager); return -ENOENT; } + + new (handle) impl(log, manager, std::move(camera), std::move(device_id)); + return 0; }