[libcamera-devel] [PATCH v2 3/4] android: camera_hal_manager: Support camera hotplug
Umang Jain
email at uajain.com
Fri Aug 14 13:53:07 CEST 2020
Hi Niklas,
On 8/14/20 2:55 PM, Niklas Söderlund wrote:
> Hi Umang,
>
> On 2020-08-14 08:29:58 +0000, Umang Jain wrote:
>> 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.
> As I stated above, having a single map is only a good simplification
> _if_ we treat each camera as a new camera. That is that we do not cache
> the cameras in order to reuse the numerical IDs.
Ok. I think I was confused as we do treat each camera as a new camera here
(context: we create new pointers, initialize them *again* etc.) and that we
don't cache the `CameraDevice`s at all, apart from their `id` part in
the map.
Anyway, the confusion is now cleared up.
> If we need to cache the ID information to fulfill some requirement of
> the HAL design then my suggestion of a single map is unsuitable, as you
> point.
>
>>>>> + 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