[libcamera-devel] [PATCH v2 3/4] android: camera_hal_manager: Support camera hotplug
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 19 18:21:40 CEST 2020
Hi Shik,
On Thu, Aug 20, 2020 at 12:04:41AM +0800, Shik Chen wrote:
> On Wed, Aug 19, 2020 at 11:52 PM Laurent Pinchart wrote:
> > On Wed, Aug 19, 2020 at 11:41:45PM +0800, Shik Chen wrote:
> > > On Tue, Aug 18, 2020 at 4:36 PM Laurent Pinchart wrote:
> > > >
> > > > Hi Shik,
> > > >
> > > > Would it be possible for you to have a look at the question below ?
> > > >
> > > > On Thu, Aug 13, 2020 at 03:05:30PM +0300, Laurent Pinchart wrote:
> > > > > 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 ?
> > >
> > > It's for persistency in settings UI. Chrome uses the camera id as the key of
> > > the default camera in chrome://settings/content/camera. Reuse the same id for
> > > the same camera so the default camera would be changed back after replug.
> > >
> > > It's introduced in this CL: https://crrev.com/c/1608985
> >
> > Thank you for the information. We have a similar caching system, which
> > however suffers from two limitations:
> >
> > - If a camera is unplugged, and a different camera with the same VID:PID
> > is plugged into the same USB port, the new camera be given the same ID
> > as the previous camera.
>
> The current CrOS implementation will also use the same id in this case, and I
> think it should be fine.
>
> > - If a camera is unplugged and replugged in a different USB port, it
> > will be given a different ID.
>
> Hmm this looks non-ideal and may confuse the users.
We will try to fix that on top
This isn't trivial to handle, and there are corner cases. I'm thinking
in particular about a system where two identical external USB cameras
would be set up to point at particular locations. How to handle cameras
stable IDs in that case is debatable, and I don't think the problem can
be solved unless the cameras report distinct serial numbers (and most
webcams don't :-S). At the moment, libcamera identifies each camera with
a stable ID (guaranteed to be unique and stable across reboots, as long
as the hardware and firmware are not modified) made of the concatenation
of
- The USB controller DT or ACPI path
- The USB port(s) number(s) (starting at the root hub, going through
every hub)
- The USB device VID:PID
The Android ID is then assigned based on the libcamera ID, with a cache
to ensure that the same HAL ID will be given to the same libcamera ID.
If we were to drop the USB port number, we'd have to find another way to
create a stable and unique camera ID.
> > Is that acceptable from a Chrome OS point of view ?
> >
> > Would you happen to know how Android uses camera IDs, and whether it
> > requires or could benefit from ID caching ?
>
> Each camera app could have a similar preference caching system like the
> settings UI, so it would be better to reuse the same id for the same camera.
> AFAIK there is no such requirement at the framework level.
Thanks.
> > > > > > > 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