[{"id":18382,"web_url":"https://patchwork.libcamera.org/comment/18382/","msgid":"","date":"2021-07-27T15:27:09","subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","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 Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote:\n> Currently, all UVC cameras are marked with CameraLocationExternal [1]\n> property, since there is no universal information or standard, to\n> know the location of these cameras. However, in the libcamera HAL layer,\n> we can make an informed decision whether its external or internal,\n> simply by checking the presence of it, in the HAL configuration file.\n> \n> If the camera is found to be present on the HAL configuration file,\n> treat it as internal and assign its numerical id accordingly.\n> \n> [1] 76809320bb1a (\"libcamera: pipeline: uvcvideo: Treat all UVC cameras\n> as external\")\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 10 +++++++++-\n> src/android/camera_hal_manager.cpp | 14 ++++++++++++++\n> 2 files changed, 23 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 678cde23..ba4e2d15 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> \t\t\tfacing_ = CAMERA_FACING_BACK;\n> \t\t\tbreak;\n> \t\tcase properties::CameraLocationExternal:\n> -\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n> +\t\t\t/*\n> +\t\t\t * If the camera is 'internal' as found by\n> +\t\t\t * CameraHalManager, use its location from\n> +\t\t\t * HAL config file.\n> +\t\t\t */\n> +\t\t\tif (id_ < 1000 && cameraConfigData)\n> +\t\t\t\tfacing_ = cameraConfigData->facing;\n> +\t\t\telse\n> +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n> \t\t\tbreak;\n> \t\t}\n> \n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 4cd67544..1a9b3413 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t\t}\n> \t}\n> \n> +\t/*\n> +\t * In some cases, particularly in UVC cameras, the camera location is defaulted\n> +\t * to 'External'. However, if the HAL config file mentions the camera in question,\n> +\t * it shall mean that the camera is integrated to the system so, override to treat\n> +\t * it correctly as a 'internal' camera.\n\nCould you please wrap this at 80 columns ?\n\nI wouldn't say \"defaulted\", but instead talk about what the libcamera\ncore does. Maybe something similar to the following.\n\n\t/*\n\t * Some cameras whose location is reported by libcamera as external may\n\t * actually be internal to the device. This is common with UVC cameras\n\t * that can be integrated in a laptop, but are all considered by\n\t * libcamera as external. The true location for those cameras is\n\t * specified in the HAL configuration file. If the camera location is\n\t * external and a configuration entry exists for it, override the\n\t * location.\n\t */\n\n> +\t */\n> +\tif (isCameraNew && isCameraExternal && halConfig_.exists()) {\n\nIs the isCameraNew check correct ? Even if the camera has been seen\nbefore, we should override isCameraExternal. The id, however, should\nonly be overridden when the camera is new.\n\n> +\t\tconst CameraConfigData *configData = halConfig_.cameraConfigData(cam->id());\n\nIt would be nice to avoid the double lookup of camera config data if\npossible. The CameraDevice::create() call could be moved further down,\njust before camera->initialize(), which may give us an opportunity to\nrefactor the code.\n\n> +\t\tif (configData && configData->facing != CAMERA_FACING_EXTERNAL) {\n> +\t\t\tisCameraExternal = false;\n> +\t\t\tid = numInternalCameras_;\n> +\t\t}\n> +\t}\n> +\n> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> \tstd::unique_ptr camera = CameraDevice::create(id, cam);\n>","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 788B4C322C\n\tfor ;\n\tTue, 27 Jul 2021 15:27:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5A78687C4;\n\tTue, 27 Jul 2021 17:27:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67AF660272\n\tfor ;\n\tTue, 27 Jul 2021 17:27:15 +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 DDDB8EE;\n\tTue, 27 Jul 2021 17:27:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QNCjRlNE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627399635;\n\tbh=R/CEUBP6yxdzcCX/qPW70kt0qhULVsL1eVtddDwYpTc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QNCjRlNEwaC+jD60e8G4XPXG+h6O1C8SYE0CIYAqr2d0/4s/+eWadczBjF4iHuOsh\n\toysTfNTNUZiqAZrFa9Sgx0LxvXGVr12QP0cq+2li2IRijVunZH75nbq58x7OnQx6T3\n\tPczvVK6xZX4FIKU0/yJQA9aRPKhdqHeBih3T6wps=","Date":"Tue, 27 Jul 2021 18:27:09 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210727135440.53640-1-umang.jain@ideasonboard.com>\n\t<20210727135440.53640-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210727135440.53640-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18385,"web_url":"https://patchwork.libcamera.org/comment/18385/","msgid":"","date":"2021-07-27T15:39:04","subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 27/07/2021 16:27, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote:\n>> Currently, all UVC cameras are marked with CameraLocationExternal [1]\n>> property, since there is no universal information or standard, to\n>> know the location of these cameras. However, in the libcamera HAL layer,\n>> we can make an informed decision whether its external or internal,\n>> simply by checking the presence of it, in the HAL configuration file.\n>>\n>> If the camera is found to be present on the HAL configuration file,\n>> treat it as internal and assign its numerical id accordingly.\n>>\n>> [1] 76809320bb1a (\"libcamera: pipeline: uvcvideo: Treat all UVC cameras\n>> as external\")\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 10 +++++++++-\n>> src/android/camera_hal_manager.cpp | 14 ++++++++++++++\n>> 2 files changed, 23 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 678cde23..ba4e2d15 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n>> \t\t\tfacing_ = CAMERA_FACING_BACK;\n>> \t\t\tbreak;\n>> \t\tcase properties::CameraLocationExternal:\n>> -\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n>> +\t\t\t/*\n>> +\t\t\t * If the camera is 'internal' as found by\n>> +\t\t\t * CameraHalManager, use its location from\n>> +\t\t\t * HAL config file.\n>> +\t\t\t */\n>> +\t\t\tif (id_ < 1000 && cameraConfigData)\n\nIs this to 'prevent' setting it to cameraConfigData->facing in the event\nthat id >= 1000?\n\nWill that work? won't it get set regardless in the lines after this case\nstatement? :\n\n>\tif (cameraConfigData && cameraConfigData->facing != -1 &&\n>\t facing_ != cameraConfigData->facing) {\n>\t\tLOG(HAL, Warning)\n>\t\t\t<< \"Camera location does not match\"\n>\t\t\t<< \" configuration file. Using \" << facing_;\n>\t}\n\n\nAlso - I see that the cameraConfigData->facing is never used without\nfirst checking that it is not set to -1, which your lines do not check\nfor ... I don't know if this is a critical requirement or not.\n\n\n\n\n>> +\t\t\t\tfacing_ = cameraConfigData->facing;\n>> +\t\t\telse\n>> +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n>> \t\t\tbreak;\n>> \t\t}\n>> \n>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n>> index 4cd67544..1a9b3413 100644\n>> --- a/src/android/camera_hal_manager.cpp\n>> +++ b/src/android/camera_hal_manager.cpp\n>> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n>> \t\t}\n>> \t}\n>> \n>> +\t/*\n>> +\t * In some cases, particularly in UVC cameras, the camera location is defaulted\n>> +\t * to 'External'. However, if the HAL config file mentions the camera in question,\n>> +\t * it shall mean that the camera is integrated to the system so, override to treat\n>> +\t * it correctly as a 'internal' camera.\n> \n> Could you please wrap this at 80 columns ?\n> \n> I wouldn't say \"defaulted\", but instead talk about what the libcamera\n> core does. Maybe something similar to the following.\n> \n> \t/*\n> \t * Some cameras whose location is reported by libcamera as external may\n> \t * actually be internal to the device. This is common with UVC cameras\n> \t * that can be integrated in a laptop, but are all considered by\n> \t * libcamera as external. The true location for those cameras is\n> \t * specified in the HAL configuration file. If the camera location is\n> \t * external and a configuration entry exists for it, override the\n> \t * location.\n> \t */\n> \n>> +\t */\n>> +\tif (isCameraNew && isCameraExternal && halConfig_.exists()) {\n> \n> Is the isCameraNew check correct ? Even if the camera has been seen\n> before, we should override isCameraExternal. The id, however, should\n> only be overridden when the camera is new.\n> \n>> +\t\tconst CameraConfigData *configData = halConfig_.cameraConfigData(cam->id());\n> \n> It would be nice to avoid the double lookup of camera config data if\n> possible. The CameraDevice::create() call could be moved further down,\n> just before camera->initialize(), which may give us an opportunity to\n> refactor the code.\n> \n>> +\t\tif (configData && configData->facing != CAMERA_FACING_EXTERNAL) {\n>> +\t\t\tisCameraExternal = false;\n>> +\t\t\tid = numInternalCameras_;\n>> +\t\t}\n>> +\t}\n>> +\n>> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n>> \tstd::unique_ptr camera = CameraDevice::create(id, cam);\n>> \n>","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 7B4FEC0109\n\tfor ;\n\tTue, 27 Jul 2021 15:39:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E63E9687C4;\n\tTue, 27 Jul 2021 17:39:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E2B760272\n\tfor ;\n\tTue, 27 Jul 2021 17:39:07 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 83208EE;\n\tTue, 27 Jul 2021 17:39:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rbwZQ6tl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627400346;\n\tbh=TvGjaC8ozMja0JukVbKLfJyAgUfN92rALbYZzVqw/Uo=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=rbwZQ6tlmVFtAv6Hk0oiwes+oScO5JruudtapqZ/EFORuJfCptyd9c7rnP+Dn50P5\n\t4lROCg99WOyX4S/IaC/6Q2m3GH9Tc+87FtKGMo0MwXB1x/zH9KfaMbDMMHQjwIUnt9\n\tEd9jfGAZg1cBWp0EPCeWxLFMujXarXTNLUrPgDP8=","To":"Laurent Pinchart ,\n\tUmang Jain ","References":"<20210727135440.53640-1-umang.jain@ideasonboard.com>\n\t<20210727135440.53640-2-umang.jain@ideasonboard.com>\n\t","From":"Kieran Bingham ","Message-ID":"","Date":"Tue, 27 Jul 2021 16:39:04 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18387,"web_url":"https://patchwork.libcamera.org/comment/18387/","msgid":"<32281839-c424-37c8-171f-bcdb8d9b5172@ideasonboard.com>","date":"2021-07-27T17:48:58","subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 7/27/21 8:57 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote:\n>> Currently, all UVC cameras are marked with CameraLocationExternal [1]\n>> property, since there is no universal information or standard, to\n>> know the location of these cameras. However, in the libcamera HAL layer,\n>> we can make an informed decision whether its external or internal,\n>> simply by checking the presence of it, in the HAL configuration file.\n>>\n>> If the camera is found to be present on the HAL configuration file,\n>> treat it as internal and assign its numerical id accordingly.\n>>\n>> [1] 76809320bb1a (\"libcamera: pipeline: uvcvideo: Treat all UVC cameras\n>> as external\")\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 10 +++++++++-\n>> src/android/camera_hal_manager.cpp | 14 ++++++++++++++\n>> 2 files changed, 23 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 678cde23..ba4e2d15 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n>> \t\t\tfacing_ = CAMERA_FACING_BACK;\n>> \t\t\tbreak;\n>> \t\tcase properties::CameraLocationExternal:\n>> -\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n>> +\t\t\t/*\n>> +\t\t\t * If the camera is 'internal' as found by\n>> +\t\t\t * CameraHalManager, use its location from\n>> +\t\t\t * HAL config file.\n>> +\t\t\t */\n>> +\t\t\tif (id_ < 1000 && cameraConfigData)\n>> +\t\t\t\tfacing_ = cameraConfigData->facing;\n>> +\t\t\telse\n>> +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n>> \t\t\tbreak;\n>> \t\t}\n>> \n>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n>> index 4cd67544..1a9b3413 100644\n>> --- a/src/android/camera_hal_manager.cpp\n>> +++ b/src/android/camera_hal_manager.cpp\n>> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n>> \t\t}\n>> \t}\n>> \n>> +\t/*\n>> +\t * In some cases, particularly in UVC cameras, the camera location is defaulted\n>> +\t * to 'External'. However, if the HAL config file mentions the camera in question,\n>> +\t * it shall mean that the camera is integrated to the system so, override to treat\n>> +\t * it correctly as a 'internal' camera.\n> Could you please wrap this at 80 columns ?\noops!\n>\n> I wouldn't say \"defaulted\", but instead talk about what the libcamera\n> core does. Maybe something similar to the following.\n>\n> \t/*\n> \t * Some cameras whose location is reported by libcamera as external may\n> \t * actually be internal to the device. This is common with UVC cameras\n> \t * that can be integrated in a laptop, but are all considered by\n> \t * libcamera as external. The true location for those cameras is\n> \t * specified in the HAL configuration file. If the camera location is\n> \t * external and a configuration entry exists for it, override the\n> \t * location.\n> \t */\n>\n>> +\t */\n>> +\tif (isCameraNew && isCameraExternal && halConfig_.exists()) {\n> Is the isCameraNew check correct ? Even if the camera has been seen\n> before, we should override isCameraExternal. The id, however, should\n> only be overridden when the camera is new.\n\nI didn't understand the part ... \"if camera has been seen before, we \nshould override isCameraExternal\" part.\n\nI think this check holds. If the internal UVC camera is seen before, it \nwill be re-assigned same numeric-id (which, as per this patch, will be < \n1000). Hence, I do not think we need to over-ride anything here, because \nCameraHalManager already knows that it is an internal camera (based on \nprevious-id re-assign at the start of the function). Am I missing \nsomething?\n\n>\n>> +\t\tconst CameraConfigData *configData = halConfig_.cameraConfigData(cam->id());\n> It would be nice to avoid the double lookup of camera config data if\n> possible. The CameraDevice::create() call could be moved further down,\n> just before camera->initialize(), which may give us an opportunity to\n> refactor the code.\nYeah makes sense, I'll see how can I refactor a bit to avoid double-lookup.\n>\n>> +\t\tif (configData && configData->facing != CAMERA_FACING_EXTERNAL) {\n>> +\t\t\tisCameraExternal = false;\n>> +\t\t\tid = numInternalCameras_;\n>> +\t\t}\n>> +\t}\n>> +\n>> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n>> \tstd::unique_ptr camera = CameraDevice::create(id, cam);\n>>","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 40F10C322D\n\tfor ;\n\tTue, 27 Jul 2021 17:49:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90746687C4;\n\tTue, 27 Jul 2021 19:49:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC97460272\n\tfor ;\n\tTue, 27 Jul 2021 19:49:03 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 04988EE;\n\tTue, 27 Jul 2021 19:49:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OPfU/+O8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627408143;\n\tbh=xRttIzUIAyAZM4qsKWlHshzZVg40hR/8flEFxgWYF9w=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=OPfU/+O8S8Mo8TtqsS5be9JbUl/xNPyGiLg6L+CMRviZz07wirioySy5bBbh8cwOg\n\tZlp8pGh3jSTKWfBh7X6Xu85CBNe32L9gET+GMOI3wyTybav13/73wv10d+qENzhW8D\n\tIS0gEgmwSQOM4s7/rxyO3sOGNVSAR8c5nl1X8jD0=","To":"Laurent Pinchart ","References":"<20210727135440.53640-1-umang.jain@ideasonboard.com>\n\t<20210727135440.53640-2-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<32281839-c424-37c8-171f-bcdb8d9b5172@ideasonboard.com>","Date":"Tue, 27 Jul 2021 23:18:58 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18389,"web_url":"https://patchwork.libcamera.org/comment/18389/","msgid":"","date":"2021-07-27T18:01:36","subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran?\n\nOn 7/27/21 9:09 PM, Kieran Bingham wrote:\n> On 27/07/2021 16:27, Laurent Pinchart wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote:\n>>> Currently, all UVC cameras are marked with CameraLocationExternal [1]\n>>> property, since there is no universal information or standard, to\n>>> know the location of these cameras. However, in the libcamera HAL layer,\n>>> we can make an informed decision whether its external or internal,\n>>> simply by checking the presence of it, in the HAL configuration file.\n>>>\n>>> If the camera is found to be present on the HAL configuration file,\n>>> treat it as internal and assign its numerical id accordingly.\n>>>\n>>> [1] 76809320bb1a (\"libcamera: pipeline: uvcvideo: Treat all UVC cameras\n>>> as external\")\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> src/android/camera_device.cpp | 10 +++++++++-\n>>> src/android/camera_hal_manager.cpp | 14 ++++++++++++++\n>>> 2 files changed, 23 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index 678cde23..ba4e2d15 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n>>> \t\t\tfacing_ = CAMERA_FACING_BACK;\n>>> \t\t\tbreak;\n>>> \t\tcase properties::CameraLocationExternal:\n>>> -\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n>>> +\t\t\t/*\n>>> +\t\t\t * If the camera is 'internal' as found by\n>>> +\t\t\t * CameraHalManager, use its location from\n>>> +\t\t\t * HAL config file.\n>>> +\t\t\t */\n>>> +\t\t\tif (id_ < 1000 && cameraConfigData)\n> Is this to 'prevent' setting it to cameraConfigData->facing in the event\n> that id >= 1000?\n\nNot sure, if I understand it correctly, but this is the check for the \nsituation where libcamera-core reports camera's properties::Location as \nCameraLocationExternal. See 76809320bb1a. So, before setting the \nfacing_, we just check whether the CameraHalManager has classified the \ncamera as internal(id < 1000) or external(remove-able, id >= 1000) to \nthe system. If it's internal, we grab/set the facing_ from the hal \nconfig file.\n\nDoes it make sense?\n\n>\n> Will that work? won't it get set regardless in the lines after this case\n> statement? :\nwhich lines? The switch-case block is inside a if block, followed by \nelse if / else.\n>\n>> \tif (cameraConfigData && cameraConfigData->facing != -1 &&\n>> \t facing_ != cameraConfigData->facing) {\n>> \t\tLOG(HAL, Warning)\n>> \t\t\t<< \"Camera location does not match\"\n>> \t\t\t<< \" configuration file. Using \" << facing_;\n>> \t}\n>\n> Also - I see that the cameraConfigData->facing is never used without\n> first checking that it is not set to -1, which your lines do not check\n> for ... I don't know if this is a critical requirement or not.\n>\n>\n>\n>\n>>> +\t\t\t\tfacing_ = cameraConfigData->facing;\n>>> +\t\t\telse\n>>> +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n>>> \t\t\tbreak;\n>>> \t\t}\n>>> \n>>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n>>> index 4cd67544..1a9b3413 100644\n>>> --- a/src/android/camera_hal_manager.cpp\n>>> +++ b/src/android/camera_hal_manager.cpp\n>>> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n>>> \t\t}\n>>> \t}\n>>> \n>>> +\t/*\n>>> +\t * In some cases, particularly in UVC cameras, the camera location is defaulted\n>>> +\t * to 'External'. However, if the HAL config file mentions the camera in question,\n>>> +\t * it shall mean that the camera is integrated to the system so, override to treat\n>>> +\t * it correctly as a 'internal' camera.\n>> Could you please wrap this at 80 columns ?\n>>\n>> I wouldn't say \"defaulted\", but instead talk about what the libcamera\n>> core does. Maybe something similar to the following.\n>>\n>> \t/*\n>> \t * Some cameras whose location is reported by libcamera as external may\n>> \t * actually be internal to the device. This is common with UVC cameras\n>> \t * that can be integrated in a laptop, but are all considered by\n>> \t * libcamera as external. The true location for those cameras is\n>> \t * specified in the HAL configuration file. If the camera location is\n>> \t * external and a configuration entry exists for it, override the\n>> \t * location.\n>> \t */\n>>\n>>> +\t */\n>>> +\tif (isCameraNew && isCameraExternal && halConfig_.exists()) {\n>> Is the isCameraNew check correct ? Even if the camera has been seen\n>> before, we should override isCameraExternal. The id, however, should\n>> only be overridden when the camera is new.\n>>\n>>> +\t\tconst CameraConfigData *configData = halConfig_.cameraConfigData(cam->id());\n>> It would be nice to avoid the double lookup of camera config data if\n>> possible. The CameraDevice::create() call could be moved further down,\n>> just before camera->initialize(), which may give us an opportunity to\n>> refactor the code.\n>>\n>>> +\t\tif (configData && configData->facing != CAMERA_FACING_EXTERNAL) {\n>>> +\t\t\tisCameraExternal = false;\n>>> +\t\t\tid = numInternalCameras_;\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n>>> \tstd::unique_ptr camera = CameraDevice::create(id, cam);\n>>>","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 DEB74C322E\n\tfor ;\n\tTue, 27 Jul 2021 18:01:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5663C687C4;\n\tTue, 27 Jul 2021 20:01:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DC89D60272\n\tfor ;\n\tTue, 27 Jul 2021 20:01:42 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90201EE;\n\tTue, 27 Jul 2021 20:01:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tY4epY8A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627408902;\n\tbh=1G+e1sd3N8DBymsui2LkkYbpxS0hy7zmKMZCRZ0OoeA=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=tY4epY8AEydXgR+T1BSkpOsDg6dh3iTxI3YU9dPJDYZ67M+sC4l4h3GECNXpPTrJy\n\tcip8j+7aRF9NVPsWPgMDVlVY880TxFLoEyc2C0qmDV6jCwj0Lpp8Z2cuYjkhut1SAI\n\tMk5CCM9fL/5s+LtyghQhxHIWcTUn6+sJ0kSSzb/U=","To":"Kieran Bingham ,\n\tLaurent Pinchart ","References":"<20210727135440.53640-1-umang.jain@ideasonboard.com>\n\t<20210727135440.53640-2-umang.jain@ideasonboard.com>\n\t\n\t","From":"Umang Jain ","Message-ID":"","Date":"Tue, 27 Jul 2021 23:31:36 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18390,"web_url":"https://patchwork.libcamera.org/comment/18390/","msgid":"","date":"2021-07-27T18:04:01","subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jul 27, 2021 at 11:18:58PM +0530, Umang Jain wrote:\n> On 7/27/21 8:57 PM, Laurent Pinchart wrote:\n> > On Tue, Jul 27, 2021 at 07:24:39PM +0530, Umang Jain wrote:\n> >> Currently, all UVC cameras are marked with CameraLocationExternal [1]\n> >> property, since there is no universal information or standard, to\n> >> know the location of these cameras. However, in the libcamera HAL layer,\n> >> we can make an informed decision whether its external or internal,\n> >> simply by checking the presence of it, in the HAL configuration file.\n> >>\n> >> If the camera is found to be present on the HAL configuration file,\n> >> treat it as internal and assign its numerical id accordingly.\n> >>\n> >> [1] 76809320bb1a (\"libcamera: pipeline: uvcvideo: Treat all UVC cameras\n> >> as external\")\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/camera_device.cpp | 10 +++++++++-\n> >> src/android/camera_hal_manager.cpp | 14 ++++++++++++++\n> >> 2 files changed, 23 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 678cde23..ba4e2d15 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -330,7 +330,15 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)\n> >> \t\t\tfacing_ = CAMERA_FACING_BACK;\n> >> \t\t\tbreak;\n> >> \t\tcase properties::CameraLocationExternal:\n> >> -\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n> >> +\t\t\t/*\n> >> +\t\t\t * If the camera is 'internal' as found by\n> >> +\t\t\t * CameraHalManager, use its location from\n> >> +\t\t\t * HAL config file.\n> >> +\t\t\t */\n> >> +\t\t\tif (id_ < 1000 && cameraConfigData)\n> >> +\t\t\t\tfacing_ = cameraConfigData->facing;\n> >> +\t\t\telse\n> >> +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n> >> \t\t\tbreak;\n> >> \t\t}\n> >> \n> >> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> >> index 4cd67544..1a9b3413 100644\n> >> --- a/src/android/camera_hal_manager.cpp\n> >> +++ b/src/android/camera_hal_manager.cpp\n> >> @@ -133,6 +133,20 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> >> \t\t}\n> >> \t}\n> >> \n> >> +\t/*\n> >> +\t * In some cases, particularly in UVC cameras, the camera location is defaulted\n> >> +\t * to 'External'. However, if the HAL config file mentions the camera in question,\n> >> +\t * it shall mean that the camera is integrated to the system so, override to treat\n> >> +\t * it correctly as a 'internal' camera.\n> >\n> > Could you please wrap this at 80 columns ?\n>\n> oops!\n>\n> > I wouldn't say \"defaulted\", but instead talk about what the libcamera\n> > core does. Maybe something similar to the following.\n> >\n> > \t/*\n> > \t * Some cameras whose location is reported by libcamera as external may\n> > \t * actually be internal to the device. This is common with UVC cameras\n> > \t * that can be integrated in a laptop, but are all considered by\n> > \t * libcamera as external. The true location for those cameras is\n> > \t * specified in the HAL configuration file. If the camera location is\n> > \t * external and a configuration entry exists for it, override the\n> > \t * location.\n> > \t */\n> >\n> >> +\t */\n> >> +\tif (isCameraNew && isCameraExternal && halConfig_.exists()) {\n> >\n> > Is the isCameraNew check correct ? Even if the camera has been seen\n> > before, we should override isCameraExternal. The id, however, should\n> > only be overridden when the camera is new.\n> \n> I didn't understand the part ... \"if camera has been seen before, we \n> should override isCameraExternal\" part.\n>\n> I think this check holds. If the internal UVC camera is seen before, it \n> will be re-assigned same numeric-id (which, as per this patch, will be < \n> 1000). Hence, I do not think we need to over-ride anything here, because \n> CameraHalManager already knows that it is an internal camera (based on \n> previous-id re-assign at the start of the function). Am I missing \n> something?\n\nYou're right, isCameraExternal is only used when isCameraNew == true.\n\n> >> +\t\tconst CameraConfigData *configData = halConfig_.cameraConfigData(cam->id());\n> >\n> > It would be nice to avoid the double lookup of camera config data if\n> > possible. The CameraDevice::create() call could be moved further down,\n> > just before camera->initialize(), which may give us an opportunity to\n> > refactor the code.\n>\n> Yeah makes sense, I'll see how can I refactor a bit to avoid double-lookup.\n>\n> >> +\t\tif (configData && configData->facing != CAMERA_FACING_EXTERNAL) {\n> >> +\t\t\tisCameraExternal = false;\n> >> +\t\t\tid = numInternalCameras_;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> >> \tstd::unique_ptr camera = CameraDevice::create(id, cam);\n> >>","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 7FDC5C322E\n\tfor ;\n\tTue, 27 Jul 2021 18:04:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD055687C4;\n\tTue, 27 Jul 2021 20:04:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4348560272\n\tfor ;\n\tTue, 27 Jul 2021 20:04:08 +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 B7075EE;\n\tTue, 27 Jul 2021 20:04:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IWgw+pT/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627409047;\n\tbh=ihvyrhSWqABvSMbs4BENlG2WnmwXw0DCAHKMTNSO+2Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IWgw+pT/dPwP1oUw9gsG80KhUoCsJWtplbVKYH/loPXE3mfhfFeWMfzWI9WVAVFfg\n\t6lDpWFOy+jls6oLkpgExBytlhhanANE4ldHLNKAvlRLhV+lWnmtoi8fqylOMZ/27LB\n\tf0WlDT0yo7nu8gDYrCki8zNl0VAnqmf/MbcvW6EQ=","Date":"Tue, 27 Jul 2021 21:04:01 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210727135440.53640-1-umang.jain@ideasonboard.com>\n\t<20210727135440.53640-2-umang.jain@ideasonboard.com>\n\t\n\t<32281839-c424-37c8-171f-bcdb8d9b5172@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<32281839-c424-37c8-171f-bcdb8d9b5172@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: Override camera is\n\t\"Internal\" provided if found on HAL config","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]