[PATCH] libcamera: ipa_manager: Remove singleton requirement
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Aug 7 15:16:39 CEST 2024
Quoting Laurent Pinchart (2024-08-03 22:28:32)
> The IPAManager class implements a singleton pattern due to the need of
> accessing the instance in a static member function. The function now
> takes a pointer to a PipelineHandler, which we can use to access the
> CameraManager, and from there, the IPAManager.
>
> Add accessors to the internal API to expose the CameraManager from the
> PipelineHandler, and the IPAManager from the CameraManager. This
> requires allocating the IPAManager dynamically to avoid a loop in
> includes. Use those accessors to replace the IPAManager singleton.
>
> Update the IPA interface unit test to instantiate a CameraManager
> instead of an IPAManager and ProcessManager, to reflect the new way that
> the IPAManager is accessed.
I guess that's a bit more heavyweight for a unit test, but nothing out
of the ordinary given 'CameraManager' is a core component of libcamera.
No objection to this, and removing seems worthwhile - but I'm curious
what the motivation was.
Anyway, I haven't spotted any errors so:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/internal/camera_manager.h | 7 +++++--
> include/libcamera/internal/ipa_manager.h | 9 +++++----
> include/libcamera/internal/pipeline_handler.h | 2 ++
> src/libcamera/camera_manager.cpp | 9 +++++++++
> src/libcamera/ipa_manager.cpp | 10 ----------
> src/libcamera/pipeline_handler.cpp | 7 +++++++
> test/ipa/ipa_interface_test.cpp | 12 +++++-------
> 7 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index af9ed60a0353..e098cb69aa43 100644
> --- a/include/libcamera/internal/camera_manager.h
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -19,13 +19,14 @@
> #include <libcamera/base/thread.h>
> #include <libcamera/base/thread_annotations.h>
>
> -#include "libcamera/internal/ipa_manager.h"
> #include "libcamera/internal/process.h"
>
> namespace libcamera {
>
> class Camera;
> class DeviceEnumerator;
> +class IPAManager;
> +class PipelineHandlerFactoryBase;
>
> class CameraManager::Private : public Extensible::Private, public Thread
> {
> @@ -38,6 +39,8 @@ public:
> void addCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
> void removeCamera(std::shared_ptr<Camera> camera) LIBCAMERA_TSA_EXCLUDES(mutex_);
>
> + IPAManager *ipaManager() const { return ipaManager_.get(); }
> +
> protected:
> void run() override;
>
> @@ -62,7 +65,7 @@ private:
>
> std::unique_ptr<DeviceEnumerator> enumerator_;
>
> - IPAManager ipaManager_;
> + std::unique_ptr<IPAManager> ipaManager_;
> ProcessManager processManager_;
> };
>
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index c6f74e11c434..16dede0c5a7e 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -15,6 +15,7 @@
> #include <libcamera/ipa/ipa_interface.h>
> #include <libcamera/ipa/ipa_module_info.h>
>
> +#include "libcamera/internal/camera_manager.h"
> #include "libcamera/internal/ipa_module.h"
> #include "libcamera/internal/pipeline_handler.h"
> #include "libcamera/internal/pub_key.h"
> @@ -34,11 +35,13 @@ public:
> uint32_t minVersion,
> uint32_t maxVersion)
> {
> - IPAModule *m = self_->module(pipe, minVersion, maxVersion);
> + CameraManager *cm = pipe->cameraManager();
> + IPAManager *self = cm->_d()->ipaManager();
> + IPAModule *m = self->module(pipe, minVersion, maxVersion);
> if (!m)
> return nullptr;
>
> - std::unique_ptr<T> proxy = std::make_unique<T>(m, !self_->isSignatureValid(m));
> + std::unique_ptr<T> proxy = std::make_unique<T>(m, !self->isSignatureValid(m));
> if (!proxy->isValid()) {
> LOG(IPAManager, Error) << "Failed to load proxy";
> return nullptr;
> @@ -55,8 +58,6 @@ public:
> #endif
>
> private:
> - static IPAManager *self_;
> -
> void parseDir(const char *libDir, unsigned int maxDepth,
> std::vector<std::string> &files);
> unsigned int addDir(const char *libDir, unsigned int maxDepth = 0);
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 746a34f8e7bd..cad5812f640c 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -70,6 +70,8 @@ public:
>
> const char *name() const { return name_; }
>
> + CameraManager *cameraManager() const { return manager_; }
> +
> protected:
> void registerCamera(std::shared_ptr<Camera> camera);
> void hotplugMediaDevice(MediaDevice *media);
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 95a9e3264526..31760a8680cc 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -15,6 +15,7 @@
>
> #include "libcamera/internal/camera.h"
> #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/ipa_manager.h"
> #include "libcamera/internal/pipeline_handler.h"
>
> /**
> @@ -37,6 +38,7 @@ LOG_DEFINE_CATEGORY(Camera)
> CameraManager::Private::Private()
> : initialized_(false)
> {
> + ipaManager_ = std::make_unique<IPAManager>();
> }
>
> int CameraManager::Private::start()
> @@ -249,6 +251,13 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
> o->cameraRemoved.emit(camera);
> }
>
> +/**
> + * \fn CameraManager::Private::ipaManager() const
> + * \brief Retrieve the IPAManager
> + * \context This function is \threadsafe.
> + * \return The IPAManager for this CameraManager
> + */
> +
> /**
> * \class CameraManager
> * \brief Provide access and manage all cameras in the system
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index f4e0b6339f08..cfc24d389c4f 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -95,8 +95,6 @@ LOG_DEFINE_CATEGORY(IPAManager)
> * IPC.
> */
>
> -IPAManager *IPAManager::self_ = nullptr;
> -
> /**
> * \brief Construct an IPAManager instance
> *
> @@ -105,10 +103,6 @@ IPAManager *IPAManager::self_ = nullptr;
> */
> IPAManager::IPAManager()
> {
> - if (self_)
> - LOG(IPAManager, Fatal)
> - << "Multiple IPAManager objects are not allowed";
> -
> #if HAVE_IPA_PUBKEY
> if (!pubKey_.isValid())
> LOG(IPAManager, Warning) << "Public key not valid";
> @@ -153,16 +147,12 @@ IPAManager::IPAManager()
> if (!ipaCount)
> LOG(IPAManager, Warning)
> << "No IPA found in '" IPA_MODULE_DIR "'";
> -
> - self_ = this;
> }
>
> IPAManager::~IPAManager()
> {
> for (IPAModule *module : modules_)
> delete module;
> -
> - self_ = nullptr;
> }
>
> /**
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 5ea2ca780b63..5a6de685b292 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -719,6 +719,13 @@ void PipelineHandler::disconnect()
> * \return The pipeline handler name
> */
>
> +/**
> + * \fn PipelineHandler::cameraManager() const
> + * \brief Retrieve the CameraManager that this pipeline handler belongs to
> + * \context This function is \threadsafe.
> + * \return The CameraManager for this pipeline handler
> + */
> +
> /**
> * \class PipelineHandlerFactoryBase
> * \brief Base class for pipeline handler factories
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index e840f6ab17c4..b81783664977 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -20,11 +20,11 @@
> #include <libcamera/base/thread.h>
> #include <libcamera/base/timer.h>
>
> +#include "libcamera/internal/camera_manager.h"
> #include "libcamera/internal/device_enumerator.h"
> #include "libcamera/internal/ipa_manager.h"
> #include "libcamera/internal/ipa_module.h"
> #include "libcamera/internal/pipeline_handler.h"
> -#include "libcamera/internal/process.h"
>
> #include "test.h"
>
> @@ -44,20 +44,20 @@ public:
> {
> delete notifier_;
> ipa_.reset();
> - ipaManager_.reset();
> + cameraManager_.reset();
> }
>
> protected:
> int init() override
> {
> - ipaManager_ = make_unique<IPAManager>();
> + cameraManager_ = make_unique<CameraManager>();
>
> /* Create a pipeline handler for vimc. */
> const std::vector<PipelineHandlerFactoryBase *> &factories =
> PipelineHandlerFactoryBase::factories();
> for (const PipelineHandlerFactoryBase *factory : factories) {
> if (factory->name() == "vimc") {
> - pipe_ = factory->create(nullptr);
> + pipe_ = factory->create(cameraManager_.get());
> break;
> }
> }
> @@ -171,11 +171,9 @@ private:
> }
> }
>
> - ProcessManager processManager_;
> -
> std::shared_ptr<PipelineHandler> pipe_;
> std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> - std::unique_ptr<IPAManager> ipaManager_;
> + std::unique_ptr<CameraManager> cameraManager_;
> enum ipa::vimc::IPAOperationCode trace_;
> EventNotifier *notifier_;
> int fd_;
>
> base-commit: 19bbca3c0b376ba0183f5db53472c8c46cd402b5
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list