[libcamera-devel] [PATCH v2 3/4] android: camera_hal_manager: Support camera hotplug
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 13 14:05:29 CEST 2020
Hi Niklas,
(CC'ing Shik)
On Thu, Aug 13, 2020 at 01:44:18PM +0200, Niklas Söderlund wrote:
> On 2020-08-10 12:04:14 +0000, 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.
>
> I wonder if keeping the cache of previously seen cameras or if we should
> not just treat any hot-plugged camera as a new one? I can't think of any
> benefit of preserving the same numerical ID between two plug events,
> while I can think of quiet a few cons.
>
> - It's confusing when debugging as un-plugging and then replugging the
> same camera will result in logs where the numerical ID is the same for
> both. This may even result in things working by "chance" is it reuses
> an already known numerical ID.
>
> - Looking at the code plugging a UVC camera in a different USB port will
> generate a different Camera::id() result and then be treated as a new
> camera vs plugging it in the same port and it then being treatad as a
> new camera.
>
> - The logic in this patch is more complex due to it both having to deal
> with known and new cameras.
>
> What is the benefit of this cache that I'm missing?
It's not clear whether Android requires it. It requires different
cameras to have different IDs, but I couldn't find a mention of how
identical cameras should be treated. The Chrome OS UVC HAL caches the
ID, I don't know if it's mandatory though.
Shik, as you've worked on the UVC HAL, would you happen to know if there
are requirements to keep the same ID when a camera is unplugged and
replugged ?
> > 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());
> > +}
>
> As Kieran points out I think this should be added in a separate as this
> one is quiet large and therefore hard to review.
>
> > +
> > /*
> > * 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(); };
>
> Same here this can be done in a separate commit. Also I think this could
> be named camera() instead of getCamera()
>
> >
> > 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()) {
>
> I think you should break this (and the similar ones below) into private
> helpers instead if implementing the logic in-place
>
> CameraHalManager::cameraFromAndroidId(..);
> CameraHalManager::androidIdFromCamera(..);
>
> > 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
> > + * 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.
> > + */
> > + 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) &
> > + properties::CameraLocationExternal) {
> > + isCameraExternal = true;
> > + id = externalCameraCounter_;
> > + } else {
> > + id = cameraCounter_;
> > + }
>
> As I understand it the information the HAL wants is whether or not the
> camera can be hot-plugged (id >= 1000) or nor (id < 1000)?
>
> If so I wonder if using the camera location as a judgment of the camera
> is hot-plugged or not is the way to go here? Imagine a device where the
> camera is permanently attached (not hot-unpluggable) but not fixated in
> a location. I'm thinking cameras mounted at the end of instruments
> (medical instruments, hand held tools) or robotics (mounted at the arm
> of a welding robot). I would imagine those cameras would be marked as
> located externally but they would not really be hot-pluggable.
>
> I understand we have no other way to report or detect this at the moment
> and I'm not pushing hard for this to be solved as part of this series if
> it's not easy. But I think a bigger comment here is needed explaining
> that the HAL wants to know if a camera is hot-pluggable or not and does
> not really care if it's located internally or externally. I also think a
> \todo should be added so it's not forgotten.
>
> > + }
> > +
> > + /*
> > + * 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);
> > + 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_;
> > }
> >
> > 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.
> > + */
> > + 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>;
> > +
> > 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_;
>
> If each hot-plugged camera where treated as a new camera cameras_ and
> camerasMap_ could be merged to a
>
> std::map<unsigned int, std::shared_ptr<CameraDevice>> cameras_;
>
> Which would eliminate the possibility of them going out-of-sync.
>
> > + Mutex mutex_;
> > +
> > + unsigned int externalCameraCounter_;
> > + unsigned int cameraCounter_;
> > };
> >
> > #endif /* __ANDROID_CAMERA_MANAGER_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list