[libcamera-devel] [PATCH 2/2] android: camera_hal_manager: Support camera hotplug
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 5 23:34:04 CEST 2020
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 at 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_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list