[libcamera-devel] [PATCH v2 09/13] libcamera: camera_manager: Run the camera manager in a thread
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Jan 23 21:44:31 CET 2020
Hi Laurent,
Thanks for breaking this apart.
On 2020-01-22 22:57:19 +0200, Laurent Pinchart wrote:
> Relying on the application event loop to process all our internal events
> is a bad idea for multiple reasons. In many cases the user of libcamera
> can't provide an event loop, for instance when running through one of
> the adaptation layers. The Android camera HAL and V4L2 compatibility
> layer create a thread for this reason, and the GStreamer element would
> need to do so as well. Furthermore, relying on the application event
> loop pushes libcamera's realtime constraints to the application, which
> isn't manageable.
>
> For these reasons it's desirable to always run the camera manager, the
> pipeline handlers and the cameras in a separate thread. Doing so isn't
> too complicated, it only involves creating the thread internally when
> starting the camera manager, and synchronizing a few methods of the
> Camera class. Do so as a first step towards defining the threading model
> of libcamera.
>
> The event dispatcher interface is still exposed to applications, to
> enable cross-thread signal delivery if desired.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v1:
>
> - Move "libcamera: camera_manager: Move private data members to private
> implementation" and "libcamera: camera_manager: Return a copy of the
> vector from cameras()" to separate patches
> ---
> src/libcamera/camera_manager.cpp | 94 ++++++++++++++++++++++++++++++--
> 1 file changed, 88 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 5fc1bba974c6..11c333ccb4e0 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -7,6 +7,7 @@
>
> #include <libcamera/camera_manager.h>
>
> +#include <condition_variable>
> #include <map>
>
> #include <libcamera/camera.h>
> @@ -28,33 +29,89 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(Camera)
>
> -class CameraManager::Private
> +class CameraManager::Private : public Thread
> {
> public:
> Private(CameraManager *cm);
>
> int start();
> - void stop();
> -
> void addCamera(std::shared_ptr<Camera> &camera, dev_t devnum);
> void removeCamera(Camera *camera);
>
> + /*
> + * This mutex protects
> + *
> + * - initialized_ and status_ during initialization
> + * - cameras_ and camerasByDevnum_ after initialization
> + */
> + Mutex mutex_;
> std::vector<std::shared_ptr<Camera>> cameras_;
> std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
>
> +protected:
> + void run() override;
> +
> private:
> + int init();
> + void cleanup();
> +
> CameraManager *cm_;
>
> + std::condition_variable cv_;
> + bool initialized_;
> + int status_;
> +
> std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> std::unique_ptr<DeviceEnumerator> enumerator_;
> };
>
> CameraManager::Private::Private(CameraManager *cm)
> - : cm_(cm)
> + : cm_(cm), initialized_(false)
> {
> }
>
> int CameraManager::Private::start()
> +{
> + /* Start the thread and wait for initialization to complete. */
> + Thread::start();
> +
> + {
> + MutexLocker locker(mutex_);
> + cv_.wait(locker, [&] { return initialized_; });
> + }
Do you not wish to copy status_ and use it bellow while the mutex is
still locked?
With this issue resolved,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> +
> + /* If a failure happened during initialization, stop the thread. */
> + if (status_ < 0) {
> + exit();
> + wait();
> + return status_;
> + }
> +
> + return 0;
> +}
> +
> +void CameraManager::Private::run()
> +{
> + LOG(Camera, Debug) << "Starting camera manager";
> +
> + int ret = init();
> +
> + mutex_.lock();
> + status_ = ret;
> + initialized_ = true;
> + mutex_.unlock();
> + cv_.notify_one();
> +
> + if (ret < 0)
> + return;
> +
> + /* Now start processing events and messages. */
> + exec();
> +
> + cleanup();
> +}
> +
> +int CameraManager::Private::init()
> {
> enumerator_ = DeviceEnumerator::create();
> if (!enumerator_ || enumerator_->enumerate())
> @@ -89,7 +146,7 @@ int CameraManager::Private::start()
> return 0;
> }
>
> -void CameraManager::Private::stop()
> +void CameraManager::Private::cleanup()
> {
> /* TODO: unregister hot-plug callback here */
>
> @@ -107,6 +164,8 @@ void CameraManager::Private::stop()
> void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
> dev_t devnum)
> {
> + MutexLocker locker(mutex_);
> +
> for (std::shared_ptr<Camera> c : cameras_) {
> if (c->name() == camera->name()) {
> LOG(Camera, Warning)
> @@ -126,6 +185,8 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> &camera,
>
> void CameraManager::Private::removeCamera(Camera *camera)
> {
> + MutexLocker locker(mutex_);
> +
> auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> [camera](std::shared_ptr<Camera> &c) {
> return c.get() == camera;
> @@ -232,7 +293,8 @@ int CameraManager::start()
> */
> void CameraManager::stop()
> {
> - p_->stop();
> + p_->exit();
> + p_->wait();
> }
>
> /**
> @@ -242,10 +304,14 @@ void CameraManager::stop()
> * Before calling this function the caller is responsible for ensuring that
> * the camera manager is running.
> *
> + * \context This function is \threadsafe.
> + *
> * \return List of all available cameras
> */
> std::vector<std::shared_ptr<Camera>> CameraManager::cameras() const
> {
> + MutexLocker locker(p_->mutex_);
> +
> return p_->cameras_;
> }
>
> @@ -256,10 +322,14 @@ std::vector<std::shared_ptr<Camera>> CameraManager::cameras() const
> * Before calling this function the caller is responsible for ensuring that
> * the camera manager is running.
> *
> + * \context This function is \threadsafe.
> + *
> * \return Shared pointer to Camera object or nullptr if camera not found
> */
> std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> {
> + MutexLocker locker(p_->mutex_);
> +
> for (std::shared_ptr<Camera> camera : p_->cameras_) {
> if (camera->name() == name)
> return camera;
> @@ -279,11 +349,15 @@ std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> * Before calling this function the caller is responsible for ensuring that
> * the camera manager is running.
> *
> + * \context This function is \threadsafe.
> + *
> * \return Shared pointer to Camera object, which is empty if the camera is
> * not found
> */
> std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> {
> + MutexLocker locker(p_->mutex_);
> +
> auto iter = p_->camerasByDevnum_.find(devnum);
> if (iter == p_->camerasByDevnum_.end())
> return nullptr;
> @@ -302,9 +376,12 @@ std::shared_ptr<Camera> CameraManager::get(dev_t devnum)
> *
> * \a devnum is used by the V4L2 compatibility layer to map V4L2 device nodes
> * to Camera instances.
> + *
> + * \context This function shall be called from the CameraManager thread.
> */
> void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
> {
> + ASSERT(Thread::current() == p_.get());
>
> p_->addCamera(camera, devnum);
> }
> @@ -316,15 +393,20 @@ void CameraManager::addCamera(std::shared_ptr<Camera> camera, dev_t devnum)
> * This function is called by pipeline handlers to unregister cameras from the
> * camera manager. Unregistered cameras won't be reported anymore by the
> * cameras() and get() calls, but references may still exist in applications.
> + *
> + * \context This function shall be called from the CameraManager thread.
> */
> void CameraManager::removeCamera(Camera *camera)
> {
> + ASSERT(Thread::current() == p_.get());
> +
> p_->removeCamera(camera);
> }
>
> /**
> * \fn const std::string &CameraManager::version()
> * \brief Retrieve the libcamera version string
> + * \context This function is \a threadsafe.
> * \return The libcamera version string
> */
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list