| Message ID | 20260918080734.1228227-12-naush@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Fri, Sep 18, 2026 at 08:59:49AM +0100, Naushir Patuck wrote: > The upcoming enumerate() and initialize() interfaces are called from the > application thread, but the device enumerator, pipeline handlers and > media devices they operate on are all bound to the camera manager > thread, so their work must be marshalled onto it. > > CameraManager::Private is a Thread and not an Object, so it cannot be > the target of Object::invokeMethod(). Derive it from Object as well and > move it to its own thread on construction. > > Changes in future commits can then start the thread and invoke work on > it regardless of the order in which they are called. notify_all() > replaces notify_one() as more than one caller can now be waiting in > startThread() for the same initialisation. Sorry, I'm jumping a bit back and forth but I'm trying to get the overall picture. This is required because you want to be able to call enumerate() and initialize() before the CameraManager is started, right ? > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/internal/camera_manager.h | 6 ++- > src/libcamera/camera_manager.cpp | 44 +++++++++++++++++++-- > 2 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > index e6df676bb655..ed90d1d471a0 100644 > --- a/include/libcamera/internal/camera_manager.h > +++ b/include/libcamera/internal/camera_manager.h > @@ -15,6 +15,7 @@ > > #include <libcamera/base/class.h> > #include <libcamera/base/mutex.h> > +#include <libcamera/base/object.h> > #include <libcamera/base/thread.h> > #include <libcamera/base/thread_annotations.h> > > @@ -30,7 +31,7 @@ class MediaDevice; > class PipelineHandler; > class PipelineHandlerFactoryBase; > > -class CameraManager::Private : public Extensible::Private, public Thread > +class CameraManager::Private : public Extensible::Private, public Thread, public Object > { > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > @@ -54,6 +55,7 @@ protected: > private: > int init(); > std::vector<const PipelineHandlerFactoryBase *> pipelineFactories() const; > + int startThread() LIBCAMERA_TSA_EXCLUDES(mutex_); > void createPipelineHandlers(); > void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory); > std::shared_ptr<PipelineHandler> findMatchingHandler(const MediaDevice *media); > @@ -72,6 +74,8 @@ private: > bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > > + bool started_ LIBCAMERA_TSA_GUARDED_BY(mutex_) = false; > + > std::unique_ptr<DeviceEnumerator> enumerator_; > > /* > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 55cf650b8e66..55532179a61c 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -41,14 +41,45 @@ LOG_DEFINE_CATEGORY(Camera) > CameraManager::Private::Private() > : Thread("CameraManager"), initialized_(false) > { > + /* > + * Bind this Object to its own thread, so that work can be marshalled > + * onto the camera manager thread with invokeMethod(). > + */ > + moveToThread(this); > } > > int CameraManager::Private::start() > { > - int status; > + return startThread(); > +} > + > +/* > + * Start the camera manager thread if not started yet, and wait for its > + * initialization to complete. Returns the initialization status. > + */ > +int CameraManager::Private::startThread() > +{ > + bool start = false; > + > + { > + MutexLocker locker(mutex_); > > - /* Start the thread and wait for initialization to complete. */ > - Thread::start(); > + if (!started_) { > + started_ = true; > + initialized_ = false; > + start = true; > + } > + } > + > + if (start) > + Thread::start(); > + > + /* > + * Wait for initialization to complete, whether this call started the > + * thread or another one did, as the caller may otherwise proceed before > + * the thread is ready. > + */ > + int status; > > { > MutexLocker locker(mutex_); > @@ -78,7 +109,7 @@ void CameraManager::Private::run() > status_ = ret; > initialized_ = true; > mutex_.unlock(); > - cv_.notify_one(); > + cv_.notify_all(); > > if (ret < 0) { > cleanup(); > @@ -228,6 +259,11 @@ void CameraManager::Private::cleanup() > { > enumerator_->devicesAdded.disconnect(this); > > + { > + MutexLocker locker(mutex_); > + started_ = false; > + } > + > /* > * Release all references to cameras to ensure they all get destroyed > * before the device enumerator deletes the media devices. Cameras are > -- > 2.53.0 >
Hi Jacopo, On Fri, 16 Oct 2026 at 14:40, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Sep 18, 2026 at 08:59:49AM +0100, Naushir Patuck wrote: > > The upcoming enumerate() and initialize() interfaces are called from the > > application thread, but the device enumerator, pipeline handlers and > > media devices they operate on are all bound to the camera manager > > thread, so their work must be marshalled onto it. > > > > CameraManager::Private is a Thread and not an Object, so it cannot be > > the target of Object::invokeMethod(). Derive it from Object as well and > > move it to its own thread on construction. > > > > Changes in future commits can then start the thread and invoke work on > > it regardless of the order in which they are called. notify_all() > > replaces notify_one() as more than one caller can now be waiting in > > startThread() for the same initialisation. > > Sorry, I'm jumping a bit back and forth but I'm trying to get the > overall picture. > > This is required because you want to be able to call enumerate() and > initialize() before the CameraManager is started, right ? Sorry, this series is quite big and touches many parts of the codebase :( That's correct. I think the Object::invokeMethod() changes would still be needed even if CameraManager was started first because enumerate() and initialize() run from the application thread while the enumerator and pipeline handlers live in the CameraManager thread... if that make sense? Regards, Naush > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/internal/camera_manager.h | 6 ++- > > src/libcamera/camera_manager.cpp | 44 +++++++++++++++++++-- > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > > index e6df676bb655..ed90d1d471a0 100644 > > --- a/include/libcamera/internal/camera_manager.h > > +++ b/include/libcamera/internal/camera_manager.h > > @@ -15,6 +15,7 @@ > > > > #include <libcamera/base/class.h> > > #include <libcamera/base/mutex.h> > > +#include <libcamera/base/object.h> > > #include <libcamera/base/thread.h> > > #include <libcamera/base/thread_annotations.h> > > > > @@ -30,7 +31,7 @@ class MediaDevice; > > class PipelineHandler; > > class PipelineHandlerFactoryBase; > > > > -class CameraManager::Private : public Extensible::Private, public Thread > > +class CameraManager::Private : public Extensible::Private, public Thread, public Object > > { > > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > > > @@ -54,6 +55,7 @@ protected: > > private: > > int init(); > > std::vector<const PipelineHandlerFactoryBase *> pipelineFactories() const; > > + int startThread() LIBCAMERA_TSA_EXCLUDES(mutex_); > > void createPipelineHandlers(); > > void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory); > > std::shared_ptr<PipelineHandler> findMatchingHandler(const MediaDevice *media); > > @@ -72,6 +74,8 @@ private: > > bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > > int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > > > > + bool started_ LIBCAMERA_TSA_GUARDED_BY(mutex_) = false; > > + > > std::unique_ptr<DeviceEnumerator> enumerator_; > > > > /* > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index 55cf650b8e66..55532179a61c 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -41,14 +41,45 @@ LOG_DEFINE_CATEGORY(Camera) > > CameraManager::Private::Private() > > : Thread("CameraManager"), initialized_(false) > > { > > + /* > > + * Bind this Object to its own thread, so that work can be marshalled > > + * onto the camera manager thread with invokeMethod(). > > + */ > > + moveToThread(this); > > } > > > > int CameraManager::Private::start() > > { > > - int status; > > + return startThread(); > > +} > > + > > +/* > > + * Start the camera manager thread if not started yet, and wait for its > > + * initialization to complete. Returns the initialization status. > > + */ > > +int CameraManager::Private::startThread() > > +{ > > + bool start = false; > > + > > + { > > + MutexLocker locker(mutex_); > > > > - /* Start the thread and wait for initialization to complete. */ > > - Thread::start(); > > + if (!started_) { > > + started_ = true; > > + initialized_ = false; > > + start = true; > > + } > > + } > > + > > + if (start) > > + Thread::start(); > > + > > + /* > > + * Wait for initialization to complete, whether this call started the > > + * thread or another one did, as the caller may otherwise proceed before > > + * the thread is ready. > > + */ > > + int status; > > > > { > > MutexLocker locker(mutex_); > > @@ -78,7 +109,7 @@ void CameraManager::Private::run() > > status_ = ret; > > initialized_ = true; > > mutex_.unlock(); > > - cv_.notify_one(); > > + cv_.notify_all(); > > > > if (ret < 0) { > > cleanup(); > > @@ -228,6 +259,11 @@ void CameraManager::Private::cleanup() > > { > > enumerator_->devicesAdded.disconnect(this); > > > > + { > > + MutexLocker locker(mutex_); > > + started_ = false; > > + } > > + > > /* > > * Release all references to cameras to ensure they all get destroyed > > * before the device enumerator deletes the media devices. Cameras are > > -- > > 2.53.0 > >
On Fri, Oct 16, 2026 at 02:52:59PM +0100, Naushir Patuck wrote: > Hi Jacopo, > > On Fri, 16 Oct 2026 at 14:40, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Fri, Sep 18, 2026 at 08:59:49AM +0100, Naushir Patuck wrote: > > > The upcoming enumerate() and initialize() interfaces are called from the > > > application thread, but the device enumerator, pipeline handlers and > > > media devices they operate on are all bound to the camera manager > > > thread, so their work must be marshalled onto it. > > > > > > CameraManager::Private is a Thread and not an Object, so it cannot be > > > the target of Object::invokeMethod(). Derive it from Object as well and > > > move it to its own thread on construction. > > > > > > Changes in future commits can then start the thread and invoke work on > > > it regardless of the order in which they are called. notify_all() > > > replaces notify_one() as more than one caller can now be waiting in > > > startThread() for the same initialisation. > > > > Sorry, I'm jumping a bit back and forth but I'm trying to get the > > overall picture. > > > > This is required because you want to be able to call enumerate() and > > initialize() before the CameraManager is started, right ? > > Sorry, this series is quite big and touches many parts of the codebase :( > > That's correct. I think the Object::invokeMethod() changes would > still be needed even if CameraManager was started first because > enumerate() and initialize() run from the application thread while the > enumerator and pipeline handlers live in the CameraManager thread... > if that make sense? It probably does. We don't need any of this today as the API we expose from the CameraManager is very thin and after start() only provides two accessors ? If we were to start the CameraManager thread and creates pipelines at start() (without creating Cameras) then we should implement an API similar to the Camera class where function calls are propagated over a signal with invokeMethod() ? > > Regards, > Naush > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/internal/camera_manager.h | 6 ++- > > > src/libcamera/camera_manager.cpp | 44 +++++++++++++++++++-- > > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > > > index e6df676bb655..ed90d1d471a0 100644 > > > --- a/include/libcamera/internal/camera_manager.h > > > +++ b/include/libcamera/internal/camera_manager.h > > > @@ -15,6 +15,7 @@ > > > > > > #include <libcamera/base/class.h> > > > #include <libcamera/base/mutex.h> > > > +#include <libcamera/base/object.h> > > > #include <libcamera/base/thread.h> > > > #include <libcamera/base/thread_annotations.h> > > > > > > @@ -30,7 +31,7 @@ class MediaDevice; > > > class PipelineHandler; > > > class PipelineHandlerFactoryBase; > > > > > > -class CameraManager::Private : public Extensible::Private, public Thread > > > +class CameraManager::Private : public Extensible::Private, public Thread, public Object > > > { > > > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > > > > > @@ -54,6 +55,7 @@ protected: > > > private: > > > int init(); > > > std::vector<const PipelineHandlerFactoryBase *> pipelineFactories() const; > > > + int startThread() LIBCAMERA_TSA_EXCLUDES(mutex_); > > > void createPipelineHandlers(); > > > void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory); > > > std::shared_ptr<PipelineHandler> findMatchingHandler(const MediaDevice *media); > > > @@ -72,6 +74,8 @@ private: > > > bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > > > int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > > > > > > + bool started_ LIBCAMERA_TSA_GUARDED_BY(mutex_) = false; > > > + > > > std::unique_ptr<DeviceEnumerator> enumerator_; > > > > > > /* > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > > index 55cf650b8e66..55532179a61c 100644 > > > --- a/src/libcamera/camera_manager.cpp > > > +++ b/src/libcamera/camera_manager.cpp > > > @@ -41,14 +41,45 @@ LOG_DEFINE_CATEGORY(Camera) > > > CameraManager::Private::Private() > > > : Thread("CameraManager"), initialized_(false) > > > { > > > + /* > > > + * Bind this Object to its own thread, so that work can be marshalled > > > + * onto the camera manager thread with invokeMethod(). > > > + */ > > > + moveToThread(this); > > > } > > > > > > int CameraManager::Private::start() > > > { > > > - int status; > > > + return startThread(); > > > +} > > > + > > > +/* > > > + * Start the camera manager thread if not started yet, and wait for its > > > + * initialization to complete. Returns the initialization status. > > > + */ > > > +int CameraManager::Private::startThread() > > > +{ > > > + bool start = false; > > > + > > > + { > > > + MutexLocker locker(mutex_); > > > > > > - /* Start the thread and wait for initialization to complete. */ > > > - Thread::start(); > > > + if (!started_) { > > > + started_ = true; > > > + initialized_ = false; > > > + start = true; > > > + } > > > + } > > > + > > > + if (start) > > > + Thread::start(); > > > + > > > + /* > > > + * Wait for initialization to complete, whether this call started the > > > + * thread or another one did, as the caller may otherwise proceed before > > > + * the thread is ready. > > > + */ > > > + int status; > > > > > > { > > > MutexLocker locker(mutex_); > > > @@ -78,7 +109,7 @@ void CameraManager::Private::run() > > > status_ = ret; > > > initialized_ = true; > > > mutex_.unlock(); > > > - cv_.notify_one(); > > > + cv_.notify_all(); > > > > > > if (ret < 0) { > > > cleanup(); > > > @@ -228,6 +259,11 @@ void CameraManager::Private::cleanup() > > > { > > > enumerator_->devicesAdded.disconnect(this); > > > > > > + { > > > + MutexLocker locker(mutex_); > > > + started_ = false; > > > + } > > > + > > > /* > > > * Release all references to cameras to ensure they all get destroyed > > > * before the device enumerator deletes the media devices. Cameras are > > > -- > > > 2.53.0 > > >
Hi Jacopo, On Fri, 16 Oct 2026 at 15:39, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > On Fri, Oct 16, 2026 at 02:52:59PM +0100, Naushir Patuck wrote: > > Hi Jacopo, > > > > On Fri, 16 Oct 2026 at 14:40, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Naush > > > > > > On Fri, Sep 18, 2026 at 08:59:49AM +0100, Naushir Patuck wrote: > > > > The upcoming enumerate() and initialize() interfaces are called from the > > > > application thread, but the device enumerator, pipeline handlers and > > > > media devices they operate on are all bound to the camera manager > > > > thread, so their work must be marshalled onto it. > > > > > > > > CameraManager::Private is a Thread and not an Object, so it cannot be > > > > the target of Object::invokeMethod(). Derive it from Object as well and > > > > move it to its own thread on construction. > > > > > > > > Changes in future commits can then start the thread and invoke work on > > > > it regardless of the order in which they are called. notify_all() > > > > replaces notify_one() as more than one caller can now be waiting in > > > > startThread() for the same initialisation. > > > > > > Sorry, I'm jumping a bit back and forth but I'm trying to get the > > > overall picture. > > > > > > This is required because you want to be able to call enumerate() and > > > initialize() before the CameraManager is started, right ? > > > > Sorry, this series is quite big and touches many parts of the codebase :( > > > > That's correct. I think the Object::invokeMethod() changes would > > still be needed even if CameraManager was started first because > > enumerate() and initialize() run from the application thread while the > > enumerator and pipeline handlers live in the CameraManager thread... > > if that make sense? > > It probably does. We don't need any of this today as the API we expose > from the CameraManager is very thin and after start() only provides > two accessors ? > > If we were to start the CameraManager thread and creates pipelines at > start() (without creating Cameras) then we should implement an API > similar to the Camera class where function calls are propagated over a > signal with invokeMethod() ? Yes, I think this makes sense. In fact, I've prototyped an implementation close to what you suggested that would be available in v2 of this work. Regards, Naush > > > > > Regards, > > Naush > > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > include/libcamera/internal/camera_manager.h | 6 ++- > > > > src/libcamera/camera_manager.cpp | 44 +++++++++++++++++++-- > > > > 2 files changed, 45 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h > > > > index e6df676bb655..ed90d1d471a0 100644 > > > > --- a/include/libcamera/internal/camera_manager.h > > > > +++ b/include/libcamera/internal/camera_manager.h > > > > @@ -15,6 +15,7 @@ > > > > > > > > #include <libcamera/base/class.h> > > > > #include <libcamera/base/mutex.h> > > > > +#include <libcamera/base/object.h> > > > > #include <libcamera/base/thread.h> > > > > #include <libcamera/base/thread_annotations.h> > > > > > > > > @@ -30,7 +31,7 @@ class MediaDevice; > > > > class PipelineHandler; > > > > class PipelineHandlerFactoryBase; > > > > > > > > -class CameraManager::Private : public Extensible::Private, public Thread > > > > +class CameraManager::Private : public Extensible::Private, public Thread, public Object > > > > { > > > > LIBCAMERA_DECLARE_PUBLIC(CameraManager) > > > > > > > > @@ -54,6 +55,7 @@ protected: > > > > private: > > > > int init(); > > > > std::vector<const PipelineHandlerFactoryBase *> pipelineFactories() const; > > > > + int startThread() LIBCAMERA_TSA_EXCLUDES(mutex_); > > > > void createPipelineHandlers(); > > > > void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory); > > > > std::shared_ptr<PipelineHandler> findMatchingHandler(const MediaDevice *media); > > > > @@ -72,6 +74,8 @@ private: > > > > bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > > > > int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_); > > > > > > > > + bool started_ LIBCAMERA_TSA_GUARDED_BY(mutex_) = false; > > > > + > > > > std::unique_ptr<DeviceEnumerator> enumerator_; > > > > > > > > /* > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > > > index 55cf650b8e66..55532179a61c 100644 > > > > --- a/src/libcamera/camera_manager.cpp > > > > +++ b/src/libcamera/camera_manager.cpp > > > > @@ -41,14 +41,45 @@ LOG_DEFINE_CATEGORY(Camera) > > > > CameraManager::Private::Private() > > > > : Thread("CameraManager"), initialized_(false) > > > > { > > > > + /* > > > > + * Bind this Object to its own thread, so that work can be marshalled > > > > + * onto the camera manager thread with invokeMethod(). > > > > + */ > > > > + moveToThread(this); > > > > } > > > > > > > > int CameraManager::Private::start() > > > > { > > > > - int status; > > > > + return startThread(); > > > > +} > > > > + > > > > +/* > > > > + * Start the camera manager thread if not started yet, and wait for its > > > > + * initialization to complete. Returns the initialization status. > > > > + */ > > > > +int CameraManager::Private::startThread() > > > > +{ > > > > + bool start = false; > > > > + > > > > + { > > > > + MutexLocker locker(mutex_); > > > > > > > > - /* Start the thread and wait for initialization to complete. */ > > > > - Thread::start(); > > > > + if (!started_) { > > > > + started_ = true; > > > > + initialized_ = false; > > > > + start = true; > > > > + } > > > > + } > > > > + > > > > + if (start) > > > > + Thread::start(); > > > > + > > > > + /* > > > > + * Wait for initialization to complete, whether this call started the > > > > + * thread or another one did, as the caller may otherwise proceed before > > > > + * the thread is ready. > > > > + */ > > > > + int status; > > > > > > > > { > > > > MutexLocker locker(mutex_); > > > > @@ -78,7 +109,7 @@ void CameraManager::Private::run() > > > > status_ = ret; > > > > initialized_ = true; > > > > mutex_.unlock(); > > > > - cv_.notify_one(); > > > > + cv_.notify_all(); > > > > > > > > if (ret < 0) { > > > > cleanup(); > > > > @@ -228,6 +259,11 @@ void CameraManager::Private::cleanup() > > > > { > > > > enumerator_->devicesAdded.disconnect(this); > > > > > > > > + { > > > > + MutexLocker locker(mutex_); > > > > + started_ = false; > > > > + } > > > > + > > > > /* > > > > * Release all references to cameras to ensure they all get destroyed > > > > * before the device enumerator deletes the media devices. Cameras are > > > > -- > > > > 2.53.0 > > > >
diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h index e6df676bb655..ed90d1d471a0 100644 --- a/include/libcamera/internal/camera_manager.h +++ b/include/libcamera/internal/camera_manager.h @@ -15,6 +15,7 @@ #include <libcamera/base/class.h> #include <libcamera/base/mutex.h> +#include <libcamera/base/object.h> #include <libcamera/base/thread.h> #include <libcamera/base/thread_annotations.h> @@ -30,7 +31,7 @@ class MediaDevice; class PipelineHandler; class PipelineHandlerFactoryBase; -class CameraManager::Private : public Extensible::Private, public Thread +class CameraManager::Private : public Extensible::Private, public Thread, public Object { LIBCAMERA_DECLARE_PUBLIC(CameraManager) @@ -54,6 +55,7 @@ protected: private: int init(); std::vector<const PipelineHandlerFactoryBase *> pipelineFactories() const; + int startThread() LIBCAMERA_TSA_EXCLUDES(mutex_); void createPipelineHandlers(); void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory); std::shared_ptr<PipelineHandler> findMatchingHandler(const MediaDevice *media); @@ -72,6 +74,8 @@ private: bool initialized_ LIBCAMERA_TSA_GUARDED_BY(mutex_); int status_ LIBCAMERA_TSA_GUARDED_BY(mutex_); + bool started_ LIBCAMERA_TSA_GUARDED_BY(mutex_) = false; + std::unique_ptr<DeviceEnumerator> enumerator_; /* diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 55cf650b8e66..55532179a61c 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -41,14 +41,45 @@ LOG_DEFINE_CATEGORY(Camera) CameraManager::Private::Private() : Thread("CameraManager"), initialized_(false) { + /* + * Bind this Object to its own thread, so that work can be marshalled + * onto the camera manager thread with invokeMethod(). + */ + moveToThread(this); } int CameraManager::Private::start() { - int status; + return startThread(); +} + +/* + * Start the camera manager thread if not started yet, and wait for its + * initialization to complete. Returns the initialization status. + */ +int CameraManager::Private::startThread() +{ + bool start = false; + + { + MutexLocker locker(mutex_); - /* Start the thread and wait for initialization to complete. */ - Thread::start(); + if (!started_) { + started_ = true; + initialized_ = false; + start = true; + } + } + + if (start) + Thread::start(); + + /* + * Wait for initialization to complete, whether this call started the + * thread or another one did, as the caller may otherwise proceed before + * the thread is ready. + */ + int status; { MutexLocker locker(mutex_); @@ -78,7 +109,7 @@ void CameraManager::Private::run() status_ = ret; initialized_ = true; mutex_.unlock(); - cv_.notify_one(); + cv_.notify_all(); if (ret < 0) { cleanup(); @@ -228,6 +259,11 @@ void CameraManager::Private::cleanup() { enumerator_->devicesAdded.disconnect(this); + { + MutexLocker locker(mutex_); + started_ = false; + } + /* * Release all references to cameras to ensure they all get destroyed * before the device enumerator deletes the media devices. Cameras are
The upcoming enumerate() and initialize() interfaces are called from the application thread, but the device enumerator, pipeline handlers and media devices they operate on are all bound to the camera manager thread, so their work must be marshalled onto it. CameraManager::Private is a Thread and not an Object, so it cannot be the target of Object::invokeMethod(). Derive it from Object as well and move it to its own thread on construction. Changes in future commits can then start the thread and invoke work on it regardless of the order in which they are called. notify_all() replaces notify_one() as more than one caller can now be waiting in startThread() for the same initialisation. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/internal/camera_manager.h | 6 ++- src/libcamera/camera_manager.cpp | 44 +++++++++++++++++++-- 2 files changed, 45 insertions(+), 5 deletions(-)