[{"id":18413,"web_url":"https://patchwork.libcamera.org/comment/18413/","msgid":"<20210729075231.kkvr6k5iw4heu5mq@uno.localdomain>","date":"2021-07-29T07:52:31","subject":"Re: [libcamera-devel] [PATCH v2 2/3] android: Override camera is\n\t\"Internal\" provided if found on 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 Wed, Jul 28, 2021 at 01:07:59PM +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 on 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> ---\n> src/android/camera_device.cpp | 10 +++++++++-\n> src/android/camera_hal_manager.cpp | 18 ++++++++++++++++++\n> 2 files changed, 27 insertions(+), 1 deletion(-)\n>\nOh my, the logic to handle location is the most complex part of the\nHAL :)\n\nI think the code is fine, I would swap comments though.\n\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 678cde23..c7f5fc4e 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\nor at least mention the UVC camera used as internal cameras here.\n\n\t\t\t/*\n\t\t\t * If the camera is reported as external, but\n * the CameraHalManager has overriden it, use\n * what is reported in the configuration file.\n * This typically happens for UVC cameras\n * reported as 'External' by libcamera but\n * installed in fixed position on the device.\n */\n\n\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..4950bd75 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -145,6 +145,24 @@ 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 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\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. In that case the real\n * location should be specified in the configuration file.\n *\n * If the camera location is external and a configuration\n * entry exists for it, override its location.\n\t */\n\nJust suggestions, take whatever you consider appropriate.\n\n> +\tif (isCameraNew && isCameraExternal) {\n> +\t\tif (cameraConfigData && cameraConfigData->facing != -1 &&\n> +\t\t cameraConfigData->facing != CAMERA_FACING_EXTERNAL) {\n\nActually I don't think location external should be specified for\ncameras listed in the config file. We should probably prohibit it in\nthe configuration file parser, something we allow at the moment. It\ncould be done on top and the check removed here.\n\nCode is good!\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> +\t\t\tisCameraExternal = false;\n> +\t\t\tid = numInternalCameras_;\n> +\t\t}\n> +\t}\n> +\n\n\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 AD31BC3230\n\tfor ;\n\tThu, 29 Jul 2021 07:51:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B664687C2;\n\tThu, 29 Jul 2021 09:51:46 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 176AD68536\n\tfor ;\n\tThu, 29 Jul 2021 09:51:45 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 87FA41BF205;\n\tThu, 29 Jul 2021 07:51:44 +0000 (UTC)"],"Date":"Thu, 29 Jul 2021 09:52:31 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210729075231.kkvr6k5iw4heu5mq@uno.localdomain>","References":"<20210728073800.93745-1-umang.jain@ideasonboard.com>\n\t<20210728073800.93745-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210728073800.93745-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] 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\" "}}]