[libcamera-devel] [PATCH 2/2] android: camera_hal_manager: Support camera hotplug

[Niklas Söderlund]

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 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 */
> +	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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund