[libcamera-devel,v2,4/4] android: camera_hal_manager: Handle hot-unplug of currently open camera

9274 diff mbox series

Maintain an extra ref for the currently open camera in CameraHalManager.
This will ensure we have an graceful handling if the currently open
camera is hot-unplugged.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/android/camera_device.h        |  1 +
 src/android/camera_hal_manager.cpp | 18 +++++++++++++++---
 src/android/camera_hal_manager.h   |  2 ++
 3 files changed, 18 insertions(+), 3 deletions(-)

Hi Umang,

On 10/08/2020 13:04, Umang Jain wrote:
> Maintain an extra ref for the currently open camera in CameraHalManager.
> This will ensure we have an graceful handling if the currently open
> camera is hot-unplugged.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/android/camera_device.h        |  1 +
>  src/android/camera_hal_manager.cpp | 18 +++++++++++++++---
>  src/android/camera_hal_manager.h   |  2 ++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 7f9e010..2dc0a20 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -61,6 +61,7 @@ public:
>  
>  	int facing() const { return facing_; }
>  	int orientation() const { return orientation_; }
> +	bool running() { return running_; }
>  
>  	void setCallbacks(const camera3_callback_ops_t *callbacks);
>  	const camera_metadata_t *getStaticMetadata();
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index fdde2c0..9db5954 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -38,6 +38,8 @@ CameraHalManager::~CameraHalManager()
>  	cameras_.clear();
>  	camerasMap_.clear();
>  
> +	cameraInUse_.reset();
> +
>  	if (cameraManager_) {
>  		cameraManager_->stop();
>  		delete cameraManager_;
> @@ -55,6 +57,7 @@ int CameraHalManager::init()
>  
>  	cameraCounter_ = 0;
>  	externalCameraCounter_ = 1000;
> +	cameraInUse_ = nullptr;

Should this be initialised at constructor? or perhaps it already is and
this is a reinitialise?


>  
>  	int ret = cameraManager_->start();
>  	if (ret) {
> @@ -82,14 +85,17 @@ CameraDevice *CameraHalManager::open(unsigned int id,
>  		return nullptr;
>  	}
>  
> -	CameraDevice *camera = iter->get();
> +	if (cameraInUse_)
> +		cameraInUse_.reset();

This looks like it's a leak when opening a second device?
As in - now it has forgotten the first ...

> +
> +	cameraInUse_ = std::move(*iter);
>  
> -	if (camera->open(hardwareModule))
> +	if (cameraInUse_->open(hardwareModule))
>  		return nullptr;
>  
>  	LOG(HAL, Info) << "Open camera '" << id << "'";
>  
> -	return camera;
> +	return cameraInUse_.get();
>  }
>  
>  void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> @@ -169,6 +175,12 @@ void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
>  	unsigned int id = (*iter)->id();
>  	callbacks_->camera_device_status_change(callbacks_, id,
>  						CAMERA_DEVICE_STATUS_NOT_PRESENT);
> +
> +	if (*iter == cameraInUse_ && cameraInUse_->running()) {
> +		cameraInUse_->close();
> +		cameraInUse_.reset();
> +	}
> +
>  	cameras_.erase(iter);
>  
>  	LOG(HAL, Debug) << "Camera ID: " << id << " removed successfully.";
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index 7c481d4..974d53d 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -48,6 +48,8 @@ private:
>  	std::map<std::string, unsigned int> camerasMap_;
>  	Mutex mutex_;
>  
> +	std::shared_ptr<CameraDevice> cameraInUse_;
> +

cameraInUse sounds a bit awkward to me, as I would probably name it
activeCamera, but I'm really weary of this as it stands as I think I can
infer that it means there is a reference of the camera in the camera
manager, but only for a single camera... what happens if there are more
than one?!

Maybe open should add a shared ptr to a vector (activeCameras_?), and
close should remove it ? Would that fix the issue you are trying to resolve?

Or maybe there are other ways ... not sure yet.


>  	unsigned int externalCameraCounter_;
>  	unsigned int cameraCounter_;
>  };
>