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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Mon, Aug 17, 2020 at 08:26:40PM +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().

s/being being/being/

> 
> Introduce a map camerasMap_ which book-keeps all cameras seen in the
> past by the CameraHalManager. If the camera is seen for the first time,
> a new id is assigned to it. If the camera has been seen before by the
> manager, it's old id is reused. IDs for internal cameras start with

s/it's/its/

> '0' and for external cameras, they start with '1000'. Note, for the
> current implementation, we assume all UVC cameras are external cameras.

I'd drop this sentence, as nothing here is UVC-specific, it's an issue
to be solved in the UVC pipeline handler.

> Also, acess to camerasMap_ and cameras_ are protected by a mutex.

s/acess/accesses/

> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/camera_hal_manager.cpp | 174 +++++++++++++++++++++++++----
>  src/android/camera_hal_manager.h   |  18 +++
>  2 files changed, 170 insertions(+), 22 deletions(-)
> 
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 3a744af..e851185 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -8,6 +8,7 @@
>  #include "camera_hal_manager.h"
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/property_ids.h>
>  
>  #include "libcamera/internal/log.h"
>  
> @@ -35,6 +36,7 @@ CameraHalManager::CameraHalManager()
>  CameraHalManager::~CameraHalManager()
>  {
>  	cameras_.clear();
> +	camerasMap_.clear();

You don't need this, the CameraHalManager is being deleted, so the
camerasMap_ is deleted too. The reason we need it for cameras_ is to
release the Camera shared pointers before calling CameraManager::stop().

>  
>  	if (cameraManager_) {
>  		cameraManager_->stop();
> @@ -47,6 +49,13 @@ int CameraHalManager::init()
>  {
>  	cameraManager_ = new CameraManager();
>  
> +	/* Support camera hotplug. */
> +	cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);
> +	cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);
> +
> +	internalCameras_ = 0;
> +	externalCameras_ = 1000;

Can you initialize those two fields in the constructor (using the member
initializer list) ?

> +
>  	int ret = cameraManager_->start();
>  	if (ret) {
>  		LOG(HAL, Error) << "Failed to start camera manager: "
> @@ -56,35 +65,22 @@ int CameraHalManager::init()
>  		return ret;
>  	}
>  
> -	/*
> -	 * 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()) {
> -		std::shared_ptr<CameraDevice> camera = CameraDevice::create(index, cam);
> -		ret = camera->initialize();
> -		if (ret)
> -			continue;
> -
> -		cameras_.emplace_back(std::move(camera));
> -		++index;
> -	}
> -
>  	return 0;
>  }
>  
>  CameraDevice *CameraHalManager::open(unsigned int id,
>  				     const hw_module_t *hardwareModule)
>  {
> -	if (id >= numCameras()) {
> +	MutexLocker locker(mutex_);
> +
> +	auto iter = cameraIterFromId(id);
> +	if (iter == cameras_.end()) {
>  		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
>  		return nullptr;
>  	}
>  
> -	CameraDevice *camera = cameras_[id].get();
> +	CameraDevice *camera = iter->get();
> +
>  	if (camera->open(hardwareModule))
>  		return nullptr;
>  
> @@ -93,9 +89,114 @@ CameraDevice *CameraHalManager::open(unsigned int id,
>  	return camera;
>  }
>  
> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> +{
> +	unsigned int id;
> +	bool isCameraExternal = false;
> +	bool isCameraNew = false;
> +
> +	MutexLocker locker(mutex_);
> +
> +	/*
> +	 * Each camera is assigned a unique integer ID when it is seen for the
> +	 * first time. If the camera has been seen before, the previous ID is
> +	 * re-used and the camera is marked as CAMERA_DEVICE_STATUS_PRESENT
> +	 * subsequently.

CAMERA_DEVICE_STATUS_PRESENT is unrelated to whether the camera is new
or has been seen before. I'd drop that part of the sentence.

> +	 *
> +	 * IDs starts from '0' for internal cameras and '1000' for external
> +	 * cameras.
> +	 */
> +	auto iter = camerasMap_.find(cam->id());
> +	if (iter != camerasMap_.end()) {
> +		id = iter->second;
> +	} else {
> +		isCameraNew = true;
> +
> +		/*
> +		 *  Now check if this is an external camera and assign
> +		 *  its id accordingly.

s/*  /* /

> +		 */
> +		const ControlList &properties = cam->properties();
> +		if (properties.contains(properties::Location) &&
> +		    properties.get(properties::Location) ==
> +		    properties::CameraLocationExternal) {
> +			isCameraExternal = true;
> +			id = externalCameras_;
> +		} else {
> +			id = internalCameras_;
> +		}
> +	}
> +
> +	/*
> +	 * For each Camera registered in the system, a CameraDevice
> +	 * gets created here to wraps a libcamera Camera instance.

s/wraps/wrap/

And this isn't about creating a CameraDevice instance for each camera
anymore. I'd write this

	/* Create a CameraDevice instance to wrap the libcamera Camera. */

> +	 */
> +	std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> +	int ret = camera->initialize();
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
> +		return;
> +	}
> +
> +	if (isCameraNew) {
> +		camerasMap_.emplace(cam->id(), id);
> +
> +		if (isCameraExternal)
> +			externalCameras_++;
> +		else
> +			internalCameras_++;
> +	}
> +
> +	cameras_.emplace_back(std::move(camera));
> +
> +	if (callbacks_)
> +		callbacks_->camera_device_status_change(callbacks_, id,
> +							CAMERA_DEVICE_STATUS_PRESENT);
> +
> +	LOG(HAL, Debug) << "Camera ID: " << id << " added successfully.";
> +}
> +
> +void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
> +{
> +	MutexLocker locker(mutex_);
> +
> +	auto iter = cameraIterFromCamera(cam.get());
> +	if (iter == cameras_.end())
> +		return;
> +
> +	unsigned int id = (*iter)->id();
> +	callbacks_->camera_device_status_change(callbacks_, id,
> +						CAMERA_DEVICE_STATUS_NOT_PRESENT);

This should only be called for external cameras. cameraRemoved()
shouldn't be called for internal cameras in the first place, so maybe
there's no need for a conditional here.

> +
> +	/*
> +	 * \todo Check if the camera is already open and running.
> +	 * Inform the framework about it's absence before deleting it's

s/it's/its/g

> +	 * reference here.

Could this be as simple as creating a

	std::list<std::shared_ptr<CameraDevice>> openCameras_;

to hold references to open cameras ? It could be done on top.

> +	 */
> +	cameras_.erase(iter);
> +
> +	LOG(HAL, Debug) << "Camera ID: " << id << " removed successfully.";
> +}
> +
> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromId(unsigned int id)
> +{
> +	return std::find_if(cameras_.begin(), cameras_.end(),
> +			    [id](std::shared_ptr<CameraDevice> &camera) {
> +				return camera->id() == id;
> +			    });
> +}
> +
> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromCamera(Camera *cam)
> +{
> +	return std::find_if(cameras_.begin(), cameras_.end(),
> +			    [cam](std::shared_ptr<CameraDevice> &camera) {
> +				return cam == camera->camera();
> +			    });
> +}
> +
>  unsigned int CameraHalManager::numCameras() const
>  {
> -	return cameraManager_->cameras().size();
> +	return internalCameras_;
>  }
>  
>  int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
> @@ -103,12 +204,15 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
>  	if (!info)
>  		return -EINVAL;
>  
> -	if (id >= numCameras()) {
> +	MutexLocker locker(mutex_);
> +
> +	auto iter = cameraIterFromId(id);
> +	if (iter == cameras_.end()) {
>  		LOG(HAL, Error) << "Invalid camera id '" << id << "'";
>  		return -EINVAL;
>  	}
>  
> -	CameraDevice *camera = cameras_[id].get();
> +	CameraDevice *camera = iter->get();
>  
>  	info->facing = camera->facing();
>  	info->orientation = camera->orientation();
> @@ -124,4 +228,30 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
>  void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
>  {
>  	callbacks_ = callbacks;
> +
> +	MutexLocker locker(mutex_);
> +
> +	/*
> +	 * Some external cameras may have been identified before the
> +	 * callbacks_ were enabled. Iterate any existing external

s/enabled/set/ ?
s/any/all/

> +	 * cameras and mark them as CAMERA_DEVICE_STATUS_PRESENT
> +	 * explicitly.
> +	 *
> +	 * Internal cameras are already assumed to be present at module
> +	 * load time by the android framework.
> +	 */
> +	for (auto &cam : cameraManager_->cameras()) {
> +		auto iter = camerasMap_.find(cam->id());
> +		if (iter == camerasMap_.end())
> +			continue;

How about iterating over cameras_ instead ? You wouldn't need to look up
the id in camerasMap_.

	for (std::shared_ptr<CameraDevice> &camera : cameras_) {
		Camera *cam = camera->camera();
		unsigned int id = camera->id();

(you could also drop some of the local variables if desired, as they are
both used once only).

> +
> +		unsigned int id = iter->second;
> +		const ControlList &properties = cam->properties();
> +		if (properties.contains(properties::Location) &&
> +		    properties.get(properties::Location) &

The Location property isn't a bitfield, it's an enum.

> +		    properties::CameraLocationExternal) {

There are two locations where you check the Location property. Would
adding the following function make sense ?

static int32_t CameraHalManager::cameraLocation(Camera *cam)
{
	const ControlList &properties = cam->properties();
	if (!properties.contains(properties::Location))
		return -1;

	return properties.get(properties::Location);
}

The caller would become

		if (cameraLocation(cam) == properties::CameraLocationExternal)

which is a bit nicer to read I think.

> +			callbacks_->camera_device_status_change(callbacks_, id,
> +								CAMERA_DEVICE_STATUS_PRESENT);
> +		}
> +	}
>  }
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index 3e34d63..f8adade 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -7,6 +7,8 @@
>  #ifndef __ANDROID_CAMERA_MANAGER_H__
>  #define __ANDROID_CAMERA_MANAGER_H__
>  
> +#include <map>
> +#include <mutex>
>  #include <stddef.h>
>  #include <vector>
>  
> @@ -18,12 +20,17 @@
>  
>  class CameraDevice;
>  
> +using Mutex = std::mutex;
> +using MutexLocker = std::unique_lock<std::mutex>;
> +
>  class CameraHalManager
>  {
>  public:
>  	CameraHalManager();
>  	~CameraHalManager();
>  
> +	using cameraDeviceIter = std::vector<std::shared_ptr<CameraDevice>>::iterator;
> +
>  	int init();
>  
>  	CameraDevice *open(unsigned int id, const hw_module_t *module);
> @@ -33,10 +40,21 @@ public:
>  	void setCallbacks(const camera_module_callbacks_t *callbacks);
>  
>  private:
> +	void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
> +	void cameraRemoved(std::shared_ptr<libcamera::Camera> cam);
> +
> +	cameraDeviceIter cameraIterFromId(unsigned int id);
> +	cameraDeviceIter cameraIterFromCamera(libcamera::Camera *cam);

How about turning these to

	CameraDevice *cameraFromId(unsigned int id);
	CameraDevice *cameraFromCamera(libcamera::Camera *cam);

It would simplify the interface. I would actually call the former
cameraFromHALId() to differentiate the two types of IDs. And maybe

	CameraDevice *cameraDeviceFromHALId(unsigned int id);
	CameraDevice *cameraDeviecFromCamera(libcamera::Camera *cam);

if it's not too long ?

> +
>  	libcamera::CameraManager *cameraManager_;
>  
>  	const camera_module_callbacks_t *callbacks_;
>  	std::vector<std::shared_ptr<CameraDevice>> cameras_;
> +	std::map<std::string, unsigned int> camerasMap_;

Maybe cameraIdsMap_ to make the contents more explicit ?

> +	Mutex mutex_;
> +
> +	unsigned int internalCameras_;

Maybe numInternalCameras_ or internalCamerasCount_ ?

> +	unsigned int externalCameras_;

And nextExternalCameraId_ ?

>  };
>  
>  #endif /* __ANDROID_CAMERA_MANAGER_H__ */

-- 
Regards,

Laurent Pinchart