[libcamera-devel] [PATCH v2 3/4] android: camera_hal_manager: Support camera hotplug
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Aug 11 17:02:44 CEST 2020
Hi Umang,
On 10/08/2020 13:04, Umang Jain wrote:
> Extend the support for camera hotplug from libcamera's CameraManager
> to CameraHalManager. Use camera module callbacks to let the framework
> know about the hotplug events and change the status of cameras being
> being hotplugged or unplugged via camera_device_status_change().
>
> Introduce a map camerasMap_ which book-keeps all cameras seen in the
> past by the CameraHalManager. If the camera is seen for the first time,
> a new id is assigned to it. If the camera has been seen before by the
> manager, it's old id is reused. IDs for internal cameras start with
> '0' and for external cameras, they start with '1000'. Note, for the
> current implementation, we assume all UVC cameras are external cameras.
>
> CameraDevice is now a shared object and cameras_ vector stores shared
> pointers to CameraDevice. This is done in order to introduce reference
> counting for CameraDevice objects - especially to handle hot-unplug
> events. Both camerasMap_ and cameras_ are protected by a mutex.
>
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
> src/android/camera_device.cpp | 15 +++
> src/android/camera_device.h | 8 +-
> src/android/camera_hal_manager.cpp | 153 ++++++++++++++++++++++++-----
> src/android/camera_hal_manager.h | 15 ++-
> 4 files changed, 166 insertions(+), 25 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index d918350..a79bb69 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -233,6 +233,21 @@ CameraDevice::~CameraDevice()
> delete it.second;
> }
>
> +std::shared_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> + const std::shared_ptr<Camera> &cam)
> +{
> + struct Deleter : std::default_delete<CameraDevice> {
> + void operator()(CameraDevice *camera)
> + {
> + delete camera;
> + }
> + };
> +
> + CameraDevice *camera = new CameraDevice(id, cam);
> +
> + return std::shared_ptr<CameraDevice>(camera, Deleter());
> +}
> +
Splitting out the making CameraDevice a shared_ptr might have been
helpful in it's own patch.
It's a single unified change,and would make the actual hotplug code
changes easier to identify.
> /*
> * Initialize the camera static information.
> * This method is called before the camera device is opened.
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 7be9e11..7f9e010 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -47,8 +47,8 @@ struct CameraStream {
> class CameraDevice : protected libcamera::Loggable
> {
> public:
> - CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> - ~CameraDevice();
> + static std::shared_ptr<CameraDevice> create(unsigned int id,
> + const std::shared_ptr<libcamera::Camera> &cam);
>
> int initialize();
>
> @@ -57,6 +57,7 @@ public:
>
> unsigned int id() const { return id_; }
> camera3_device_t *camera3Device() { return &camera3Device_; }
> + const libcamera::Camera *getCamera() { return camera_.get(); };
>
> int facing() const { return facing_; }
> int orientation() const { return orientation_; }
> @@ -72,6 +73,9 @@ protected:
> std::string logPrefix() const override;
>
> private:
> + CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> + ~CameraDevice();
> +
> struct Camera3RequestDescriptor {
> Camera3RequestDescriptor(unsigned int frameNumber,
> unsigned int numBuffers);
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 3d6d2b4..fdde2c0 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -8,6 +8,7 @@
> #include "camera_hal_manager.h"
>
> #include <libcamera/camera.h>
> +#include <libcamera/property_ids.h>
>
> #include "libcamera/internal/log.h"
>
> @@ -35,6 +36,7 @@ CameraHalManager::CameraHalManager()
> CameraHalManager::~CameraHalManager()
> {
> cameras_.clear();
> + camerasMap_.clear();
>
> if (cameraManager_) {
> cameraManager_->stop();
> @@ -47,6 +49,13 @@ int CameraHalManager::init()
> {
> cameraManager_ = new CameraManager();
>
> + /* Support camera hotplug. */
> + cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);
> + cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);
> +
> + cameraCounter_ = 0;
> + externalCameraCounter_ = 1000;
> +
> int ret = cameraManager_->start();
> if (ret) {
> LOG(HAL, Error) << "Failed to start camera manager: "
> @@ -56,35 +65,25 @@ int CameraHalManager::init()
> return ret;
> }
>
> - /*
> - * For each Camera registered in the system, a CameraDevice
> - * gets created here to wraps a libcamera Camera instance.
> - *
> - * \todo Support camera hotplug.
> - */
> - unsigned int index = 0;
> - for (auto &cam : cameraManager_->cameras()) {
> - CameraDevice *camera = new CameraDevice(index, cam);
> - ret = camera->initialize();
> - if (ret)
> - continue;
> -
> - cameras_.emplace_back(camera);
> - ++index;
> - }
> -
> return 0;
> }
>
> CameraDevice *CameraHalManager::open(unsigned int id,
> const hw_module_t *hardwareModule)
> {
> - if (id >= numCameras()) {
> + MutexLocker locker(mutex_);
> +
> + auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> + [id](std::shared_ptr<CameraDevice> &cam) {
> + return cam->id() == id;
> + });
> + if (iter == cameras_.end()) {
> LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> return nullptr;
> }
>
> - CameraDevice *camera = cameras_[id].get();
> + CameraDevice *camera = iter->get();
> +
> if (camera->open(hardwareModule))
> return nullptr;
>
> @@ -93,9 +92,91 @@ CameraDevice *CameraHalManager::open(unsigned int id,
> return camera;
> }
>
> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> +{
> + unsigned int id;
> + bool isCameraExternal = false;
> + bool isCameraNew = false;
> +
> + MutexLocker locker(mutex_);
> +
> + /* Each camera is assigned a unique integer id when it is seen for the
Block comments are opened with an empty line:
/*
* Each camera ...
I guess I should really get round to doing the checkpatch style checker
for comments. I don't want to turn in to Laurent ;-) hehe.
> + * first time. If the camera has been seen before, the id is reused and
> + * the camera is marked as CAMERA_DEVICE_STATUS_PRESENT subsequently.
> + *
> + * ID starts from '0' for internal cameras and '1000' for external cameras.
I'd pluralise this at ID
: IDs start from ...
> + */
> + auto iter = camerasMap_.find(cam->id());
> + if (iter != camerasMap_.end()) {
> + id = iter->second;
> + } else {
> + isCameraNew = true;
> +
> + /*
> + * Now check if this is an external camera and assign
> + * its id accordingly.
> + */
> + const ControlList &properties = cam->properties();
> + if (properties.contains(properties::Location) &&
> + properties.get(properties::Location) &
Should that be & or == ?
Can cameras have multiple locations?
> + properties::CameraLocationExternal) {
> + isCameraExternal = true;
> + id = externalCameraCounter_;
> + } else {
> + id = cameraCounter_;
> + }
> + }
> +
> + /*
> + * For each Camera registered in the system, a CameraDevice
> + * gets created here to wraps a libcamera Camera instance.
> + */
> + std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> + int ret = camera->initialize();
> + if (ret) {
> + LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
> + return;
> + }
> +
> + if (isCameraNew) {
> + camerasMap_.emplace(cam->id(), id);
> +
> + if (isCameraExternal)
> + externalCameraCounter_++;
> + else
> + cameraCounter_++;
> + }
> +
> + cameras_.emplace_back(std::move(camera));
> +
> + if (callbacks_)
> + callbacks_->camera_device_status_change(callbacks_, id,
> + CAMERA_DEVICE_STATUS_PRESENT);
I'd add a new line here before the debug print to keep it separate.
> + LOG(HAL, Debug) << "Camera ID: " << id << " added successfully.";
> +}
> +
> +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
> +{
> + MutexLocker locker(mutex_);
> +
> + auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> + [cam](std::shared_ptr<CameraDevice> &camera) {
> + return cam.get() == camera->getCamera();
> + });
> + if (iter == cameras_.end())
> + return;
> +
> + unsigned int id = (*iter)->id();
> + callbacks_->camera_device_status_change(callbacks_, id,
> + CAMERA_DEVICE_STATUS_NOT_PRESENT);
> + cameras_.erase(iter);
> +
> + LOG(HAL, Debug) << "Camera ID: " << id << " removed successfully.";
> +}
> +
> unsigned int CameraHalManager::numCameras() const
> {
> - return cameraManager_->cameras().size();
> + return cameraCounter_;
Is this right?
This won't count external cameras which are connected at the time
library startup.
But perhaps it's only meant to count 'internal' cameras anyway...#
I think cameraCounter_ probably needs to be named
internalCameras_
or such maybe, as 'cameraCounter_' to me counts all cameras including
external ones...
> }
>
> int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
> @@ -103,12 +184,18 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
> if (!info)
> return -EINVAL;
>
> - if (id >= numCameras()) {
> + MutexLocker locker(mutex_);
> +
> + auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> + [id](std::shared_ptr<CameraDevice> &cam) {
> + return cam->id() == id;
> + });
> + if (iter == cameras_.end()) {
> LOG(HAL, Error) << "Invalid camera id '" << id << "'";
> return -EINVAL;
> }
>
> - CameraDevice *camera = cameras_[id].get();
> + CameraDevice *camera = iter->get();
>
> info->facing = camera->facing();
> info->orientation = camera->orientation();
> @@ -124,4 +211,26 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
> void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
> {
> callbacks_ = callbacks;
> +
> + MutexLocker locker(mutex_);
> + /*
> + * Few cameras might have been hotplugged before setting callbacks_ here.
> + * We need to mark CAMERA_DEVICE_STATUS_PRESENT for them explicitly.
> + * This hold only for external cameras, as internal cameras are assumed to
> + * be present at module load time, by the framework.
Just to try to fix some of the grammar/flow here:
/*
* Some external cameras may have been identified before the
* callbacks_ were enabled. Iterate any existing external
* cameras and mark them as CAMERA_DEVICE_STATUS_PRESENT
* explicitly.
*
* Internal cameras are already assumed to be present at module
* load time by the android framework.
*/
Is this an actual possibility?
I assume it's just a race... so not expected.
> + */
> + for (auto &cam : cameraManager_->cameras()) {
> + auto iter = camerasMap_.find(cam->id());
> + if (iter == camerasMap_.end())
> + continue;
> +
> + unsigned int id = iter->second;
> + const ControlList &properties = cam->properties();
> + if (properties.contains(properties::Location) &&
> + properties.get(properties::Location) &
> + properties::CameraLocationExternal) {
> + callbacks_->camera_device_status_change(callbacks_, id,
> + CAMERA_DEVICE_STATUS_PRESENT);
> + }
> + }
> }
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index a582f04..7c481d4 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -7,6 +7,8 @@
> #ifndef __ANDROID_CAMERA_MANAGER_H__
> #define __ANDROID_CAMERA_MANAGER_H__
>
> +#include <map>
> +#include <mutex>
> #include <stddef.h>
> #include <vector>
>
> @@ -18,6 +20,9 @@
>
> class CameraDevice;
>
> +using Mutex = std::mutex;
> +using MutexLocker = std::unique_lock<std::mutex>;
Perhaps it doesn't matter too much, but should those be in the class
CameraHalManager scope?
I guess it depends on who includes this file.
> +
> class CameraHalManager
> {
> public:
> @@ -33,10 +38,18 @@ public:
> void setCallbacks(const camera_module_callbacks_t *callbacks);
>
> private:
> + void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
> + void cameraRemoved(std::shared_ptr<libcamera::Camera> cam);
> +
> libcamera::CameraManager *cameraManager_;
>
> const camera_module_callbacks_t *callbacks_;
> - std::vector<std::unique_ptr<CameraDevice>> cameras_;
> + std::vector<std::shared_ptr<CameraDevice>> cameras_;
> + std::map<std::string, unsigned int> camerasMap_;
> + Mutex mutex_;
> +
> + unsigned int externalCameraCounter_;
> + unsigned int cameraCounter_;
> };
>
> #endif /* __ANDROID_CAMERA_MANAGER_H__ */
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list