[{"id":12097,"web_url":"https://patchwork.libcamera.org/comment/12097/","msgid":"<362a2248-ca54-2887-87e2-e55fd539d314@uajain.com>","date":"2020-08-23T17:17:34","subject":"Re: [libcamera-devel] [PATCH v4.1 5/5] android: camera_hal_manager:\n\tSupport camera hotplug","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nI have reviewed the changes and also tested on CrOS\nwith my setup. Things look good to me.\n\nThanks for the writing last final fixes of your review. :)\n\nOn 8/23/20 2:59 AM, Laurent Pinchart wrote:\n> From: Umang Jain \n>\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 cameraIdsMap_ which book-keeps all cameras seen in the\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> cameraIdsMap_ and cameras_ are protected by a mutex.\n>\n> Signed-off-by: Umang Jain \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v4:\n>\n> - Rename cameraDeviceFromHALId() to cameraDeviceFromHalId()\n> - Return a CameraDevice from cameraDeviceFromHalId()\n> - Inline cameraDeviceFromCamera() in its only caller\n> - Drop duplicate location check\n> - Drop cameraDeviceIter type\n> - Add firstExternalCameraId_\n> - Make Mutex and MutexLocker private member types\n> - Replace camerasMap_ with cameraIdsMap_ in commit message\n> - Reflow some comments\n>\n> I've applied these changes locally during review of v4 to compile-test\n> my comments, so I'm posting them to avoid duplicating work.\n> ---\n> src/android/camera_hal_manager.cpp | 168 ++++++++++++++++++++++++-----\n> src/android/camera_hal_manager.h | 19 ++++\n> 2 files changed, 163 insertions(+), 24 deletions(-)\n>\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 3a744af5f231..a1805ebccf24 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,8 @@ LOG_DECLARE_CATEGORY(HAL);\n> */\n> \n> CameraHalManager::CameraHalManager()\n> -\t: cameraManager_(nullptr)\n> +\t: cameraManager_(nullptr), numInternalCameras_(0),\n> +\t nextExternalCameraId_(firstExternalCameraId_)\n> {\n> }\n> \n> @@ -47,6 +49,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 +62,20 @@ 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> +\tCameraDevice *camera = cameraDeviceFromHalId(id);\n> +\tif (!camera) {\n> \t\tLOG(HAL, Error) << \"Invalid camera id '\" << id << \"'\";\n> \t\treturn nullptr;\n> \t}\n> \n> -\tCameraDevice *camera = cameras_[id].get();\n> \tif (camera->open(hardwareModule))\n> \t\treturn nullptr;\n> \n> @@ -93,9 +84,120 @@ 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 = std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t\t [&cam](std::shared_ptr &camera) {\n> +\t\t\t\t\t return cam.get() == camera->camera();\n> +\t\t\t\t });\n> +\tif (iter == cameras_.end())\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * CAMERA_DEVICE_STATUS_NOT_PRESENT should be set for external cameras\n> +\t * only.\n> +\t */\n> +\tunsigned int id = (*iter)->id();\n> +\tif (id >= firstExternalCameraId_)\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> +CameraDevice *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> 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,13 +205,14 @@ 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> +\tCameraDevice *camera = cameraDeviceFromHalId(id);\n> +\tif (!camera) {\n> \t\tLOG(HAL, Error) << \"Invalid camera id '\" << id << \"'\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tCameraDevice *camera = cameras_[id].get();\n> -\n> \tinfo->facing = camera->facing();\n> \tinfo->orientation = camera->orientation();\n> \tinfo->device_version = CAMERA_DEVICE_API_VERSION_3_3;\n> @@ -124,4 +227,21 @@ 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 callbacks_\n> +\t * were set. Iterate all existing external cameras and mark them as\n> +\t * CAMERA_DEVICE_STATUS_PRESENT explicitly.\n> +\t *\n> +\t * Internal cameras are already assumed to be present at module load\n> +\t * time by the Android framework.\n> +\t */\n> +\tfor (std::shared_ptr &camera : cameras_) {\n> +\t\tunsigned int id = camera->id();\n> +\t\tif (id >= firstExternalCameraId_)\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 3e34d63ff96c..a91decc7d8fe 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> @@ -33,10 +35,27 @@ public:\n> \tvoid setCallbacks(const camera_module_callbacks_t *callbacks);\n> \n> private:\n> +\tusing Mutex = std::mutex;\n> +\tusing MutexLocker = std::unique_lock;\n> +\n> +\tstatic constexpr unsigned int firstExternalCameraId_ = 1000;\n> +\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> +\tCameraDevice *cameraDeviceFromHalId(unsigned int id);\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 310CBBD87C\n\tfor ;\n\tSun, 23 Aug 2020 17:17:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E984626DB;\n\tSun, 23 Aug 2020 19:17:38 +0200 (CEST)","from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B75E16037E\n\tfor ;\n\tSun, 23 Aug 2020 19:17:35 +0200 (CEST)","by filterdrecv-p3iad2-86945d9569-2qwtv with SMTP id\n\tfilterdrecv-p3iad2-86945d9569-2qwtv-20-5F42A4AE-4\n\t2020-08-23 17:17:34.285967902 +0000 UTC m=+256474.174171224","from mail.uajain.com (unknown)\n\tby ismtpd0002p1maa1.sendgrid.net (SG) with ESMTP\n\tid 6oT1pU1RSTaXt5ysQpa9pA Sun, 23 Aug 2020 17:17:33.598 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"kQc+AVG4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=j2uEyUuYJXf0WjySrx8LLa9jTDabNAM3MEj4WVgTPP0=;\n\tb=kQc+AVG4TkddBzoHXm5gtSHQd4wp1H36LTJIAqkKJxQK8caA4xA8YW2Fl3usMsAQyPv3\n\tVia9W7k+bEmmp9Ep6bVudbtZdqtGsX8DEpNyqQ/O4Vu/1TXxXpKtR4P7DAf4Dfhgjm3LH+\n\t2ltkaKSjUArbUxr1sHrKk6TUfeo/yKkFE=","References":"<20200821144601.67860-6-email@uajain.com>\n\t<20200822212913.28635-1-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<362a2248-ca54-2887-87e2-e55fd539d314@uajain.com>","Date":"Sun, 23 Aug 2020 17:17:34 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200822212913.28635-1-laurent.pinchart@ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcgGRjBZ5qHMmenUzbN5SXMK6jDN19JvhwWnsb5CCFavhkn8lpnQ5oST8y0q1C70qPTGa8mrzhNGwTDpJ5rsF/IW/gy9MbJfqtm8SC/jMcNDJ3xdTVU4m0WVjbxZzmR8ret0MMAfXDZzM2B4Nb7p6kXx7g2xMzVnrgIn2USvQOQVdnloACWTcOYcrTuAYO5kRYLBydIsxYaU++6ZX6P3BeTg==","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4.1 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","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]