[libcamera-devel] [PATCH 13/19] libcamera: camera_manager: Run the camera manager in a thread
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jan 22 17:13:19 CET 2020
Hi Laurent,
Thanks for your work.
On 2020-01-20 02:24:31 +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.
>
> Making CameraManager::cameras() thread-safe requires returning a copy of
> the cameras vector instead of a reference. This is also required for
> hot-plugging support and is thus desirable.
>
> 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>
This patch is quiet hard to review as it do multiple things, would it be
possible to split it a bit? I think it can be split into
- Change the behavior of CameraManager::cameras()
- Breakout of CameraManager::Private
- New thread related code.
Small comments bellow tho.
> ---
> Documentation/Doxyfile.in | 1 +
> include/libcamera/camera_manager.h | 13 +-
> src/libcamera/camera_manager.cpp | 295 +++++++++++++++++++++--------
> 3 files changed, 224 insertions(+), 85 deletions(-)
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 734672ed15dc..1c46b04b3f7e 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -879,6 +879,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \
> libcamera::BoundMethodPackBase \
> libcamera::BoundMethodStatic \
> libcamera::SignalBase \
> + libcamera::*::Private \
> std::*
>
> # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 094197668698..079f848aec79 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -7,7 +7,6 @@
> #ifndef __LIBCAMERA_CAMERA_MANAGER_H__
> #define __LIBCAMERA_CAMERA_MANAGER_H__
>
> -#include <map>
> #include <memory>
> #include <string>
> #include <sys/types.h>
> @@ -18,9 +17,7 @@
> namespace libcamera {
>
> class Camera;
> -class DeviceEnumerator;
> class EventDispatcher;
> -class PipelineHandler;
>
> class CameraManager : public Object
> {
> @@ -33,7 +30,7 @@ public:
> int start();
> void stop();
>
> - const std::vector<std::shared_ptr<Camera>> &cameras() const { return cameras_; }
> + std::vector<std::shared_ptr<Camera>> cameras() const;
> std::shared_ptr<Camera> get(const std::string &name);
> std::shared_ptr<Camera> get(dev_t devnum);
>
> @@ -46,13 +43,11 @@ public:
> EventDispatcher *eventDispatcher();
>
> private:
> - std::unique_ptr<DeviceEnumerator> enumerator_;
> - std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> - std::vector<std::shared_ptr<Camera>> cameras_;
> - std::map<dev_t, std::weak_ptr<Camera>> camerasByDevnum_;
> -
> static const std::string version_;
> static CameraManager *self_;
> +
> + class Private;
> + std::unique_ptr<Private> p_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index a71caf8536bb..64aa89975701 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -7,6 +7,9 @@
>
> #include <libcamera/camera_manager.h>
>
> +#include <condition_variable>
> +#include <map>
> +
> #include <libcamera/camera.h>
> #include <libcamera/event_dispatcher.h>
>
> @@ -26,6 +29,184 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(Camera)
>
> +class CameraManager::Private : public Thread
> +{
> +public:
> + Private(CameraManager *cm);
> +
> + int start();
> + 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), 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_; });
> + }
For my education, is this in its a own block so that locker will go out
of scope and free the locker after the wait returns?
> +
> + /* If a failure happened during initialization, stop the thread. */
> + if (status_ < 0) {
> + exit();
> + wait();
> + return status_;
> + }
> +
> + return 0;
> +}
> +
> +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)
> + << "Registering camera with duplicate name '"
> + << camera->name() << "'";
> + break;
> + }
> + }
> +
> + cameras_.push_back(std::move(camera));
> +
> + if (devnum) {
> + unsigned int index = cameras_.size() - 1;
> + camerasByDevnum_[devnum] = cameras_[index];
> + }
> +}
> +
> +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;
> + });
> + if (iter == cameras_.end())
> + return;
> +
> + LOG(Camera, Debug)
> + << "Unregistering camera '" << camera->name() << "'";
> +
> + auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
> + [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
> + return p.second.lock().get() == camera;
> + });
> + if (iter_d != camerasByDevnum_.end())
> + camerasByDevnum_.erase(iter_d);
> +
> + cameras_.erase(iter);
> +}
> +
> +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())
> + return -ENODEV;
> +
> + /*
> + * TODO: Try to read handlers and order from configuration
> + * file and only fallback on all handlers if there is no
> + * configuration file.
> + */
> + std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> +
> + for (PipelineHandlerFactory *factory : factories) {
> + /*
> + * Try each pipeline handler until it exhaust
> + * all pipelines it can provide.
> + */
> + while (1) {
> + std::shared_ptr<PipelineHandler> pipe = factory->create(cm_);
> + if (!pipe->match(enumerator_.get()))
> + break;
> +
> + LOG(Camera, Debug)
> + << "Pipeline handler \"" << factory->name()
> + << "\" matched";
> + pipes_.push_back(std::move(pipe));
> + }
> + }
> +
> + /* TODO: register hot-plug callback here */
> +
> + return 0;
> +}
> +
> +void CameraManager::Private::cleanup()
> +{
> + /* TODO: unregister hot-plug callback here */
> +
> + /*
> + * Release all references to cameras and pipeline handlers to ensure
> + * they all get destroyed before the device enumerator deletes the
> + * media devices.
> + */
> + pipes_.clear();
> + cameras_.clear();
> +
> + enumerator_.reset(nullptr);
> +}
> +
> /**
> * \class CameraManager
> * \brief Provide access and manage all cameras in the system
> @@ -62,7 +243,7 @@ LOG_DEFINE_CATEGORY(Camera)
> CameraManager *CameraManager::self_ = nullptr;
>
> CameraManager::CameraManager()
> - : enumerator_(nullptr)
> + : p_(new CameraManager::Private(this))
> {
> if (self_)
> LOG(Camera, Fatal)
> @@ -73,6 +254,8 @@ CameraManager::CameraManager()
>
> CameraManager::~CameraManager()
> {
> + stop();
> +
> self_ = nullptr;
> }
>
> @@ -88,41 +271,16 @@ CameraManager::~CameraManager()
> */
> int CameraManager::start()
> {
> - if (enumerator_)
> - return -EBUSY;
> -
> LOG(Camera, Info) << "libcamera " << version_;
>
> - enumerator_ = DeviceEnumerator::create();
> - if (!enumerator_ || enumerator_->enumerate())
> - return -ENODEV;
> -
> - /*
> - * TODO: Try to read handlers and order from configuration
> - * file and only fallback on all handlers if there is no
> - * configuration file.
> - */
> - std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> + int ret = p_->start();
> + if (ret < 0) {
> + LOG(Camera, Error) << "Failed to start camera manager: "
> + << strerror(-ret);
>
> - for (PipelineHandlerFactory *factory : factories) {
> - /*
> - * Try each pipeline handler until it exhaust
> - * all pipelines it can provide.
> - */
> - while (1) {
> - std::shared_ptr<PipelineHandler> pipe = factory->create(this);
> - if (!pipe->match(enumerator_.get()))
> - break;
> -
> - LOG(Camera, Debug)
> - << "Pipeline handler \"" << factory->name()
> - << "\" matched";
> - pipes_.push_back(std::move(pipe));
> - }
> + return ret;
> }
>
> - /* TODO: register hot-plug callback here */
> -
> return 0;
I think you can simplify this to
int ret = p_->start();
if (ret)
LOG(Camera, Error) << "Failed to start camera manager: "
<< strerror(-ret);
return ret;
as start() shall return 0 on success.
> }
>
> @@ -138,17 +296,8 @@ int CameraManager::start()
> */
> void CameraManager::stop()
> {
> - /* TODO: unregister hot-plug callback here */
> -
> - /*
> - * Release all references to cameras and pipeline handlers to ensure
> - * they all get destroyed before the device enumerator deletes the
> - * media devices.
> - */
> - pipes_.clear();
> - cameras_.clear();
> -
> - enumerator_.reset(nullptr);
> + p_->exit();
> + p_->wait();
> }
>
> /**
> @@ -158,8 +307,16 @@ 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_;
> +}
>
> /**
> * \brief Get a camera based on name
> @@ -168,11 +325,15 @@ 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 Shared pointer to Camera object or nullptr if camera not found
> */
> std::shared_ptr<Camera> CameraManager::get(const std::string &name)
> {
> - for (std::shared_ptr<Camera> camera : cameras_) {
> + MutexLocker locker(p_->mutex_);
> +
> + for (std::shared_ptr<Camera> camera : p_->cameras_) {
> if (camera->name() == name)
> return camera;
> }
> @@ -191,13 +352,17 @@ 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)
> {
> - auto iter = camerasByDevnum_.find(devnum);
> - if (iter == camerasByDevnum_.end())
> + MutexLocker locker(p_->mutex_);
> +
> + auto iter = p_->camerasByDevnum_.find(devnum);
> + if (iter == p_->camerasByDevnum_.end())
> return nullptr;
>
> return iter->second.lock();
> @@ -214,24 +379,14 @@ 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)
> {
> - for (std::shared_ptr<Camera> c : cameras_) {
> - if (c->name() == camera->name()) {
> - LOG(Camera, Warning)
> - << "Registering camera with duplicate name '"
> - << camera->name() << "'";
> - break;
> - }
> - }
> -
> - cameras_.push_back(std::move(camera));
> + ASSERT(Thread::current() == p_.get());
>
> - if (devnum) {
> - unsigned int index = cameras_.size() - 1;
> - camerasByDevnum_[devnum] = cameras_[index];
> - }
> + p_->addCamera(camera, devnum);
> }
>
> /**
> @@ -241,32 +396,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)
> {
> - auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> - [camera](std::shared_ptr<Camera> &c) {
> - return c.get() == camera;
> - });
> - if (iter == cameras_.end())
> - return;
> -
> - LOG(Camera, Debug)
> - << "Unregistering camera '" << camera->name() << "'";
> -
> - auto iter_d = std::find_if(camerasByDevnum_.begin(), camerasByDevnum_.end(),
> - [camera](const std::pair<dev_t, std::weak_ptr<Camera>> &p) {
> - return p.second.lock().get() == camera;
> - });
> - if (iter_d != camerasByDevnum_.end())
> - camerasByDevnum_.erase(iter_d);
> + ASSERT(Thread::current() == p_.get());
>
> - cameras_.erase(iter);
> + 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