[{"id":15443,"web_url":"https://patchwork.libcamera.org/comment/15443/","msgid":"<20210303133401.5fkrlfgnknnf35li@uno.localdomain>","date":"2021-03-03T13:34:01","subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Wed, Mar 03, 2021 at 06:37:56PM +0900, Paul Elder wrote:\n> In the static metadata, we claim that we don't support external cameras,\n> yet libcamera core defaults the camera location to external. In the HAL\n> CameraDevice we defaulted external to front, but we hadn't handled\n> CameraHalManager when counting the internal vs external cameras for\n> hal_get_number_of_cameras().\n>\n> To solve this, add a flag to hold if we have external camera support\n> (hardcoded to false until we do), and add that to the condition in\n> CameraHalManager when deciding whether a camera should be counted as\n> internal or external. CameraDevice also uses this flag to decide on the\n> default location.\n\nI have missed what condition should trigger for us setting\nexternalCameraSupport_ to true in the camera_hal_manager.\n\nand once we do so, won't cameras that report location=external because\nof the missing information from the firmware be reported as external\neven if they're not ?\n\nI think we're a bit running in circles here. In my opinion the best\ncourse of actions would have been not registering properties::Location\nif libcamera cannot find that information from firmware (or from\nsomewhere else in future), and the camera HAL should resort to a\nconfiguration file in that case, and fail registering the camera if\nsaid file is not available.\n\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/android/camera_device.cpp | 24 ++++++++++++++----------\n> src/android/camera_device.h | 8 ++++++--\n> src/android/camera_hal_manager.cpp | 11 +++++++----\n> src/android/camera_hal_manager.h | 2 ++\n> 4 files changed, 29 insertions(+), 16 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ae01c362..bb33c922 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -311,9 +311,11 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> * back to the framework using the designated callbacks.\n> */\n>\n> -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> +\t\t\t bool externalCameraSupport)\n> \t: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> +\t facing_(CAMERA_FACING_FRONT), orientation_(0),\n> +\t externalCameraSupport_(externalCameraSupport)\n> {\n> \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n>\n> @@ -350,9 +352,10 @@ CameraDevice::~CameraDevice()\n> }\n>\n> std::shared_ptr CameraDevice::create(unsigned int id,\n> -\t\t\t\t\t\t const std::shared_ptr &cam)\n> +\t\t\t\t\t\t const std::shared_ptr &cam,\n> +\t\t\t\t\t\t bool externalCameraSupport)\n> {\n> -\tCameraDevice *camera = new CameraDevice(id, cam);\n> +\tCameraDevice *camera = new CameraDevice(id, cam, externalCameraSupport);\n> \treturn std::shared_ptr(camera);\n> }\n>\n> @@ -375,11 +378,10 @@ int CameraDevice::initialize()\n> \t\t\tfacing_ = CAMERA_FACING_BACK;\n> \t\t\tbreak;\n> \t\tcase properties::CameraLocationExternal:\n> -\t\t\t/*\n> -\t\t\t * \\todo Set this to EXTERNAL once we support\n> -\t\t\t * HARDWARE_LEVEL_EXTERNAL\n> -\t\t\t */\n> -\t\t\tfacing_ = CAMERA_FACING_FRONT;\n> +\t\t\tif (externalCameraSupport_)\n> +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n> +\t\t\telse\n> +\t\t\t\tfacing_ = CAMERA_FACING_FRONT;\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> @@ -1121,7 +1123,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n>\n> \t/* Info static metadata. */\n> -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\tuint8_t supportedHWLevel = externalCameraSupport_ ?\n> +\t\t\t\t ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL :\n> +\t\t\t\t ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> \tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> \t\t\t\t &supportedHWLevel, 1);\n>\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 4905958e..f96934db 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -33,7 +33,8 @@ class CameraDevice : protected libcamera::Loggable\n> {\n> public:\n> \tstatic std::shared_ptr create(unsigned int id,\n> -\t\t\t\t\t\t const std::shared_ptr &cam);\n> +\t\t\t\t\t\t const std::shared_ptr &cam,\n> +\t\t\t\t\t\t bool externalCameraSupport);\n> \t~CameraDevice();\n>\n> \tint initialize();\n> @@ -66,7 +67,8 @@ protected:\n> \tstd::string logPrefix() const override;\n>\n> private:\n> -\tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> +\tCameraDevice(unsigned int id, const std::shared_ptr &camera,\n> +\t\t bool externalCameraSupport);\n>\n> \tstruct Camera3RequestDescriptor {\n> \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> @@ -130,6 +132,8 @@ private:\n> \tunsigned int maxJpegBufferSize_;\n>\n> \tCameraMetadata lastSettings_;\n> +\n> +\tconst bool externalCameraSupport_;\n> };\n>\n> #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 189eda2b..e4ca4c82 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -29,7 +29,8 @@ LOG_DECLARE_CATEGORY(HAL)\n> */\n>\n> CameraHalManager::CameraHalManager()\n> -\t: cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),\n> +\t: cameraManager_(nullptr), callbacks_(nullptr),\n> +\t externalCameraSupport_(false), numInternalCameras_(0),\n> \t nextExternalCameraId_(firstExternalCameraId_)\n> {\n> }\n> @@ -115,7 +116,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t\t * Now check if this is an external camera and assign\n> \t\t * its id accordingly.\n> \t\t */\n> -\t\tif (cameraLocation(cam.get()) == properties::CameraLocationExternal) {\n> +\t\tif (cameraLocation(cam.get()) == properties::CameraLocationExternal &&\n> +\t\t externalCameraSupport_) {\n> \t\t\tisCameraExternal = true;\n> \t\t\tid = nextExternalCameraId_;\n> \t\t} else {\n> @@ -124,7 +126,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t}\n>\n> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> -\tstd::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> +\tstd::shared_ptr camera =\n> +\t\tCameraDevice::create(id, std::move(cam), externalCameraSupport_);\n> \tint ret = camera->initialize();\n> \tif (ret) {\n> \t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> @@ -134,7 +137,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \tif (isCameraNew) {\n> \t\tcameraIdsMap_.emplace(cam->id(), id);\n>\n> -\t\tif (isCameraExternal)\n> +\t\tif (isCameraExternal && externalCameraSupport_)\n> \t\t\tnextExternalCameraId_++;\n> \t\telse\n> \t\t\tnumInternalCameras_++;\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index a91decc7..74dc6b3e 100644\n> --- a/src/android/camera_hal_manager.h\n> +++ b/src/android/camera_hal_manager.h\n> @@ -54,6 +54,8 @@ private:\n> \tstd::map cameraIdsMap_;\n> \tMutex mutex_;\n>\n> +\tconst bool externalCameraSupport_;\n> +\n> \tunsigned int numInternalCameras_;\n> \tunsigned int nextExternalCameraId_;\n> };\n> --\n> 2.27.0\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 7E6BDBD1F1\n\tfor ;\n\tWed, 3 Mar 2021 13:33:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15C8468A91;\n\tWed, 3 Mar 2021 14:33:35 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E1B668A7E\n\tfor ;\n\tWed, 3 Mar 2021 14:33:33 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id D131840006;\n\tWed, 3 Mar 2021 13:33:32 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Wed, 3 Mar 2021 14:34:01 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210303133401.5fkrlfgnknnf35li@uno.localdomain>","References":"<20210303093756.490065-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210303093756.490065-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","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":15472,"web_url":"https://patchwork.libcamera.org/comment/15472/","msgid":"<20210304085659.GC2049@pyrite.rasen.tech>","date":"2021-03-04T08:56:59","subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Mar 03, 2021 at 02:34:01PM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Wed, Mar 03, 2021 at 06:37:56PM +0900, Paul Elder wrote:\n> > In the static metadata, we claim that we don't support external cameras,\n> > yet libcamera core defaults the camera location to external. In the HAL\n> > CameraDevice we defaulted external to front, but we hadn't handled\n> > CameraHalManager when counting the internal vs external cameras for\n> > hal_get_number_of_cameras().\n> >\n> > To solve this, add a flag to hold if we have external camera support\n> > (hardcoded to false until we do), and add that to the condition in\n> > CameraHalManager when deciding whether a camera should be counted as\n> > internal or external. CameraDevice also uses this flag to decide on the\n> > default location.\n> \n> I have missed what condition should trigger for us setting\n> externalCameraSupport_ to true in the camera_hal_manager.\n\nOnce we support the external hardware level...?\n\n> \n> and once we do so, won't cameras that report location=external because\n> of the missing information from the firmware be reported as external\n> even if they're not ?\n\nYes that will happen, but it should be fine since the hardware level\nsays that we support it.\n\nThis is just one step before adding HAL config support (unless we should\ngo for that directly instead?).\n\n> \n> I think we're a bit running in circles here. In my opinion the best\n> course of actions would have been not registering properties::Location\n> if libcamera cannot find that information from firmware (or from\n\nYes.\n\n> somewhere else in future), and the camera HAL should resort to a\n> configuration file in that case, and fail registering the camera if\n> said file is not available.\n\nOh, okay. I guess the question then is, in the event that the HAL config\nfile isn't available and no location is specified, should 1) we fail to\nregister the camera, or 2) default to external (or front, depending on\nthe hardware level).\n\nI'm fine with having HAL config files. I was thinking of this patch more\nlike an intermediate step.\n\n\nPaul\n\n> >\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/android/camera_device.cpp | 24 ++++++++++++++----------\n> > src/android/camera_device.h | 8 ++++++--\n> > src/android/camera_hal_manager.cpp | 11 +++++++----\n> > src/android/camera_hal_manager.h | 2 ++\n> > 4 files changed, 29 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ae01c362..bb33c922 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -311,9 +311,11 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > * back to the framework using the designated callbacks.\n> > */\n> >\n> > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> > +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > +\t\t\t bool externalCameraSupport)\n> > \t: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> > -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > +\t facing_(CAMERA_FACING_FRONT), orientation_(0),\n> > +\t externalCameraSupport_(externalCameraSupport)\n> > {\n> > \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> >\n> > @@ -350,9 +352,10 @@ CameraDevice::~CameraDevice()\n> > }\n> >\n> > std::shared_ptr CameraDevice::create(unsigned int id,\n> > -\t\t\t\t\t\t const std::shared_ptr &cam)\n> > +\t\t\t\t\t\t const std::shared_ptr &cam,\n> > +\t\t\t\t\t\t bool externalCameraSupport)\n> > {\n> > -\tCameraDevice *camera = new CameraDevice(id, cam);\n> > +\tCameraDevice *camera = new CameraDevice(id, cam, externalCameraSupport);\n> > \treturn std::shared_ptr(camera);\n> > }\n> >\n> > @@ -375,11 +378,10 @@ int CameraDevice::initialize()\n> > \t\t\tfacing_ = CAMERA_FACING_BACK;\n> > \t\t\tbreak;\n> > \t\tcase properties::CameraLocationExternal:\n> > -\t\t\t/*\n> > -\t\t\t * \\todo Set this to EXTERNAL once we support\n> > -\t\t\t * HARDWARE_LEVEL_EXTERNAL\n> > -\t\t\t */\n> > -\t\t\tfacing_ = CAMERA_FACING_FRONT;\n> > +\t\t\tif (externalCameraSupport_)\n> > +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n> > +\t\t\telse\n> > +\t\t\t\tfacing_ = CAMERA_FACING_FRONT;\n> > \t\t\tbreak;\n> > \t\t}\n> > \t}\n> > @@ -1121,7 +1123,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n> >\n> > \t/* Info static metadata. */\n> > -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\tuint8_t supportedHWLevel = externalCameraSupport_ ?\n> > +\t\t\t\t ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL :\n> > +\t\t\t\t ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > \tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > \t\t\t\t &supportedHWLevel, 1);\n> >\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 4905958e..f96934db 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -33,7 +33,8 @@ class CameraDevice : protected libcamera::Loggable\n> > {\n> > public:\n> > \tstatic std::shared_ptr create(unsigned int id,\n> > -\t\t\t\t\t\t const std::shared_ptr &cam);\n> > +\t\t\t\t\t\t const std::shared_ptr &cam,\n> > +\t\t\t\t\t\t bool externalCameraSupport);\n> > \t~CameraDevice();\n> >\n> > \tint initialize();\n> > @@ -66,7 +67,8 @@ protected:\n> > \tstd::string logPrefix() const override;\n> >\n> > private:\n> > -\tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> > +\tCameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > +\t\t bool externalCameraSupport);\n> >\n> > \tstruct Camera3RequestDescriptor {\n> > \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > @@ -130,6 +132,8 @@ private:\n> > \tunsigned int maxJpegBufferSize_;\n> >\n> > \tCameraMetadata lastSettings_;\n> > +\n> > +\tconst bool externalCameraSupport_;\n> > };\n> >\n> > #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > index 189eda2b..e4ca4c82 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -29,7 +29,8 @@ LOG_DECLARE_CATEGORY(HAL)\n> > */\n> >\n> > CameraHalManager::CameraHalManager()\n> > -\t: cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),\n> > +\t: cameraManager_(nullptr), callbacks_(nullptr),\n> > +\t externalCameraSupport_(false), numInternalCameras_(0),\n> > \t nextExternalCameraId_(firstExternalCameraId_)\n> > {\n> > }\n> > @@ -115,7 +116,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > \t\t * Now check if this is an external camera and assign\n> > \t\t * its id accordingly.\n> > \t\t */\n> > -\t\tif (cameraLocation(cam.get()) == properties::CameraLocationExternal) {\n> > +\t\tif (cameraLocation(cam.get()) == properties::CameraLocationExternal &&\n> > +\t\t externalCameraSupport_) {\n> > \t\t\tisCameraExternal = true;\n> > \t\t\tid = nextExternalCameraId_;\n> > \t\t} else {\n> > @@ -124,7 +126,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > \t}\n> >\n> > \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > -\tstd::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > +\tstd::shared_ptr camera =\n> > +\t\tCameraDevice::create(id, std::move(cam), externalCameraSupport_);\n> > \tint ret = camera->initialize();\n> > \tif (ret) {\n> > \t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > @@ -134,7 +137,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > \tif (isCameraNew) {\n> > \t\tcameraIdsMap_.emplace(cam->id(), id);\n> >\n> > -\t\tif (isCameraExternal)\n> > +\t\tif (isCameraExternal && externalCameraSupport_)\n> > \t\t\tnextExternalCameraId_++;\n> > \t\telse\n> > \t\t\tnumInternalCameras_++;\n> > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > index a91decc7..74dc6b3e 100644\n> > --- a/src/android/camera_hal_manager.h\n> > +++ b/src/android/camera_hal_manager.h\n> > @@ -54,6 +54,8 @@ private:\n> > \tstd::map cameraIdsMap_;\n> > \tMutex mutex_;\n> >\n> > +\tconst bool externalCameraSupport_;\n> > +\n> > \tunsigned int numInternalCameras_;\n> > \tunsigned int nextExternalCameraId_;\n> > };\n> > --\n> > 2.27.0\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 40ACABD1F1\n\tfor ;\n\tThu, 4 Mar 2021 08:57:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B30F268A98;\n\tThu, 4 Mar 2021 09:57:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40FED602EC\n\tfor ;\n\tThu, 4 Mar 2021 09:57:11 +0100 (CET)","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 B858827A;\n\tThu, 4 Mar 2021 09:57:06 +0100 (CET)"],"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=\"mF1fxRdI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614848230;\n\tbh=hH0J48sKMMnz31MYlVSNsnO51Oqraj8vorlVKW/ZqB0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mF1fxRdIosJSnKDwGp0aogvRtr/Tgg0y2nS+23gX+6ims9azhWPcq5R3V9D+QJ3Up\n\tEzKH6k8/Bcq9iw4jhT//OXFHQzG6vprUtPVHxQJTdNDYQl267dL6VhZJPiy5tBLHk4\n\teeFGWrj29oE6qMMyPnZQ+9R5bTgxLTGbvJ7yhKbI=","Date":"Thu, 4 Mar 2021 17:56:59 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210304085659.GC2049@pyrite.rasen.tech>","References":"<20210303093756.490065-1-paul.elder@ideasonboard.com>\n\t<20210303133401.5fkrlfgnknnf35li@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210303133401.5fkrlfgnknnf35li@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","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":15478,"web_url":"https://patchwork.libcamera.org/comment/15478/","msgid":"<20210304094736.bjz3kho6pjc6bir3@uno.localdomain>","date":"2021-03-04T09:47:36","subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Thu, Mar 04, 2021 at 05:56:59PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Wed, Mar 03, 2021 at 02:34:01PM +0100, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Wed, Mar 03, 2021 at 06:37:56PM +0900, Paul Elder wrote:\n> > > In the static metadata, we claim that we don't support external cameras,\n> > > yet libcamera core defaults the camera location to external. In the HAL\n> > > CameraDevice we defaulted external to front, but we hadn't handled\n> > > CameraHalManager when counting the internal vs external cameras for\n> > > hal_get_number_of_cameras().\n> > >\n> > > To solve this, add a flag to hold if we have external camera support\n> > > (hardcoded to false until we do), and add that to the condition in\n> > > CameraHalManager when deciding whether a camera should be counted as\n> > > internal or external. CameraDevice also uses this flag to decide on the\n> > > default location.\n> >\n> > I have missed what condition should trigger for us setting\n> > externalCameraSupport_ to true in the camera_hal_manager.\n>\n> Once we support the external hardware level...?\n>\n> >\n> > and once we do so, won't cameras that report location=external because\n> > of the missing information from the firmware be reported as external\n> > even if they're not ?\n>\n> Yes that will happen, but it should be fine since the hardware level\n> says that we support it.\n>\n> This is just one step before adding HAL config support (unless we should\n> go for that directly instead?).\n>\n> >\n> > I think we're a bit running in circles here. In my opinion the best\n> > course of actions would have been not registering properties::Location\n> > if libcamera cannot find that information from firmware (or from\n>\n> Yes.\n>\n> > somewhere else in future), and the camera HAL should resort to a\n> > configuration file in that case, and fail registering the camera if\n> > said file is not available.\n>\n> Oh, okay. I guess the question then is, in the event that the HAL config\n> file isn't available and no location is specified, should 1) we fail to\n> register the camera, or 2) default to external (or front, depending on\n> the hardware level).\n\nI would refrain from registering the camera but it might be too harsh.\nTrue that Android is not a regular distro and integrators should know\nwhat they're doing and what they need to do.\n\nI'm skeptical about defaulting to anything as it's proving to be doing\nmore harm than good, but again, I might be too harsh.\n\n>\n> I'm fine with having HAL config files. I was thinking of this patch more\n> like an intermediate step.\n>\n>\n> Paul\n>\n> > >\n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/android/camera_device.cpp | 24 ++++++++++++++----------\n> > > src/android/camera_device.h | 8 ++++++--\n> > > src/android/camera_hal_manager.cpp | 11 +++++++----\n> > > src/android/camera_hal_manager.h | 2 ++\n> > > 4 files changed, 29 insertions(+), 16 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ae01c362..bb33c922 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -311,9 +311,11 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > * back to the framework using the designated callbacks.\n> > > */\n> > >\n> > > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> > > +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > +\t\t\t bool externalCameraSupport)\n> > > \t: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> > > -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > > +\t facing_(CAMERA_FACING_FRONT), orientation_(0),\n> > > +\t externalCameraSupport_(externalCameraSupport)\n> > > {\n> > > \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> > >\n> > > @@ -350,9 +352,10 @@ CameraDevice::~CameraDevice()\n> > > }\n> > >\n> > > std::shared_ptr CameraDevice::create(unsigned int id,\n> > > -\t\t\t\t\t\t const std::shared_ptr &cam)\n> > > +\t\t\t\t\t\t const std::shared_ptr &cam,\n> > > +\t\t\t\t\t\t bool externalCameraSupport)\n> > > {\n> > > -\tCameraDevice *camera = new CameraDevice(id, cam);\n> > > +\tCameraDevice *camera = new CameraDevice(id, cam, externalCameraSupport);\n> > > \treturn std::shared_ptr(camera);\n> > > }\n> > >\n> > > @@ -375,11 +378,10 @@ int CameraDevice::initialize()\n> > > \t\t\tfacing_ = CAMERA_FACING_BACK;\n> > > \t\t\tbreak;\n> > > \t\tcase properties::CameraLocationExternal:\n> > > -\t\t\t/*\n> > > -\t\t\t * \\todo Set this to EXTERNAL once we support\n> > > -\t\t\t * HARDWARE_LEVEL_EXTERNAL\n> > > -\t\t\t */\n> > > -\t\t\tfacing_ = CAMERA_FACING_FRONT;\n> > > +\t\t\tif (externalCameraSupport_)\n> > > +\t\t\t\tfacing_ = CAMERA_FACING_EXTERNAL;\n> > > +\t\t\telse\n> > > +\t\t\t\tfacing_ = CAMERA_FACING_FRONT;\n> > > \t\t\tbreak;\n> > > \t\t}\n> > > \t}\n> > > @@ -1121,7 +1123,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n> > >\n> > > \t/* Info static metadata. */\n> > > -\tuint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > +\tuint8_t supportedHWLevel = externalCameraSupport_ ?\n> > > +\t\t\t\t ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL :\n> > > +\t\t\t\t ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > \tstaticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > \t\t\t\t &supportedHWLevel, 1);\n> > >\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 4905958e..f96934db 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -33,7 +33,8 @@ class CameraDevice : protected libcamera::Loggable\n> > > {\n> > > public:\n> > > \tstatic std::shared_ptr create(unsigned int id,\n> > > -\t\t\t\t\t\t const std::shared_ptr &cam);\n> > > +\t\t\t\t\t\t const std::shared_ptr &cam,\n> > > +\t\t\t\t\t\t bool externalCameraSupport);\n> > > \t~CameraDevice();\n> > >\n> > > \tint initialize();\n> > > @@ -66,7 +67,8 @@ protected:\n> > > \tstd::string logPrefix() const override;\n> > >\n> > > private:\n> > > -\tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> > > +\tCameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > +\t\t bool externalCameraSupport);\n> > >\n> > > \tstruct Camera3RequestDescriptor {\n> > > \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > > @@ -130,6 +132,8 @@ private:\n> > > \tunsigned int maxJpegBufferSize_;\n> > >\n> > > \tCameraMetadata lastSettings_;\n> > > +\n> > > +\tconst bool externalCameraSupport_;\n> > > };\n> > >\n> > > #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > index 189eda2b..e4ca4c82 100644\n> > > --- a/src/android/camera_hal_manager.cpp\n> > > +++ b/src/android/camera_hal_manager.cpp\n> > > @@ -29,7 +29,8 @@ LOG_DECLARE_CATEGORY(HAL)\n> > > */\n> > >\n> > > CameraHalManager::CameraHalManager()\n> > > -\t: cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),\n> > > +\t: cameraManager_(nullptr), callbacks_(nullptr),\n> > > +\t externalCameraSupport_(false), numInternalCameras_(0),\n> > > \t nextExternalCameraId_(firstExternalCameraId_)\n> > > {\n> > > }\n> > > @@ -115,7 +116,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > \t\t * Now check if this is an external camera and assign\n> > > \t\t * its id accordingly.\n> > > \t\t */\n> > > -\t\tif (cameraLocation(cam.get()) == properties::CameraLocationExternal) {\n> > > +\t\tif (cameraLocation(cam.get()) == properties::CameraLocationExternal &&\n> > > +\t\t externalCameraSupport_) {\n> > > \t\t\tisCameraExternal = true;\n> > > \t\t\tid = nextExternalCameraId_;\n> > > \t\t} else {\n> > > @@ -124,7 +126,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > \t}\n> > >\n> > > \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > -\tstd::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > +\tstd::shared_ptr camera =\n> > > +\t\tCameraDevice::create(id, std::move(cam), externalCameraSupport_);\n> > > \tint ret = camera->initialize();\n> > > \tif (ret) {\n> > > \t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > @@ -134,7 +137,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > \tif (isCameraNew) {\n> > > \t\tcameraIdsMap_.emplace(cam->id(), id);\n> > >\n> > > -\t\tif (isCameraExternal)\n> > > +\t\tif (isCameraExternal && externalCameraSupport_)\n> > > \t\t\tnextExternalCameraId_++;\n> > > \t\telse\n> > > \t\t\tnumInternalCameras_++;\n> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > index a91decc7..74dc6b3e 100644\n> > > --- a/src/android/camera_hal_manager.h\n> > > +++ b/src/android/camera_hal_manager.h\n> > > @@ -54,6 +54,8 @@ private:\n> > > \tstd::map cameraIdsMap_;\n> > > \tMutex mutex_;\n> > >\n> > > +\tconst bool externalCameraSupport_;\n> > > +\n> > > \tunsigned int numInternalCameras_;\n> > > \tunsigned int nextExternalCameraId_;\n> > > };\n> > > --\n> > > 2.27.0\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 169CFBD80C\n\tfor ;\n\tThu, 4 Mar 2021 09:47:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 83A1E68A92;\n\tThu, 4 Mar 2021 10:47:08 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A9E3B602EC\n\tfor ;\n\tThu, 4 Mar 2021 10:47:07 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 17AACE0006;\n\tThu, 4 Mar 2021 09:47:06 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 4 Mar 2021 10:47:36 +0100","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20210304094736.bjz3kho6pjc6bir3@uno.localdomain>","References":"<20210303093756.490065-1-paul.elder@ideasonboard.com>\n\t<20210303133401.5fkrlfgnknnf35li@uno.localdomain>\n\t<20210304085659.GC2049@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210304085659.GC2049@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","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":15481,"web_url":"https://patchwork.libcamera.org/comment/15481/","msgid":"","date":"2021-03-04T14:20:16","subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"Hi Jacopo and Paul,\n\nOn Thu, Mar 4, 2021 at 6:47 PM Jacopo Mondi wrote:\n>\n> Hi Paul,\n>\n> On Thu, Mar 04, 2021 at 05:56:59PM +0900, paul.elder@ideasonboard.com wrote:\n> > Hi Jacopo,\n> >\n> > On Wed, Mar 03, 2021 at 02:34:01PM +0100, Jacopo Mondi wrote:\n> > > Hi Paul,\n> > >\n> > > On Wed, Mar 03, 2021 at 06:37:56PM +0900, Paul Elder wrote:\n> > > > In the static metadata, we claim that we don't support external cameras,\n> > > > yet libcamera core defaults the camera location to external. In the HAL\n> > > > CameraDevice we defaulted external to front, but we hadn't handled\n> > > > CameraHalManager when counting the internal vs external cameras for\n> > > > hal_get_number_of_cameras().\n> > > >\n> > > > To solve this, add a flag to hold if we have external camera support\n> > > > (hardcoded to false until we do), and add that to the condition in\n> > > > CameraHalManager when deciding whether a camera should be counted as\n> > > > internal or external. CameraDevice also uses this flag to decide on the\n> > > > default location.\n> > >\n> > > I have missed what condition should trigger for us setting\n> > > externalCameraSupport_ to true in the camera_hal_manager.\n> >\n> > Once we support the external hardware level...?\n> >\n> > >\n> > > and once we do so, won't cameras that report location=external because\n> > > of the missing information from the firmware be reported as external\n> > > even if they're not ?\n> >\n> > Yes that will happen, but it should be fine since the hardware level\n> > says that we support it.\n> >\n\nHmm, wouldn't the camera end up with the EXTERNAL capability level\nrather than LIMITED or FULL we're targeting? Also on Chrome OS we\ncount internal cameras and things would error out if we don't get the\nright count.\n\n> > This is just one step before adding HAL config support (unless we should\n> > go for that directly instead?).\n> >\n> > >\n> > > I think we're a bit running in circles here. In my opinion the best\n> > > course of actions would have been not registering properties::Location\n> > > if libcamera cannot find that information from firmware (or from\n> >\n> > Yes.\n> >\n> > > somewhere else in future), and the camera HAL should resort to a\n> > > configuration file in that case, and fail registering the camera if\n> > > said file is not available.\n> >\n> > Oh, okay. I guess the question then is, in the event that the HAL config\n> > file isn't available and no location is specified, should 1) we fail to\n> > register the camera, or 2) default to external (or front, depending on\n> > the hardware level).\n>\n> I would refrain from registering the camera but it might be too harsh.\n> True that Android is not a regular distro and integrators should know\n> what they're doing and what they need to do.\n>\n> I'm skeptical about defaulting to anything as it's proving to be doing\n> more harm than good, but again, I might be too harsh.\n>\n\nAgreed with Jacopo. I'd log an error and fail to allow detecting problems early.\n\nBest regards,\nTomasz\n\n> >\n> > I'm fine with having HAL config files. I was thinking of this patch more\n> > like an intermediate step.\n> >\n> >\n> > Paul\n> >\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > > ---\n> > > > src/android/camera_device.cpp | 24 ++++++++++++++----------\n> > > > src/android/camera_device.h | 8 ++++++--\n> > > > src/android/camera_hal_manager.cpp | 11 +++++++----\n> > > > src/android/camera_hal_manager.h | 2 ++\n> > > > 4 files changed, 29 insertions(+), 16 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index ae01c362..bb33c922 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -311,9 +311,11 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > > * back to the framework using the designated callbacks.\n> > > > */\n> > > >\n> > > > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> > > > +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > > + bool externalCameraSupport)\n> > > > : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> > > > - facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > > > + facing_(CAMERA_FACING_FRONT), orientation_(0),\n> > > > + externalCameraSupport_(externalCameraSupport)\n> > > > {\n> > > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> > > >\n> > > > @@ -350,9 +352,10 @@ CameraDevice::~CameraDevice()\n> > > > }\n> > > >\n> > > > std::shared_ptr CameraDevice::create(unsigned int id,\n> > > > - const std::shared_ptr &cam)\n> > > > + const std::shared_ptr &cam,\n> > > > + bool externalCameraSupport)\n> > > > {\n> > > > - CameraDevice *camera = new CameraDevice(id, cam);\n> > > > + CameraDevice *camera = new CameraDevice(id, cam, externalCameraSupport);\n> > > > return std::shared_ptr(camera);\n> > > > }\n> > > >\n> > > > @@ -375,11 +378,10 @@ int CameraDevice::initialize()\n> > > > facing_ = CAMERA_FACING_BACK;\n> > > > break;\n> > > > case properties::CameraLocationExternal:\n> > > > - /*\n> > > > - * \\todo Set this to EXTERNAL once we support\n> > > > - * HARDWARE_LEVEL_EXTERNAL\n> > > > - */\n> > > > - facing_ = CAMERA_FACING_FRONT;\n> > > > + if (externalCameraSupport_)\n> > > > + facing_ = CAMERA_FACING_EXTERNAL;\n> > > > + else\n> > > > + facing_ = CAMERA_FACING_FRONT;\n> > > > break;\n> > > > }\n> > > > }\n> > > > @@ -1121,7 +1123,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > staticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n> > > >\n> > > > /* Info static metadata. */\n> > > > - uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > + uint8_t supportedHWLevel = externalCameraSupport_ ?\n> > > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL :\n> > > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > > &supportedHWLevel, 1);\n> > > >\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index 4905958e..f96934db 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -33,7 +33,8 @@ class CameraDevice : protected libcamera::Loggable\n> > > > {\n> > > > public:\n> > > > static std::shared_ptr create(unsigned int id,\n> > > > - const std::shared_ptr &cam);\n> > > > + const std::shared_ptr &cam,\n> > > > + bool externalCameraSupport);\n> > > > ~CameraDevice();\n> > > >\n> > > > int initialize();\n> > > > @@ -66,7 +67,8 @@ protected:\n> > > > std::string logPrefix() const override;\n> > > >\n> > > > private:\n> > > > - CameraDevice(unsigned int id, const std::shared_ptr &camera);\n> > > > + CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > > + bool externalCameraSupport);\n> > > >\n> > > > struct Camera3RequestDescriptor {\n> > > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > > @@ -130,6 +132,8 @@ private:\n> > > > unsigned int maxJpegBufferSize_;\n> > > >\n> > > > CameraMetadata lastSettings_;\n> > > > +\n> > > > + const bool externalCameraSupport_;\n> > > > };\n> > > >\n> > > > #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n> > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > index 189eda2b..e4ca4c82 100644\n> > > > --- a/src/android/camera_hal_manager.cpp\n> > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > @@ -29,7 +29,8 @@ LOG_DECLARE_CATEGORY(HAL)\n> > > > */\n> > > >\n> > > > CameraHalManager::CameraHalManager()\n> > > > - : cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),\n> > > > + : cameraManager_(nullptr), callbacks_(nullptr),\n> > > > + externalCameraSupport_(false), numInternalCameras_(0),\n> > > > nextExternalCameraId_(firstExternalCameraId_)\n> > > > {\n> > > > }\n> > > > @@ -115,7 +116,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > * Now check if this is an external camera and assign\n> > > > * its id accordingly.\n> > > > */\n> > > > - if (cameraLocation(cam.get()) == properties::CameraLocationExternal) {\n> > > > + if (cameraLocation(cam.get()) == properties::CameraLocationExternal &&\n> > > > + externalCameraSupport_) {\n> > > > isCameraExternal = true;\n> > > > id = nextExternalCameraId_;\n> > > > } else {\n> > > > @@ -124,7 +126,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > }\n> > > >\n> > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > - std::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > > + std::shared_ptr camera =\n> > > > + CameraDevice::create(id, std::move(cam), externalCameraSupport_);\n> > > > int ret = camera->initialize();\n> > > > if (ret) {\n> > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > @@ -134,7 +137,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > if (isCameraNew) {\n> > > > cameraIdsMap_.emplace(cam->id(), id);\n> > > >\n> > > > - if (isCameraExternal)\n> > > > + if (isCameraExternal && externalCameraSupport_)\n> > > > nextExternalCameraId_++;\n> > > > else\n> > > > numInternalCameras_++;\n> > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > index a91decc7..74dc6b3e 100644\n> > > > --- a/src/android/camera_hal_manager.h\n> > > > +++ b/src/android/camera_hal_manager.h\n> > > > @@ -54,6 +54,8 @@ private:\n> > > > std::map cameraIdsMap_;\n> > > > Mutex mutex_;\n> > > >\n> > > > + const bool externalCameraSupport_;\n> > > > +\n> > > > unsigned int numInternalCameras_;\n> > > > unsigned int nextExternalCameraId_;\n> > > > };\n> > > > --\n> > > > 2.27.0\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\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 B3044BD1F1\n\tfor ;\n\tThu, 4 Mar 2021 14:20:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27C32602ED;\n\tThu, 4 Mar 2021 15:20:31 +0100 (CET)","from mail-ej1-x630.google.com (mail-ej1-x630.google.com\n\t[IPv6:2a00:1450:4864:20::630])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B506E602ED\n\tfor ;\n\tThu, 4 Mar 2021 15:20:29 +0100 (CET)","by mail-ej1-x630.google.com with SMTP id w17so1793612ejc.6\n\tfor ;\n\tThu, 04 Mar 2021 06:20:29 -0800 (PST)","from mail-wm1-f53.google.com (mail-wm1-f53.google.com.\n\t[209.85.128.53]) by smtp.gmail.com with ESMTPSA id\n\ty24sm22529046eds.23.2021.03.04.06.20.28\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tThu, 04 Mar 2021 06:20:28 -0800 (PST)","by mail-wm1-f53.google.com with SMTP id\n\to7-20020a05600c4fc7b029010a0247d5f0so4587183wmq.1\n\tfor ;\n\tThu, 04 Mar 2021 06:20:28 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"cQXN4kr9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=R77nvPwR6IW1Jbw3CV1Op/BxDYr0zbQw4og3AFK31Dg=;\n\tb=cQXN4kr9fXUkfDFRXKp/4cC0cjn1VVnNeslre0afYa1b5uzznB+ULzIVu99DEUvzZ+\n\t8gVbaFjW1iOC569vxBfCQ4TduVIKJcRmNNp3p7ecrw1Dex6UJg5cbaQOHK3XNvytJm7o\n\tRWceDsxEavh0tFz4Wm+kjVlbnoUv8gkS5glqw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=R77nvPwR6IW1Jbw3CV1Op/BxDYr0zbQw4og3AFK31Dg=;\n\tb=Gg11V7Y1SiCwiZpp+jt38yNmLcaZiwTvD4XNdLDEF5jPXbeARCwRFiLzcM1dWCbJta\n\tnG2wjBL0AxutAyRQLMWyrcVZ0D+FQILjmjeEhLYkEFd5C3/7bPB1PVJhioI/x8EJXVHY\n\trloXYn1u7tOedjHCEDSQMTf8aAtwGa8ui++YL4huEKMccUriXTjZj0xj5BBkvkeH6IE8\n\t0iFmv8P9WUwNP0dS8niFeMY8761np3mjuYes0L6i8/k45LOzLRbDQ14hqI3r3QWaPLGj\n\tiKE55FOc/Gg0bh7d8ibnSYmqwJrdueQ5N7I/ZpiGxoqZOSeyaONaurput/7tWSrnbLL2\n\tbkkA==","X-Gm-Message-State":"AOAM531d/wpsOdZaDzT8KEo993JYJw3Pa8KuGfn6uv/AD8oYVussqdM4\n\tklYoDfXBhv45KqvuHlg3AcZGCmXXUvfLTw==","X-Google-Smtp-Source":"ABdhPJxeKiHqr+eA2b4Gbov/Ae1xNuH0vObfNT1CiAmP2+rUmhrKjTXNWvp+RGoz2rDuUDSV/R7zxg==","X-Received":["by 2002:a17:906:eb4e:: with SMTP id\n\tmc14mr4311033ejb.169.1614867628815; \n\tThu, 04 Mar 2021 06:20:28 -0800 (PST)","by 2002:a05:600c:247:: with SMTP id\n\t7mr4244744wmj.116.1614867627565; \n\tThu, 04 Mar 2021 06:20:27 -0800 (PST)"],"MIME-Version":"1.0","References":"<20210303093756.490065-1-paul.elder@ideasonboard.com>\n\t<20210303133401.5fkrlfgnknnf35li@uno.localdomain>\n\t<20210304085659.GC2049@pyrite.rasen.tech>\n\t<20210304094736.bjz3kho6pjc6bir3@uno.localdomain>","In-Reply-To":"<20210304094736.bjz3kho6pjc6bir3@uno.localdomain>","From":"Tomasz Figa ","Date":"Thu, 4 Mar 2021 23:20:16 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Jacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15499,"web_url":"https://patchwork.libcamera.org/comment/15499/","msgid":"","date":"2021-03-08T03:01:55","subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello everybody,\n\nOn Thu, Mar 04, 2021 at 11:20:16PM +0900, Tomasz Figa wrote:\n> On Thu, Mar 4, 2021 at 6:47 PM Jacopo Mondi wrote:\n> > On Thu, Mar 04, 2021 at 05:56:59PM +0900, wrote:\n> > > On Wed, Mar 03, 2021 at 02:34:01PM +0100, Jacopo Mondi wrote:\n> > > > On Wed, Mar 03, 2021 at 06:37:56PM +0900, Paul Elder wrote:\n> > > > > In the static metadata, we claim that we don't support external cameras,\n> > > > > yet libcamera core defaults the camera location to external. In the HAL\n> > > > > CameraDevice we defaulted external to front, but we hadn't handled\n> > > > > CameraHalManager when counting the internal vs external cameras for\n> > > > > hal_get_number_of_cameras().\n> > > > >\n> > > > > To solve this, add a flag to hold if we have external camera support\n> > > > > (hardcoded to false until we do), and add that to the condition in\n> > > > > CameraHalManager when deciding whether a camera should be counted as\n> > > > > internal or external. CameraDevice also uses this flag to decide on the\n> > > > > default location.\n> > > >\n> > > > I have missed what condition should trigger for us setting\n> > > > externalCameraSupport_ to true in the camera_hal_manager.\n> > >\n> > > Once we support the external hardware level...?\n> > >\n> > > > and once we do so, won't cameras that report location=external because\n> > > > of the missing information from the firmware be reported as external\n> > > > even if they're not ?\n> > >\n> > > Yes that will happen, but it should be fine since the hardware level\n> > > says that we support it.\n> \n> Hmm, wouldn't the camera end up with the EXTERNAL capability level\n> rather than LIMITED or FULL we're targeting? Also on Chrome OS we\n> count internal cameras and things would error out if we don't get the\n> right count.\n\nIn the context of Chrome OS, external cameras are limited to USB\nwebcams, right ? In that case, the EXTERNAL hardware level seems more\nappropriate than LIMITED or FULL, as external UVC devices can't support\nLIMITED or FULL (they don't provide all the information required by\nthose hardware levels). Is there something I'm missing ?\n\n> > > This is just one step before adding HAL config support (unless we should\n> > > go for that directly instead?).\n> > >\n> > > > I think we're a bit running in circles here. In my opinion the best\n> > > > course of actions would have been not registering properties::Location\n> > > > if libcamera cannot find that information from firmware (or from\n> > >\n> > > Yes.\n\nI'm fine with this.\n\n> > > > somewhere else in future), and the camera HAL should resort to a\n> > > > configuration file in that case, and fail registering the camera if\n> > > > said file is not available.\n> > >\n> > > Oh, okay. I guess the question then is, in the event that the HAL config\n> > > file isn't available and no location is specified, should 1) we fail to\n> > > register the camera, or 2) default to external (or front, depending on\n> > > the hardware level).\n> >\n> > I would refrain from registering the camera but it might be too harsh.\n> > True that Android is not a regular distro and integrators should know\n> > what they're doing and what they need to do.\n> >\n> > I'm skeptical about defaulting to anything as it's proving to be doing\n> > more harm than good, but again, I might be too harsh.\n> \n> Agreed with Jacopo. I'd log an error and fail to allow detecting problems early.\n\nOnce we have support for a configuration file, I'm fine with this, but I\nthink we can let the HAL default to front if the location isn't reported\nby the libcamera core in the meantime.\n\n> > > I'm fine with having HAL config files. I was thinking of this patch more\n> > > like an intermediate step.\n> > >\n> > > > > Signed-off-by: Paul Elder \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 24 ++++++++++++++----------\n> > > > > src/android/camera_device.h | 8 ++++++--\n> > > > > src/android/camera_hal_manager.cpp | 11 +++++++----\n> > > > > src/android/camera_hal_manager.h | 2 ++\n> > > > > 4 files changed, 29 insertions(+), 16 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index ae01c362..bb33c922 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -311,9 +311,11 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > > > * back to the framework using the designated callbacks.\n> > > > > */\n> > > > >\n> > > > > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> > > > > +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > > > + bool externalCameraSupport)\n> > > > > : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> > > > > - facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > > > > + facing_(CAMERA_FACING_FRONT), orientation_(0),\n> > > > > + externalCameraSupport_(externalCameraSupport)\n> > > > > {\n> > > > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> > > > >\n> > > > > @@ -350,9 +352,10 @@ CameraDevice::~CameraDevice()\n> > > > > }\n> > > > >\n> > > > > std::shared_ptr CameraDevice::create(unsigned int id,\n> > > > > - const std::shared_ptr &cam)\n> > > > > + const std::shared_ptr &cam,\n> > > > > + bool externalCameraSupport)\n> > > > > {\n> > > > > - CameraDevice *camera = new CameraDevice(id, cam);\n> > > > > + CameraDevice *camera = new CameraDevice(id, cam, externalCameraSupport);\n> > > > > return std::shared_ptr(camera);\n> > > > > }\n> > > > >\n> > > > > @@ -375,11 +378,10 @@ int CameraDevice::initialize()\n> > > > > facing_ = CAMERA_FACING_BACK;\n> > > > > break;\n> > > > > case properties::CameraLocationExternal:\n> > > > > - /*\n> > > > > - * \\todo Set this to EXTERNAL once we support\n> > > > > - * HARDWARE_LEVEL_EXTERNAL\n> > > > > - */\n> > > > > - facing_ = CAMERA_FACING_FRONT;\n> > > > > + if (externalCameraSupport_)\n> > > > > + facing_ = CAMERA_FACING_EXTERNAL;\n> > > > > + else\n> > > > > + facing_ = CAMERA_FACING_FRONT;\n> > > > > break;\n> > > > > }\n> > > > > }\n> > > > > @@ -1121,7 +1123,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > staticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n> > > > >\n> > > > > /* Info static metadata. */\n> > > > > - uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > + uint8_t supportedHWLevel = externalCameraSupport_ ?\n> > > > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL :\n> > > > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > > > &supportedHWLevel, 1);\n> > > > >\n> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > index 4905958e..f96934db 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -33,7 +33,8 @@ class CameraDevice : protected libcamera::Loggable\n> > > > > {\n> > > > > public:\n> > > > > static std::shared_ptr create(unsigned int id,\n> > > > > - const std::shared_ptr &cam);\n> > > > > + const std::shared_ptr &cam,\n> > > > > + bool externalCameraSupport);\n> > > > > ~CameraDevice();\n> > > > >\n> > > > > int initialize();\n> > > > > @@ -66,7 +67,8 @@ protected:\n> > > > > std::string logPrefix() const override;\n> > > > >\n> > > > > private:\n> > > > > - CameraDevice(unsigned int id, const std::shared_ptr &camera);\n> > > > > + CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > > > + bool externalCameraSupport);\n> > > > >\n> > > > > struct Camera3RequestDescriptor {\n> > > > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > > > @@ -130,6 +132,8 @@ private:\n> > > > > unsigned int maxJpegBufferSize_;\n> > > > >\n> > > > > CameraMetadata lastSettings_;\n> > > > > +\n> > > > > + const bool externalCameraSupport_;\n> > > > > };\n> > > > >\n> > > > > #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n> > > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > > index 189eda2b..e4ca4c82 100644\n> > > > > --- a/src/android/camera_hal_manager.cpp\n> > > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > > @@ -29,7 +29,8 @@ LOG_DECLARE_CATEGORY(HAL)\n> > > > > */\n> > > > >\n> > > > > CameraHalManager::CameraHalManager()\n> > > > > - : cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),\n> > > > > + : cameraManager_(nullptr), callbacks_(nullptr),\n> > > > > + externalCameraSupport_(false), numInternalCameras_(0),\n> > > > > nextExternalCameraId_(firstExternalCameraId_)\n> > > > > {\n> > > > > }\n> > > > > @@ -115,7 +116,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > * Now check if this is an external camera and assign\n> > > > > * its id accordingly.\n> > > > > */\n> > > > > - if (cameraLocation(cam.get()) == properties::CameraLocationExternal) {\n> > > > > + if (cameraLocation(cam.get()) == properties::CameraLocationExternal &&\n> > > > > + externalCameraSupport_) {\n> > > > > isCameraExternal = true;\n> > > > > id = nextExternalCameraId_;\n> > > > > } else {\n> > > > > @@ -124,7 +126,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > }\n> > > > >\n> > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > > - std::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > > > + std::shared_ptr camera =\n> > > > > + CameraDevice::create(id, std::move(cam), externalCameraSupport_);\n> > > > > int ret = camera->initialize();\n> > > > > if (ret) {\n> > > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > > @@ -134,7 +137,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > if (isCameraNew) {\n> > > > > cameraIdsMap_.emplace(cam->id(), id);\n> > > > >\n> > > > > - if (isCameraExternal)\n> > > > > + if (isCameraExternal && externalCameraSupport_)\n> > > > > nextExternalCameraId_++;\n> > > > > else\n> > > > > numInternalCameras_++;\n> > > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > > index a91decc7..74dc6b3e 100644\n> > > > > --- a/src/android/camera_hal_manager.h\n> > > > > +++ b/src/android/camera_hal_manager.h\n> > > > > @@ -54,6 +54,8 @@ private:\n> > > > > std::map cameraIdsMap_;\n> > > > > Mutex mutex_;\n> > > > >\n> > > > > + const bool externalCameraSupport_;\n> > > > > +\n> > > > > unsigned int numInternalCameras_;\n> > > > > unsigned int nextExternalCameraId_;\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 DA603BD80C\n\tfor ;\n\tMon, 8 Mar 2021 03:02:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59CD268A91;\n\tMon, 8 Mar 2021 04:02:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E21D760106\n\tfor ;\n\tMon, 8 Mar 2021 04:02:26 +0100 (CET)","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 39712527;\n\tMon, 8 Mar 2021 04:02:26 +0100 (CET)"],"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=\"hvnwkzMR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615172546;\n\tbh=DFjAzxy0vG4azzoVl/hhvI59HOq0XyyQ31viaPuShLk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hvnwkzMRYlwj5jv45WC98DZPmnGanmz+8YZZHwowcK+42f5WkCXhHMyov49PPmiEW\n\tXjFYtUlbq7+0tn6aTFF5LGegaxmqzRt0nc731S/0aYuBfzYiaFpNbVkR+Y/0TQiUFl\n\tm81lb+pFS1tfMF+jMSGokfDxzea8BRGX7Zl8llbo=","Date":"Mon, 8 Mar 2021 05:01:55 +0200","From":"Laurent Pinchart ","To":"Tomasz Figa ","Message-ID":"","References":"<20210303093756.490065-1-paul.elder@ideasonboard.com>\n\t<20210303133401.5fkrlfgnknnf35li@uno.localdomain>\n\t<20210304085659.GC2049@pyrite.rasen.tech>\n\t<20210304094736.bjz3kho6pjc6bir3@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15500,"web_url":"https://patchwork.libcamera.org/comment/15500/","msgid":"","date":"2021-03-08T03:09:08","subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Mon, Mar 8, 2021 at 12:02 PM Laurent Pinchart\n wrote:\n>\n> Hello everybody,\n>\n> On Thu, Mar 04, 2021 at 11:20:16PM +0900, Tomasz Figa wrote:\n> > On Thu, Mar 4, 2021 at 6:47 PM Jacopo Mondi wrote:\n> > > On Thu, Mar 04, 2021 at 05:56:59PM +0900, wrote:\n> > > > On Wed, Mar 03, 2021 at 02:34:01PM +0100, Jacopo Mondi wrote:\n> > > > > On Wed, Mar 03, 2021 at 06:37:56PM +0900, Paul Elder wrote:\n> > > > > > In the static metadata, we claim that we don't support external cameras,\n> > > > > > yet libcamera core defaults the camera location to external. In the HAL\n> > > > > > CameraDevice we defaulted external to front, but we hadn't handled\n> > > > > > CameraHalManager when counting the internal vs external cameras for\n> > > > > > hal_get_number_of_cameras().\n> > > > > >\n> > > > > > To solve this, add a flag to hold if we have external camera support\n> > > > > > (hardcoded to false until we do), and add that to the condition in\n> > > > > > CameraHalManager when deciding whether a camera should be counted as\n> > > > > > internal or external. CameraDevice also uses this flag to decide on the\n> > > > > > default location.\n> > > > >\n> > > > > I have missed what condition should trigger for us setting\n> > > > > externalCameraSupport_ to true in the camera_hal_manager.\n> > > >\n> > > > Once we support the external hardware level...?\n> > > >\n> > > > > and once we do so, won't cameras that report location=external because\n> > > > > of the missing information from the firmware be reported as external\n> > > > > even if they're not ?\n> > > >\n> > > > Yes that will happen, but it should be fine since the hardware level\n> > > > says that we support it.\n> >\n> > Hmm, wouldn't the camera end up with the EXTERNAL capability level\n> > rather than LIMITED or FULL we're targeting? Also on Chrome OS we\n> > count internal cameras and things would error out if we don't get the\n> > right count.\n>\n> In the context of Chrome OS, external cameras are limited to USB\n> webcams, right ? In that case, the EXTERNAL hardware level seems more\n> appropriate than LIMITED or FULL, as external UVC devices can't support\n> LIMITED or FULL (they don't provide all the information required by\n> those hardware levels). Is there something I'm missing ?\n>\n\nI thought we were talking here about internal cameras that just lack\nthe location information because of an old or otherwise buggy firmware\nimplementation? In any case, my comment refers specifically to\ninternal MIPI cameras.\n\n> > > > This is just one step before adding HAL config support (unless we should\n> > > > go for that directly instead?).\n> > > >\n> > > > > I think we're a bit running in circles here. In my opinion the best\n> > > > > course of actions would have been not registering properties::Location\n> > > > > if libcamera cannot find that information from firmware (or from\n> > > >\n> > > > Yes.\n>\n> I'm fine with this.\n>\n> > > > > somewhere else in future), and the camera HAL should resort to a\n> > > > > configuration file in that case, and fail registering the camera if\n> > > > > said file is not available.\n> > > >\n> > > > Oh, okay. I guess the question then is, in the event that the HAL config\n> > > > file isn't available and no location is specified, should 1) we fail to\n> > > > register the camera, or 2) default to external (or front, depending on\n> > > > the hardware level).\n> > >\n> > > I would refrain from registering the camera but it might be too harsh.\n> > > True that Android is not a regular distro and integrators should know\n> > > what they're doing and what they need to do.\n> > >\n> > > I'm skeptical about defaulting to anything as it's proving to be doing\n> > > more harm than good, but again, I might be too harsh.\n> >\n> > Agreed with Jacopo. I'd log an error and fail to allow detecting problems early.\n>\n> Once we have support for a configuration file, I'm fine with this, but I\n> think we can let the HAL default to front if the location isn't reported\n> by the libcamera core in the meantime.\n>\n\nI thought Paul was specifically referring to the case when we have the\nconfiguration file implemented, but it's not provided by the system\nintegrator.\n\nBest regards,\nTomasz\n\n> > > > I'm fine with having HAL config files. I was thinking of this patch more\n> > > > like an intermediate step.\n> > > >\n> > > > > > Signed-off-by: Paul Elder \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 24 ++++++++++++++----------\n> > > > > > src/android/camera_device.h | 8 ++++++--\n> > > > > > src/android/camera_hal_manager.cpp | 11 +++++++----\n> > > > > > src/android/camera_hal_manager.h | 2 ++\n> > > > > > 4 files changed, 29 insertions(+), 16 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index ae01c362..bb33c922 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -311,9 +311,11 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()\n> > > > > > * back to the framework using the designated callbacks.\n> > > > > > */\n> > > > > >\n> > > > > > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera)\n> > > > > > +CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > > > > + bool externalCameraSupport)\n> > > > > > : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),\n> > > > > > - facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > > > > > + facing_(CAMERA_FACING_FRONT), orientation_(0),\n> > > > > > + externalCameraSupport_(externalCameraSupport)\n> > > > > > {\n> > > > > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> > > > > >\n> > > > > > @@ -350,9 +352,10 @@ CameraDevice::~CameraDevice()\n> > > > > > }\n> > > > > >\n> > > > > > std::shared_ptr CameraDevice::create(unsigned int id,\n> > > > > > - const std::shared_ptr &cam)\n> > > > > > + const std::shared_ptr &cam,\n> > > > > > + bool externalCameraSupport)\n> > > > > > {\n> > > > > > - CameraDevice *camera = new CameraDevice(id, cam);\n> > > > > > + CameraDevice *camera = new CameraDevice(id, cam, externalCameraSupport);\n> > > > > > return std::shared_ptr(camera);\n> > > > > > }\n> > > > > >\n> > > > > > @@ -375,11 +378,10 @@ int CameraDevice::initialize()\n> > > > > > facing_ = CAMERA_FACING_BACK;\n> > > > > > break;\n> > > > > > case properties::CameraLocationExternal:\n> > > > > > - /*\n> > > > > > - * \\todo Set this to EXTERNAL once we support\n> > > > > > - * HARDWARE_LEVEL_EXTERNAL\n> > > > > > - */\n> > > > > > - facing_ = CAMERA_FACING_FRONT;\n> > > > > > + if (externalCameraSupport_)\n> > > > > > + facing_ = CAMERA_FACING_EXTERNAL;\n> > > > > > + else\n> > > > > > + facing_ = CAMERA_FACING_FRONT;\n> > > > > > break;\n> > > > > > }\n> > > > > > }\n> > > > > > @@ -1121,7 +1123,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > staticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n> > > > > >\n> > > > > > /* Info static metadata. */\n> > > > > > - uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > > + uint8_t supportedHWLevel = externalCameraSupport_ ?\n> > > > > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL :\n> > > > > > + ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > > > > staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,\n> > > > > > &supportedHWLevel, 1);\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > > index 4905958e..f96934db 100644\n> > > > > > --- a/src/android/camera_device.h\n> > > > > > +++ b/src/android/camera_device.h\n> > > > > > @@ -33,7 +33,8 @@ class CameraDevice : protected libcamera::Loggable\n> > > > > > {\n> > > > > > public:\n> > > > > > static std::shared_ptr create(unsigned int id,\n> > > > > > - const std::shared_ptr &cam);\n> > > > > > + const std::shared_ptr &cam,\n> > > > > > + bool externalCameraSupport);\n> > > > > > ~CameraDevice();\n> > > > > >\n> > > > > > int initialize();\n> > > > > > @@ -66,7 +67,8 @@ protected:\n> > > > > > std::string logPrefix() const override;\n> > > > > >\n> > > > > > private:\n> > > > > > - CameraDevice(unsigned int id, const std::shared_ptr &camera);\n> > > > > > + CameraDevice(unsigned int id, const std::shared_ptr &camera,\n> > > > > > + bool externalCameraSupport);\n> > > > > >\n> > > > > > struct Camera3RequestDescriptor {\n> > > > > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > > > > @@ -130,6 +132,8 @@ private:\n> > > > > > unsigned int maxJpegBufferSize_;\n> > > > > >\n> > > > > > CameraMetadata lastSettings_;\n> > > > > > +\n> > > > > > + const bool externalCameraSupport_;\n> > > > > > };\n> > > > > >\n> > > > > > #endif /* __ANDROID_CAMERA_DEVICE_H__ */\n> > > > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > > > index 189eda2b..e4ca4c82 100644\n> > > > > > --- a/src/android/camera_hal_manager.cpp\n> > > > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > > > @@ -29,7 +29,8 @@ LOG_DECLARE_CATEGORY(HAL)\n> > > > > > */\n> > > > > >\n> > > > > > CameraHalManager::CameraHalManager()\n> > > > > > - : cameraManager_(nullptr), callbacks_(nullptr), numInternalCameras_(0),\n> > > > > > + : cameraManager_(nullptr), callbacks_(nullptr),\n> > > > > > + externalCameraSupport_(false), numInternalCameras_(0),\n> > > > > > nextExternalCameraId_(firstExternalCameraId_)\n> > > > > > {\n> > > > > > }\n> > > > > > @@ -115,7 +116,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > > * Now check if this is an external camera and assign\n> > > > > > * its id accordingly.\n> > > > > > */\n> > > > > > - if (cameraLocation(cam.get()) == properties::CameraLocationExternal) {\n> > > > > > + if (cameraLocation(cam.get()) == properties::CameraLocationExternal &&\n> > > > > > + externalCameraSupport_) {\n> > > > > > isCameraExternal = true;\n> > > > > > id = nextExternalCameraId_;\n> > > > > > } else {\n> > > > > > @@ -124,7 +126,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > > }\n> > > > > >\n> > > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > > > - std::shared_ptr camera = CameraDevice::create(id, std::move(cam));\n> > > > > > + std::shared_ptr camera =\n> > > > > > + CameraDevice::create(id, std::move(cam), externalCameraSupport_);\n> > > > > > int ret = camera->initialize();\n> > > > > > if (ret) {\n> > > > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > > > @@ -134,7 +137,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > > if (isCameraNew) {\n> > > > > > cameraIdsMap_.emplace(cam->id(), id);\n> > > > > >\n> > > > > > - if (isCameraExternal)\n> > > > > > + if (isCameraExternal && externalCameraSupport_)\n> > > > > > nextExternalCameraId_++;\n> > > > > > else\n> > > > > > numInternalCameras_++;\n> > > > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > > > index a91decc7..74dc6b3e 100644\n> > > > > > --- a/src/android/camera_hal_manager.h\n> > > > > > +++ b/src/android/camera_hal_manager.h\n> > > > > > @@ -54,6 +54,8 @@ private:\n> > > > > > std::map cameraIdsMap_;\n> > > > > > Mutex mutex_;\n> > > > > >\n> > > > > > + const bool externalCameraSupport_;\n> > > > > > +\n> > > > > > unsigned int numInternalCameras_;\n> > > > > > unsigned int nextExternalCameraId_;\n> > > > > > };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 C8398BD1F1\n\tfor ;\n\tMon, 8 Mar 2021 03:09:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EBA168A9A;\n\tMon, 8 Mar 2021 04:09:24 +0100 (CET)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF2E160106\n\tfor ;\n\tMon, 8 Mar 2021 04:09:22 +0100 (CET)","by mail-ej1-x631.google.com with SMTP id jt13so17394756ejb.0\n\tfor ;\n\tSun, 07 Mar 2021 19:09:22 -0800 (PST)","from mail-wm1-f54.google.com (mail-wm1-f54.google.com.\n\t[209.85.128.54]) by smtp.gmail.com with ESMTPSA id\n\tcw14sm6497177edb.8.2021.03.07.19.09.20\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 07 Mar 2021 19:09:20 -0800 (PST)","by mail-wm1-f54.google.com with SMTP id\n\tt5-20020a1c77050000b029010e62cea9deso2876100wmi.0\n\tfor ;\n\tSun, 07 Mar 2021 19:09:20 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"PCYdpg8T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=DtXPUFNBDCz1UXtON/AdK2uoAoWWkeJi2MzNYrU56vw=;\n\tb=PCYdpg8TzT0FwqpY1PFeDfwsafystvUiBE0Ni48RkVPIJJ3jhKwZyGeitkH/gulhs0\n\tAOs+Q8lqsBYJVkm15f3Ep2VfrLiHHfdUg71s5/ppQ1TToisv3i2X+spkaJceeQVRpPaR\n\tNHlsWKZR2wNTWy+KDQ02w4ayFJENsbJe0iH/c=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=DtXPUFNBDCz1UXtON/AdK2uoAoWWkeJi2MzNYrU56vw=;\n\tb=EcL3K1tLL025jYVo8qTcdnjQukDt0EgUmDn4F32tmFlYhPQHnbiGIdHX1vKEhxaosI\n\txykL98pXGflLslC4kQidlqVZ6FRkDI1WdOiNkKN3QoW6vQnhxd8wYGTQWRXW2bprfRz6\n\t8yNuy9KV9cc7LzBXmNiio+ImReFCGi6VlzlWKYonUQT+2K0u9cQuckgMg7QFc3Zm7WZP\n\tHXCZ4ReYqxvRKr9/bcvgOJkaCs3eKxurWLgkiR9gVKCsBdbysYK8rucjqOdehUJ0iaQ9\n\tSL1WSNEqQ49lDu9uKQUBUuNmGsOYcEpP/AXJYLRvGJj7Yx8NvSmNLevqcaMyX2YjxGYn\n\tJ1wg==","X-Gm-Message-State":"AOAM532fQw0CRBtvo38AqmiD43RpJMql/FftoyQolxc/6iuBsEEwnPp6\n\tRhIB7pwL85nkTE7NVDFvOF//hViUmXv04w==","X-Google-Smtp-Source":"ABdhPJzjLuYgpc29fMFNtyDvYIZ9n0abiz/Hc6kDrqJGHTN6cZDoNuREI7RhA97OP+Cy2N3aaQ1xHQ==","X-Received":["by 2002:a17:907:f97:: with SMTP id\n\tkb23mr12898044ejc.33.1615172961945; \n\tSun, 07 Mar 2021 19:09:21 -0800 (PST)","by 2002:a1c:c282:: with SMTP id\n\ts124mr19594611wmf.99.1615172959654; \n\tSun, 07 Mar 2021 19:09:19 -0800 (PST)"],"MIME-Version":"1.0","References":"<20210303093756.490065-1-paul.elder@ideasonboard.com>\n\t<20210303133401.5fkrlfgnknnf35li@uno.localdomain>\n\t<20210304085659.GC2049@pyrite.rasen.tech>\n\t<20210304094736.bjz3kho6pjc6bir3@uno.localdomain>\n\t\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Mon, 8 Mar 2021 12:09:08 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] android: Add flag for external camera\n\tsupport","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]