[libcamera-devel] [PATCH 13/14] libcamera: camera_manager: Construct CameraManager instances manually
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 19 10:40:00 CEST 2019
On Sun, Aug 18, 2019 at 04:13:28AM +0300, Laurent Pinchart wrote:
> The CameraManager class is not supposed to be instantiated multiple
> times, which led to a singleton implementation. This requires a global
> instance of the CameraManager, which is destroyed when the global
> destructors are executed.
>
> Relying on global instances causes issues with cleanup, as the order in
> which the global destructors are run can't be controlled. In particular,
> the Android camera HAL implementation ends up destroying the
> CameraHalManager after the CameraManager, which leads to use-after-free
> problems.
>
> To solve this, remove the CameraManager::instance() method and make the
> CameraManager class instantiable directly. Multiple instances are still
> not allowed, and this is enforced by storing the instance pointer
> internally to be checked when an instance is created.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/camera_manager.h | 12 ++++----
> src/android/camera_hal_manager.cpp | 5 +++-
> src/cam/main.cpp | 8 ++++-
> src/libcamera/camera_manager.cpp | 36 ++++++++++-------------
> src/qcam/main.cpp | 4 ++-
> test/camera/camera_test.cpp | 3 +-
> test/controls/control_list.cpp | 3 +-
> test/list-cameras.cpp | 11 +++----
> test/pipeline/ipu3/ipu3_pipeline_test.cpp | 3 +-
> 9 files changed, 48 insertions(+), 37 deletions(-)
>
[snip]
>
> +CameraManager *CameraManager::self_ = nullptr;
> +
> CameraManager::CameraManager()
> : enumerator_(nullptr)
> {
> + if (self_)
> + LOG(Camera, Fatal)
> + << "Multiple CameraManager objects are not allowed";
> +
> + self_ = this;
This is a very weak guarantee that the CameraManager would not be
created twice...
I was wondering, our previous singleton implementation was based on
lazy evaluation, by returning a statically allocated CameraManager instance:
CameraManager *CameraManager::instance()
{
static CameraManager manager;
return &manager;
}
This mean it gets deleted at program termination time causing the
issue you're here trying to solve.
What if the make the manager creation dynamic instead and provide a
static destructor method, that applications could call to guarantee
the manager is deleted at the right time?
Would something like this work in your opinion?
diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
index ff7d4c7c6745..a29a24f5e37a 100644
--- a/include/libcamera/camera_manager.h
+++ b/include/libcamera/camera_manager.h
@@ -33,6 +33,7 @@ public:
void removeCamera(Camera *camera);
static CameraManager *instance();
+ static void destroy();
static const std::string &version() { return version_; }
void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
@@ -49,6 +50,8 @@ private:
std::vector<std::shared_ptr<Camera>> cameras_;
static const std::string version_;
+
+ static CameraManager *manager_;
};
} /* namespace libcamera */
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 4a880684c5cb..ed175ccc5860 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -26,6 +26,8 @@ namespace libcamera {
LOG_DEFINE_CATEGORY(Camera)
+CameraManager *CameraManager::manager_ = nullptr;
+
/**
* \class CameraManager
* \brief Provide access and manage all cameras in the system
@@ -223,8 +225,16 @@ void CameraManager::removeCamera(Camera *camera)
*/
CameraManager *CameraManager::instance()
{
- static CameraManager manager;
- return &manager;
+ if (!manager_)
+ manager_ = new CameraManager();
+
+ return manager_;
+}
+
+void CameraManager::destroy()
+{
+ delete manager_;
+ manager_ = nullptr;
}
/**
Thanks
j
> }
>
> CameraManager::~CameraManager()
> {
> + self_ = nullptr;
> }
>
> /**
> @@ -212,21 +223,6 @@ void CameraManager::removeCamera(Camera *camera)
> }
> }
>
> -/**
> - * \brief Retrieve the camera manager instance
> - *
> - * The CameraManager is a singleton and can't be constructed manually. This
> - * function shall instead be used to retrieve the single global instance of the
> - * manager.
> - *
> - * \return The camera manager instance
> - */
> -CameraManager *CameraManager::instance()
> -{
> - static CameraManager manager;
> - return &manager;
> -}
> -
> /**
> * \fn const std::string &CameraManager::version()
> * \brief Retrieve the libcamera version string
> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> index 05d3b77e9edb..a7ff5c52663b 100644
> --- a/src/qcam/main.cpp
> +++ b/src/qcam/main.cpp
> @@ -63,7 +63,7 @@ int main(int argc, char **argv)
> sigaction(SIGINT, &sa, nullptr);
>
> std::unique_ptr<EventDispatcher> dispatcher(new QtEventDispatcher());
> - CameraManager *cm = CameraManager::instance();
> + CameraManager *cm = new CameraManager();
> cm->setEventDispatcher(std::move(dispatcher));
>
> ret = cm->start();
> @@ -79,5 +79,7 @@ int main(int argc, char **argv)
> delete mainWindow;
>
> cm->stop();
> + delete cm;
> +
> return ret;
> }
> diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp
> index 24ff5fe0c64d..0e105414bf46 100644
> --- a/test/camera/camera_test.cpp
> +++ b/test/camera/camera_test.cpp
> @@ -14,7 +14,7 @@ using namespace std;
>
> int CameraTest::init()
> {
> - cm_ = CameraManager::instance();
> + cm_ = new CameraManager();
>
> if (cm_->start()) {
> cout << "Failed to start camera manager" << endl;
> @@ -44,4 +44,5 @@ void CameraTest::cleanup()
> }
>
> cm_->stop();
> + delete cm_;
> };
> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> index c834edc352f5..f1d79ff8fcfd 100644
> --- a/test/controls/control_list.cpp
> +++ b/test/controls/control_list.cpp
> @@ -21,7 +21,7 @@ class ControlListTest : public Test
> protected:
> int init()
> {
> - cm_ = CameraManager::instance();
> + cm_ = new CameraManager();
>
> if (cm_->start()) {
> cout << "Failed to start camera manager" << endl;
> @@ -203,6 +203,7 @@ protected:
> }
>
> cm_->stop();
> + delete cm_;
> }
>
> private:
> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
> index 070cbf2be977..55551d7e7e10 100644
> --- a/test/list-cameras.cpp
> +++ b/test/list-cameras.cpp
> @@ -20,8 +20,8 @@ class ListTest : public Test
> protected:
> int init()
> {
> - cm = CameraManager::instance();
> - cm->start();
> + cm_ = new CameraManager();
> + cm_->start();
>
> return 0;
> }
> @@ -30,7 +30,7 @@ protected:
> {
> unsigned int count = 0;
>
> - for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
> + for (const std::shared_ptr<Camera> &camera : cm_->cameras()) {
> cout << "- " << camera->name() << endl;
> count++;
> }
> @@ -40,11 +40,12 @@ protected:
>
> void cleanup()
> {
> - cm->stop();
> + cm_->stop();
> + delete cm_;
> }
>
> private:
> - CameraManager *cm;
> + CameraManager *cm_;
> };
>
> TEST_REGISTER(ListTest)
> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> index 1d4cc4d4950b..8bfcd609a071 100644
> --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> @@ -92,7 +92,7 @@ int IPU3PipelineTest::init()
>
> enumerator.reset(nullptr);
>
> - cameraManager_ = CameraManager::instance();
> + cameraManager_ = new CameraManager();
> ret = cameraManager_->start();
> if (ret) {
> cerr << "Failed to start the CameraManager" << endl;
> @@ -120,6 +120,7 @@ int IPU3PipelineTest::run()
> void IPU3PipelineTest::cleanup()
> {
> cameraManager_->stop();
> + delete cameraManager_;
> }
>
> TEST_REGISTER(IPU3PipelineTest)
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190819/6d050240/attachment.sig>
More information about the libcamera-devel
mailing list