[{"id":16036,"web_url":"https://patchwork.libcamera.org/comment/16036/","msgid":"","date":"2021-03-30T00:17:20","subject":"Re: [libcamera-devel] [PATCH v2 5/6] android: camera_device: Get\n\tlocation from config","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Mar 29, 2021 at 05:28:06PM +0200, Jacopo Mondi wrote:\n> Create the CameraDevice with a reference to the HAL configuration\n> file and use it to retrieve the camera location if not available\n> from the Camera.\n\nOverall this looks good, but I still think we should pass the camera\nconfiguration to the CameraDevice class, not the full CameraHalConfig.\nI'll comment further on this topic in the review of 4/6.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 18 +++++++++---------\n> src/android/camera_device.h | 8 ++++++--\n> src/android/camera_hal_manager.cpp | 3 ++-\n> 3 files changed, 17 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ae6936647660..1731fe166887 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -309,9 +309,10 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> * back to the framework using the designated callbacks.\n> */\n> \n> -CameraDevice::CameraDevice(unsigned int id, std::shared_ptr camera)\n> +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr camera,\n> +\t\t\t CameraHalConfig &halConfig)\n> \t: id_(id), running_(false), camera_(std::move(camera)),\n> -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> +\t halConfig_(halConfig), facing_(CAMERA_FACING_FRONT), orientation_(0)\n> {\n> \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> \n> @@ -341,10 +342,11 @@ CameraDevice::CameraDevice(unsigned int id, std::shared_ptr camera)\n> CameraDevice::~CameraDevice() = default;\n> \n> std::unique_ptr CameraDevice::create(unsigned int id,\n> -\t\t\t\t\t\t std::shared_ptr cam)\n> +\t\t\t\t\t\t std::shared_ptr cam,\n> +\t\t\t\t\t\t CameraHalConfig &halConfig)\n> {\n> \treturn std::unique_ptr(\n> -\t\tnew CameraDevice(id, std::move(cam)));\n> +\t\tnew CameraDevice(id, std::move(cam), halConfig));\n> }\n> \n> /*\n> @@ -370,11 +372,9 @@ int CameraDevice::initialize()\n> \t\t\tbreak;\n> \t\t}\n> \t} else {\n> -\t\t/*\n> -\t\t * \\todo Retrieve the camera location from configuration file\n> -\t\t * if not available from the library.\n> -\t\t */\n> -\t\tfacing_ = CAMERA_FACING_FRONT;\n> +\t\tfacing_ = halConfig_.cameraLocation(camera_->id());\n> +\t\tif (facing_ < 0)\n> +\t\t\treturn facing_;\n> \t}\n> \n> \t/*\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 11bdfec8d587..6355e8d8c26a 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -24,6 +24,7 @@\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/message.h\"\n> \n> +#include \"camera_hal_config.h\"\n> #include \"camera_metadata.h\"\n> #include \"camera_stream.h\"\n> #include \"camera_worker.h\"\n> @@ -33,7 +34,8 @@ class CameraDevice : protected libcamera::Loggable\n> {\n> public:\n> \tstatic std::unique_ptr create(unsigned int id,\n> -\t\t\t\t\t\t std::shared_ptr cam);\n> +\t\t\t\t\t\t std::shared_ptr cam,\n> +\t\t\t\t\t\t CameraHalConfig &halConfig);\n> \t~CameraDevice();\n> \n> \tint initialize();\n> @@ -66,7 +68,8 @@ protected:\n> \tstd::string logPrefix() const override;\n> \n> private:\n> -\tCameraDevice(unsigned int id, std::shared_ptr camera);\n> +\tCameraDevice(unsigned int id, std::shared_ptr camera,\n> +\t\t CameraHalConfig &halConfig);\n> \n> \tstruct Camera3RequestDescriptor {\n> \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> @@ -113,6 +116,7 @@ private:\n> \tbool running_;\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> +\tconst CameraHalConfig &halConfig_;\n> \n> \tstd::unique_ptr staticMetadata_;\n> \tstd::map> requestTemplates_;\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index f79789b5bfb8..9ff7534a16f3 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -120,7 +120,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t}\n> \n> \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> -\tstd::unique_ptr camera = CameraDevice::create(id, cam);\n> +\tstd::unique_ptr camera = CameraDevice::create(id, cam,\n> +\t\t\t\t\t\t\t\t halConfig_);\n> \tint ret = camera->initialize();\n> \tif (ret) {\n> \t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();","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 255C6C32F0\n\tfor ;\n\tTue, 30 Mar 2021 00:18:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F3E968782;\n\tTue, 30 Mar 2021 02:18:06 +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 042D9602D7\n\tfor ;\n\tTue, 30 Mar 2021 02:18:04 +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 60F86102;\n\tTue, 30 Mar 2021 02:18:04 +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=\"s3aB4ITf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617063484;\n\tbh=zdGjYUokrVeTkNxNiLkfiOURgQPGTZ6rLGjAW8mihMI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s3aB4ITfJ0WUaj190Xf3Q+1YxjmSSwAXYfD8OvLbJM9ceLnNoPqT9wXcdh9DJMCfA\n\tJK/WR5Oy0RxEl8Uxt+mJyvE/yz7aGpUsqerb49rpJ5vG8j6GO07cqLMX9jX7DoQAqn\n\tAbEIiBTSB9FvuhCnEEiBUx+maAuAhRGYC/8s5Fyw=","Date":"Tue, 30 Mar 2021 03:17:20 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329152807.28331-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] android: camera_device: Get\n\tlocation from 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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16037,"web_url":"https://patchwork.libcamera.org/comment/16037/","msgid":"","date":"2021-03-30T00:17:41","subject":"Re: [libcamera-devel] [PATCH v2 5/6] android: camera_device: Get\n\tlocation from config","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Mar 30, 2021 at 03:17:21AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 29, 2021 at 05:28:06PM +0200, Jacopo Mondi wrote:\n> > Create the CameraDevice with a reference to the HAL configuration\n> > file and use it to retrieve the camera location if not available\n> > from the Camera.\n> \n> Overall this looks good, but I still think we should pass the camera\n> configuration to the CameraDevice class, not the full CameraHalConfig.\n> I'll comment further on this topic in the review of 4/6.\n\nSorry, I meant 3/6.\n\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 18 +++++++++---------\n> > src/android/camera_device.h | 8 ++++++--\n> > src/android/camera_hal_manager.cpp | 3 ++-\n> > 3 files changed, 17 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ae6936647660..1731fe166887 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -309,9 +309,10 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > * back to the framework using the designated callbacks.\n> > */\n> > \n> > -CameraDevice::CameraDevice(unsigned int id, std::shared_ptr camera)\n> > +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr camera,\n> > +\t\t\t CameraHalConfig &halConfig)\n> > \t: id_(id), running_(false), camera_(std::move(camera)),\n> > -\t facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > +\t halConfig_(halConfig), facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > {\n> > \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> > \n> > @@ -341,10 +342,11 @@ CameraDevice::CameraDevice(unsigned int id, std::shared_ptr camera)\n> > CameraDevice::~CameraDevice() = default;\n> > \n> > std::unique_ptr CameraDevice::create(unsigned int id,\n> > -\t\t\t\t\t\t std::shared_ptr cam)\n> > +\t\t\t\t\t\t std::shared_ptr cam,\n> > +\t\t\t\t\t\t CameraHalConfig &halConfig)\n> > {\n> > \treturn std::unique_ptr(\n> > -\t\tnew CameraDevice(id, std::move(cam)));\n> > +\t\tnew CameraDevice(id, std::move(cam), halConfig));\n> > }\n> > \n> > /*\n> > @@ -370,11 +372,9 @@ int CameraDevice::initialize()\n> > \t\t\tbreak;\n> > \t\t}\n> > \t} else {\n> > -\t\t/*\n> > -\t\t * \\todo Retrieve the camera location from configuration file\n> > -\t\t * if not available from the library.\n> > -\t\t */\n> > -\t\tfacing_ = CAMERA_FACING_FRONT;\n> > +\t\tfacing_ = halConfig_.cameraLocation(camera_->id());\n> > +\t\tif (facing_ < 0)\n> > +\t\t\treturn facing_;\n> > \t}\n> > \n> > \t/*\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 11bdfec8d587..6355e8d8c26a 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -24,6 +24,7 @@\n> > #include \"libcamera/internal/log.h\"\n> > #include \"libcamera/internal/message.h\"\n> > \n> > +#include \"camera_hal_config.h\"\n> > #include \"camera_metadata.h\"\n> > #include \"camera_stream.h\"\n> > #include \"camera_worker.h\"\n> > @@ -33,7 +34,8 @@ class CameraDevice : protected libcamera::Loggable\n> > {\n> > public:\n> > \tstatic std::unique_ptr create(unsigned int id,\n> > -\t\t\t\t\t\t std::shared_ptr cam);\n> > +\t\t\t\t\t\t std::shared_ptr cam,\n> > +\t\t\t\t\t\t CameraHalConfig &halConfig);\n> > \t~CameraDevice();\n> > \n> > \tint initialize();\n> > @@ -66,7 +68,8 @@ protected:\n> > \tstd::string logPrefix() const override;\n> > \n> > private:\n> > -\tCameraDevice(unsigned int id, std::shared_ptr camera);\n> > +\tCameraDevice(unsigned int id, std::shared_ptr camera,\n> > +\t\t CameraHalConfig &halConfig);\n> > \n> > \tstruct Camera3RequestDescriptor {\n> > \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > @@ -113,6 +116,7 @@ private:\n> > \tbool running_;\n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;\n> > +\tconst CameraHalConfig &halConfig_;\n> > \n> > \tstd::unique_ptr staticMetadata_;\n> > \tstd::map> requestTemplates_;\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > index f79789b5bfb8..9ff7534a16f3 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -120,7 +120,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > \t}\n> > \n> > \t/* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > -\tstd::unique_ptr camera = CameraDevice::create(id, cam);\n> > +\tstd::unique_ptr camera = CameraDevice::create(id, cam,\n> > +\t\t\t\t\t\t\t\t halConfig_);\n> > \tint ret = camera->initialize();\n> > \tif (ret) {\n> > \t\tLOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();","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 87970C32F0\n\tfor ;\n\tTue, 30 Mar 2021 00:18:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43B9A68782;\n\tTue, 30 Mar 2021 02:18:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A75A602D7\n\tfor ;\n\tTue, 30 Mar 2021 02:18:26 +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 09CB7102;\n\tTue, 30 Mar 2021 02:18:25 +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=\"JFSB3gzV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617063506;\n\tbh=JnKbQBO4vqfoHPGkOzRTNNFlxCbmcXWTNgSHbx4stgw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JFSB3gzV1Mba7tNwMptKfTN1fySLcJ0Zx3oc4qsA2sTLgWDb0D3H33lzZbXILf2V7\n\tc32nQCMtDaKT90pPXFATdk8oBwTX2OLGXu1B5skAQK3KWJ30O1POiOub7EiQudrQ3y\n\tdfx3cjDm5rff6shRBAC6gRurdHqspKY3qRBsLX3Y=","Date":"Tue, 30 Mar 2021 03:17:41 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210329152807.28331-1-jacopo@jmondi.org>\n\t<20210329152807.28331-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] android: camera_device: Get\n\tlocation from 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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]