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

[Kieran Bingham]

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 at 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_;
>  };
> 

-- 
Regards
--
Kieran