[libcamera-devel] [PATCH 13/14] libcamera: camera_manager: Construct CameraManager instances manually
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 19 18:05:00 CEST 2019
Hi Laurent,
On Mon, Aug 19, 2019 at 06:37:52PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Aug 19, 2019 at 10:40:00AM +0200, Jacopo Mondi wrote:
> > 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...
>
> Note that a fatal error message causes an std::abort() (in all builds,
> including release builds), so it's not *that* weak :-)
>
Oh I see, I thought it applied to debug builds only (and I initially
missed it was "Fatal")
> > 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?
>
> I've considered various options when working on this patch. What I don't
> like about an explicit destroy() method like proposed below is that it
> breaks the symmetry between construction and destruction. The instance()
> method dilutes the responsibility of creating the camera manager
> instance (the first call creates it, and the callers don't know if
> they're the first caller or not, which is the main purpose of a
> singleton), but requires giving the responsibility for deleting the
> instance to one particular actor in the system. I think it would be best
> to give the responsibility to create and destroy the camera manager
> explicitly.
I understand, and it would probably create a more confused API to deal
with.
>
> This being said, if that ends up being helpful, we could restore the
> instance() method but simply make it return self_. The method would thus
> return nullptr before the camera manager instance is constructed, or
> after it gets deleted. For now we don't have a need for that so I've
> simply dropped the instance() method.
>
As long as application instantiating multiple camera manager instance
are loudly notified (and crashing is loud enough), I'm fine with this
approach.
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> > 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;
> > }
> >
> > /**
> >
> > > }
> > >
> > > 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
-------------- 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/68e766ff/attachment.sig>
More information about the libcamera-devel
mailing list