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

[Umang Jain]

Hi Laurent,

On 8/19/20 9:03 PM, Laurent Pinchart wrote:
> 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.
cameraRemoved() shouldn't be called for internal cameras?
Well, I wonder what exacts means by "unplugging an internal camera",
but I think *if* somehow, internal cameras are unplugged, we do need to
drop the reference from cameras_, no?
>
>> +
>> +	/*
>> +	 * \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);
> }
Do you think it should be a 'static' method? I am curious why?
Any special reasons?
>
> 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__ */