[libcamera-devel] [PATCH v2 3/4] android: camera_hal_manager: Support camera hotplug
Umang Jain
email at uajain.com
Thu Aug 13 14:22:22 CEST 2020
Hi Niklas
On 8/13/20 5:14 PM, Niklas Söderlund wrote:
> Hi Umang,
>
> Thanks for your work.
>
> 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.
Ah, I just noticed this point. I assumed each camera will generate it's
own unique ID, plugged in for any of the ports. I am not sure, what actions/
measure we can take here to address this.
>
> - 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?
Laurent is following up too here, so I'll wait.
I basically took his suggestion from the v1 review [1] and steered the
patch into that
direction.
>
>> 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()
Noted. Well, this would then be a single-line patch and we won't be able
to see where
it's used. I am under the impression that API introduction and it's
relevant usage should
be done in a single commit(this patch uses the ->getCamera()) so ....
one can the entire
picture of the diff change.
>
>>
>> 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)?
Again, the spec is not clear on the hot-plugged / removable camera IDs
front.
It is though clear on the internal cameras IDs - should start from 0, hence,
we thought 1000 is a good number for assigning IDs for external cameras.
>
> 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.
Yes, I get your point. Their location is external, but from the
point-of-view
of HAL, they are still needed to be treated as "internal" cameras, no?
I guess, in HAL, internal cameras are the ones, which cannot be detached
from the device, whereas external cameras are the one, which can,
(Just a guess, I might be entirely wrong)
>
> 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.
Great point. I hope it's not too cubersome to iterate over all the entires,
to find the exact CameraDevice we want for various plug events handler.
I will look into it.
>
>> + Mutex mutex_;
>> +
>> + unsigned int externalCameraCounter_;
>> + unsigned int cameraCounter_;
>> };
>>
>> #endif /* __ANDROID_CAMERA_MANAGER_H__ */
>> --
>> 2.26.2
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
[1] :
https://lists.libcamera.org/pipermail/libcamera-devel/2020-August/011857.html
More information about the libcamera-devel
mailing list