[libcamera-devel] [PATCH v3 5/5] android: camera_hal_manager: Support camera hotplug
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Aug 22 03:28:32 CEST 2020
Hi Umang,
On Fri, Aug 21, 2020 at 12:03:28PM +0000, Umang Jain wrote:
> On 8/19/20 9:03 PM, Laurent Pinchart wrote:
> > On Mon, Aug 17, 2020 at 08:26:40PM +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().
> >
> > s/being being/being/
> >
> >> 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
> >
> > s/it's/its/
> >
> >> '0' and for external cameras, they start with '1000'. Note, for the
> >> current implementation, we assume all UVC cameras are external cameras.
> > I'd drop this sentence, as nothing here is UVC-specific, it's an issue
> > to be solved in the UVC pipeline handler.
> >
> >> Also, acess to camerasMap_ and cameras_ are protected by a mutex.
> >
> > s/acess/accesses/
> >
> >> Signed-off-by: Umang Jain <email at uajain.com>
> >> ---
> >> src/android/camera_hal_manager.cpp | 174 +++++++++++++++++++++++++----
> >> src/android/camera_hal_manager.h | 18 +++
> >> 2 files changed, 170 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> >> index 3a744af..e851185 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();
> >
> > You don't need this, the CameraHalManager is being deleted, so the
> > camerasMap_ is deleted too. The reason we need it for cameras_ is to
> > release the Camera shared pointers before calling CameraManager::stop().
> >
> >>
> >> 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);
> >> +
> >> + internalCameras_ = 0;
> >> + externalCameras_ = 1000;
> >
> > Can you initialize those two fields in the constructor (using the member
> > initializer list) ?
> >
> >> +
> >> int ret = cameraManager_->start();
> >> if (ret) {
> >> LOG(HAL, Error) << "Failed to start camera manager: "
> >> @@ -56,35 +65,22 @@ 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()) {
> >> - std::shared_ptr<CameraDevice> camera = CameraDevice::create(index, cam);
> >> - ret = camera->initialize();
> >> - if (ret)
> >> - continue;
> >> -
> >> - cameras_.emplace_back(std::move(camera));
> >> - ++index;
> >> - }
> >> -
> >> return 0;
> >> }
> >>
> >> CameraDevice *CameraHalManager::open(unsigned int id,
> >> const hw_module_t *hardwareModule)
> >> {
> >> - if (id >= numCameras()) {
> >> + MutexLocker locker(mutex_);
> >> +
> >> + auto iter = cameraIterFromId(id);
> >> + if (iter == cameras_.end()) {
> >> 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 +89,114 @@ 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 previous ID is
> >> + * re-used and the camera is marked as CAMERA_DEVICE_STATUS_PRESENT
> >> + * subsequently.
> >
> > CAMERA_DEVICE_STATUS_PRESENT is unrelated to whether the camera is new
> > or has been seen before. I'd drop that part of the sentence.
> >
> >> + *
> >> + * IDs 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.
> >
> > s/* /* /
> >
> >> + */
> >> + const ControlList &properties = cam->properties();
> >> + if (properties.contains(properties::Location) &&
> >> + properties.get(properties::Location) ==
> >> + properties::CameraLocationExternal) {
> >> + isCameraExternal = true;
> >> + id = externalCameras_;
> >> + } else {
> >> + id = internalCameras_;
> >> + }
> >> + }
> >> +
> >> + /*
> >> + * For each Camera registered in the system, a CameraDevice
> >> + * gets created here to wraps a libcamera Camera instance.
> >
> > s/wraps/wrap/
> >
> > And this isn't about creating a CameraDevice instance for each camera
> > anymore. I'd write this
> >
> > /* Create a CameraDevice instance to wrap the libcamera Camera. */
> >
> >> + */
> >> + 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)
> >> + externalCameras_++;
> >> + else
> >> + internalCameras_++;
> >> + }
> >> +
> >> + 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 = cameraIterFromCamera(cam.get());
> >> + if (iter == cameras_.end())
> >> + return;
> >> +
> >> + unsigned int id = (*iter)->id();
> >> + callbacks_->camera_device_status_change(callbacks_, id,
> >> + CAMERA_DEVICE_STATUS_NOT_PRESENT);
> >
> > This should only be called for external cameras. cameraRemoved()
> > shouldn't be called for internal cameras in the first place, so maybe
> > there's no need for a conditional here.
>
> cameraRemoved() shouldn't be called for internal cameras?
> Well, I wonder what exacts means by "unplugging an internal camera",
> but I think *if* somehow, internal cameras are unplugged, we do need to
> drop the reference from cameras_, no?
I meant that if a camera is internal, removing it from the system at
runtime shouldn't occur. If someone decides to tear the system apart,
worse problems will appear :-) If course we should support this to the
best of our ability in the HAL, but the Android camera service doesn't
expect an internal camera to be unplugged, so I doubt it will be handled
gracefully there.
> >> +
> >> + /*
> >> + * \todo Check if the camera is already open and running.
> >> + * Inform the framework about it's absence before deleting it's
> >
> > s/it's/its/g
> >
> >> + * reference here.
> >
> > Could this be as simple as creating a
> >
> > std::list<std::shared_ptr<CameraDevice>> openCameras_;
> >
> > to hold references to open cameras ? It could be done on top.
> >
> >> + */
> >> + cameras_.erase(iter);
> >> +
> >> + LOG(HAL, Debug) << "Camera ID: " << id << " removed successfully.";
> >> +}
> >> +
> >> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromId(unsigned int id)
> >> +{
> >> + return std::find_if(cameras_.begin(), cameras_.end(),
> >> + [id](std::shared_ptr<CameraDevice> &camera) {
> >> + return camera->id() == id;
> >> + });
> >> +}
> >> +
> >> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromCamera(Camera *cam)
> >> +{
> >> + return std::find_if(cameras_.begin(), cameras_.end(),
> >> + [cam](std::shared_ptr<CameraDevice> &camera) {
> >> + return cam == camera->camera();
> >> + });
> >> +}
> >> +
> >> unsigned int CameraHalManager::numCameras() const
> >> {
> >> - return cameraManager_->cameras().size();
> >> + return internalCameras_;
> >> }
> >>
> >> int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
> >> @@ -103,12 +204,15 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
> >> if (!info)
> >> return -EINVAL;
> >>
> >> - if (id >= numCameras()) {
> >> + MutexLocker locker(mutex_);
> >> +
> >> + auto iter = cameraIterFromId(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 +228,30 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
> >> void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
> >> {
> >> callbacks_ = callbacks;
> >> +
> >> + MutexLocker locker(mutex_);
> >> +
> >> + /*
> >> + * Some external cameras may have been identified before the
> >> + * callbacks_ were enabled. Iterate any existing external
> >
> > s/enabled/set/ ?
> > s/any/all/
> >
> >> + * cameras and mark them as CAMERA_DEVICE_STATUS_PRESENT
> >> + * explicitly.
> >> + *
> >> + * Internal cameras are already assumed to be present at module
> >> + * load time by the android framework.
> >> + */
> >> + for (auto &cam : cameraManager_->cameras()) {
> >> + auto iter = camerasMap_.find(cam->id());
> >> + if (iter == camerasMap_.end())
> >> + continue;
> >
> > How about iterating over cameras_ instead ? You wouldn't need to look up
> > the id in camerasMap_.
> >
> > for (std::shared_ptr<CameraDevice> &camera : cameras_) {
> > Camera *cam = camera->camera();
> > unsigned int id = camera->id();
> >
> > (you could also drop some of the local variables if desired, as they are
> > both used once only).
> >
> >> +
> >> + unsigned int id = iter->second;
> >> + const ControlList &properties = cam->properties();
> >> + if (properties.contains(properties::Location) &&
> >> + properties.get(properties::Location) &
> >
> > The Location property isn't a bitfield, it's an enum.
> >
> >> + properties::CameraLocationExternal) {
> >
> > There are two locations where you check the Location property. Would
> > adding the following function make sense ?
> >
> > static int32_t CameraHalManager::cameraLocation(Camera *cam)
> > {
> > const ControlList &properties = cam->properties();
> > if (!properties.contains(properties::Location))
> > return -1;
> >
> > return properties.get(properties::Location);
> > }
>
> Do you think it should be a 'static' method? I am curious why?
> Any special reasons?
The function doesn't access any member of CameraHalManager, so it can be
static. The static keyword should only occur in the .h file though, not
here.
> >
> > The caller would become
> >
> > if (cameraLocation(cam) == properties::CameraLocationExternal)
> >
> > which is a bit nicer to read I think.
> >
> >> + 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 3e34d63..f8adade 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,12 +20,17 @@
> >>
> >> class CameraDevice;
> >>
> >> +using Mutex = std::mutex;
> >> +using MutexLocker = std::unique_lock<std::mutex>;
> >> +
> >> class CameraHalManager
> >> {
> >> public:
> >> CameraHalManager();
> >> ~CameraHalManager();
> >>
> >> + using cameraDeviceIter = std::vector<std::shared_ptr<CameraDevice>>::iterator;
> >> +
> >> int init();
> >>
> >> CameraDevice *open(unsigned int id, const hw_module_t *module);
> >> @@ -33,10 +40,21 @@ 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);
> >> +
> >> + cameraDeviceIter cameraIterFromId(unsigned int id);
> >> + cameraDeviceIter cameraIterFromCamera(libcamera::Camera *cam);
> >
> > How about turning these to
> >
> > CameraDevice *cameraFromId(unsigned int id);
> > CameraDevice *cameraFromCamera(libcamera::Camera *cam);
> >
> > It would simplify the interface. I would actually call the former
> > cameraFromHALId() to differentiate the two types of IDs. And maybe
> >
> > CameraDevice *cameraDeviceFromHALId(unsigned int id);
> > CameraDevice *cameraDeviecFromCamera(libcamera::Camera *cam);
> >
> > if it's not too long ?
> >
> >> +
> >> libcamera::CameraManager *cameraManager_;
> >>
> >> const camera_module_callbacks_t *callbacks_;
> >> std::vector<std::shared_ptr<CameraDevice>> cameras_;
> >> + std::map<std::string, unsigned int> camerasMap_;
> >
> > Maybe cameraIdsMap_ to make the contents more explicit ?
> >
> >> + Mutex mutex_;
> >> +
> >> + unsigned int internalCameras_;
> >
> > Maybe numInternalCameras_ or internalCamerasCount_ ?
> >
> >> + unsigned int externalCameras_;
> >
> > And nextExternalCameraId_ ?
> >
> >> };
> >>
> >> #endif /* __ANDROID_CAMERA_MANAGER_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list