[{"id":11901,"web_url":"https://patchwork.libcamera.org/comment/11901/","msgid":"<20200805213406.GI2712616@oden.dyn.berto.se>","date":"2020-08-05T21:34:06","subject":"Re: [libcamera-devel] [PATCH 2/2] android: camera_hal_manager:\n\tSupport camera hotplug","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Umang,\n\nThanks for your work.\n\nOn 2020-08-05 15:14:44 +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> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.h | 1 +\n> src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++--\n> src/android/camera_hal_manager.h | 3 +++\n> 3 files changed, 42 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 4e5fb98..fa9706c 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -47,6 +47,7 @@ public:\n> \n> \tunsigned int id() const { return id_; }\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> +\tconst libcamera::Camera *getCamera() { return camera_.get(); };\n> \n> \tint facing() const { return facing_; }\n> \tint orientation() const { return orientation_; }\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 3ddcab5..b498278 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -59,8 +59,6 @@ int CameraHalManager::init()\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> @@ -73,6 +71,10 @@ int CameraHalManager::init()\n> \t\t++index;\n> \t}\n> \n> +\t/* Support camera hotplug */\n> +\tcameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> +\tcameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> +\n> \treturn 0;\n> }\n> \n> @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n> \treturn camera;\n> }\n> \n> +void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> +{\n> +\tunsigned int id = cameras_.size();\n\nThis is dangerous.\n\nIf you have two USB cameras A and B and plug them in and out as \nfollowing you will have two cameras with the same numerical id:\n\nNumber of internal cameras: 2 -> id = 0 and id = 1\n\nPlugin A -> id = 2\nPlugin B -> id = 3\nRemove A\nPlugin A -> id = 3\n\nI'm wondering if an ever increasing counter would be a solution here? \nIt's not likely we will see 2^31 (2147483648) devices plugged in over \nthe runtime of the system. And if we fear that we can test that the next \nID in line is indeed free and if not keep incrementing.\n\n> +\tCameraDevice *camera = new CameraDevice(id, cam);\n> +\tint ret = camera->initialize();\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->name();\n> +\t\treturn;\n> +\t}\n> +\n> +\tcameras_.emplace_back(camera);\n> +\tcallbacks_->camera_device_status_change(callbacks_, id,\n> +\t\t\t\t\t\tCAMERA_DEVICE_STATUS_PRESENT);\n> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n> +\t\t\t<< \" added and initialized successfully.\";\n> +}\n> +\n> +void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> +{\n> +\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t\t[cam](std::unique_ptr &camera) {\n> +\t\t\t\t\treturn cam.get() == camera->getCamera();\n> +\t\t\t\t});\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> +\tcameras_.erase(iter);\n> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n> +\t\t\t<< \" removed successfully.\";\n> +}\n> +\n> unsigned int CameraHalManager::numCameras() const\n> {\n> \treturn cameraManager_->cameras().size();\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index 4345b1e..52ab9e2 100644\n> --- a/src/android/camera_hal_manager.h\n> +++ b/src/android/camera_hal_manager.h\n> @@ -33,6 +33,9 @@ public:\n> \tint setCallbacks(const camera_module_callbacks_t *callbacks);\n> \n> private:\n> +\tvoid cameraAdded(std::shared_ptr cam);\n> +\tvoid cameraRemoved(std::shared_ptr cam);\n> +\n> \tlibcamera::CameraManager *cameraManager_;\n> \n> \tconst camera_module_callbacks_t *callbacks_;\n> -- \n> 2.26.2\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 48F2CBD87A\n\tfor ;\n\tWed, 5 Aug 2020 21:34:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDB3D6059F;\n\tWed, 5 Aug 2020 23:34:08 +0200 (CEST)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B22136039D\n\tfor ;\n\tWed, 5 Aug 2020 23:34:07 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id s9so25068642lfs.4\n\tfor ;\n\tWed, 05 Aug 2020 14:34:07 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tz25sm1401399ljz.13.2020.08.05.14.34.06\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 05 Aug 2020 14:34:06 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"0jqWl0ly\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=0NAXaQjUR3aJfYz35fMgkl2qHWwRJsUmzfYCPPcZyPE=;\n\tb=0jqWl0ly9zKjYjOUJRBruqYrbkqGeVgYljkF49pT6YVByfQtJM+PYr5BK4s0rkWFjl\n\tb+4a+Cby4E8yxwhYOtAFAv1JbJ8HDBxZ+SLkixjdOx/IhcyHYpsOp4c+q6/j1xQG5wq6\n\tP9R/SyclLNVB3fhqy3bHBfp+sSmazvtlqSiKC1yzk3ZMvPR9/xbLFpjFLe8ncBYRE1ou\n\tGZfZZwTq5vtaqVJdujvix2J2mL6GhvwwMhoeAUv9RLphcaPsqSftB4h5tmcNW4TmKiAH\n\tCrXFw0/y8XIFqLdBkDqVR0TIIu42/6xTrTl70BspwRATfh9DJ6Acb/9mj3wRht5SatdR\n\tvILQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=0NAXaQjUR3aJfYz35fMgkl2qHWwRJsUmzfYCPPcZyPE=;\n\tb=jH/4+GYlTs43zshSLUlx8bnz6OvFxI4gOSUXXqMY/mv1Dynury2Lkf88vw9i8yWMkR\n\tlSV7+1Dspy1mf4JzI+3oH2pkN/nHUb6VY+BfKxu2dsOLlWGHkJWwc/dhLQhrs7PaKjUw\n\tfBpRksQMBLE6RmOB7mC965xeqmRZr0hQ/YoT5twgc3aiUAeg19IQ9KBGVsRsvfYBYD5P\n\tNlEZJXiICcR3sX7DQAijXG3U57pKY54PRU6ZlJpA0ia5RTcGtMrI3thBOji1z4+0B0pe\n\t3lVAuiu8x/pjSWMODWlHgZtSyUiIAA5y03IK+RRjr7ts56inPtTNU4MhKlDRiL0nPffy\n\tEfpw==","X-Gm-Message-State":"AOAM530T/9/2kccrkN0oGtn7wgvN4OncdxjERGPPQukWGPA4HqwJ7WDv\n\tC6QpzkjF+yh7lgqx2hQHRCHHrw==","X-Google-Smtp-Source":"ABdhPJzyfstOigT9Ufd8hM/cdCmzKhKtO3RKpgpPpkUl3m4U2LCkBp5s9S3VY4K7ZiCBvcyNOquubA==","X-Received":"by 2002:a19:428c:: with SMTP id\n\tp134mr2433134lfa.70.1596663247058; \n\tWed, 05 Aug 2020 14:34:07 -0700 (PDT)","Date":"Wed, 5 Aug 2020 23:34:06 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Umang Jain ","Message-ID":"<20200805213406.GI2712616@oden.dyn.berto.se>","References":"<20200805151437.157155-1-email@uajain.com>\n\t<20200805151437.157155-3-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200805151437.157155-3-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11902,"web_url":"https://patchwork.libcamera.org/comment/11902/","msgid":"<20200805213404.GM6751@pendragon.ideasonboard.com>","date":"2020-08-05T21:34:04","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Wed, Aug 05, 2020 at 03:14:44PM +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> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.h | 1 +\n> src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++--\n> src/android/camera_hal_manager.h | 3 +++\n> 3 files changed, 42 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 4e5fb98..fa9706c 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -47,6 +47,7 @@ public:\n> \n> \tunsigned int id() const { return id_; }\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> +\tconst libcamera::Camera *getCamera() { return camera_.get(); };\n> \n> \tint facing() const { return facing_; }\n> \tint orientation() const { return orientation_; }\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 3ddcab5..b498278 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -59,8 +59,6 @@ int CameraHalManager::init()\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> @@ -73,6 +71,10 @@ int CameraHalManager::init()\n> \t\t++index;\n> \t}\n> \n> +\t/* Support camera hotplug */\n\ns/hotplug/hotplug./\n\n> +\tcameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> +\tcameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n\nThis creates a race condition, as the hotplug (and hotunplug) could\noccur after the camera manager is started and before you connect the\nsignals. I think you should connect the signals before starting the\ncamera manager, and remove the loop above that creates CameraDevice\ninstances. CameraHalManager::cameraAdded will add instances as needed.\n\n> +\n> \treturn 0;\n> }\n> \n> @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n> \treturn camera;\n> }\n\nYou also need to update .get_number_of_cameras() to handle hotplug.\nAccording to its documentation ([1]),\n\n * The value here must be static, and must count only built-in cameras,\n * which have CAMERA_FACING_BACK or CAMERA_FACING_FRONT camera facing values\n * (camera_info.facing). The HAL must not include the external cameras\n * (camera_info.facing == CAMERA_FACING_EXTERNAL) into the return value\n * of this call. Frameworks will use camera_device_status_change callback\n * to manage number of external cameras.\n\nWe need to count the number of internal cameras at initialization time,\nand cache the value. That's fairly easy, except for the fact that USB\ncameras could be either internal or external. There's no way we can\nfigure this out without some sort a platform-specific configuration\ndata. In theory some information could be reported through ACPI, but\nlooking at what my laptop reports there for the internal camera, I think\nmost vendors just provide bogus information.\n\nInternal USB cameras can be ignored for now I believe, we can just\nconsider that all cameras detected when the HAL is started are external.\n.get_number_of_cameras() need to be updated accordingly, and we can then\nwork on better handling of internal USB cameras.\n\nOr, after having thought a bit more about it, maybe we should check the\nLocation property of the Camera instead. That should be an authoritative\nsource of information. The UVC pipeline handler would need to be fixed\nto report the Location (it doesn't at the moment), and we can hardcode\nit to CameraLocationExternal there for now until we get support for\nplatform-specific configuration data. What do you think ?\n\nAlso, the id >= numCameras() in open() and getCameraInfo() needs to be\nreplaced, with checks that lookup the id in the cameras_ map.\n\n> +void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> +{\n> +\tunsigned int id = cameras_.size();\n\nI think we need something a bit more advanced to create camera IDs.\nThe constraints on the camera ID are not well documented. [2] states\nthat\n\n\"Non-removable cameras use integers starting at 0 for their identifiers,\nwhile removable cameras have a unique identifier for each individual\ndevice, even if they are the same model.\"\n\nNote that this is the Java API, so the camera service may perform ID\nmapping, although that would surprise me.\n\nAs the .camera_device_status_change() callback uses an int camera_id, we\nneed integers for external cameras too, unless there's something I'm\nmissing. IDs of different cameras need to be unique, so I think we\nshouldn't reuse the same ID if we unplug a USB camera and plug another\none.\n\nOne way to handle this is to keep a may of Camera::id() (Niklas has just\nmerged the patch series that adds this) to HAL integer IDs, for all\ncameras we see. If a new camera is added and was seen before, we can\nreuse the same HAL ID. Otherwise we assign a new ID (we thus also need a\ncounter of used HAL IDs).\n\nFurthermore, as cameraAdded() would be called for all cameras when we\nstart the camera manager (assuming you agree with my suggestion to do so\nabove), we won't know here, when we encounter an external device, how\nmany internal devices there will be. We thus can't assign the HAL ID for\nexternal devices right away, unless we set a large enough base number to\nmake sure it will always be above the number of internal devices. Even\nif it's a bit of a hack, I'd go in that direction, starting at 1000 for\ninstance.\n\n> +\tCameraDevice *camera = new CameraDevice(id, cam);\n> +\tint ret = camera->initialize();\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->name();\n> +\t\treturn;\n> +\t}\n> +\n> +\tcameras_.emplace_back(camera);\n> +\tcallbacks_->camera_device_status_change(callbacks_, id,\n> +\t\t\t\t\t\tCAMERA_DEVICE_STATUS_PRESENT);\n\nYou first need to check if the callbacks_ are not null, as a camera\ncould be hotplugged before callbacks are set. Access to callbacks_ will\nneed to be protected with a mutex.\n\n> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n> +\t\t\t<< \" added and initialized successfully.\";\n> +}\n> +\n> +void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> +{\n> +\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> +\t\t\t\t[cam](std::unique_ptr &camera) {\n> +\t\t\t\t\treturn cam.get() == camera->getCamera();\n> +\t\t\t\t});\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> +\tcameras_.erase(iter);\n\nThis will delete the CameraDevice instance, which is \"slightly\"\nproblematic if the camera is already open. It also creates race\nconditions with the CameraHalManager::open() function. I wonder if we\nneed to turn cameras_ in a vector of shared pointers to handle this.\n\nI had a look at how the Chrome OS USB HAL implements unplugging when the\ncamera is already opened ([3]):\n\n // TODO(shik): Handle this more gracefully, sometimes it even trigger a kernel\n // panic.\n if (cameras_.find(id) != cameras_.end()) {\n LOGF(WARNING)\n << \"Unplug an opening camera, exit the camera service to cleanup\";\n // Upstart will start the service again.\n _exit(EIO);\n }\n\nI'm not sure what to say :-)\n\nShould we try to refcount CameraDevice with std::shared_ptr<>, and\nprotect access to camera_ with a mutex ?\n\n> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n> +\t\t\t<< \" removed successfully.\";\n> +}\n\n[1] https://android.googlesource.com/platform/hardware/libhardware/+/refs/heads/master/include/hardware/camera_common.h#881\n[2] https://developer.android.com/reference/android/hardware/camera2/CameraManager#getCameraIdList()\n[3] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/camera/hal/usb/camera_hal.cc#498\n\n> +\n> unsigned int CameraHalManager::numCameras() const\n> {\n> \treturn cameraManager_->cameras().size();\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index 4345b1e..52ab9e2 100644\n> --- a/src/android/camera_hal_manager.h\n> +++ b/src/android/camera_hal_manager.h\n> @@ -33,6 +33,9 @@ public:\n> \tint setCallbacks(const camera_module_callbacks_t *callbacks);\n> \n> private:\n> +\tvoid cameraAdded(std::shared_ptr cam);\n> +\tvoid cameraRemoved(std::shared_ptr cam);\n> +\n> \tlibcamera::CameraManager *cameraManager_;\n> \n> \tconst camera_module_callbacks_t *callbacks_;","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 67B0EBD87A\n\tfor ;\n\tWed, 5 Aug 2020 21:34:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 338FF60737;\n\tWed, 5 Aug 2020 23:34:18 +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 44BAA6039D\n\tfor ;\n\tWed, 5 Aug 2020 23:34:17 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B88D2277;\n\tWed, 5 Aug 2020 23: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=\"FdFfn4gY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596663256;\n\tbh=rXisQdfoMhV1gC3CopC5pqM7lTZHcb5CsUiImniU41g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FdFfn4gYZ50aVaJaueHGWJ3bWv0/u1hBIeeR6JlS2H7a+Ymb9Mt914WAwJhOAyhYh\n\tXCJ78+QlGmoLBeOBu7/iR/AVPGp5jP2ahWpfD/8iu6x+gOdyD7sxJZBNAo/S+Y1Crx\n\tMgxfk/yA9IAx1nSe3msa5RIVwAm9cS7jDdHGpV+8=","Date":"Thu, 6 Aug 2020 00:34:04 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200805213404.GM6751@pendragon.ideasonboard.com>","References":"<20200805151437.157155-1-email@uajain.com>\n\t<20200805151437.157155-3-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200805151437.157155-3-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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\" "}},{"id":11906,"web_url":"https://patchwork.libcamera.org/comment/11906/","msgid":"","date":"2020-08-06T08:26:45","subject":"Re: [libcamera-devel] [PATCH 2/2] 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\nThanks for the detailed review.\nI need a few clarifications.\n\nOn 8/6/20 3:04 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Wed, Aug 05, 2020 at 03:14:44PM +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>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.h | 1 +\n>> src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++--\n>> src/android/camera_hal_manager.h | 3 +++\n>> 3 files changed, 42 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 4e5fb98..fa9706c 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -47,6 +47,7 @@ public:\n>> \n>> \tunsigned int id() const { return id_; }\n>> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n>> +\tconst libcamera::Camera *getCamera() { return camera_.get(); };\n>> \n>> \tint facing() const { return facing_; }\n>> \tint orientation() const { return orientation_; }\n>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n>> index 3ddcab5..b498278 100644\n>> --- a/src/android/camera_hal_manager.cpp\n>> +++ b/src/android/camera_hal_manager.cpp\n>> @@ -59,8 +59,6 @@ int CameraHalManager::init()\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>> @@ -73,6 +71,10 @@ int CameraHalManager::init()\n>> \t\t++index;\n>> \t}\n>> \n>> +\t/* Support camera hotplug */\n> s/hotplug/hotplug./\n>\n>> +\tcameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n>> +\tcameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> This creates a race condition, as the hotplug (and hotunplug) could\n> occur after the camera manager is started and before you connect the\n> signals. I think you should connect the signals before starting the\n> camera manager, and remove the loop above that creates CameraDevice\n> instances. CameraHalManager::cameraAdded will add instances as needed.\nI can agree with this suggestion, I don't see it being problematic.\n>\n>> +\n>> \treturn 0;\n>> }\n>> \n>> @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n>> \treturn camera;\n>> }\n> You also need to update .get_number_of_cameras() to handle hotplug.\n> According to its documentation ([1]),\n>\n> * The value here must be static, and must count only built-in cameras,\n> * which have CAMERA_FACING_BACK or CAMERA_FACING_FRONT camera facing values\n> * (camera_info.facing). The HAL must not include the external cameras\n> * (camera_info.facing == CAMERA_FACING_EXTERNAL) into the return value\n> * of this call. Frameworks will use camera_device_status_change callback\n> * to manage number of external cameras.\n>\n> We need to count the number of internal cameras at initialization time,\n> and cache the value. That's fairly easy, except for the fact that USB\n> cameras could be either internal or external. There's no way we can\n> figure this out without some sort a platform-specific configuration\n> data. In theory some information could be reported through ACPI, but\n> looking at what my laptop reports there for the internal camera, I think\n> most vendors just provide bogus information.\n>\n> Internal USB cameras can be ignored for now I believe, we can just\n> consider that all cameras detected when the HAL is started are external.\nDo you mean \"all cameras detected\" or \"all UVC cameras detected\" as \nexternal?\nBecause the next line of explanation confuses me a bit.\n> \n> .get_number_of_cameras() need to be updated accordingly, and we can then\n> work on better handling of internal USB cameras.\nI think you meant \"all UVC cameras\" just above, no?\n>\n> Or, after having thought a bit more about it, maybe we should check the\n> Location property of the Camera instead. That should be an authoritative\n> source of information. The UVC pipeline handler would need to be fixed\n> to report the Location (it doesn't at the moment), and we can hardcode\n> it to CameraLocationExternal there for now until we get support for\n> platform-specific configuration data. What do you think ?\nhmm, Is this Location property can be queried from any existing metadata or,\ndo you mean that we should declare it inside pipeline-handler/camera.\n\nThe way I am understanding things here is, you are suggesting that each \npipeline\nhandler sets the CameraLocationProperty for the camera being created. \nFor e.g.\nif IPU3 is used, that particular camera will be created with \nCameraLocationInternal\nwhereas UVC pipelinehandler will create a camera with \nCameraLocationExternal;\nwhich then can be queried by a convenient Camera API. Is this what are \nyou referring here?\n>\n> Also, the id >= numCameras() in open() and getCameraInfo() needs to be\n> replaced, with checks that lookup the id in the cameras_ map.\n>\n>> +void CameraHalManager::cameraAdded(std::shared_ptr cam)\n>> +{\n>> +\tunsigned int id = cameras_.size();\n> I think we need something a bit more advanced to create camera IDs.\n> The constraints on the camera ID are not well documented. [2] states\n> that\n>\n> \"Non-removable cameras use integers starting at 0 for their identifiers,\n> while removable cameras have a unique identifier for each individual\n> device, even if they are the same model.\"\n>\n> Note that this is the Java API, so the camera service may perform ID\n> mapping, although that would surprise me.\n>\n> As the .camera_device_status_change() callback uses an int camera_id, we\n> need integers for external cameras too, unless there's something I'm\n> missing. IDs of different cameras need to be unique, so I think we\n> shouldn't reuse the same ID if we unplug a USB camera and plug another\n> one.\n>\n> One way to handle this is to keep a may of Camera::id() (Niklas has just\n> merged the patch series that adds this) to HAL integer IDs, for all\n> cameras we see. If a new camera is added and was seen before, we can\n> reuse the same HAL ID. Otherwise we assign a new ID (we thus also need a\n> counter of used HAL IDs).\n>\n> Furthermore, as cameraAdded() would be called for all cameras when we\n> start the camera manager (assuming you agree with my suggestion to do so\n> above), we won't know here, when we encounter an external device, how\n> many internal devices there will be. We thus can't assign the HAL ID for\n> external devices right away, unless we set a large enough base number to\n> make sure it will always be above the number of internal devices. Even\n> if it's a bit of a hack, I'd go in that direction, starting at 1000 for\n> instance.\n>\n>> +\tCameraDevice *camera = new CameraDevice(id, cam);\n>> +\tint ret = camera->initialize();\n>> +\tif (ret) {\n>> +\t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->name();\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tcameras_.emplace_back(camera);\n>> +\tcallbacks_->camera_device_status_change(callbacks_, id,\n>> +\t\t\t\t\t\tCAMERA_DEVICE_STATUS_PRESENT);\n> You first need to check if the callbacks_ are not null, as a camera\n> could be hotplugged before callbacks are set. Access to callbacks_ will\n> need to be protected with a mutex.\n>\n>> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n>> +\t\t\t<< \" added and initialized successfully.\";\n>> +}\n>> +\n>> +void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n>> +{\n>> +\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n>> +\t\t\t\t[cam](std::unique_ptr &camera) {\n>> +\t\t\t\t\treturn cam.get() == camera->getCamera();\n>> +\t\t\t\t});\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>> +\tcameras_.erase(iter);\n> This will delete the CameraDevice instance, which is \"slightly\"\n> problematic if the camera is already open. It also creates race\n> conditions with the CameraHalManager::open() function. I wonder if we\n> need to turn cameras_ in a vector of shared pointers to handle this.\n>\n> I had a look at how the Chrome OS USB HAL implements unplugging when the\n> camera is already opened ([3]):\n>\n> // TODO(shik): Handle this more gracefully, sometimes it even trigger a kernel\n> // panic.\n> if (cameras_.find(id) != cameras_.end()) {\n> LOGF(WARNING)\n> << \"Unplug an opening camera, exit the camera service to cleanup\";\n> // Upstart will start the service again.\n> _exit(EIO);\n> }\n>\n> I'm not sure what to say :-)\n>\n> Should we try to refcount CameraDevice with std::shared_ptr<>, and\n> protect access to camera_ with a mutex ?\nCan you explain a bit, why would we need to protect access to camera_ \nwith a mutex?\n>\n>> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n>> +\t\t\t<< \" removed successfully.\";\n>> +}\n> [1] https://android.googlesource.com/platform/hardware/libhardware/+/refs/heads/master/include/hardware/camera_common.h#881\n> [2] https://developer.android.com/reference/android/hardware/camera2/CameraManager#getCameraIdList()\n> [3] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/camera/hal/usb/camera_hal.cc#498\n>\n>> +\n>> unsigned int CameraHalManager::numCameras() const\n>> {\n>> \treturn cameraManager_->cameras().size();\n>> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n>> index 4345b1e..52ab9e2 100644\n>> --- a/src/android/camera_hal_manager.h\n>> +++ b/src/android/camera_hal_manager.h\n>> @@ -33,6 +33,9 @@ public:\n>> \tint setCallbacks(const camera_module_callbacks_t *callbacks);\n>> \n>> private:\n>> +\tvoid cameraAdded(std::shared_ptr cam);\n>> +\tvoid cameraRemoved(std::shared_ptr cam);\n>> +\n>> \tlibcamera::CameraManager *cameraManager_;\n>> \n>> \tconst camera_module_callbacks_t *callbacks_;","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 79E41BD86F\n\tfor ;\n\tThu, 6 Aug 2020 08:26:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08D036071B;\n\tThu, 6 Aug 2020 10:26:50 +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 8253060390\n\tfor ;\n\tThu, 6 Aug 2020 10:26:47 +0200 (CEST)","by filterdrecv-p3las1-559bd7b968-v8dwq with SMTP id\n\tfilterdrecv-p3las1-559bd7b968-v8dwq-16-5F2BBEC5-8\n\t2020-08-06 08:26:45.215290609 +0000 UTC m=+655829.090459646","from mail.uajain.com (unknown)\n\tby ismtpd0003p1hnd1.sendgrid.net (SG) with ESMTP\n\tid dhs3jnzqTp-hm8ao3nzrwg Thu, 06 Aug 2020 08:26:44.695 +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=\"UXT2t3+C\"; 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=tlvpPLfRDje1djbYvnF9PJUaZni4peooeQrydTtAHa0=;\n\tb=UXT2t3+CO+u583tFbiNlU8dvRlES8x5lejzmdbC8mtaWds7+UrV68PKvwYUW+qvXVDbO\n\t912Y3yTM9xTLqpFFyjMxzhU7p16cMnnzS4UzSgQTAXSWYJCuFhll6hlDG45s5HYfbAxq/b\n\thDo3vk8kefaYDuTKC9NNQ5t2GnkwFHMYQ=","References":"<20200805151437.157155-1-email@uajain.com>\n\t<20200805151437.157155-3-email@uajain.com>\n\t<20200805213404.GM6751@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Thu, 06 Aug 2020 08:26:45 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200805213404.GM6751@pendragon.ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcO0wIVNwfic3Culi7Uik3l9n+qaT60RdkhXqGVryKPV9ltB2+mLcHspo6VNNbk3IXtMsE+MhyHLLHQxe3LViwpALSD7kZ0eQxN0NkYs2MpWG28gwJCGZmlxQyfIFkzvFxfQUIsT6ksXUGoPro4uNsNFDqg+3iOp7MWhdHEy+za2wk/yU8nrVuGLUHHl0me1fCrcYODTsBbTZ8dkMiXfdM1w==","To":"Laurent Pinchart ","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11940,"web_url":"https://patchwork.libcamera.org/comment/11940/","msgid":"<20200807044655.GB6127@pendragon.ideasonboard.com>","date":"2020-08-07T04:46:55","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Thu, Aug 06, 2020 at 08:26:45AM +0000, Umang Jain wrote:\n> Hi Laurent,\n> \n> Thanks for the detailed review.\n> I need a few clarifications.\n> \n> On 8/6/20 3:04 AM, Laurent Pinchart wrote:\n> > On Wed, Aug 05, 2020 at 03:14:44PM +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> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.h | 1 +\n> >> src/android/camera_hal_manager.cpp | 40 ++++++++++++++++++++++++++++--\n> >> src/android/camera_hal_manager.h | 3 +++\n> >> 3 files changed, 42 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index 4e5fb98..fa9706c 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -47,6 +47,7 @@ public:\n> >> \n> >> \tunsigned int id() const { return id_; }\n> >> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> >> +\tconst libcamera::Camera *getCamera() { return camera_.get(); };\n> >> \n> >> \tint facing() const { return facing_; }\n> >> \tint orientation() const { return orientation_; }\n> >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> >> index 3ddcab5..b498278 100644\n> >> --- a/src/android/camera_hal_manager.cpp\n> >> +++ b/src/android/camera_hal_manager.cpp\n> >> @@ -59,8 +59,6 @@ int CameraHalManager::init()\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> >> @@ -73,6 +71,10 @@ int CameraHalManager::init()\n> >> \t\t++index;\n> >> \t}\n> >> \n> >> +\t/* Support camera hotplug */\n> >\n> > s/hotplug/hotplug./\n> >\n> >> +\tcameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> >> +\tcameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> >\n> > This creates a race condition, as the hotplug (and hotunplug) could\n> > occur after the camera manager is started and before you connect the\n> > signals. I think you should connect the signals before starting the\n> > camera manager, and remove the loop above that creates CameraDevice\n> > instances. CameraHalManager::cameraAdded will add instances as needed.\n>\n> I can agree with this suggestion, I don't see it being problematic.\n>\n> >> +\n> >> \treturn 0;\n> >> }\n> >> \n> >> @@ -93,6 +95,40 @@ CameraDevice *CameraHalManager::open(unsigned int id,\n> >> \treturn camera;\n> >> }\n> >\n> > You also need to update .get_number_of_cameras() to handle hotplug.\n> > According to its documentation ([1]),\n> >\n> > * The value here must be static, and must count only built-in cameras,\n> > * which have CAMERA_FACING_BACK or CAMERA_FACING_FRONT camera facing values\n> > * (camera_info.facing). The HAL must not include the external cameras\n> > * (camera_info.facing == CAMERA_FACING_EXTERNAL) into the return value\n> > * of this call. Frameworks will use camera_device_status_change callback\n> > * to manage number of external cameras.\n> >\n> > We need to count the number of internal cameras at initialization time,\n> > and cache the value. That's fairly easy, except for the fact that USB\n> > cameras could be either internal or external. There's no way we can\n> > figure this out without some sort a platform-specific configuration\n> > data. In theory some information could be reported through ACPI, but\n> > looking at what my laptop reports there for the internal camera, I think\n> > most vendors just provide bogus information.\n> >\n> > Internal USB cameras can be ignored for now I believe, we can just\n> > consider that all cameras detected when the HAL is started are external.\n>\n> Do you mean \"all cameras detected\" or \"all UVC cameras detected\" as \n> external?\n> Because the next line of explanation confuses me a bit.\n\nRe-reading my text, I'm not sure what I meant :-) I may have mixed \"all\nUVC cameras are external\" and \"all cameras detected when the HAL is\nstarted are internal\". I'd go for the latter, as we have no way to know\nhere if a camera is UVC or not. And doesn't matter much though, if we\nuse the Location properly this concern will go away.\n\n> > .get_number_of_cameras() need to be updated accordingly, and we can then\n> > work on better handling of internal USB cameras.\n>\n> I think you meant \"all UVC cameras\" just above, no?\n>\n> > Or, after having thought a bit more about it, maybe we should check the\n> > Location property of the Camera instead. That should be an authoritative\n> > source of information. The UVC pipeline handler would need to be fixed\n> > to report the Location (it doesn't at the moment), and we can hardcode\n> > it to CameraLocationExternal there for now until we get support for\n> > platform-specific configuration data. What do you think ?\n>\n> hmm, Is this Location property can be queried from any existing metadata or,\n> do you mean that we should declare it inside pipeline-handler/camera.\n\nThe Location property is reported through Camera::properties(). Pipeline\nhandlers need to set it.\n\n> The way I am understanding things here is, you are suggesting that each pipeline\n> handler sets the CameraLocationProperty for the camera being created. For e.g.\n> if IPU3 is used, that particular camera will be created with CameraLocationInternal\n> whereas UVC pipelinehandler will create a camera with CameraLocationExternal;\n\nThe supported values are CameraLocationFront, CameraLocationBack and\nCameraLocationExternal (see property_ids.yaml).\n\nThe property is currently created by the CameraSensor class. The UVC\npipeline handler doesn't use CameraSensor and thus needs to add the\nproperty manually.\n\n> which then can be queried by a convenient Camera API. Is this what are \n> you referring here?\n\nCorrect.\n\n> > Also, the id >= numCameras() in open() and getCameraInfo() needs to be\n> > replaced, with checks that lookup the id in the cameras_ map.\n> >\n> >> +void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> >> +{\n> >> +\tunsigned int id = cameras_.size();\n> >\n> > I think we need something a bit more advanced to create camera IDs.\n> > The constraints on the camera ID are not well documented. [2] states\n> > that\n> >\n> > \"Non-removable cameras use integers starting at 0 for their identifiers,\n> > while removable cameras have a unique identifier for each individual\n> > device, even if they are the same model.\"\n> >\n> > Note that this is the Java API, so the camera service may perform ID\n> > mapping, although that would surprise me.\n> >\n> > As the .camera_device_status_change() callback uses an int camera_id, we\n> > need integers for external cameras too, unless there's something I'm\n> > missing. IDs of different cameras need to be unique, so I think we\n> > shouldn't reuse the same ID if we unplug a USB camera and plug another\n> > one.\n> >\n> > One way to handle this is to keep a may of Camera::id() (Niklas has just\n> > merged the patch series that adds this) to HAL integer IDs, for all\n> > cameras we see. If a new camera is added and was seen before, we can\n> > reuse the same HAL ID. Otherwise we assign a new ID (we thus also need a\n> > counter of used HAL IDs).\n> >\n> > Furthermore, as cameraAdded() would be called for all cameras when we\n> > start the camera manager (assuming you agree with my suggestion to do so\n> > above), we won't know here, when we encounter an external device, how\n> > many internal devices there will be. We thus can't assign the HAL ID for\n> > external devices right away, unless we set a large enough base number to\n> > make sure it will always be above the number of internal devices. Even\n> > if it's a bit of a hack, I'd go in that direction, starting at 1000 for\n> > instance.\n> >\n> >> +\tCameraDevice *camera = new CameraDevice(id, cam);\n> >> +\tint ret = camera->initialize();\n> >> +\tif (ret) {\n> >> +\t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->name();\n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tcameras_.emplace_back(camera);\n> >> +\tcallbacks_->camera_device_status_change(callbacks_, id,\n> >> +\t\t\t\t\t\tCAMERA_DEVICE_STATUS_PRESENT);\n> >\n> > You first need to check if the callbacks_ are not null, as a camera\n> > could be hotplugged before callbacks are set. Access to callbacks_ will\n> > need to be protected with a mutex.\n> >\n> >> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n> >> +\t\t\t<< \" added and initialized successfully.\";\n> >> +}\n> >> +\n> >> +void CameraHalManager::cameraRemoved(std::shared_ptr cam)\n> >> +{\n> >> +\tauto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> >> +\t\t\t\t[cam](std::unique_ptr &camera) {\n> >> +\t\t\t\t\treturn cam.get() == camera->getCamera();\n> >> +\t\t\t\t});\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> >> +\tcameras_.erase(iter);\n> >\n> > This will delete the CameraDevice instance, which is \"slightly\"\n> > problematic if the camera is already open. It also creates race\n> > conditions with the CameraHalManager::open() function. I wonder if we\n> > need to turn cameras_ in a vector of shared pointers to handle this.\n> >\n> > I had a look at how the Chrome OS USB HAL implements unplugging when the\n> > camera is already opened ([3]):\n> >\n> > // TODO(shik): Handle this more gracefully, sometimes it even trigger a kernel\n> > // panic.\n> > if (cameras_.find(id) != cameras_.end()) {\n> > LOGF(WARNING)\n> > << \"Unplug an opening camera, exit the camera service to cleanup\";\n> > // Upstart will start the service again.\n> > _exit(EIO);\n> > }\n> >\n> > I'm not sure what to say :-)\n> >\n> > Should we try to refcount CameraDevice with std::shared_ptr<>, and\n> > protect access to camera_ with a mutex ?\n>\n> Can you explain a bit, why would we need to protect access to camera_ \n> with a mutex?\n\nI meant cameras_, not camera_, sorry. cameras_ can be accessed from\nmultiple threads (all the HAL operations run in an external thread, and\nthe cameraAdded and cameraRemoved signals are emitted from the internal\nCameraManager thread). It thus needs to be protected against race\nconditions.\n\n> >> +\tLOG(HAL, Debug) << \"Camera \" << cam->name() << \" ID:\" << id\n> >> +\t\t\t<< \" removed successfully.\";\n> >> +}\n> >\n> > [1] https://android.googlesource.com/platform/hardware/libhardware/+/refs/heads/master/include/hardware/camera_common.h#881\n> > [2] https://developer.android.com/reference/android/hardware/camera2/CameraManager#getCameraIdList()\n> > [3] https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/master/camera/hal/usb/camera_hal.cc#498\n> >\n> >> +\n> >> unsigned int CameraHalManager::numCameras() const\n> >> {\n> >> \treturn cameraManager_->cameras().size();\n> >> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> >> index 4345b1e..52ab9e2 100644\n> >> --- a/src/android/camera_hal_manager.h\n> >> +++ b/src/android/camera_hal_manager.h\n> >> @@ -33,6 +33,9 @@ public:\n> >> \tint setCallbacks(const camera_module_callbacks_t *callbacks);\n> >> \n> >> private:\n> >> +\tvoid cameraAdded(std::shared_ptr cam);\n> >> +\tvoid cameraRemoved(std::shared_ptr cam);\n> >> +\n> >> \tlibcamera::CameraManager *cameraManager_;\n> >> \n> >> \tconst camera_module_callbacks_t *callbacks_;","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 744E9BD87A\n\tfor ;\n\tFri, 7 Aug 2020 04:47:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC44D60DDE;\n\tFri, 7 Aug 2020 06:47:10 +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 97E566038C\n\tfor ;\n\tFri, 7 Aug 2020 06:47:08 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EF289267;\n\tFri, 7 Aug 2020 06:47:07 +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=\"DIVz1bxI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596775628;\n\tbh=1TmdoGbN2xO7ltgI8Z5g84rEhRYJ7irdSkXrPhcK/lc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DIVz1bxIvEDTPS/ljOJoHdPCjfNqxfBn0pcNTyEfgJmZ0OtNZn9m8flVSzzZZ5imW\n\t60x3sI5sFK/m40IuWPKuZ2EVDG68ayN42+u3+2hQd+3+zuaXLN0yCPZEEqzuDFooIZ\n\t/tt3LrO9m8BQNxaWjQR5r3Il1gYy2DHfzhNkXKa8=","Date":"Fri, 7 Aug 2020 07:46:55 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200807044655.GB6127@pendragon.ideasonboard.com>","References":"<20200805151437.157155-1-email@uajain.com>\n\t<20200805151437.157155-3-email@uajain.com>\n\t<20200805213404.GM6751@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] 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\" "}}]