| Message ID | 20200817202629.4277-6-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. 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@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. > + > + /* > + * \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); } 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__ */
Hi Laurent, On 8/19/20 9:03 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > 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@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? > >> + >> + /* >> + * \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 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__ */
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@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__ */
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(); 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; + 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. + * + * 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. + */ + 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. + */ + 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); + + /* + * \todo Check if the camera is already open and running. + * Inform the framework about it's absence before deleting it's + * reference here. + */ + 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 + * 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; + + 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 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); + libcamera::CameraManager *cameraManager_; const camera_module_callbacks_t *callbacks_; std::vector<std::shared_ptr<CameraDevice>> cameras_; + std::map<std::string, unsigned int> camerasMap_; + Mutex mutex_; + + unsigned int internalCameras_; + unsigned int externalCameras_; }; #endif /* __ANDROID_CAMERA_MANAGER_H__ */
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. Also, acess to camerasMap_ and cameras_ are protected by a mutex. Signed-off-by: Umang Jain <email@uajain.com> --- src/android/camera_hal_manager.cpp | 174 +++++++++++++++++++++++++---- src/android/camera_hal_manager.h | 18 +++ 2 files changed, 170 insertions(+), 22 deletions(-)