[{"id":12091,"web_url":"https://patchwork.libcamera.org/comment/12091/","msgid":"<20200822212314.GA25161@pendragon.ideasonboard.com>","date":"2020-08-22T21:23:14","subject":"Re: [libcamera-devel] [PATCH v4 5/5] android: camera_hal_manager:\n\tSupport camera hotplug","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Aug 21, 2020 at 02:46:11PM +0000, Umang Jain wrote:\n> Extend the support for camera hotplug from libcamera's CameraManager\n> to CameraHalManager. Use camera module callbacks to let the framework\n> know about the hotplug events and change the status of cameras being\n> hotplugged or unplugged via camera_device_status_change().\n> \n> Introduce a map camerasMap_ which book-keeps all cameras seen in the\n\ns/camerasMap_/cameraIdsMap_/\n\n> past by the CameraHalManager. If the camera is seen for the first time,\n> a new id is assigned to it. If the camera has been seen before by the\n> manager, its old id is reused. IDs for internal cameras start with\n> '0' and for external cameras, they start with '1000'. Accesses to\n> camerasMap_ and cameras_ are protected by a mutex.\n\ns/camerasMap_/cameraIdsMap_/\n\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_hal_manager.cpp | 171 +++++++++++++++++++++++++----\n> src/android/camera_hal_manager.h | 20 ++++\n> 2 files changed, 168 insertions(+), 23 deletions(-)\n> \n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 3a744af..4b32c4f 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -8,6 +8,7 @@\n> #include \"camera_hal_manager.h\"\n> \n> #include \n> +#include \n> \n> #include \"libcamera/internal/log.h\"\n> \n> @@ -28,7 +29,7 @@ LOG_DECLARE_CATEGORY(HAL);\n> */\n> \n> CameraHalManager::CameraHalManager()\n> -\t: cameraManager_(nullptr)\n> +\t: cameraManager_(nullptr), numInternalCameras_(0), nextExternalCameraId_(1000)\n\n1000 is used a few times below. A static constexpr member variable would\nbe nice to avoid repeating the constant.\n\n> {\n> }\n> \n> @@ -47,6 +48,10 @@ int CameraHalManager::init()\n> {\n> \tcameraManager_ = new CameraManager();\n> \n> +\t/* Support camera hotplug. */\n> +\tcameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> +\tcameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> +\n> \tint ret = cameraManager_->start();\n> \tif (ret) {\n> \t\tLOG(HAL, Error) << \"Failed to start camera manager: \"\n> @@ -56,35 +61,22 @@ int CameraHalManager::init()\n> \t\treturn ret;\n> \t}\n> \n> -\t/*\n> -\t * For each Camera registered in the system, a CameraDevice\n> -\t * gets created here to wraps a libcamera Camera instance.\n> -\t *\n> -\t * \\todo Support camera hotplug.\n> -\t */\n> -\tunsigned int index = 0;\n> -\tfor (auto &cam : cameraManager_->cameras()) {\n> -\t\tstd::shared_ptr camera = CameraDevice::create(index, cam);\n> -\t\tret = camera->initialize();\n> -\t\tif (ret)\n> -\t\t\tcontinue;\n> -\n> -\t\tcameras_.emplace_back(std::move(camera));\n> -\t\t++index;\n> -\t}\n> -\n> \treturn 0;\n> }\n> \n> CameraDevice *CameraHalManager::open(unsigned int id,\n> \t\t\t\t const hw_module_t *hardwareModule)\n> {\n> -\tif (id >= numCameras()) {\n> +\tMutexLocker locker(mutex_);\n> +\n> +\tauto iter = cameraDeviceFromHALId(id);\n> +\tif (iter == cameras_.end()) {\n> \t\tLOG(HAL, Error) << \"Invalid camera id '\" << id << \"'\";\n> \t\treturn nullptr;\n> \t}\n> \n> -\tCameraDevice *camera = cameras_[id].get();\n> +\tCameraDevice *camera = iter->get();\n> +\n> \tif (camera->open(hardwareModule))\n> \t\treturn nullptr;\n> \n> @@ -93,9 +85,118 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n> \treturn camera;\n> }\n> \n> +void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> +{\n> +\tunsigned int id;\n> +\tbool isCameraExternal = false;\n> +\tbool isCameraNew = false;\n> +\n> +\tMutexLocker locker(mutex_);\n> +\n> +\t/*\n> +\t * Each camera is assigned a unique integer ID when it is seen for the\n> +\t * first time. If the camera has been seen before, the previous ID is\n> +\t * re-used.\n> +\t *\n> +\t * IDs starts from '0' for internal cameras and '1000' for external\n> +\t * cameras.\n> +\t */\n> +\tauto iter = cameraIdsMap_.find(cam->id());\n> +\tif (iter != cameraIdsMap_.end()) {\n> +\t\tid = iter->second;\n> +\t} else {\n> +\t\tisCameraNew = true;\n> +\n> +\t\t/*\n> +\t\t * Now check if this is an external camera and assign\n> +\t\t * its id accordingly.\n> +\t\t */\n> +\t\tif (cameraLocation(cam.get()) == properties::CameraLocationExternal) {\n> +\t\t\tisCameraExternal = true;\n> +\t\t\tid = nextExternalCameraId_;\n> +\t\t} else {\n> +\t\t\tid = numInternalCameras_;\n> +\t\t}\n> +\t}\n> +\n> +\t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> +\tstd::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> +\tint ret = camera->initialize();\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (isCameraNew) {\n> +\t\tcameraIdsMap_.emplace(cam->id(), id);\n> +\n> +\t\tif (isCameraExternal)\n> +\t\t\tnextExternalCameraId_++;\n> +\t\telse\n> +\t\t\tnumInternalCameras_++;\n> +\t}\n> +\n> +\tcameras_.emplace_back(std::move(camera));\n> +\n> +\tif (callbacks_)\n> +\t\tcallbacks_->camera_device_status_change(callbacks_, id,\n> +\t\t\t\t\t\t\tCAMERA_DEVICE_STATUS_PRESENT);\n> +\n> +\tLOG(HAL, Debug) << \"Camera ID: \" << id << \" added successfully.\";\n> +}\n> +\n> +void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> +{\n> +\tMutexLocker locker(mutex_);\n> +\n> +\tauto iter = cameraDeviceFromCamera(cam.get());\n> +\tif (iter == cameras_.end())\n> +\t\treturn;\n> +\n> +\t/* CAMERA_DEVICE_STATUS_NOT_PRESENT should be set for external cameras only. */\n> +\tunsigned int id = (*iter)->id();\n> +\tif (id >= 1000)\n> +\t\tcallbacks_->camera_device_status_change(callbacks_, id,\n> +\t\t\t\t\t\t\tCAMERA_DEVICE_STATUS_NOT_PRESENT);\n> +\n> +\t/*\n> +\t * \\todo Check if the camera is already open and running.\n> +\t * Inform the framework about its absence before deleting its\n> +\t * reference here.\n> +\t */\n> +\tcameras_.erase(iter);\n> +\n> +\tLOG(HAL, Debug) << \"Camera ID: \" << id << \" removed successfully.\";\n> +}\n> +\n> +int32_t CameraHalManager::cameraLocation(const Camera *cam)\n> +{\n> +\tconst ControlList &properties = cam->properties();\n> +\tif (!properties.contains(properties::Location))\n> +\t\treturn -1;\n> +\n> +\treturn properties.get(properties::Location);\n> +}\n> +\n> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraDeviceFromHALId(unsigned int id)\n> +{\n> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t [id](std::shared_ptr &camera) {\n> +\t\t\t\treturn camera->id() == id;\n> +\t\t\t });\n\nHow about returning a CameraDevice * instead of an iterator ? That would\nsimplify the caller a little bit.\n\nCameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)\n{\n\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n\t\t\t\t [id](std::shared_ptr &camera) {\n\t\t\t\t\t return camera->id() == id;\n\t\t\t\t });\n\tif (iter == cameras_.end())\n\t\treturn nullptr;\n\n\treturn iter->get();\n}\n\n> +}\n> +\n> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraDeviceFromCamera(Camera *cam)\n> +{\n> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t [cam](std::shared_ptr &camera) {\n> +\t\t\t\treturn cam == camera->camera();\n> +\t\t\t });\n> +}\n\nIf you inlined this function in its single caller, you could get rid of\nthe cameraDeviceIter type. Otherwise it should be CameraDeviceIter. I\nthink I'd prefer dropping it, it would be simpler.\n\n> +\n> unsigned int CameraHalManager::numCameras() const\n> {\n> -\treturn cameraManager_->cameras().size();\n> +\treturn numInternalCameras_;\n> }\n> \n> int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)\n> @@ -103,12 +204,15 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)\n> \tif (!info)\n> \t\treturn -EINVAL;\n> \n> -\tif (id >= numCameras()) {\n> +\tMutexLocker locker(mutex_);\n> +\n> +\tauto iter = cameraDeviceFromHALId(id);\n> +\tif (iter == cameras_.end()) {\n> \t\tLOG(HAL, Error) << \"Invalid camera id '\" << id << \"'\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tCameraDevice *camera = cameras_[id].get();\n> +\tCameraDevice *camera = iter->get();\n> \n> \tinfo->facing = camera->facing();\n> \tinfo->orientation = camera->orientation();\n> @@ -124,4 +228,25 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)\n> void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)\n> {\n> \tcallbacks_ = callbacks;\n> +\n> +\tMutexLocker locker(mutex_);\n> +\n> +\t/*\n> +\t * Some external cameras may have been identified before the\n> +\t * callbacks_ were set. Iterate all existing external cameras\n> +\t * and mark them as CAMERA_DEVICE_STATUS_PRESENT explicitly.\n> +\t *\n> +\t * Internal cameras are already assumed to be present at module\n> +\t * load time by the Android framework.\n> +\t */\n> +\tfor (std::shared_ptr &camera : cameras_) {\n> +\t\tconst Camera *cam = camera->camera();\n> +\t\tunsigned int id = camera->id();\n> +\t\tif (id < 1000)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (cameraLocation(cam) == properties::CameraLocationExternal)\n\nDo we need both checks, aren't they equivalent ?\n\n> +\t\t\tcallbacks_->camera_device_status_change(callbacks_, id,\n> +\t\t\t\t\t\t\t\tCAMERA_DEVICE_STATUS_PRESENT);\n> +\t}\n> }\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index 3e34d63..3fecaa5 100644\n> --- a/src/android/camera_hal_manager.h\n> +++ b/src/android/camera_hal_manager.h\n> @@ -7,6 +7,8 @@\n> #ifndef __ANDROID_CAMERA_MANAGER_H__\n> #define __ANDROID_CAMERA_MANAGER_H__\n> \n> +#include \n> +#include \n> #include \n> #include \n> \n> @@ -18,12 +20,17 @@\n> \n> class CameraDevice;\n> \n> +using Mutex = std::mutex;\n> +using MutexLocker = std::unique_lock;\n> +\n\nI'd make these private class member, to avoid polluting the global\nnamespace.\n\n> class CameraHalManager\n> {\n> public:\n> \tCameraHalManager();\n> \t~CameraHalManager();\n> \n> +\tusing cameraDeviceIter = std::vector>::iterator;\n> +\n> \tint init();\n> \n> \tCameraDevice *open(unsigned int id, const hw_module_t *module);\n> @@ -33,10 +40,23 @@ public:\n> \tvoid setCallbacks(const camera_module_callbacks_t *callbacks);\n> \n> private:\n> +\tstatic int32_t cameraLocation(const libcamera::Camera *cam);\n> +\n> +\tvoid cameraAdded(std::shared_ptr cam);\n> +\tvoid cameraRemoved(std::shared_ptr cam);\n> +\n> +\tcameraDeviceIter cameraDeviceFromHALId(unsigned int id);\n\nI'd name this cameraDeviceFromHalId to match CameraHalManager (sorry\nabout the incorrect suggestion in reply to v3).\n\nWith these small issues address,\n\nReviewed-by: Laurent Pinchart \n\nI'd applied these changes locally to test compilation before sending\nthis review. I'll thus post the patch as v4.1 to avoid duplicating work.\nIf you're fine with it please let me know, and I'll apply the whole\nseries without a need to resubmit it. It would be nice if you could test\nit though.\n\n> +\tcameraDeviceIter cameraDeviceFromCamera(libcamera::Camera *cam);\n> +\n> \tlibcamera::CameraManager *cameraManager_;\n> \n> \tconst camera_module_callbacks_t *callbacks_;\n> \tstd::vector> cameras_;\n> +\tstd::map cameraIdsMap_;\n> +\tMutex mutex_;\n> +\n> +\tunsigned int numInternalCameras_;\n> +\tunsigned int nextExternalCameraId_;\n> };\n> \n> #endif /* __ANDROID_CAMERA_MANAGER_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AA20EBD87C\n\tfor ;\n\tSat, 22 Aug 2020 21:23:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18A46626DB;\n\tSat, 22 Aug 2020 23:23:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5329461EA0\n\tfor ;\n\tSat, 22 Aug 2020 23:23:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC37529E;\n\tSat, 22 Aug 2020 23:23:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VQD7kbfi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598131413;\n\tbh=vqphvCMntrmjMtQiz7gxpuN3/8pmoUjIsU/O8givQkI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VQD7kbfiAT/cZPhXRAt1OR87wJLr00hDU8wrAIGsK1Z+lyIeFf2/udTVXl6EWMVf+\n\t4m3ij3GfQUY9QHF2xag8CSVD6CUs9HCZz0dsoA+erryReisrgWYeNrEILaHAg5za2s\n\tfz1xiBnCrekURwOIlm02OMVg3t+/iVAa06TbEZoA=","Date":"Sun, 23 Aug 2020 00:23:14 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200822212314.GA25161@pendragon.ideasonboard.com>","References":"<20200821144601.67860-1-email@uajain.com>\n\t<20200821144601.67860-6-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200821144601.67860-6-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v4 5/5] android: camera_hal_manager:\n\tSupport camera hotplug","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]