[PATCH] libcamera: ipa_manager: Remove singleton requirement
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 7 15:28:47 CEST 2024
On Wed, Aug 07, 2024 at 02:16:39PM +0100, Kieran Bingham wrote:
> 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.
I was trying to remove all singletons. Some are more difficult, so this
is still work in progress.
> 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