[{"id":12048,"web_url":"https://patchwork.libcamera.org/comment/12048/","msgid":"<20200819153358.GO6049@pendragon.ideasonboard.com>","date":"2020-08-19T15:33:58","subject":"Re: [libcamera-devel] [PATCH v3 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 Mon, Aug 17, 2020 at 08:26:40PM +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> being hotplugged or unplugged via camera_device_status_change().\n\ns/being being/being/\n\n> \n> Introduce a map camerasMap_ 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, it's old id is reused. IDs for internal cameras start with\n\ns/it's/its/\n\n> '0' and for external cameras, they start with '1000'. Note, for the\n> current implementation, we assume all UVC cameras are external cameras.\n\nI'd drop this sentence, as nothing here is UVC-specific, it's an issue\nto be solved in the UVC pipeline handler.\n\n> Also, acess to camerasMap_ and cameras_ are protected by a mutex.\n\ns/acess/accesses/\n\n> Signed-off-by: Umang Jain <email@uajain.com>\n> ---\n>  src/android/camera_hal_manager.cpp | 174 +++++++++++++++++++++++++----\n>  src/android/camera_hal_manager.h   |  18 +++\n>  2 files changed, 170 insertions(+), 22 deletions(-)\n> \n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 3a744af..e851185 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 <libcamera/camera.h>\n> +#include <libcamera/property_ids.h>\n>  \n>  #include \"libcamera/internal/log.h\"\n>  \n> @@ -35,6 +36,7 @@ CameraHalManager::CameraHalManager()\n>  CameraHalManager::~CameraHalManager()\n>  {\n>  \tcameras_.clear();\n> +\tcamerasMap_.clear();\n\nYou don't need this, the CameraHalManager is being deleted, so the\ncamerasMap_ is deleted too. The reason we need it for cameras_ is to\nrelease the Camera shared pointers before calling CameraManager::stop().\n\n>  \n>  \tif (cameraManager_) {\n>  \t\tcameraManager_->stop();\n> @@ -47,6 +49,13 @@ 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> +\tinternalCameras_ = 0;\n> +\texternalCameras_ = 1000;\n\nCan you initialize those two fields in the constructor (using the member\ninitializer list) ?\n\n> +\n>  \tint ret = cameraManager_->start();\n>  \tif (ret) {\n>  \t\tLOG(HAL, Error) << \"Failed to start camera manager: \"\n> @@ -56,35 +65,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<CameraDevice> 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 = cameraIterFromId(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 +89,114 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n>  \treturn camera;\n>  }\n>  \n> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> 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 and the camera is marked as CAMERA_DEVICE_STATUS_PRESENT\n> +\t * subsequently.\n\nCAMERA_DEVICE_STATUS_PRESENT is unrelated to whether the camera is new\nor has been seen before. I'd drop that part of the sentence.\n\n> +\t *\n> +\t * IDs starts from '0' for internal cameras and '1000' for external\n> +\t * cameras.\n> +\t */\n> +\tauto iter = camerasMap_.find(cam->id());\n> +\tif (iter != camerasMap_.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\ns/*  /* /\n\n> +\t\t */\n> +\t\tconst ControlList &properties = cam->properties();\n> +\t\tif (properties.contains(properties::Location) &&\n> +\t\t    properties.get(properties::Location) ==\n> +\t\t    properties::CameraLocationExternal) {\n> +\t\t\tisCameraExternal = true;\n> +\t\t\tid = externalCameras_;\n> +\t\t} else {\n> +\t\t\tid = internalCameras_;\n> +\t\t}\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\ns/wraps/wrap/\n\nAnd this isn't about creating a CameraDevice instance for each camera\nanymore. I'd write this\n\n\t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n\n> +\t */\n> +\tstd::shared_ptr<CameraDevice> 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\tcamerasMap_.emplace(cam->id(), id);\n> +\n> +\t\tif (isCameraExternal)\n> +\t\t\texternalCameras_++;\n> +\t\telse\n> +\t\t\tinternalCameras_++;\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<Camera> cam)\n> +{\n> +\tMutexLocker locker(mutex_);\n> +\n> +\tauto iter = cameraIterFromCamera(cam.get());\n> +\tif (iter == cameras_.end())\n> +\t\treturn;\n> +\n> +\tunsigned int id = (*iter)->id();\n> +\tcallbacks_->camera_device_status_change(callbacks_, id,\n> +\t\t\t\t\t\tCAMERA_DEVICE_STATUS_NOT_PRESENT);\n\nThis should only be called for external cameras. cameraRemoved()\nshouldn't be called for internal cameras in the first place, so maybe\nthere's no need for a conditional here.\n\n> +\n> +\t/*\n> +\t * \\todo Check if the camera is already open and running.\n> +\t * Inform the framework about it's absence before deleting it's\n\ns/it's/its/g\n\n> +\t * reference here.\n\nCould this be as simple as creating a\n\n\tstd::list<std::shared_ptr<CameraDevice>> openCameras_;\n\nto hold references to open cameras ? It could be done on top.\n\n> +\t */\n> +\tcameras_.erase(iter);\n> +\n> +\tLOG(HAL, Debug) << \"Camera ID: \" << id << \" removed successfully.\";\n> +}\n> +\n> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromId(unsigned int id)\n> +{\n> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t    [id](std::shared_ptr<CameraDevice> &camera) {\n> +\t\t\t\treturn camera->id() == id;\n> +\t\t\t    });\n> +}\n> +\n> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromCamera(Camera *cam)\n> +{\n> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t    [cam](std::shared_ptr<CameraDevice> &camera) {\n> +\t\t\t\treturn cam == camera->camera();\n> +\t\t\t    });\n> +}\n> +\n>  unsigned int CameraHalManager::numCameras() const\n>  {\n> -\treturn cameraManager_->cameras().size();\n> +\treturn internalCameras_;\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 = cameraIterFromId(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,30 @@ 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 enabled. Iterate any existing external\n\ns/enabled/set/ ?\ns/any/all/\n\n> +\t * cameras and mark them as CAMERA_DEVICE_STATUS_PRESENT\n> +\t * 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 (auto &cam : cameraManager_->cameras()) {\n> +\t\tauto iter = camerasMap_.find(cam->id());\n> +\t\tif (iter == camerasMap_.end())\n> +\t\t\tcontinue;\n\nHow about iterating over cameras_ instead ? You wouldn't need to look up\nthe id in camerasMap_.\n\n\tfor (std::shared_ptr<CameraDevice> &camera : cameras_) {\n\t\tCamera *cam = camera->camera();\n\t\tunsigned int id = camera->id();\n\n(you could also drop some of the local variables if desired, as they are\nboth used once only).\n\n> +\n> +\t\tunsigned int id = iter->second;\n> +\t\tconst ControlList &properties = cam->properties();\n> +\t\tif (properties.contains(properties::Location) &&\n> +\t\t    properties.get(properties::Location) &\n\nThe Location property isn't a bitfield, it's an enum.\n\n> +\t\t    properties::CameraLocationExternal) {\n\nThere are two locations where you check the Location property. Would\nadding the following function make sense ?\n\nstatic int32_t CameraHalManager::cameraLocation(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\nThe caller would become\n\n\t\tif (cameraLocation(cam) == properties::CameraLocationExternal)\n\nwhich is a bit nicer to read I think.\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\t}\n> +\t}\n>  }\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index 3e34d63..f8adade 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 <map>\n> +#include <mutex>\n>  #include <stddef.h>\n>  #include <vector>\n>  \n> @@ -18,12 +20,17 @@\n>  \n>  class CameraDevice;\n>  \n> +using Mutex = std::mutex;\n> +using MutexLocker = std::unique_lock<std::mutex>;\n> +\n>  class CameraHalManager\n>  {\n>  public:\n>  \tCameraHalManager();\n>  \t~CameraHalManager();\n>  \n> +\tusing cameraDeviceIter = std::vector<std::shared_ptr<CameraDevice>>::iterator;\n> +\n>  \tint init();\n>  \n>  \tCameraDevice *open(unsigned int id, const hw_module_t *module);\n> @@ -33,10 +40,21 @@ public:\n>  \tvoid setCallbacks(const camera_module_callbacks_t *callbacks);\n>  \n>  private:\n> +\tvoid cameraAdded(std::shared_ptr<libcamera::Camera> cam);\n> +\tvoid cameraRemoved(std::shared_ptr<libcamera::Camera> cam);\n> +\n> +\tcameraDeviceIter cameraIterFromId(unsigned int id);\n> +\tcameraDeviceIter cameraIterFromCamera(libcamera::Camera *cam);\n\nHow about turning these to\n\n\tCameraDevice *cameraFromId(unsigned int id);\n\tCameraDevice *cameraFromCamera(libcamera::Camera *cam);\n\nIt would simplify the interface. I would actually call the former\ncameraFromHALId() to differentiate the two types of IDs. And maybe\n\n\tCameraDevice *cameraDeviceFromHALId(unsigned int id);\n\tCameraDevice *cameraDeviecFromCamera(libcamera::Camera *cam);\n\nif it's not too long ?\n\n> +\n>  \tlibcamera::CameraManager *cameraManager_;\n>  \n>  \tconst camera_module_callbacks_t *callbacks_;\n>  \tstd::vector<std::shared_ptr<CameraDevice>> cameras_;\n> +\tstd::map<std::string, unsigned int> camerasMap_;\n\nMaybe cameraIdsMap_ to make the contents more explicit ?\n\n> +\tMutex mutex_;\n> +\n> +\tunsigned int internalCameras_;\n\nMaybe numInternalCameras_ or internalCamerasCount_ ?\n\n> +\tunsigned int externalCameras_;\n\nAnd nextExternalCameraId_ ?\n\n>  };\n>  \n>  #endif /* __ANDROID_CAMERA_MANAGER_H__ */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D16E6BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 19 Aug 2020 15:34:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 580A861ED9;\n\tWed, 19 Aug 2020 17:34:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C94F760382\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 19 Aug 2020 17:34:16 +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 4447729E;\n\tWed, 19 Aug 2020 17:34:16 +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=\"MTsC4Osr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1597851256;\n\tbh=dF9dMenV4nrTnb7+4PCmEtFBTOVGl8ErZCosP88hlPM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MTsC4OsrLqlcxQXws9P1AXTVMN3yIVAorOeN9s5VoZpsokAKwOVVDe+wUI8kG4Fxt\n\tefMRgtRpFD39Eh17FTFUtpf/z8Ddr7ViYwxB2A+8SIbRLKZoIqEddVn2K19Gv00DRg\n\t9STxMbcEKgNbN7G4ap2r4UI+dEy0t9FikmVmy0s8=","Date":"Wed, 19 Aug 2020 18:33:58 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Umang Jain <email@uajain.com>","Message-ID":"<20200819153358.GO6049@pendragon.ideasonboard.com>","References":"<20200817202629.4277-1-email@uajain.com>\n\t<20200817202629.4277-6-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200817202629.4277-6-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12089,"web_url":"https://patchwork.libcamera.org/comment/12089/","msgid":"<ccb57844-84a7-4a2e-5499-5ce125afa5b4@uajain.com>","date":"2020-08-21T12:03:28","subject":"Re: [libcamera-devel] [PATCH v3 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\nOn 8/19/20 9:03 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 17, 2020 at 08:26:40PM +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>> being hotplugged or unplugged via camera_device_status_change().\n> s/being being/being/\n>\n>> Introduce a map camerasMap_ 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, it's old id is reused. IDs for internal cameras start with\n> s/it's/its/\n>\n>> '0' and for external cameras, they start with '1000'. Note, for the\n>> current implementation, we assume all UVC cameras are external cameras.\n> I'd drop this sentence, as nothing here is UVC-specific, it's an issue\n> to be solved in the UVC pipeline handler.\n>\n>> Also, acess to camerasMap_ and cameras_ are protected by a mutex.\n> s/acess/accesses/\n>\n>> Signed-off-by: Umang Jain <email@uajain.com>\n>> ---\n>>   src/android/camera_hal_manager.cpp | 174 +++++++++++++++++++++++++----\n>>   src/android/camera_hal_manager.h   |  18 +++\n>>   2 files changed, 170 insertions(+), 22 deletions(-)\n>>\n>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n>> index 3a744af..e851185 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 <libcamera/camera.h>\n>> +#include <libcamera/property_ids.h>\n>>   \n>>   #include \"libcamera/internal/log.h\"\n>>   \n>> @@ -35,6 +36,7 @@ CameraHalManager::CameraHalManager()\n>>   CameraHalManager::~CameraHalManager()\n>>   {\n>>   \tcameras_.clear();\n>> +\tcamerasMap_.clear();\n> You don't need this, the CameraHalManager is being deleted, so the\n> camerasMap_ is deleted too. The reason we need it for cameras_ is to\n> release the Camera shared pointers before calling CameraManager::stop().\n>\n>>   \n>>   \tif (cameraManager_) {\n>>   \t\tcameraManager_->stop();\n>> @@ -47,6 +49,13 @@ 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>> +\tinternalCameras_ = 0;\n>> +\texternalCameras_ = 1000;\n> Can you initialize those two fields in the constructor (using the member\n> initializer list) ?\n>\n>> +\n>>   \tint ret = cameraManager_->start();\n>>   \tif (ret) {\n>>   \t\tLOG(HAL, Error) << \"Failed to start camera manager: \"\n>> @@ -56,35 +65,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<CameraDevice> 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 = cameraIterFromId(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 +89,114 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n>>   \treturn camera;\n>>   }\n>>   \n>> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> 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 and the camera is marked as CAMERA_DEVICE_STATUS_PRESENT\n>> +\t * subsequently.\n> CAMERA_DEVICE_STATUS_PRESENT is unrelated to whether the camera is new\n> or has been seen before. I'd drop that part of the sentence.\n>\n>> +\t *\n>> +\t * IDs starts from '0' for internal cameras and '1000' for external\n>> +\t * cameras.\n>> +\t */\n>> +\tauto iter = camerasMap_.find(cam->id());\n>> +\tif (iter != camerasMap_.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> s/*  /* /\n>\n>> +\t\t */\n>> +\t\tconst ControlList &properties = cam->properties();\n>> +\t\tif (properties.contains(properties::Location) &&\n>> +\t\t    properties.get(properties::Location) ==\n>> +\t\t    properties::CameraLocationExternal) {\n>> +\t\t\tisCameraExternal = true;\n>> +\t\t\tid = externalCameras_;\n>> +\t\t} else {\n>> +\t\t\tid = internalCameras_;\n>> +\t\t}\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> s/wraps/wrap/\n>\n> And this isn't about creating a CameraDevice instance for each camera\n> anymore. I'd write this\n>\n> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n>\n>> +\t */\n>> +\tstd::shared_ptr<CameraDevice> 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\tcamerasMap_.emplace(cam->id(), id);\n>> +\n>> +\t\tif (isCameraExternal)\n>> +\t\t\texternalCameras_++;\n>> +\t\telse\n>> +\t\t\tinternalCameras_++;\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<Camera> cam)\n>> +{\n>> +\tMutexLocker locker(mutex_);\n>> +\n>> +\tauto iter = cameraIterFromCamera(cam.get());\n>> +\tif (iter == cameras_.end())\n>> +\t\treturn;\n>> +\n>> +\tunsigned int id = (*iter)->id();\n>> +\tcallbacks_->camera_device_status_change(callbacks_, id,\n>> +\t\t\t\t\t\tCAMERA_DEVICE_STATUS_NOT_PRESENT);\n> This should only be called for external cameras. cameraRemoved()\n> shouldn't be called for internal cameras in the first place, so maybe\n> there's no need for a conditional here.\ncameraRemoved() shouldn't be called for internal cameras?\nWell, I wonder what exacts means by \"unplugging an internal camera\",\nbut I think *if* somehow, internal cameras are unplugged, we do need to\ndrop the reference from cameras_, no?\n>\n>> +\n>> +\t/*\n>> +\t * \\todo Check if the camera is already open and running.\n>> +\t * Inform the framework about it's absence before deleting it's\n> s/it's/its/g\n>\n>> +\t * reference here.\n> Could this be as simple as creating a\n>\n> \tstd::list<std::shared_ptr<CameraDevice>> openCameras_;\n>\n> to hold references to open cameras ? It could be done on top.\n>\n>> +\t */\n>> +\tcameras_.erase(iter);\n>> +\n>> +\tLOG(HAL, Debug) << \"Camera ID: \" << id << \" removed successfully.\";\n>> +}\n>> +\n>> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromId(unsigned int id)\n>> +{\n>> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n>> +\t\t\t    [id](std::shared_ptr<CameraDevice> &camera) {\n>> +\t\t\t\treturn camera->id() == id;\n>> +\t\t\t    });\n>> +}\n>> +\n>> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromCamera(Camera *cam)\n>> +{\n>> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n>> +\t\t\t    [cam](std::shared_ptr<CameraDevice> &camera) {\n>> +\t\t\t\treturn cam == camera->camera();\n>> +\t\t\t    });\n>> +}\n>> +\n>>   unsigned int CameraHalManager::numCameras() const\n>>   {\n>> -\treturn cameraManager_->cameras().size();\n>> +\treturn internalCameras_;\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 = cameraIterFromId(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,30 @@ 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 enabled. Iterate any existing external\n> s/enabled/set/ ?\n> s/any/all/\n>\n>> +\t * cameras and mark them as CAMERA_DEVICE_STATUS_PRESENT\n>> +\t * 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 (auto &cam : cameraManager_->cameras()) {\n>> +\t\tauto iter = camerasMap_.find(cam->id());\n>> +\t\tif (iter == camerasMap_.end())\n>> +\t\t\tcontinue;\n> How about iterating over cameras_ instead ? You wouldn't need to look up\n> the id in camerasMap_.\n>\n> \tfor (std::shared_ptr<CameraDevice> &camera : cameras_) {\n> \t\tCamera *cam = camera->camera();\n> \t\tunsigned int id = camera->id();\n>\n> (you could also drop some of the local variables if desired, as they are\n> both used once only).\n>\n>> +\n>> +\t\tunsigned int id = iter->second;\n>> +\t\tconst ControlList &properties = cam->properties();\n>> +\t\tif (properties.contains(properties::Location) &&\n>> +\t\t    properties.get(properties::Location) &\n> The Location property isn't a bitfield, it's an enum.\n>\n>> +\t\t    properties::CameraLocationExternal) {\n> There are two locations where you check the Location property. Would\n> adding the following function make sense ?\n>\n> static int32_t CameraHalManager::cameraLocation(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> }\nDo you think it should be a 'static' method? I am curious why?\nAny special reasons?\n>\n> The caller would become\n>\n> \t\tif (cameraLocation(cam) == properties::CameraLocationExternal)\n>\n> which is a bit nicer to read I think.\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\t}\n>> +\t}\n>>   }\n>> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n>> index 3e34d63..f8adade 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 <map>\n>> +#include <mutex>\n>>   #include <stddef.h>\n>>   #include <vector>\n>>   \n>> @@ -18,12 +20,17 @@\n>>   \n>>   class CameraDevice;\n>>   \n>> +using Mutex = std::mutex;\n>> +using MutexLocker = std::unique_lock<std::mutex>;\n>> +\n>>   class CameraHalManager\n>>   {\n>>   public:\n>>   \tCameraHalManager();\n>>   \t~CameraHalManager();\n>>   \n>> +\tusing cameraDeviceIter = std::vector<std::shared_ptr<CameraDevice>>::iterator;\n>> +\n>>   \tint init();\n>>   \n>>   \tCameraDevice *open(unsigned int id, const hw_module_t *module);\n>> @@ -33,10 +40,21 @@ public:\n>>   \tvoid setCallbacks(const camera_module_callbacks_t *callbacks);\n>>   \n>>   private:\n>> +\tvoid cameraAdded(std::shared_ptr<libcamera::Camera> cam);\n>> +\tvoid cameraRemoved(std::shared_ptr<libcamera::Camera> cam);\n>> +\n>> +\tcameraDeviceIter cameraIterFromId(unsigned int id);\n>> +\tcameraDeviceIter cameraIterFromCamera(libcamera::Camera *cam);\n> How about turning these to\n>\n> \tCameraDevice *cameraFromId(unsigned int id);\n> \tCameraDevice *cameraFromCamera(libcamera::Camera *cam);\n>\n> It would simplify the interface. I would actually call the former\n> cameraFromHALId() to differentiate the two types of IDs. And maybe\n>\n> \tCameraDevice *cameraDeviceFromHALId(unsigned int id);\n> \tCameraDevice *cameraDeviecFromCamera(libcamera::Camera *cam);\n>\n> if it's not too long ?\n>\n>> +\n>>   \tlibcamera::CameraManager *cameraManager_;\n>>   \n>>   \tconst camera_module_callbacks_t *callbacks_;\n>>   \tstd::vector<std::shared_ptr<CameraDevice>> cameras_;\n>> +\tstd::map<std::string, unsigned int> camerasMap_;\n> Maybe cameraIdsMap_ to make the contents more explicit ?\n>\n>> +\tMutex mutex_;\n>> +\n>> +\tunsigned int internalCameras_;\n> Maybe numInternalCameras_ or internalCamerasCount_ ?\n>\n>> +\tunsigned int externalCameras_;\n> And nextExternalCameraId_ ?\n>\n>>   };\n>>   \n>>   #endif /* __ANDROID_CAMERA_MANAGER_H__ */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6D2F4BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 21 Aug 2020 12:03:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EA42861B4F;\n\tFri, 21 Aug 2020 14:03:33 +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 B56EA60383\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 21 Aug 2020 14:03:31 +0200 (CEST)","by filterdrecv-p3las1-7c6d97d9cb-q97nk with SMTP id\n\tfilterdrecv-p3las1-7c6d97d9cb-q97nk-20-5F3FB810-2E\n\t2020-08-21 12:03:28.35551421 +0000 UTC m=+64819.923053652","from mail.uajain.com (unknown)\n\tby ismtpd0004p1hnd1.sendgrid.net (SG) with ESMTP\n\tid LuAouH-AQSCPFIBDJ5v51Q Fri, 21 Aug 2020 12:03:27.954 +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=\"eTppTz4Y\"; 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:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=146bh3+gtrjlx1i3Ny1osjIqsV1RjS5mb8bNbJKnHJ0=;\n\tb=eTppTz4Y+yQIu5KzvUOTVD7iSgsqScfLjnZnhEOG9KPf5djK+O+vak6bM8m8DkKCBYxZ\n\tslGWPcrphufOFYCBxsbynewGBjffP+cD5NUKC8HL9BlthfdY2nOsGEhmuj2cdMdvtPBF+k\n\tCIkyOBwCT/6xHpifOSlRww00LeOXiRPDc=","References":"<20200817202629.4277-1-email@uajain.com>\n\t<20200817202629.4277-6-email@uajain.com>\n\t<20200819153358.GO6049@pendragon.ideasonboard.com>","From":"Umang Jain <email@uajain.com>","Message-ID":"<ccb57844-84a7-4a2e-5499-5ce125afa5b4@uajain.com>","Date":"Fri, 21 Aug 2020 12:03:28 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200819153358.GO6049@pendragon.ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcQ/i1ji5k4eTSSnQ1oTeZ4m1NvXZuxnYnqnViPsstVuxTov0aHWfahwcgL9vuADYDZAv6X8CaWp4bGyjIH9mPLj1g5gtP0FEPYd4hWnSwu7LRWonYH+y1JdH/f6ulQLuZCyEW99MwGPsIAtG6F0JksmnE/XaOgk8+qTeEbuwc209Po6Ws8HsFfGuuWZhQwQkBOJsqr5MWZKFRHsM0tAOwMw==","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12090,"web_url":"https://patchwork.libcamera.org/comment/12090/","msgid":"<20200822012832.GL5967@pendragon.ideasonboard.com>","date":"2020-08-22T01:28:32","subject":"Re: [libcamera-devel] [PATCH v3 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\nOn Fri, Aug 21, 2020 at 12:03:28PM +0000, Umang Jain wrote:\n> On 8/19/20 9:03 PM, Laurent Pinchart wrote:\n> > On Mon, Aug 17, 2020 at 08:26:40PM +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> >> being hotplugged or unplugged via camera_device_status_change().\n> >\n> > s/being being/being/\n> >\n> >> Introduce a map camerasMap_ 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, it's old id is reused. IDs for internal cameras start with\n> >\n> > s/it's/its/\n> >\n> >> '0' and for external cameras, they start with '1000'. Note, for the\n> >> current implementation, we assume all UVC cameras are external cameras.\n> > I'd drop this sentence, as nothing here is UVC-specific, it's an issue\n> > to be solved in the UVC pipeline handler.\n> >\n> >> Also, acess to camerasMap_ and cameras_ are protected by a mutex.\n> >\n> > s/acess/accesses/\n> >\n> >> Signed-off-by: Umang Jain <email@uajain.com>\n> >> ---\n> >>   src/android/camera_hal_manager.cpp | 174 +++++++++++++++++++++++++----\n> >>   src/android/camera_hal_manager.h   |  18 +++\n> >>   2 files changed, 170 insertions(+), 22 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> >> index 3a744af..e851185 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 <libcamera/camera.h>\n> >> +#include <libcamera/property_ids.h>\n> >>   \n> >>   #include \"libcamera/internal/log.h\"\n> >>   \n> >> @@ -35,6 +36,7 @@ CameraHalManager::CameraHalManager()\n> >>   CameraHalManager::~CameraHalManager()\n> >>   {\n> >>   \tcameras_.clear();\n> >> +\tcamerasMap_.clear();\n> >\n> > You don't need this, the CameraHalManager is being deleted, so the\n> > camerasMap_ is deleted too. The reason we need it for cameras_ is to\n> > release the Camera shared pointers before calling CameraManager::stop().\n> >\n> >>   \n> >>   \tif (cameraManager_) {\n> >>   \t\tcameraManager_->stop();\n> >> @@ -47,6 +49,13 @@ 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> >> +\tinternalCameras_ = 0;\n> >> +\texternalCameras_ = 1000;\n> >\n> > Can you initialize those two fields in the constructor (using the member\n> > initializer list) ?\n> >\n> >> +\n> >>   \tint ret = cameraManager_->start();\n> >>   \tif (ret) {\n> >>   \t\tLOG(HAL, Error) << \"Failed to start camera manager: \"\n> >> @@ -56,35 +65,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<CameraDevice> 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 = cameraIterFromId(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 +89,114 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n> >>   \treturn camera;\n> >>   }\n> >>   \n> >> +void CameraHalManager::cameraAdded(std::shared_ptr<Camera> 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 and the camera is marked as CAMERA_DEVICE_STATUS_PRESENT\n> >> +\t * subsequently.\n> >\n> > CAMERA_DEVICE_STATUS_PRESENT is unrelated to whether the camera is new\n> > or has been seen before. I'd drop that part of the sentence.\n> >\n> >> +\t *\n> >> +\t * IDs starts from '0' for internal cameras and '1000' for external\n> >> +\t * cameras.\n> >> +\t */\n> >> +\tauto iter = camerasMap_.find(cam->id());\n> >> +\tif (iter != camerasMap_.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> >\n> > s/*  /* /\n> >\n> >> +\t\t */\n> >> +\t\tconst ControlList &properties = cam->properties();\n> >> +\t\tif (properties.contains(properties::Location) &&\n> >> +\t\t    properties.get(properties::Location) ==\n> >> +\t\t    properties::CameraLocationExternal) {\n> >> +\t\t\tisCameraExternal = true;\n> >> +\t\t\tid = externalCameras_;\n> >> +\t\t} else {\n> >> +\t\t\tid = internalCameras_;\n> >> +\t\t}\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> >\n> > s/wraps/wrap/\n> >\n> > And this isn't about creating a CameraDevice instance for each camera\n> > anymore. I'd write this\n> >\n> > \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> >\n> >> +\t */\n> >> +\tstd::shared_ptr<CameraDevice> 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\tcamerasMap_.emplace(cam->id(), id);\n> >> +\n> >> +\t\tif (isCameraExternal)\n> >> +\t\t\texternalCameras_++;\n> >> +\t\telse\n> >> +\t\t\tinternalCameras_++;\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<Camera> cam)\n> >> +{\n> >> +\tMutexLocker locker(mutex_);\n> >> +\n> >> +\tauto iter = cameraIterFromCamera(cam.get());\n> >> +\tif (iter == cameras_.end())\n> >> +\t\treturn;\n> >> +\n> >> +\tunsigned int id = (*iter)->id();\n> >> +\tcallbacks_->camera_device_status_change(callbacks_, id,\n> >> +\t\t\t\t\t\tCAMERA_DEVICE_STATUS_NOT_PRESENT);\n> >\n> > This should only be called for external cameras. cameraRemoved()\n> > shouldn't be called for internal cameras in the first place, so maybe\n> > there's no need for a conditional here.\n>\n> cameraRemoved() shouldn't be called for internal cameras?\n> Well, I wonder what exacts means by \"unplugging an internal camera\",\n> but I think *if* somehow, internal cameras are unplugged, we do need to\n> drop the reference from cameras_, no?\n\nI meant that if a camera is internal, removing it from the system at\nruntime shouldn't occur. If someone decides to tear the system apart,\nworse problems will appear :-) If course we should support this to the\nbest of our ability in the HAL, but the Android camera service doesn't\nexpect an internal camera to be unplugged, so I doubt it will be handled\ngracefully there.\n\n> >> +\n> >> +\t/*\n> >> +\t * \\todo Check if the camera is already open and running.\n> >> +\t * Inform the framework about it's absence before deleting it's\n> >\n> > s/it's/its/g\n> >\n> >> +\t * reference here.\n> >\n> > Could this be as simple as creating a\n> >\n> > \tstd::list<std::shared_ptr<CameraDevice>> openCameras_;\n> >\n> > to hold references to open cameras ? It could be done on top.\n> >\n> >> +\t */\n> >> +\tcameras_.erase(iter);\n> >> +\n> >> +\tLOG(HAL, Debug) << \"Camera ID: \" << id << \" removed successfully.\";\n> >> +}\n> >> +\n> >> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromId(unsigned int id)\n> >> +{\n> >> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n> >> +\t\t\t    [id](std::shared_ptr<CameraDevice> &camera) {\n> >> +\t\t\t\treturn camera->id() == id;\n> >> +\t\t\t    });\n> >> +}\n> >> +\n> >> +CameraHalManager::cameraDeviceIter CameraHalManager::cameraIterFromCamera(Camera *cam)\n> >> +{\n> >> +\treturn std::find_if(cameras_.begin(), cameras_.end(),\n> >> +\t\t\t    [cam](std::shared_ptr<CameraDevice> &camera) {\n> >> +\t\t\t\treturn cam == camera->camera();\n> >> +\t\t\t    });\n> >> +}\n> >> +\n> >>   unsigned int CameraHalManager::numCameras() const\n> >>   {\n> >> -\treturn cameraManager_->cameras().size();\n> >> +\treturn internalCameras_;\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 = cameraIterFromId(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,30 @@ 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 enabled. Iterate any existing external\n> >\n> > s/enabled/set/ ?\n> > s/any/all/\n> >\n> >> +\t * cameras and mark them as CAMERA_DEVICE_STATUS_PRESENT\n> >> +\t * 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 (auto &cam : cameraManager_->cameras()) {\n> >> +\t\tauto iter = camerasMap_.find(cam->id());\n> >> +\t\tif (iter == camerasMap_.end())\n> >> +\t\t\tcontinue;\n> >\n> > How about iterating over cameras_ instead ? You wouldn't need to look up\n> > the id in camerasMap_.\n> >\n> > \tfor (std::shared_ptr<CameraDevice> &camera : cameras_) {\n> > \t\tCamera *cam = camera->camera();\n> > \t\tunsigned int id = camera->id();\n> >\n> > (you could also drop some of the local variables if desired, as they are\n> > both used once only).\n> >\n> >> +\n> >> +\t\tunsigned int id = iter->second;\n> >> +\t\tconst ControlList &properties = cam->properties();\n> >> +\t\tif (properties.contains(properties::Location) &&\n> >> +\t\t    properties.get(properties::Location) &\n> >\n> > The Location property isn't a bitfield, it's an enum.\n> >\n> >> +\t\t    properties::CameraLocationExternal) {\n> >\n> > There are two locations where you check the Location property. Would\n> > adding the following function make sense ?\n> >\n> > static int32_t CameraHalManager::cameraLocation(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> Do you think it should be a 'static' method? I am curious why?\n> Any special reasons?\n\nThe function doesn't access any member of CameraHalManager, so it can be\nstatic. The static keyword should only occur in the .h file though, not\nhere.\n\n> >\n> > The caller would become\n> >\n> > \t\tif (cameraLocation(cam) == properties::CameraLocationExternal)\n> >\n> > which is a bit nicer to read I think.\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\t}\n> >> +\t}\n> >>   }\n> >> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> >> index 3e34d63..f8adade 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 <map>\n> >> +#include <mutex>\n> >>   #include <stddef.h>\n> >>   #include <vector>\n> >>   \n> >> @@ -18,12 +20,17 @@\n> >>   \n> >>   class CameraDevice;\n> >>   \n> >> +using Mutex = std::mutex;\n> >> +using MutexLocker = std::unique_lock<std::mutex>;\n> >> +\n> >>   class CameraHalManager\n> >>   {\n> >>   public:\n> >>   \tCameraHalManager();\n> >>   \t~CameraHalManager();\n> >>   \n> >> +\tusing cameraDeviceIter = std::vector<std::shared_ptr<CameraDevice>>::iterator;\n> >> +\n> >>   \tint init();\n> >>   \n> >>   \tCameraDevice *open(unsigned int id, const hw_module_t *module);\n> >> @@ -33,10 +40,21 @@ public:\n> >>   \tvoid setCallbacks(const camera_module_callbacks_t *callbacks);\n> >>   \n> >>   private:\n> >> +\tvoid cameraAdded(std::shared_ptr<libcamera::Camera> cam);\n> >> +\tvoid cameraRemoved(std::shared_ptr<libcamera::Camera> cam);\n> >> +\n> >> +\tcameraDeviceIter cameraIterFromId(unsigned int id);\n> >> +\tcameraDeviceIter cameraIterFromCamera(libcamera::Camera *cam);\n> >\n> > How about turning these to\n> >\n> > \tCameraDevice *cameraFromId(unsigned int id);\n> > \tCameraDevice *cameraFromCamera(libcamera::Camera *cam);\n> >\n> > It would simplify the interface. I would actually call the former\n> > cameraFromHALId() to differentiate the two types of IDs. And maybe\n> >\n> > \tCameraDevice *cameraDeviceFromHALId(unsigned int id);\n> > \tCameraDevice *cameraDeviecFromCamera(libcamera::Camera *cam);\n> >\n> > if it's not too long ?\n> >\n> >> +\n> >>   \tlibcamera::CameraManager *cameraManager_;\n> >>   \n> >>   \tconst camera_module_callbacks_t *callbacks_;\n> >>   \tstd::vector<std::shared_ptr<CameraDevice>> cameras_;\n> >> +\tstd::map<std::string, unsigned int> camerasMap_;\n> >\n> > Maybe cameraIdsMap_ to make the contents more explicit ?\n> >\n> >> +\tMutex mutex_;\n> >> +\n> >> +\tunsigned int internalCameras_;\n> >\n> > Maybe numInternalCameras_ or internalCamerasCount_ ?\n> >\n> >> +\tunsigned int externalCameras_;\n> >\n> > And nextExternalCameraId_ ?\n> >\n> >>   };\n> >>   \n> >>   #endif /* __ANDROID_CAMERA_MANAGER_H__ */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 609C1BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 22 Aug 2020 01:28:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB63B61FE7;\n\tSat, 22 Aug 2020 03:28:52 +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 7FB5160385\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 22 Aug 2020 03:28:51 +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 96F7929E;\n\tSat, 22 Aug 2020 03:28:50 +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=\"LUPzPmUX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598059730;\n\tbh=uzI3RjKelaQHHPaWXLS/fFzNuXHiD5q/7FfgqhFtzqA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LUPzPmUXScvF4H1C8n0LnsHSUfEQR/UKPzGq9vGfiD7sg4xhhPwjeN9ZqzXjARrCe\n\tqtsMeGgMOImshDYbg2GUSc4jZ6OP1WjJIXFnsub0z9vUgfFrVChIB/DwIBNtd9Z6Ur\n\tG4BScNk1TYKbrqsUDM2JNvx3vMUst96V0ve5Ad8c=","Date":"Sat, 22 Aug 2020 04:28:32 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Umang Jain <email@uajain.com>","Message-ID":"<20200822012832.GL5967@pendragon.ideasonboard.com>","References":"<20200817202629.4277-1-email@uajain.com>\n\t<20200817202629.4277-6-email@uajain.com>\n\t<20200819153358.GO6049@pendragon.ideasonboard.com>\n\t<ccb57844-84a7-4a2e-5499-5ce125afa5b4@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<ccb57844-84a7-4a2e-5499-5ce125afa5b4@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v3 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]