[{"id":18486,"web_url":"https://patchwork.libcamera.org/comment/18486/","msgid":"<20210802084640.GV2167@pyrite.rasen.tech>","date":"2021-08-02T08:46:40","subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in HAL config","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote:\n> Currently, all UVC cameras are reported with CameraLocationExternal [1]\n> by libcamera-core, since there is no universal information or standard,\n\ns/,$//\n\n> to know the location of these cameras. However, in the libcamera HAL\n> layer, we can make an informed decision whether its external or\n\ns/its/it's/\n\n> internal, simply by checking the presence of it in the HAL\n\ns/the presence of it/its presence/\n\n> configuration file.\n> \n> If the camera is found to be present in the HAL configuration file,\n\ns/to be present//\n\n> treat it as internal. CameraHalManager will now assign the numerical id\n> of the camera accordingly, based on which the facing of the camera is set\n\nI didn't think the facing of the camera affects the id?\n\n> as well (as per the HAL config file) while initializing the CameraDevice\n> wrapper.\n\nI don't quite understand this last part.\n\n\nThanks,\n\nPaul\n\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 | 13 ++++++++++++-\n> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++\n> 2 files changed, 29 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 678cde23..394ebc84 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -330,7 +330,18 @@ 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 reported as external, but\n> +\t\t\t * the CameraHalManager has overriden it, use\n> +\t\t\t * what is reported in the configuration file.\n> +\t\t\t * This typically happens for UVC cameras\n> +\t\t\t * reported as 'External' by libcamera but\n> +\t\t\t * installed in fixed position on the device.\n> +\t\t\t */\n> +\t\t\tif (id_ < 1000 && cameraConfigData->facing != -1)\n> +\t\t\t facing_ = cameraConfigData->facing;\n> +\t\t\telse\n> +\t\t\t facing_ = 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 b364f62a..cce98fad 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t}\n> \n> \tconst CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());\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 are integrated in a laptop. In that case the real location\n> +\t * should be specified in the configuration file.\n> +\t *\n> +\t * If the camera location is external and a configuration\n> +\t * entry exists for it, override its location.\n> +\t */\n> +\tif (isCameraNew && isCameraExternal) {\n> +\t\tif (cameraConfigData && cameraConfigData->facing != -1) {\n> +\t\t\tisCameraExternal = false;\n> +\t\t\tid = numInternalCameras_;\n> +\t\t}\n> +\t}\n> +\n> \tif (!isCameraExternal && !cameraConfigData) {\n> \t\tLOG(HAL, Error)\n> \t\t\t<< \"HAL configuration entry for internal camera \"\n> -- \n> 2.31.0\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 5C37DC3232\n\tfor ;\n\tMon, 2 Aug 2021 08:46:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 05551687C3;\n\tMon, 2 Aug 2021 10:46:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDE056026E\n\tfor ;\n\tMon, 2 Aug 2021 10:46:48 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42A494A3;\n\tMon, 2 Aug 2021 10:46:46 +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=\"IN0YXj39\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627894008;\n\tbh=VHoWkAmbX5VuMxEMvsh/CrMTdGG0IRkWwMAl9vB0Kjk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IN0YXj39f2VT8BaD5JSc9ezxbtD6XEEadt/blC4ezrkKoTeCE1SmN0eAEdx/OYtDX\n\tJgtgVEIDjDogtdTwEcNTOgx6zYRcjW3rgb5kRwtLOpsnxdfDKJ7AujCAtiYRjisraa\n\tAjlzcH9ErLwEoOBM4vV96J1VPPEncPxrbjEdIG7o=","Date":"Mon, 2 Aug 2021 17:46:40 +0900","From":"paul.elder@ideasonboard.com","To":"Umang Jain ","Message-ID":"<20210802084640.GV2167@pyrite.rasen.tech>","References":"<20210730110154.181370-1-umang.jain@ideasonboard.com>\n\t<20210730110154.181370-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210730110154.181370-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in 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":18488,"web_url":"https://patchwork.libcamera.org/comment/18488/","msgid":"","date":"2021-08-02T09:51:14","subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in HAL config","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 8/2/21 2:16 PM, paul.elder@ideasonboard.com wrote:\n> Hi Umang,\n>\n> On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote:\n>> Currently, all UVC cameras are reported with CameraLocationExternal [1]\n>> by libcamera-core, since there is no universal information or standard,\n> s/,$//\n>\n>> to know the location of these cameras. However, in the libcamera HAL\n>> layer, we can make an informed decision whether its external or\n> s/its/it's/\n>\n>> internal, simply by checking the presence of it in the HAL\n> s/the presence of it/its presence/\n>\n>> configuration file.\n>>\n>> If the camera is found to be present in the HAL configuration file,\n> s/to be present//\n>\n>> treat it as internal. CameraHalManager will now assign the numerical id\n>> of the camera accordingly, based on which the facing of the camera is set\n> I didn't think the facing of the camera affects the id?\n\nWell, it should be kept in sync. If the camera is internal (id < 1000), \nbut it's facing is set to be external (as per checks in CameraDevice), \nthen these are not in sync. We need to take care of that.\n\nI don't think anything bad will happen if these are out of sync. But why \nshould we allow it to be out-of-sync if we handle it gracefully?\n\n>\n>> as well (as per the HAL config file) while initializing the CameraDevice\n>> wrapper.\n> I don't quite understand this last part.\n\nSo in the  CameraDevice::initialize(), the facing of the camera is set. \nIf properties::Location doesn't match the location in the HAL config, \nCameraDevice complains a warning.\n\nBut, we know that for UVC properties:::Location won't be matching the \nHAL config. In that case we rely on id assigned by CameraManager to see \nif its a  internal camera or not. If yes, we just assign facing whatever \nis mentioned in the HAL config file :)\n>\n>\n> Thanks,\n>\n> Paul\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 | 13 ++++++++++++-\n>> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++\n>> 2 files changed, 29 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 678cde23..394ebc84 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -330,7 +330,18 @@ 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 reported as external, but\n>> +\t\t\t * the CameraHalManager has overriden it, use\n>> +\t\t\t * what is reported in the configuration file.\n>> +\t\t\t * This typically happens for UVC cameras\n>> +\t\t\t * reported as 'External' by libcamera but\n>> +\t\t\t * installed in fixed position on the device.\n>> +\t\t\t */\n>> +\t\t\tif (id_ < 1000 && cameraConfigData->facing != -1)\n>> +\t\t\t facing_ = cameraConfigData->facing;\n>> +\t\t\telse\n>> +\t\t\t facing_ = 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 b364f62a..cce98fad 100644\n>> --- a/src/android/camera_hal_manager.cpp\n>> +++ b/src/android/camera_hal_manager.cpp\n>> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n>> \t}\n>> \n>> \tconst CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());\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 are integrated in a laptop. In that case the real location\n>> +\t * should be specified in the configuration file.\n>> +\t *\n>> +\t * If the camera location is external and a configuration\n>> +\t * entry exists for it, override its location.\n>> +\t */\n>> +\tif (isCameraNew && isCameraExternal) {\n>> +\t\tif (cameraConfigData && cameraConfigData->facing != -1) {\n>> +\t\t\tisCameraExternal = false;\n>> +\t\t\tid = numInternalCameras_;\n>> +\t\t}\n>> +\t}\n>> +\n>> \tif (!isCameraExternal && !cameraConfigData) {\n>> \t\tLOG(HAL, Error)\n>> \t\t\t<< \"HAL configuration entry for internal camera \"\n>> -- \n>> 2.31.0\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 8A63BC3232\n\tfor ;\n\tMon, 2 Aug 2021 09:51:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB00C687C4;\n\tMon, 2 Aug 2021 11:51:21 +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 65A58687B6\n\tfor ;\n\tMon, 2 Aug 2021 11:51:20 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.227])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 627EA4A3;\n\tMon, 2 Aug 2021 11:51:19 +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=\"hy3ElRGq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627897880;\n\tbh=yyd9FzudRSWdyPgWLWgU4h2WsdXtBNnaFFgyrMDKl1w=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=hy3ElRGqb7VHRq9w9Dy2DNEBnIiMq8nYB3qNGkhErmqxOkycW+uJ7VKrM+ZYFGzGR\n\teT1a6FOZAsajdydziiSBZWaqXd3pArsqo0GqPWBCEOAo5xI562iNM/i3VDAoEGTPhk\n\t6QAhPGXc8AZEXgaNSKY/lDxgmt4QSa/WmxaVnWpM=","To":"paul.elder@ideasonboard.com","References":"<20210730110154.181370-1-umang.jain@ideasonboard.com>\n\t<20210730110154.181370-4-umang.jain@ideasonboard.com>\n\t<20210802084640.GV2167@pyrite.rasen.tech>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 2 Aug 2021 15:21:14 +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":"<20210802084640.GV2167@pyrite.rasen.tech>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in 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":18494,"web_url":"https://patchwork.libcamera.org/comment/18494/","msgid":"<20210802165818.ltgm7f3jicrtwjez@uno.localdomain>","date":"2021-08-02T16:58:18","subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in HAL config","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote:\n> Currently, all UVC cameras are reported with CameraLocationExternal [1]\n> by libcamera-core, since there is no universal information or standard,\n> to know the location of these cameras. However, in the libcamera HAL\n> layer, we can make an informed decision whether its external or\n> internal, simply by checking the presence of it in the HAL\n> configuration file.\n>\n> If the camera is found to be present in the HAL configuration file,\n> treat it as internal. CameraHalManager will now assign the numerical id\n> of the camera accordingly, based on which the facing of the camera is set\n> as well (as per the HAL config file) while initializing the CameraDevice\n> wrapper.\n>\n> [1] 76809320bb1a (\"libcamera: pipeline: uvcvideo: Treat all UVC cameras\n> as external\")\n>\n> Signed-off-by: Umang Jain \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/android/camera_device.cpp | 13 ++++++++++++-\n> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++\n> 2 files changed, 29 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 678cde23..394ebc84 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -330,7 +330,18 @@ 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 reported as external, but\n> +\t\t\t * the CameraHalManager has overriden it, use\n> +\t\t\t * what is reported in the configuration file.\n> +\t\t\t * This typically happens for UVC cameras\n> +\t\t\t * reported as 'External' by libcamera but\n> +\t\t\t * installed in fixed position on the device.\n> +\t\t\t */\n> +\t\t\tif (id_ < 1000 && cameraConfigData->facing != -1)\n> +\t\t\t facing_ = cameraConfigData->facing;\n> +\t\t\telse\n> +\t\t\t facing_ = 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 b364f62a..cce98fad 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t}\n>\n> \tconst CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());\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 are integrated in a laptop. In that case the real location\n> +\t * should be specified in the configuration file.\n> +\t *\n> +\t * If the camera location is external and a configuration\n> +\t * entry exists for it, override its location.\n> +\t */\n> +\tif (isCameraNew && isCameraExternal) {\n> +\t\tif (cameraConfigData && cameraConfigData->facing != -1) {\n> +\t\t\tisCameraExternal = false;\n> +\t\t\tid = numInternalCameras_;\n> +\t\t}\n> +\t}\n> +\n> \tif (!isCameraExternal && !cameraConfigData) {\n> \t\tLOG(HAL, Error)\n> \t\t\t<< \"HAL configuration entry for internal camera \"\n> --\n> 2.31.0\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 E8A39C3232\n\tfor ;\n\tMon, 2 Aug 2021 16:57:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6522E687CC;\n\tMon, 2 Aug 2021 18:57:33 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B8CC6026F\n\tfor ;\n\tMon, 2 Aug 2021 18:57:32 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id CCCEF240006;\n\tMon, 2 Aug 2021 16:57:31 +0000 (UTC)"],"Date":"Mon, 2 Aug 2021 18:58:18 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210802165818.ltgm7f3jicrtwjez@uno.localdomain>","References":"<20210730110154.181370-1-umang.jain@ideasonboard.com>\n\t<20210730110154.181370-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210730110154.181370-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in 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":18495,"web_url":"https://patchwork.libcamera.org/comment/18495/","msgid":"","date":"2021-08-02T17:24:45","subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in 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 Mon, Aug 02, 2021 at 03:21:14PM +0530, Umang Jain wrote:\n> On 8/2/21 2:16 PM, paul.elder@ideasonboard.com wrote:\n> > On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote:\n> >> Currently, all UVC cameras are reported with CameraLocationExternal [1]\n> >> by libcamera-core, since there is no universal information or standard,\n> >\n> > s/,$//\n> >\n> >> to know the location of these cameras. However, in the libcamera HAL\n> >> layer, we can make an informed decision whether its external or\n> >\n> > s/its/it's/\n> >\n> >> internal, simply by checking the presence of it in the HAL\n> >\n> > s/the presence of it/its presence/\n> >\n> >> configuration file.\n> >>\n> >> If the camera is found to be present in the HAL configuration file,\n> >\n> > s/to be present//\n> >\n> >> treat it as internal. CameraHalManager will now assign the numerical id\n> >> of the camera accordingly, based on which the facing of the camera is set\n> >\n> > I didn't think the facing of the camera affects the id?\n> \n> Well, it should be kept in sync. If the camera is internal (id < 1000), \n> but it's facing is set to be external (as per checks in CameraDevice), \n> then these are not in sync. We need to take care of that.\n> \n> I don't think anything bad will happen if these are out of sync. But why \n> should we allow it to be out-of-sync if we handle it gracefully?\n\nI agree, it's best to keep them in sync.\n\n> >> as well (as per the HAL config file) while initializing the CameraDevice\n> >> wrapper.\n> >\n> > I don't quite understand this last part.\n>\n> So in the  CameraDevice::initialize(), the facing of the camera is set. \n> If properties::Location doesn't match the location in the HAL config, \n> CameraDevice complains a warning.\n> \n> But, we know that for UVC properties:::Location won't be matching the \n> HAL config. In that case we rely on id assigned by CameraManager to see \n> if its a  internal camera or not. If yes, we just assign facing whatever \n> is mentioned in the HAL config file :)\n\nThe last sentence of the commit message can probably be clarified a bit.\nHow about this ?\n\n\"The CameraHalManager will now assign the numerical id of the camera\naccordingly when initializing the CameraDevice, based on the camera\nfacing value set in the HAL config file.\"\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 | 13 ++++++++++++-\n> >> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++\n> >> 2 files changed, 29 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 678cde23..394ebc84 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -330,7 +330,18 @@ 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 reported as external, but\n> >> +\t\t\t * the CameraHalManager has overriden it, use\n> >> +\t\t\t * what is reported in the configuration file.\n> >> +\t\t\t * This typically happens for UVC cameras\n> >> +\t\t\t * reported as 'External' by libcamera but\n> >> +\t\t\t * installed in fixed position on the device.\n> >> +\t\t\t */\n\nThis could be reflowed to 80 columns.\n\n> >> +\t\t\tif (id_ < 1000 && cameraConfigData->facing != -1)\n\nIs the id_ check needed ? If libcamera reports the camera as external\nand the camera data overrides the facing value, the id will always be <\n1000, won't it ?\n\n> >> +\t\t\t facing_ = cameraConfigData->facing;\n> >> +\t\t\telse\n> >> +\t\t\t facing_ = 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 b364f62a..cce98fad 100644\n> >> --- a/src/android/camera_hal_manager.cpp\n> >> +++ b/src/android/camera_hal_manager.cpp\n> >> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> >> \t}\n> >> \n> >> \tconst CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());\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 are integrated in a laptop. In that case the real location\n> >> +\t * should be specified in the configuration file.\n> >> +\t *\n> >> +\t * If the camera location is external and a configuration\n> >> +\t * entry exists for it, override its location.\n\nYou could also reflow this to 80 columns.\n\nReviewed-by: Laurent Pinchart \n\n> >> +\t */\n> >> +\tif (isCameraNew && isCameraExternal) {\n> >> +\t\tif (cameraConfigData && cameraConfigData->facing != -1) {\n> >> +\t\t\tisCameraExternal = false;\n> >> +\t\t\tid = numInternalCameras_;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> \tif (!isCameraExternal && !cameraConfigData) {\n> >> \t\tLOG(HAL, Error)\n> >> \t\t\t<< \"HAL configuration entry for internal camera \"","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 38666C3235\n\tfor ;\n\tMon, 2 Aug 2021 17:25:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC2DA687CC;\n\tMon, 2 Aug 2021 19:24:59 +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 D4A3C6026F\n\tfor ;\n\tMon, 2 Aug 2021 19:24:57 +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 3B1839FB;\n\tMon, 2 Aug 2021 19:24:57 +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=\"XS06FlKE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627925097;\n\tbh=aQNrHwp4QiBwZARVM1dOoFjXZ9WcxFqwH21rdm/L150=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XS06FlKERSipyQpUnVzivjMQ7JJjT3l/6A9t47mLTfEoyi3z2a9DRTd+IiwINBvzB\n\tW9i/zflVhsyGY/RIcz9ycYRYYrm+gO84N9S7hws3tdYFX1dcwV0HYGSWWM5tru9+Z/\n\t6jhzFHSKp56RKF6ToseXdCyfOjtIB6fADUHaUYHE=","Date":"Mon, 2 Aug 2021 20:24:45 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210730110154.181370-1-umang.jain@ideasonboard.com>\n\t<20210730110154.181370-4-umang.jain@ideasonboard.com>\n\t<20210802084640.GV2167@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in 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":18498,"web_url":"https://patchwork.libcamera.org/comment/18498/","msgid":"<076fe745-de52-116b-cb8c-2eedafe63490@ideasonboard.com>","date":"2021-08-03T03:32:01","subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in 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 8/2/21 10:54 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 02, 2021 at 03:21:14PM +0530, Umang Jain wrote:\n>> On 8/2/21 2:16 PM, paul.elder@ideasonboard.com wrote:\n>>> On Fri, Jul 30, 2021 at 04:31:53PM +0530, Umang Jain wrote:\n>>>> Currently, all UVC cameras are reported with CameraLocationExternal [1]\n>>>> by libcamera-core, since there is no universal information or standard,\n>>> s/,$//\n>>>\n>>>> to know the location of these cameras. However, in the libcamera HAL\n>>>> layer, we can make an informed decision whether its external or\n>>> s/its/it's/\n>>>\n>>>> internal, simply by checking the presence of it in the HAL\n>>> s/the presence of it/its presence/\n>>>\n>>>> configuration file.\n>>>>\n>>>> If the camera is found to be present in the HAL configuration file,\n>>> s/to be present//\n>>>\n>>>> treat it as internal. CameraHalManager will now assign the numerical id\n>>>> of the camera accordingly, based on which the facing of the camera is set\n>>> I didn't think the facing of the camera affects the id?\n>> Well, it should be kept in sync. If the camera is internal (id < 1000),\n>> but it's facing is set to be external (as per checks in CameraDevice),\n>> then these are not in sync. We need to take care of that.\n>>\n>> I don't think anything bad will happen if these are out of sync. But why\n>> should we allow it to be out-of-sync if we handle it gracefully?\n> I agree, it's best to keep them in sync.\n>\n>>>> as well (as per the HAL config file) while initializing the CameraDevice\n>>>> wrapper.\n>>> I don't quite understand this last part.\n>> So in the  CameraDevice::initialize(), the facing of the camera is set.\n>> If properties::Location doesn't match the location in the HAL config,\n>> CameraDevice complains a warning.\n>>\n>> But, we know that for UVC properties:::Location won't be matching the\n>> HAL config. In that case we rely on id assigned by CameraManager to see\n>> if its a  internal camera or not. If yes, we just assign facing whatever\n>> is mentioned in the HAL config file :)\n> The last sentence of the commit message can probably be clarified a bit.\n> How about this ?\n>\n> \"The CameraHalManager will now assign the numerical id of the camera\n> accordingly when initializing the CameraDevice, based on the camera\n> facing value set in the HAL config file.\"\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 | 13 ++++++++++++-\n>>>> src/android/camera_hal_manager.cpp | 17 +++++++++++++++++\n>>>> 2 files changed, 29 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 678cde23..394ebc84 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -330,7 +330,18 @@ 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 reported as external, but\n>>>> +\t\t\t * the CameraHalManager has overriden it, use\n>>>> +\t\t\t * what is reported in the configuration file.\n>>>> +\t\t\t * This typically happens for UVC cameras\n>>>> +\t\t\t * reported as 'External' by libcamera but\n>>>> +\t\t\t * installed in fixed position on the device.\n>>>> +\t\t\t */\n> This could be reflowed to 80 columns.\n>\n>>>> +\t\t\tif (id_ < 1000 && cameraConfigData->facing != -1)\n> Is the id_ check needed ? If libcamera reports the camera as external\n> and the camera data overrides the facing value, the id will always be <\n> 1000, won't it ?\n\nAh yes, we can drop the id_ check (although it helps to read the code \ntbh) and instead have\n\n+\t\t\tif (cameraConfigData && cameraConfigData->facing != -1)\n\nShould be fine as well.\n>\n>>>> +\t\t\t facing_ = cameraConfigData->facing;\n>>>> +\t\t\telse\n>>>> +\t\t\t facing_ = 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 b364f62a..cce98fad 100644\n>>>> --- a/src/android/camera_hal_manager.cpp\n>>>> +++ b/src/android/camera_hal_manager.cpp\n>>>> @@ -145,6 +145,23 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n>>>> \t}\n>>>> \n>>>> \tconst CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());\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 are integrated in a laptop. In that case the real location\n>>>> +\t * should be specified in the configuration file.\n>>>> +\t *\n>>>> +\t * If the camera location is external and a configuration\n>>>> +\t * entry exists for it, override its location.\n> You could also reflow this to 80 columns.\n>\n> Reviewed-by: Laurent Pinchart \n>\n>>>> +\t */\n>>>> +\tif (isCameraNew && isCameraExternal) {\n>>>> +\t\tif (cameraConfigData && cameraConfigData->facing != -1) {\n>>>> +\t\t\tisCameraExternal = false;\n>>>> +\t\t\tid = numInternalCameras_;\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> \tif (!isCameraExternal && !cameraConfigData) {\n>>>> \t\tLOG(HAL, Error)\n>>>> \t\t\t<< \"HAL configuration entry for internal camera \"","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 302F3C3232\n\tfor ;\n\tTue, 3 Aug 2021 03:32:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 950BD687CF;\n\tTue, 3 Aug 2021 05:32: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 9FAEC60269\n\tfor ;\n\tTue, 3 Aug 2021 05:32:06 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.12])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7781C4A3;\n\tTue, 3 Aug 2021 05:32:05 +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=\"oYENXmZT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627961526;\n\tbh=q7rhOeK+jMIdljaOILtfn2Zh1DaGqG3+1FW4R9CkiBg=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=oYENXmZTUEx2wr9aq8vdCb1Scu3kExnon9G6hpY3XIPKzgbJ//FxQ9w+jsY5RNHY+\n\tOemWYtH4MPS6Dy5qpxcRcNno7kC3KFdv2SJ4nAWlX3Sol/u0cYRBleglaWsahfW1wL\n\tSrQEUcpSHeP9HZ/CIUK62lYcXa0J7TgPJvfvDmnA=","To":"Laurent Pinchart ","References":"<20210730110154.181370-1-umang.jain@ideasonboard.com>\n\t<20210730110154.181370-4-umang.jain@ideasonboard.com>\n\t<20210802084640.GV2167@pyrite.rasen.tech>\n\t\n\t","From":"Umang Jain ","Message-ID":"<076fe745-de52-116b-cb8c-2eedafe63490@ideasonboard.com>","Date":"Tue, 3 Aug 2021 09:02:01 +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":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 3/4] android: Override camera as\n\t\"Internal\" provided found in 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\" "}}]