[libcamera-devel] [PATCH v2 3/4] android: camera_hal_manager: Support camera hotplug
Umang Jain
email at uajain.com
Fri Aug 14 10:29:58 CEST 2020
Hi Niklas,
On 8/13/20 5:52 PM, Umang Jain wrote:
> 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.
I found this to be a bit cubersome to implement. The foundation issue
here is:
we do need to cache the android HAL's ID<->libcamera::Camera::id mapping
to identify if we are replugging an already-seen camera (I am assuming that
libcamera::Camera::id is a unique ID generated by libcamera for each camera
and is constant whenever the camera is replugged).
If I just maintain a single map std::map<unsigned int,
std::shared_ptr<CameraDevice>>
the CameraDevice will be deleted on hot-unplug. Then I do not have a
comparator to
compare against, to know if the ID key should be reused or not, when
hotplug event
handlers.
>>
>>> + 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list