| Message ID | 20200805151437.157155-3-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Wed, Aug 05, 2020 at 03:14:44PM +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(). > > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/android/camera_device.h | 1 + > src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++-- > src/android/camera_hal_manager.h | 3 +++ > 3 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 4e5fb98..fa9706c 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -47,6 +47,7 @@ public: > > unsigned int id() const { return id_; } > camera3_device_t *camera3Device() { return &camera3Device_; } > + const libcamera::Camera *getCamera() { return camera_.get(); }; > > int facing() const { return facing_; } > int orientation() const { return orientation_; } > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index 3ddcab5..b498278 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -59,8 +59,6 @@ int CameraHalManager::init() > /* > * 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()) { > @@ -73,6 +71,10 @@ int CameraHalManager::init() > ++index; > } > > + /* Support camera hotplug */ s/hotplug/hotplug./ > + cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); > + cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); This creates a race condition, as the hotplug (and hotunplug) could occur after the camera manager is started and before you connect the signals. I think you should connect the signals before starting the camera manager, and remove the loop above that creates CameraDevice instances. CameraHalManager::cameraAdded will add instances as needed. > + > return 0; > } > > @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id, > return camera; > } You also need to update .get_number_of_cameras() to handle hotplug. According to its documentation ([1]), * The value here must be static, and must count only built-in cameras, * which have CAMERA_FACING_BACK or CAMERA_FACING_FRONT camera facing values * (camera_info.facing). The HAL must not include the external cameras * (camera_info.facing == CAMERA_FACING_EXTERNAL) into the return value * of this call. Frameworks will use camera_device_status_change callback * to manage number of external cameras. We need to count the number of internal cameras at initialization time, and cache the value. That's fairly easy, except for the fact that USB cameras could be either internal or external. There's no way we can figure this out without some sort a platform-specific configuration data. In theory some information could be reported through ACPI, but looking at what my laptop reports there for the internal camera, I think most vendors just provide bogus information. Internal USB cameras can be ignored for now I believe, we can just consider that all cameras detected when the HAL is started are external. .get_number_of_cameras() need to be updated accordingly, and we can then work on better handling of internal USB cameras. Or, after having thought a bit more about it, maybe we should check the Location property of the Camera instead. That should be an authoritative source of information. The UVC pipeline handler would need to be fixed to report the Location (it doesn't at the moment), and we can hardcode it to CameraLocationExternal there for now until we get support for platform-specific configuration data. What do you think ? Also, the id >= numCameras() in open() and getCameraInfo() needs to be replaced, with checks that lookup the id in the cameras_ map. > +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > +{ > + unsigned int id = cameras_.size(); I think we need something a bit more advanced to create camera IDs. The constraints on the camera ID are not well documented. [2] states that "Non-removable cameras use integers starting at 0 for their identifiers, while removable cameras have a unique identifier for each individual device, even if they are the same model." Note that this is the Java API, so the camera service may perform ID mapping, although that would surprise me. As the .camera_device_status_change() callback uses an int camera_id, we need integers for external cameras too, unless there's something I'm missing. IDs of different cameras need to be unique, so I think we shouldn't reuse the same ID if we unplug a USB camera and plug another one. One way to handle this is to keep a may of Camera::id() (Niklas has just merged the patch series that adds this) to HAL integer IDs, for all cameras we see. If a new camera is added and was seen before, we can reuse the same HAL ID. Otherwise we assign a new ID (we thus also need a counter of used HAL IDs). Furthermore, as cameraAdded() would be called for all cameras when we start the camera manager (assuming you agree with my suggestion to do so above), we won't know here, when we encounter an external device, how many internal devices there will be. We thus can't assign the HAL ID for external devices right away, unless we set a large enough base number to make sure it will always be above the number of internal devices. Even if it's a bit of a hack, I'd go in that direction, starting at 1000 for instance. > + CameraDevice *camera = new CameraDevice(id, cam); > + int ret = camera->initialize(); > + if (ret) { > + LOG(HAL, Error) << "Failed to initialize camera: " << cam->name(); > + return; > + } > + > + cameras_.emplace_back(camera); > + callbacks_->camera_device_status_change(callbacks_, id, > + CAMERA_DEVICE_STATUS_PRESENT); You first need to check if the callbacks_ are not null, as a camera could be hotplugged before callbacks are set. Access to callbacks_ will need to be protected with a mutex. > + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id > + << " added and initialized successfully."; > +} > + > +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) > +{ > + auto iter = std::find_if(cameras_.begin(), cameras_.end(), > + [cam](std::unique_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); This will delete the CameraDevice instance, which is "slightly" problematic if the camera is already open. It also creates race conditions with the CameraHalManager::open() function. I wonder if we need to turn cameras_ in a vector of shared pointers to handle this. I had a look at how the Chrome OS USB HAL implements unplugging when the camera is already opened ([3]): // TODO(shik): Handle this more gracefully, sometimes it even trigger a kernel // panic. if (cameras_.find(id) != cameras_.end()) { LOGF(WARNING) << "Unplug an opening camera, exit the camera service to cleanup"; // Upstart will start the service again. _exit(EIO); } I'm not sure what to say :-) Should we try to refcount CameraDevice with std::shared_ptr<>, and protect access to camera_ with a mutex ? > + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id > + << " removed successfully."; > +} [1] https://android.googlesource.com/platform/hardware/libhardware/+/refs/heads/master/include/hardware/camera_common.h#881 [2] https://developer.android.com/reference/android/hardware/camera2/CameraManager#getCameraIdList() [3] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/camera/hal/usb/camera_hal.cc#498 > + > unsigned int CameraHalManager::numCameras() const > { > return cameraManager_->cameras().size(); > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > index 4345b1e..52ab9e2 100644 > --- a/src/android/camera_hal_manager.h > +++ b/src/android/camera_hal_manager.h > @@ -33,6 +33,9 @@ public: > int 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_;
Hi Umang, Thanks for your work. On 2020-08-05 15:14:44 +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(). > > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/android/camera_device.h | 1 + > src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++-- > src/android/camera_hal_manager.h | 3 +++ > 3 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 4e5fb98..fa9706c 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -47,6 +47,7 @@ public: > > unsigned int id() const { return id_; } > camera3_device_t *camera3Device() { return &camera3Device_; } > + const libcamera::Camera *getCamera() { return camera_.get(); }; > > int facing() const { return facing_; } > int orientation() const { return orientation_; } > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index 3ddcab5..b498278 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -59,8 +59,6 @@ int CameraHalManager::init() > /* > * 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()) { > @@ -73,6 +71,10 @@ int CameraHalManager::init() > ++index; > } > > + /* Support camera hotplug */ > + cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); > + cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); > + > return 0; > } > > @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id, > return camera; > } > > +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > +{ > + unsigned int id = cameras_.size(); This is dangerous. If you have two USB cameras A and B and plug them in and out as following you will have two cameras with the same numerical id: Number of internal cameras: 2 -> id = 0 and id = 1 Plugin A -> id = 2 Plugin B -> id = 3 Remove A Plugin A -> id = 3 I'm wondering if an ever increasing counter would be a solution here? It's not likely we will see 2^31 (2147483648) devices plugged in over the runtime of the system. And if we fear that we can test that the next ID in line is indeed free and if not keep incrementing. > + CameraDevice *camera = new CameraDevice(id, cam); > + int ret = camera->initialize(); > + if (ret) { > + LOG(HAL, Error) << "Failed to initialize camera: " << cam->name(); > + return; > + } > + > + cameras_.emplace_back(camera); > + callbacks_->camera_device_status_change(callbacks_, id, > + CAMERA_DEVICE_STATUS_PRESENT); > + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id > + << " added and initialized successfully."; > +} > + > +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) > +{ > + auto iter = std::find_if(cameras_.begin(), cameras_.end(), > + [cam](std::unique_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 " << cam->name() << " ID:" << id > + << " removed successfully."; > +} > + > unsigned int CameraHalManager::numCameras() const > { > return cameraManager_->cameras().size(); > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > index 4345b1e..52ab9e2 100644 > --- a/src/android/camera_hal_manager.h > +++ b/src/android/camera_hal_manager.h > @@ -33,6 +33,9 @@ public: > int 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_; > -- > 2.26.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for the detailed review. I need a few clarifications. On 8/6/20 3:04 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Wed, Aug 05, 2020 at 03:14:44PM +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(). >> >> Signed-off-by: Umang Jain <email@uajain.com> >> --- >> src/android/camera_device.h | 1 + >> src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++-- >> src/android/camera_hal_manager.h | 3 +++ >> 3 files changed, 42 insertions(+), 2 deletions(-) >> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h >> index 4e5fb98..fa9706c 100644 >> --- a/src/android/camera_device.h >> +++ b/src/android/camera_device.h >> @@ -47,6 +47,7 @@ public: >> >> unsigned int id() const { return id_; } >> camera3_device_t *camera3Device() { return &camera3Device_; } >> + const libcamera::Camera *getCamera() { return camera_.get(); }; >> >> int facing() const { return facing_; } >> int orientation() const { return orientation_; } >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp >> index 3ddcab5..b498278 100644 >> --- a/src/android/camera_hal_manager.cpp >> +++ b/src/android/camera_hal_manager.cpp >> @@ -59,8 +59,6 @@ int CameraHalManager::init() >> /* >> * 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()) { >> @@ -73,6 +71,10 @@ int CameraHalManager::init() >> ++index; >> } >> >> + /* Support camera hotplug */ > s/hotplug/hotplug./ > >> + cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); >> + cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); > This creates a race condition, as the hotplug (and hotunplug) could > occur after the camera manager is started and before you connect the > signals. I think you should connect the signals before starting the > camera manager, and remove the loop above that creates CameraDevice > instances. CameraHalManager::cameraAdded will add instances as needed. I can agree with this suggestion, I don't see it being problematic. > >> + >> return 0; >> } >> >> @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id, >> return camera; >> } > You also need to update .get_number_of_cameras() to handle hotplug. > According to its documentation ([1]), > > * The value here must be static, and must count only built-in cameras, > * which have CAMERA_FACING_BACK or CAMERA_FACING_FRONT camera facing values > * (camera_info.facing). The HAL must not include the external cameras > * (camera_info.facing == CAMERA_FACING_EXTERNAL) into the return value > * of this call. Frameworks will use camera_device_status_change callback > * to manage number of external cameras. > > We need to count the number of internal cameras at initialization time, > and cache the value. That's fairly easy, except for the fact that USB > cameras could be either internal or external. There's no way we can > figure this out without some sort a platform-specific configuration > data. In theory some information could be reported through ACPI, but > looking at what my laptop reports there for the internal camera, I think > most vendors just provide bogus information. > > Internal USB cameras can be ignored for now I believe, we can just > consider that all cameras detected when the HAL is started are external. Do you mean "all cameras detected" or "all UVC cameras detected" as external? Because the next line of explanation confuses me a bit. > > .get_number_of_cameras() need to be updated accordingly, and we can then > work on better handling of internal USB cameras. I think you meant "all UVC cameras" just above, no? > > Or, after having thought a bit more about it, maybe we should check the > Location property of the Camera instead. That should be an authoritative > source of information. The UVC pipeline handler would need to be fixed > to report the Location (it doesn't at the moment), and we can hardcode > it to CameraLocationExternal there for now until we get support for > platform-specific configuration data. What do you think ? hmm, Is this Location property can be queried from any existing metadata or, do you mean that we should declare it inside pipeline-handler/camera. The way I am understanding things here is, you are suggesting that each pipeline handler sets the CameraLocationProperty for the camera being created. For e.g. if IPU3 is used, that particular camera will be created with CameraLocationInternal whereas UVC pipelinehandler will create a camera with CameraLocationExternal; which then can be queried by a convenient Camera API. Is this what are you referring here? > > Also, the id >= numCameras() in open() and getCameraInfo() needs to be > replaced, with checks that lookup the id in the cameras_ map. > >> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) >> +{ >> + unsigned int id = cameras_.size(); > I think we need something a bit more advanced to create camera IDs. > The constraints on the camera ID are not well documented. [2] states > that > > "Non-removable cameras use integers starting at 0 for their identifiers, > while removable cameras have a unique identifier for each individual > device, even if they are the same model." > > Note that this is the Java API, so the camera service may perform ID > mapping, although that would surprise me. > > As the .camera_device_status_change() callback uses an int camera_id, we > need integers for external cameras too, unless there's something I'm > missing. IDs of different cameras need to be unique, so I think we > shouldn't reuse the same ID if we unplug a USB camera and plug another > one. > > One way to handle this is to keep a may of Camera::id() (Niklas has just > merged the patch series that adds this) to HAL integer IDs, for all > cameras we see. If a new camera is added and was seen before, we can > reuse the same HAL ID. Otherwise we assign a new ID (we thus also need a > counter of used HAL IDs). > > Furthermore, as cameraAdded() would be called for all cameras when we > start the camera manager (assuming you agree with my suggestion to do so > above), we won't know here, when we encounter an external device, how > many internal devices there will be. We thus can't assign the HAL ID for > external devices right away, unless we set a large enough base number to > make sure it will always be above the number of internal devices. Even > if it's a bit of a hack, I'd go in that direction, starting at 1000 for > instance. > >> + CameraDevice *camera = new CameraDevice(id, cam); >> + int ret = camera->initialize(); >> + if (ret) { >> + LOG(HAL, Error) << "Failed to initialize camera: " << cam->name(); >> + return; >> + } >> + >> + cameras_.emplace_back(camera); >> + callbacks_->camera_device_status_change(callbacks_, id, >> + CAMERA_DEVICE_STATUS_PRESENT); > You first need to check if the callbacks_ are not null, as a camera > could be hotplugged before callbacks are set. Access to callbacks_ will > need to be protected with a mutex. > >> + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id >> + << " added and initialized successfully."; >> +} >> + >> +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) >> +{ >> + auto iter = std::find_if(cameras_.begin(), cameras_.end(), >> + [cam](std::unique_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); > This will delete the CameraDevice instance, which is "slightly" > problematic if the camera is already open. It also creates race > conditions with the CameraHalManager::open() function. I wonder if we > need to turn cameras_ in a vector of shared pointers to handle this. > > I had a look at how the Chrome OS USB HAL implements unplugging when the > camera is already opened ([3]): > > // TODO(shik): Handle this more gracefully, sometimes it even trigger a kernel > // panic. > if (cameras_.find(id) != cameras_.end()) { > LOGF(WARNING) > << "Unplug an opening camera, exit the camera service to cleanup"; > // Upstart will start the service again. > _exit(EIO); > } > > I'm not sure what to say :-) > > Should we try to refcount CameraDevice with std::shared_ptr<>, and > protect access to camera_ with a mutex ? Can you explain a bit, why would we need to protect access to camera_ with a mutex? > >> + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id >> + << " removed successfully."; >> +} > [1] https://android.googlesource.com/platform/hardware/libhardware/+/refs/heads/master/include/hardware/camera_common.h#881 > [2] https://developer.android.com/reference/android/hardware/camera2/CameraManager#getCameraIdList() > [3] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/camera/hal/usb/camera_hal.cc#498 > >> + >> unsigned int CameraHalManager::numCameras() const >> { >> return cameraManager_->cameras().size(); >> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h >> index 4345b1e..52ab9e2 100644 >> --- a/src/android/camera_hal_manager.h >> +++ b/src/android/camera_hal_manager.h >> @@ -33,6 +33,9 @@ public: >> int 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_;
Hi Umang, On Thu, Aug 06, 2020 at 08:26:45AM +0000, Umang Jain wrote: > Hi Laurent, > > Thanks for the detailed review. > I need a few clarifications. > > On 8/6/20 3:04 AM, Laurent Pinchart wrote: > > On Wed, Aug 05, 2020 at 03:14:44PM +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(). > >> > >> Signed-off-by: Umang Jain <email@uajain.com> > >> --- > >> src/android/camera_device.h | 1 + > >> src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++-- > >> src/android/camera_hal_manager.h | 3 +++ > >> 3 files changed, 42 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > >> index 4e5fb98..fa9706c 100644 > >> --- a/src/android/camera_device.h > >> +++ b/src/android/camera_device.h > >> @@ -47,6 +47,7 @@ public: > >> > >> unsigned int id() const { return id_; } > >> camera3_device_t *camera3Device() { return &camera3Device_; } > >> + const libcamera::Camera *getCamera() { return camera_.get(); }; > >> > >> int facing() const { return facing_; } > >> int orientation() const { return orientation_; } > >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > >> index 3ddcab5..b498278 100644 > >> --- a/src/android/camera_hal_manager.cpp > >> +++ b/src/android/camera_hal_manager.cpp > >> @@ -59,8 +59,6 @@ int CameraHalManager::init() > >> /* > >> * 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()) { > >> @@ -73,6 +71,10 @@ int CameraHalManager::init() > >> ++index; > >> } > >> > >> + /* Support camera hotplug */ > > > > s/hotplug/hotplug./ > > > >> + cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); > >> + cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); > > > > This creates a race condition, as the hotplug (and hotunplug) could > > occur after the camera manager is started and before you connect the > > signals. I think you should connect the signals before starting the > > camera manager, and remove the loop above that creates CameraDevice > > instances. CameraHalManager::cameraAdded will add instances as needed. > > I can agree with this suggestion, I don't see it being problematic. > > >> + > >> return 0; > >> } > >> > >> @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id, > >> return camera; > >> } > > > > You also need to update .get_number_of_cameras() to handle hotplug. > > According to its documentation ([1]), > > > > * The value here must be static, and must count only built-in cameras, > > * which have CAMERA_FACING_BACK or CAMERA_FACING_FRONT camera facing values > > * (camera_info.facing). The HAL must not include the external cameras > > * (camera_info.facing == CAMERA_FACING_EXTERNAL) into the return value > > * of this call. Frameworks will use camera_device_status_change callback > > * to manage number of external cameras. > > > > We need to count the number of internal cameras at initialization time, > > and cache the value. That's fairly easy, except for the fact that USB > > cameras could be either internal or external. There's no way we can > > figure this out without some sort a platform-specific configuration > > data. In theory some information could be reported through ACPI, but > > looking at what my laptop reports there for the internal camera, I think > > most vendors just provide bogus information. > > > > Internal USB cameras can be ignored for now I believe, we can just > > consider that all cameras detected when the HAL is started are external. > > Do you mean "all cameras detected" or "all UVC cameras detected" as > external? > Because the next line of explanation confuses me a bit. Re-reading my text, I'm not sure what I meant :-) I may have mixed "all UVC cameras are external" and "all cameras detected when the HAL is started are internal". I'd go for the latter, as we have no way to know here if a camera is UVC or not. And doesn't matter much though, if we use the Location properly this concern will go away. > > .get_number_of_cameras() need to be updated accordingly, and we can then > > work on better handling of internal USB cameras. > > I think you meant "all UVC cameras" just above, no? > > > Or, after having thought a bit more about it, maybe we should check the > > Location property of the Camera instead. That should be an authoritative > > source of information. The UVC pipeline handler would need to be fixed > > to report the Location (it doesn't at the moment), and we can hardcode > > it to CameraLocationExternal there for now until we get support for > > platform-specific configuration data. What do you think ? > > hmm, Is this Location property can be queried from any existing metadata or, > do you mean that we should declare it inside pipeline-handler/camera. The Location property is reported through Camera::properties(). Pipeline handlers need to set it. > The way I am understanding things here is, you are suggesting that each pipeline > handler sets the CameraLocationProperty for the camera being created. For e.g. > if IPU3 is used, that particular camera will be created with CameraLocationInternal > whereas UVC pipelinehandler will create a camera with CameraLocationExternal; The supported values are CameraLocationFront, CameraLocationBack and CameraLocationExternal (see property_ids.yaml). The property is currently created by the CameraSensor class. The UVC pipeline handler doesn't use CameraSensor and thus needs to add the property manually. > which then can be queried by a convenient Camera API. Is this what are > you referring here? Correct. > > Also, the id >= numCameras() in open() and getCameraInfo() needs to be > > replaced, with checks that lookup the id in the cameras_ map. > > > >> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) > >> +{ > >> + unsigned int id = cameras_.size(); > > > > I think we need something a bit more advanced to create camera IDs. > > The constraints on the camera ID are not well documented. [2] states > > that > > > > "Non-removable cameras use integers starting at 0 for their identifiers, > > while removable cameras have a unique identifier for each individual > > device, even if they are the same model." > > > > Note that this is the Java API, so the camera service may perform ID > > mapping, although that would surprise me. > > > > As the .camera_device_status_change() callback uses an int camera_id, we > > need integers for external cameras too, unless there's something I'm > > missing. IDs of different cameras need to be unique, so I think we > > shouldn't reuse the same ID if we unplug a USB camera and plug another > > one. > > > > One way to handle this is to keep a may of Camera::id() (Niklas has just > > merged the patch series that adds this) to HAL integer IDs, for all > > cameras we see. If a new camera is added and was seen before, we can > > reuse the same HAL ID. Otherwise we assign a new ID (we thus also need a > > counter of used HAL IDs). > > > > Furthermore, as cameraAdded() would be called for all cameras when we > > start the camera manager (assuming you agree with my suggestion to do so > > above), we won't know here, when we encounter an external device, how > > many internal devices there will be. We thus can't assign the HAL ID for > > external devices right away, unless we set a large enough base number to > > make sure it will always be above the number of internal devices. Even > > if it's a bit of a hack, I'd go in that direction, starting at 1000 for > > instance. > > > >> + CameraDevice *camera = new CameraDevice(id, cam); > >> + int ret = camera->initialize(); > >> + if (ret) { > >> + LOG(HAL, Error) << "Failed to initialize camera: " << cam->name(); > >> + return; > >> + } > >> + > >> + cameras_.emplace_back(camera); > >> + callbacks_->camera_device_status_change(callbacks_, id, > >> + CAMERA_DEVICE_STATUS_PRESENT); > > > > You first need to check if the callbacks_ are not null, as a camera > > could be hotplugged before callbacks are set. Access to callbacks_ will > > need to be protected with a mutex. > > > >> + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id > >> + << " added and initialized successfully."; > >> +} > >> + > >> +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) > >> +{ > >> + auto iter = std::find_if(cameras_.begin(), cameras_.end(), > >> + [cam](std::unique_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); > > > > This will delete the CameraDevice instance, which is "slightly" > > problematic if the camera is already open. It also creates race > > conditions with the CameraHalManager::open() function. I wonder if we > > need to turn cameras_ in a vector of shared pointers to handle this. > > > > I had a look at how the Chrome OS USB HAL implements unplugging when the > > camera is already opened ([3]): > > > > // TODO(shik): Handle this more gracefully, sometimes it even trigger a kernel > > // panic. > > if (cameras_.find(id) != cameras_.end()) { > > LOGF(WARNING) > > << "Unplug an opening camera, exit the camera service to cleanup"; > > // Upstart will start the service again. > > _exit(EIO); > > } > > > > I'm not sure what to say :-) > > > > Should we try to refcount CameraDevice with std::shared_ptr<>, and > > protect access to camera_ with a mutex ? > > Can you explain a bit, why would we need to protect access to camera_ > with a mutex? I meant cameras_, not camera_, sorry. cameras_ can be accessed from multiple threads (all the HAL operations run in an external thread, and the cameraAdded and cameraRemoved signals are emitted from the internal CameraManager thread). It thus needs to be protected against race conditions. > >> + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id > >> + << " removed successfully."; > >> +} > > > > [1] https://android.googlesource.com/platform/hardware/libhardware/+/refs/heads/master/include/hardware/camera_common.h#881 > > [2] https://developer.android.com/reference/android/hardware/camera2/CameraManager#getCameraIdList() > > [3] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/camera/hal/usb/camera_hal.cc#498 > > > >> + > >> unsigned int CameraHalManager::numCameras() const > >> { > >> return cameraManager_->cameras().size(); > >> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h > >> index 4345b1e..52ab9e2 100644 > >> --- a/src/android/camera_hal_manager.h > >> +++ b/src/android/camera_hal_manager.h > >> @@ -33,6 +33,9 @@ public: > >> int 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_;
diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 4e5fb98..fa9706c 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -47,6 +47,7 @@ public: unsigned int id() const { return id_; } camera3_device_t *camera3Device() { return &camera3Device_; } + const libcamera::Camera *getCamera() { return camera_.get(); }; int facing() const { return facing_; } int orientation() const { return orientation_; } diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index 3ddcab5..b498278 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -59,8 +59,6 @@ int CameraHalManager::init() /* * 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()) { @@ -73,6 +71,10 @@ int CameraHalManager::init() ++index; } + /* Support camera hotplug */ + cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded); + cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved); + return 0; } @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id, return camera; } +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam) +{ + unsigned int id = cameras_.size(); + CameraDevice *camera = new CameraDevice(id, cam); + int ret = camera->initialize(); + if (ret) { + LOG(HAL, Error) << "Failed to initialize camera: " << cam->name(); + return; + } + + cameras_.emplace_back(camera); + callbacks_->camera_device_status_change(callbacks_, id, + CAMERA_DEVICE_STATUS_PRESENT); + LOG(HAL, Debug) << "Camera " << cam->name() << " ID:" << id + << " added and initialized successfully."; +} + +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) +{ + auto iter = std::find_if(cameras_.begin(), cameras_.end(), + [cam](std::unique_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 " << cam->name() << " ID:" << id + << " removed successfully."; +} + unsigned int CameraHalManager::numCameras() const { return cameraManager_->cameras().size(); diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h index 4345b1e..52ab9e2 100644 --- a/src/android/camera_hal_manager.h +++ b/src/android/camera_hal_manager.h @@ -33,6 +33,9 @@ public: int 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_;
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(). Signed-off-by: Umang Jain <email@uajain.com> --- src/android/camera_device.h | 1 + src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++-- src/android/camera_hal_manager.h | 3 +++ 3 files changed, 42 insertions(+), 2 deletions(-)