[{"id":16072,"web_url":"https://patchwork.libcamera.org/comment/16072/","msgid":"","date":"2021-03-31T09:12:10","subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thanks for the patch.\n\nOn Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi wrote:\n>\n> Open the HAL configuration file in the Camera HAL manager and get\n> the camera properties for each created CameraDevice and initialize it\n> with them.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 9 +++------\n> src/android/camera_device.h | 3 ++-\n> src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--\n> src/android/camera_hal_manager.h | 3 +++\n> 4 files changed, 27 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index eb327978c84e..e59f25c68d6b 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -6,6 +6,7 @@\n> */\n>\n> #include \"camera_device.h\"\n> +#include \"camera_hal_config.h\"\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n>\n> @@ -351,7 +352,7 @@ std::unique_ptr CameraDevice::create(unsigned int id,\n> * Initialize the camera static information.\n> * This method is called before the camera device is opened.\n> */\n> -int CameraDevice::initialize()\n> +int CameraDevice::initialize(const CameraProps &cameraProps)\n> {\n> /* Initialize orientation and facing side of the camera. */\n> const ControlList &properties = camera_->properties();\n> @@ -370,11 +371,7 @@ int CameraDevice::initialize()\n> break;\n> }\n> } else {\n> - /*\n> - * \\todo Retrieve the camera location from configuration file\n> - * if not available from the library.\n> - */\n> - facing_ = CAMERA_FACING_FRONT;\n> + facing_ = cameraProps.facing;\n> }\n>\n\nCan you explain the rule of updating facing_ and rotation_?\nHow do we prioritize camera_->properties() and cameraProps?\n\n-Hiro\n\n> /*\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 11bdfec8d587..ba3ec8770e11 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -29,6 +29,7 @@\n> #include \"camera_worker.h\"\n> #include \"jpeg/encoder.h\"\n>\n> +class CameraProps;\n> class CameraDevice : protected libcamera::Loggable\n> {\n> public:\n> @@ -36,7 +37,7 @@ public:\n> std::shared_ptr cam);\n> ~CameraDevice();\n>\n> - int initialize();\n> + int initialize(const CameraProps &cameraProps);\n>\n> int open(const hw_module_t *hardwareModule);\n> void close();\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index bf3fcda75237..a517727ea0b8 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default;\n>\n> int CameraHalManager::init()\n> {\n> + int ret = halConfig_.open();\n> + if (ret)\n> + return ret;\n> +\n> cameraManager_ = std::make_unique();\n>\n> /* Support camera hotplug. */\n> cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n>\n> - int ret = cameraManager_->start();\n> + ret = cameraManager_->start();\n> if (ret) {\n> LOG(HAL, Error) << \"Failed to start camera manager: \"\n> << strerror(-ret);\n> @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> }\n> }\n>\n> + /*\n> + * Get camera properties from the configuration file.\n> + *\n> + * The camera properties as recorded in the configuration file\n> + * supplement information that cannot be retrieved from the\n> + * libcamera::Camera at run time.\n> + */\n> + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());\n> + if (!cameraProps.valid) {\n> + LOG(HAL, Error) << \"Failed to register camera: \" << cam->id();\n> + return;\n> + }\n> +\n> /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> std::unique_ptr camera = CameraDevice::create(id, cam);\n> - int ret = camera->initialize();\n> + int ret = camera->initialize(cameraProps);\n> if (ret) {\n> LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> return;\n> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> index d9bf27989965..af1581da6579 100644\n> --- a/src/android/camera_hal_manager.h\n> +++ b/src/android/camera_hal_manager.h\n> @@ -19,6 +19,8 @@\n>\n> #include \n>\n> +#include \"camera_hal_config.h\"\n> +\n> class CameraDevice;\n>\n> class CameraHalManager\n> @@ -50,6 +52,7 @@ private:\n> CameraDevice *cameraDeviceFromHalId(unsigned int id);\n>\n> std::unique_ptr cameraManager_;\n> + CameraHalConfig halConfig_;\n>\n> const camera_module_callbacks_t *callbacks_;\n> std::vector> cameras_;\n> --\n> 2.30.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 734CDC0DA3\n\tfor ;\n\tWed, 31 Mar 2021 09:12:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAF7268785;\n\tWed, 31 Mar 2021 11:12:21 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5429602D7\n\tfor ;\n\tWed, 31 Mar 2021 11:12:20 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id x21so21420521eds.4\n\tfor ;\n\tWed, 31 Mar 2021 02:12:20 -0700 (PDT)"],"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=\"HAhTewrH\"; 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=Ar4CllS5+gS4sPjThO+8er8b9amqWFR8TXWWJW1+NzU=;\n\tb=HAhTewrHVB/KRX00/A/z8GmLvzdfRh7UNHXkzKhcwwuebj0gK3pa729wFRX+7z6r/k\n\tv7U35Pix6qC/Nf+ss7L1Z692Gyv8NuHSkMM2vk0x3Hgug++Dy4HnHihKghzxC2exRL5G\n\tG9R+6RwEEEx4UdCvSao6XYbhyZMAr5CunZL/k=","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=Ar4CllS5+gS4sPjThO+8er8b9amqWFR8TXWWJW1+NzU=;\n\tb=pMOoKmR/+TfHyprI0lYpLPX49ICzisb80XB5WNHdSx/OaLzN/qDHpKJ6BKfM4tRjNB\n\tyXemcdt/h2CJC56j3knfz1Cx+Zoome6li97MvHrBkqIjwlNQSUD2yi54MIvBjterSz0r\n\tvGV1O86s0WS49hjc7FRMb2Fri3kME/89VPqd9+2HUrZ5D4fv0cXj9FyBpH/iBPtQJc0M\n\tQqSOywFarPD7SrhQx5iSvElucVuk38E4AvsdkkjWw1upIc14FeWNHuiTa7teJZhoHaEC\n\tR375DMHYcqiDkV8zb8pwO4PniiI59hmHV3OvEb1ZJYNv5RRsMMKev2tQhexJtaFZdIlX\n\teLMQ==","X-Gm-Message-State":"AOAM533gv9WcM6yjSkTBDJaVasSbNA9CIV/nm73tjTZY6BREARTzKE4r\n\tbk8a1Vzk9hQaRn+kSpgujYCVoCZZ9eOn5A0tJ3udFsn1wsA=","X-Google-Smtp-Source":"ABdhPJzeVdR4OLNzSQOQR9g1NZxEtHN8Og6zHtkHMi60NY+f1okjiBT7RnjvVn4jDZ4Nnm1soUJV8QHm2eBXcd+GKp8=","X-Received":"by 2002:a50:fb10:: with SMTP id d16mr2464720edq.73.1617181940373;\n\tWed, 31 Mar 2021 02:12:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-5-jacopo@jmondi.org>","In-Reply-To":"<20210330142113.37457-5-jacopo@jmondi.org>","From":"Hirokazu Honda ","Date":"Wed, 31 Mar 2021 18:12:10 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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":16074,"web_url":"https://patchwork.libcamera.org/comment/16074/","msgid":"<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>","date":"2021-03-31T11:08:51","subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Mar 31, 2021 at 06:12:10PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thanks for the patch.\n>\n> On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi wrote:\n> >\n> > Open the HAL configuration file in the Camera HAL manager and get\n> > the camera properties for each created CameraDevice and initialize it\n> > with them.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 9 +++------\n> > src/android/camera_device.h | 3 ++-\n> > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--\n> > src/android/camera_hal_manager.h | 3 +++\n> > 4 files changed, 27 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index eb327978c84e..e59f25c68d6b 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> >\n> > #include \"camera_device.h\"\n> > +#include \"camera_hal_config.h\"\n> > #include \"camera_ops.h\"\n> > #include \"post_processor.h\"\n> >\n> > @@ -351,7 +352,7 @@ std::unique_ptr CameraDevice::create(unsigned int id,\n> > * Initialize the camera static information.\n> > * This method is called before the camera device is opened.\n> > */\n> > -int CameraDevice::initialize()\n> > +int CameraDevice::initialize(const CameraProps &cameraProps)\n> > {\n> > /* Initialize orientation and facing side of the camera. */\n> > const ControlList &properties = camera_->properties();\n> > @@ -370,11 +371,7 @@ int CameraDevice::initialize()\n> > break;\n> > }\n> > } else {\n> > - /*\n> > - * \\todo Retrieve the camera location from configuration file\n> > - * if not available from the library.\n> > - */\n> > - facing_ = CAMERA_FACING_FRONT;\n> > + facing_ = cameraProps.facing;\n> > }\n> >\n>\n> Can you explain the rule of updating facing_ and rotation_?\n> How do we prioritize camera_->properties() and cameraProps?\n\n\nDo you mean clarifying which one has precendence ?\n\nIf the information is available from the Camera (which means it has\nbeen collected from the firmware interface) then it has prcedence.\nOtherwise the configuration file is used as a fallback.\n\nThanks\n j\n\n>\n> -Hiro\n>\n> > /*\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 11bdfec8d587..ba3ec8770e11 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -29,6 +29,7 @@\n> > #include \"camera_worker.h\"\n> > #include \"jpeg/encoder.h\"\n> >\n> > +class CameraProps;\n> > class CameraDevice : protected libcamera::Loggable\n> > {\n> > public:\n> > @@ -36,7 +37,7 @@ public:\n> > std::shared_ptr cam);\n> > ~CameraDevice();\n> >\n> > - int initialize();\n> > + int initialize(const CameraProps &cameraProps);\n> >\n> > int open(const hw_module_t *hardwareModule);\n> > void close();\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > index bf3fcda75237..a517727ea0b8 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default;\n> >\n> > int CameraHalManager::init()\n> > {\n> > + int ret = halConfig_.open();\n> > + if (ret)\n> > + return ret;\n> > +\n> > cameraManager_ = std::make_unique();\n> >\n> > /* Support camera hotplug. */\n> > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> >\n> > - int ret = cameraManager_->start();\n> > + ret = cameraManager_->start();\n> > if (ret) {\n> > LOG(HAL, Error) << \"Failed to start camera manager: \"\n> > << strerror(-ret);\n> > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > }\n> > }\n> >\n> > + /*\n> > + * Get camera properties from the configuration file.\n> > + *\n> > + * The camera properties as recorded in the configuration file\n> > + * supplement information that cannot be retrieved from the\n> > + * libcamera::Camera at run time.\n> > + */\n> > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());\n> > + if (!cameraProps.valid) {\n> > + LOG(HAL, Error) << \"Failed to register camera: \" << cam->id();\n> > + return;\n> > + }\n> > +\n> > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > std::unique_ptr camera = CameraDevice::create(id, cam);\n> > - int ret = camera->initialize();\n> > + int ret = camera->initialize(cameraProps);\n> > if (ret) {\n> > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > return;\n> > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > index d9bf27989965..af1581da6579 100644\n> > --- a/src/android/camera_hal_manager.h\n> > +++ b/src/android/camera_hal_manager.h\n> > @@ -19,6 +19,8 @@\n> >\n> > #include \n> >\n> > +#include \"camera_hal_config.h\"\n> > +\n> > class CameraDevice;\n> >\n> > class CameraHalManager\n> > @@ -50,6 +52,7 @@ private:\n> > CameraDevice *cameraDeviceFromHalId(unsigned int id);\n> >\n> > std::unique_ptr cameraManager_;\n> > + CameraHalConfig halConfig_;\n> >\n> > const camera_module_callbacks_t *callbacks_;\n> > std::vector> cameras_;\n> > --\n> > 2.30.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 78CB7C0DA3\n\tfor ;\n\tWed, 31 Mar 2021 11:08:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB3976877F;\n\tWed, 31 Mar 2021 13:08:18 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E08F6051B\n\tfor ;\n\tWed, 31 Mar 2021 13:08:17 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 6CBD4100008;\n\tWed, 31 Mar 2021 11:08:16 +0000 (UTC)"],"Date":"Wed, 31 Mar 2021 13:08:51 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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":16077,"web_url":"https://patchwork.libcamera.org/comment/16077/","msgid":"","date":"2021-04-01T02:02:30","subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Wed, Mar 31, 2021 at 8:08 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Mar 31, 2021 at 06:12:10PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thanks for the patch.\n> >\n> > On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi wrote:\n> > >\n> > > Open the HAL configuration file in the Camera HAL manager and get\n> > > the camera properties for each created CameraDevice and initialize it\n> > > with them.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 9 +++------\n> > > src/android/camera_device.h | 3 ++-\n> > > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--\n> > > src/android/camera_hal_manager.h | 3 +++\n> > > 4 files changed, 27 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index eb327978c84e..e59f25c68d6b 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -6,6 +6,7 @@\n> > > */\n> > >\n> > > #include \"camera_device.h\"\n> > > +#include \"camera_hal_config.h\"\n> > > #include \"camera_ops.h\"\n> > > #include \"post_processor.h\"\n> > >\n> > > @@ -351,7 +352,7 @@ std::unique_ptr CameraDevice::create(unsigned int id,\n> > > * Initialize the camera static information.\n> > > * This method is called before the camera device is opened.\n> > > */\n> > > -int CameraDevice::initialize()\n> > > +int CameraDevice::initialize(const CameraProps &cameraProps)\n> > > {\n> > > /* Initialize orientation and facing side of the camera. */\n> > > const ControlList &properties = camera_->properties();\n> > > @@ -370,11 +371,7 @@ int CameraDevice::initialize()\n> > > break;\n> > > }\n> > > } else {\n> > > - /*\n> > > - * \\todo Retrieve the camera location from configuration file\n> > > - * if not available from the library.\n> > > - */\n> > > - facing_ = CAMERA_FACING_FRONT;\n> > > + facing_ = cameraProps.facing;\n> > > }\n> > >\n> >\n> > Can you explain the rule of updating facing_ and rotation_?\n> > How do we prioritize camera_->properties() and cameraProps?\n>\n>\n> Do you mean clarifying which one has precendence ?\n>\n> If the information is available from the Camera (which means it has\n> been collected from the firmware interface) then it has prcedence.\n> Otherwise the configuration file is used as a fallback.\n>\n\nYes, thanks for explaining.\nShall we add the comment in code about it?\n\nRegardless, the code change looks good.\n\nReviewed-by: Hirokazu Honda \n\n-Hiro\n\n> Thanks\n> j\n>\n> >\n> > -Hiro\n> >\n> > > /*\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 11bdfec8d587..ba3ec8770e11 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -29,6 +29,7 @@\n> > > #include \"camera_worker.h\"\n> > > #include \"jpeg/encoder.h\"\n> > >\n> > > +class CameraProps;\n> > > class CameraDevice : protected libcamera::Loggable\n> > > {\n> > > public:\n> > > @@ -36,7 +37,7 @@ public:\n> > > std::shared_ptr cam);\n> > > ~CameraDevice();\n> > >\n> > > - int initialize();\n> > > + int initialize(const CameraProps &cameraProps);\n> > >\n> > > int open(const hw_module_t *hardwareModule);\n> > > void close();\n> > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > index bf3fcda75237..a517727ea0b8 100644\n> > > --- a/src/android/camera_hal_manager.cpp\n> > > +++ b/src/android/camera_hal_manager.cpp\n> > > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default;\n> > >\n> > > int CameraHalManager::init()\n> > > {\n> > > + int ret = halConfig_.open();\n> > > + if (ret)\n> > > + return ret;\n> > > +\n> > > cameraManager_ = std::make_unique();\n> > >\n> > > /* Support camera hotplug. */\n> > > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> > > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> > >\n> > > - int ret = cameraManager_->start();\n> > > + ret = cameraManager_->start();\n> > > if (ret) {\n> > > LOG(HAL, Error) << \"Failed to start camera manager: \"\n> > > << strerror(-ret);\n> > > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > }\n> > > }\n> > >\n> > > + /*\n> > > + * Get camera properties from the configuration file.\n> > > + *\n> > > + * The camera properties as recorded in the configuration file\n> > > + * supplement information that cannot be retrieved from the\n> > > + * libcamera::Camera at run time.\n> > > + */\n> > > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());\n> > > + if (!cameraProps.valid) {\n> > > + LOG(HAL, Error) << \"Failed to register camera: \" << cam->id();\n> > > + return;\n> > > + }\n> > > +\n> > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > std::unique_ptr camera = CameraDevice::create(id, cam);\n> > > - int ret = camera->initialize();\n> > > + int ret = camera->initialize(cameraProps);\n> > > if (ret) {\n> > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > return;\n> > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > index d9bf27989965..af1581da6579 100644\n> > > --- a/src/android/camera_hal_manager.h\n> > > +++ b/src/android/camera_hal_manager.h\n> > > @@ -19,6 +19,8 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"camera_hal_config.h\"\n> > > +\n> > > class CameraDevice;\n> > >\n> > > class CameraHalManager\n> > > @@ -50,6 +52,7 @@ private:\n> > > CameraDevice *cameraDeviceFromHalId(unsigned int id);\n> > >\n> > > std::unique_ptr cameraManager_;\n> > > + CameraHalConfig halConfig_;\n> > >\n> > > const camera_module_callbacks_t *callbacks_;\n> > > std::vector> cameras_;\n> > > --\n> > > 2.30.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 D4524C0DA4\n\tfor ;\n\tThu, 1 Apr 2021 02:02:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3463F6877C;\n\tThu, 1 Apr 2021 04:02:42 +0200 (CEST)","from mail-yb1-xb35.google.com (mail-yb1-xb35.google.com\n\t[IPv6:2607:f8b0:4864:20::b35])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FD21602D2\n\tfor ;\n\tThu, 1 Apr 2021 04:02:40 +0200 (CEST)","by mail-yb1-xb35.google.com with SMTP id m132so168504ybf.2\n\tfor ;\n\tWed, 31 Mar 2021 19:02:40 -0700 (PDT)"],"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=\"ewohLiw7\"; 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=bi67HXzg+MZDAb2hKQjc11mze8JMDZtNfEUaMbQB6ww=;\n\tb=ewohLiw7wZoEEnH95IqKnml2H+/rmRUj2o/t88zR9FRjQY17YMfYN3QvbqsPtla0R4\n\ty3ecCCKVWAqVAFJUof/CJB7sOREWfjNHH+jYAA3XeUHPKJVPMYBVkjOADO2lDd7G6OQm\n\tl/sWAlsjMgyutj2h6uZ1td1lzhdL6qgPC+Xsg=","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=bi67HXzg+MZDAb2hKQjc11mze8JMDZtNfEUaMbQB6ww=;\n\tb=FiPer5b6IDitTkGDAv+HeDAA6jV8uP8TOnyXvyd2DaGvH6R7GQ4nT99JNVW2EQ20G/\n\tt1DaNZb/8H+lSW/92fvIAeSeO4iFOzfxfg+RiICy7JQtdcJKlBfKOE+LKJ6s4oWPxw9i\n\t+IXWs2QoRYSfby9r0u1ibjRN46XrWiO2x5nokRijqqnl2R0fpS38hm7jEDWgL5aBFZGq\n\teHCNXVPjnPnjsiDaDHzj4UVvb/E+BBFqw7QiKz9C9gaHU802XkY5qby2Tf2K3rxhTnY+\n\tvdcqILHy0mWGSXYjW65R3ubHpUiMbLjBhJQd96Hizcilo4ZVqSsm59rgWG4Kyk/k/w3b\n\tIT6g==","X-Gm-Message-State":"AOAM533l5+L93ipPJ1Xc6TipAUVXUh8vSTVOx2TXU+mqDP393hPilYrl\n\tXyy4RQcQxmYCxT6k898ITgShzoBts8TregVNVo4gWA==","X-Google-Smtp-Source":"ABdhPJwlxx7C9bmK3K8KeqCRK1y7QPYiZnmMhzdHyqhRtGj1Kp0DPg8/5gTbsOWZSUam0WBZssCptcqZjgV/l0rUkEE=","X-Received":"by 2002:a25:d905:: with SMTP id q5mr8204325ybg.401.1617242558965;\n\tWed, 31 Mar 2021 19:02:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-5-jacopo@jmondi.org>\n\t\n\t<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>","In-Reply-To":"<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 1 Apr 2021 11:02:30 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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":16108,"web_url":"https://patchwork.libcamera.org/comment/16108/","msgid":"","date":"2021-04-03T03:08:48","subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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 Thu, Apr 01, 2021 at 11:02:30AM +0900, Hirokazu Honda wrote:\n> On Wed, Mar 31, 2021 at 8:08 PM Jacopo Mondi wrote:\n> > On Wed, Mar 31, 2021 at 06:12:10PM +0900, Hirokazu Honda wrote:\n> > > On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi wrote:\n> > > >\n> > > > Open the HAL configuration file in the Camera HAL manager and get\n> > > > the camera properties for each created CameraDevice and initialize it\n> > > > with them.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_device.cpp | 9 +++------\n> > > > src/android/camera_device.h | 3 ++-\n> > > > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--\n> > > > src/android/camera_hal_manager.h | 3 +++\n> > > > 4 files changed, 27 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index eb327978c84e..e59f25c68d6b 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -6,6 +6,7 @@\n> > > > */\n> > > >\n> > > > #include \"camera_device.h\"\n> > > > +#include \"camera_hal_config.h\"\n> > > > #include \"camera_ops.h\"\n> > > > #include \"post_processor.h\"\n> > > >\n> > > > @@ -351,7 +352,7 @@ std::unique_ptr CameraDevice::create(unsigned int id,\n> > > > * Initialize the camera static information.\n> > > > * This method is called before the camera device is opened.\n> > > > */\n> > > > -int CameraDevice::initialize()\n> > > > +int CameraDevice::initialize(const CameraProps &cameraProps)\n> > > > {\n> > > > /* Initialize orientation and facing side of the camera. */\n> > > > const ControlList &properties = camera_->properties();\n> > > > @@ -370,11 +371,7 @@ int CameraDevice::initialize()\n> > > > break;\n> > > > }\n> > > > } else {\n> > > > - /*\n> > > > - * \\todo Retrieve the camera location from configuration file\n> > > > - * if not available from the library.\n> > > > - */\n> > > > - facing_ = CAMERA_FACING_FRONT;\n> > > > + facing_ = cameraProps.facing;\n\nShould we guard against the case where the property isn't specified in\nthe configuration file either ? How about the case where it's specified\nbut has a different value than the one retrieved from the firmware, I\nwonder if we should warn the user ?\n\n> > > > }\n> > > >\n> > >\n> > > Can you explain the rule of updating facing_ and rotation_?\n> > > How do we prioritize camera_->properties() and cameraProps?\n> >\n> > Do you mean clarifying which one has precendence ?\n> >\n> > If the information is available from the Camera (which means it has\n> > been collected from the firmware interface) then it has prcedence.\n> > Otherwise the configuration file is used as a fallback.\n> \n> Yes, thanks for explaining.\n> Shall we add the comment in code about it?\n\nA comment could be nice indeed.\n\n> Regardless, the code change looks good.\n> \n> Reviewed-by: Hirokazu Honda \n> \n> > > > /*\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index 11bdfec8d587..ba3ec8770e11 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -29,6 +29,7 @@\n> > > > #include \"camera_worker.h\"\n> > > > #include \"jpeg/encoder.h\"\n> > > >\n> > > > +class CameraProps;\n> > > > class CameraDevice : protected libcamera::Loggable\n> > > > {\n> > > > public:\n> > > > @@ -36,7 +37,7 @@ public:\n> > > > std::shared_ptr cam);\n> > > > ~CameraDevice();\n> > > >\n> > > > - int initialize();\n> > > > + int initialize(const CameraProps &cameraProps);\n> > > >\n> > > > int open(const hw_module_t *hardwareModule);\n> > > > void close();\n> > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > index bf3fcda75237..a517727ea0b8 100644\n> > > > --- a/src/android/camera_hal_manager.cpp\n> > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default;\n> > > >\n> > > > int CameraHalManager::init()\n> > > > {\n> > > > + int ret = halConfig_.open();\n> > > > + if (ret)\n> > > > + return ret;\n> > > > +\n> > > > cameraManager_ = std::make_unique();\n> > > >\n> > > > /* Support camera hotplug. */\n> > > > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> > > > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> > > >\n> > > > - int ret = cameraManager_->start();\n> > > > + ret = cameraManager_->start();\n> > > > if (ret) {\n> > > > LOG(HAL, Error) << \"Failed to start camera manager: \"\n> > > > << strerror(-ret);\n> > > > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > }\n> > > > }\n> > > >\n> > > > + /*\n> > > > + * Get camera properties from the configuration file.\n> > > > + *\n> > > > + * The camera properties as recorded in the configuration file\n> > > > + * supplement information that cannot be retrieved from the\n> > > > + * libcamera::Camera at run time.\n> > > > + */\n> > > > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());\n> > > > + if (!cameraProps.valid) {\n> > > > + LOG(HAL, Error) << \"Failed to register camera: \" << cam->id();\n> > > > + return;\n> > > > + }\n\nWhat if we have a fully populated libcamera::Camera (with properties\nretrieved from the firmware) ? Should we still require a configuration\nfile then ? The configuration file will likely be mandatory in the\nfuture to provide additional information that the firmware doesn't\nprovide (such as lens-related information for instance), so I suppose\nit's fine to already make it mandatory.\n\nWe should however not make the configuration mandatory for external\ncameras, this would break UVC support.\n\nRelated to breakages, we need to also ensure that the configuration file\nwill get deployed correctly in Chrome OS builds of libcamera, or the\nexisting tests will start failing. I'm not sure how that's best handled,\nHiro, maybe this is something that you could help us with ?\n\n> > > > +\n> > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > std::unique_ptr camera = CameraDevice::create(id, cam);\n> > > > - int ret = camera->initialize();\n> > > > + int ret = camera->initialize(cameraProps);\n> > > > if (ret) {\n> > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > return;\n> > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > index d9bf27989965..af1581da6579 100644\n> > > > --- a/src/android/camera_hal_manager.h\n> > > > +++ b/src/android/camera_hal_manager.h\n> > > > @@ -19,6 +19,8 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \"camera_hal_config.h\"\n> > > > +\n> > > > class CameraDevice;\n> > > >\n> > > > class CameraHalManager\n> > > > @@ -50,6 +52,7 @@ private:\n> > > > CameraDevice *cameraDeviceFromHalId(unsigned int id);\n> > > >\n> > > > std::unique_ptr cameraManager_;\n> > > > + CameraHalConfig halConfig_;\n> > > >\n> > > > const camera_module_callbacks_t *callbacks_;\n> > > > std::vector> cameras_;","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 CC3BCC0DA3\n\tfor ;\n\tSat, 3 Apr 2021 03:09:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3BBA868788;\n\tSat, 3 Apr 2021 05:09:35 +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 64B68602D7\n\tfor ;\n\tSat, 3 Apr 2021 05:09:33 +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 C522C3D7;\n\tSat, 3 Apr 2021 05:09:32 +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=\"N92hep1b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617419373;\n\tbh=J4bsF7iQ2j/qlaHQt3gE1Mjq755YW406Pq7npVRCLKU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=N92hep1bTEPIGHuIwJCLwHTeS7X48fJcePAasM00jBZ8LwSOh02CDjZU9pe3QChxZ\n\tdb3M4tpEmakfzZjmshZdRYc742AML8JhDASKAmYnWRpeb3oxeYeGP69PUbmBFX/Iv/\n\tiLYFCYUf/l9ahyKSvcKzSRv60X2GHesUa0epA9L4=","Date":"Sat, 3 Apr 2021 06:08:48 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-5-jacopo@jmondi.org>\n\t\n\t<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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":16126,"web_url":"https://patchwork.libcamera.org/comment/16126/","msgid":"<20210406135321.2gwibiku5sjhlplm@uno.localdomain>","date":"2021-04-06T13:53:21","subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Apr 03, 2021 at 06:08:48AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 01, 2021 at 11:02:30AM +0900, Hirokazu Honda wrote:\n> > On Wed, Mar 31, 2021 at 8:08 PM Jacopo Mondi wrote:\n> > > On Wed, Mar 31, 2021 at 06:12:10PM +0900, Hirokazu Honda wrote:\n> > > > On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi wrote:\n> > > > >\n> > > > > Open the HAL configuration file in the Camera HAL manager and get\n> > > > > the camera properties for each created CameraDevice and initialize it\n> > > > > with them.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 9 +++------\n> > > > > src/android/camera_device.h | 3 ++-\n> > > > > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--\n> > > > > src/android/camera_hal_manager.h | 3 +++\n> > > > > 4 files changed, 27 insertions(+), 9 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index eb327978c84e..e59f25c68d6b 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -6,6 +6,7 @@\n> > > > > */\n> > > > >\n> > > > > #include \"camera_device.h\"\n> > > > > +#include \"camera_hal_config.h\"\n> > > > > #include \"camera_ops.h\"\n> > > > > #include \"post_processor.h\"\n> > > > >\n> > > > > @@ -351,7 +352,7 @@ std::unique_ptr CameraDevice::create(unsigned int id,\n> > > > > * Initialize the camera static information.\n> > > > > * This method is called before the camera device is opened.\n> > > > > */\n> > > > > -int CameraDevice::initialize()\n> > > > > +int CameraDevice::initialize(const CameraProps &cameraProps)\n> > > > > {\n> > > > > /* Initialize orientation and facing side of the camera. */\n> > > > > const ControlList &properties = camera_->properties();\n> > > > > @@ -370,11 +371,7 @@ int CameraDevice::initialize()\n> > > > > break;\n> > > > > }\n> > > > > } else {\n> > > > > - /*\n> > > > > - * \\todo Retrieve the camera location from configuration file\n> > > > > - * if not available from the library.\n> > > > > - */\n> > > > > - facing_ = CAMERA_FACING_FRONT;\n> > > > > + facing_ = cameraProps.facing;\n>\n> Should we guard against the case where the property isn't specified in\n> the configuration file either ? How about the case where it's specified\n\nI think we should assume the configuration file is complete and\ncorrect. It's structure is validated at parsing time, we should make\nsure all mandatory information are there by validating it through a\nschema ?\n\n> but has a different value than the one retrieved from the firmware, I\n> wonder if we should warn the user ?\n>\n\nIf the information is available from firmware it takes precedence and\nwe don't get here at all\n\n> > > > > }\n> > > > >\n> > > >\n> > > > Can you explain the rule of updating facing_ and rotation_?\n> > > > How do we prioritize camera_->properties() and cameraProps?\n> > >\n> > > Do you mean clarifying which one has precendence ?\n> > >\n> > > If the information is available from the Camera (which means it has\n> > > been collected from the firmware interface) then it has prcedence.\n> > > Otherwise the configuration file is used as a fallback.\n> >\n> > Yes, thanks for explaining.\n> > Shall we add the comment in code about it?\n>\n> A comment could be nice indeed.\n>\n\nand that's the reason why the code layout is self-explanatory enough\nthat I considered it was not worth a comment. But it's cheap, I can\nadd one.\n\n> > Regardless, the code change looks good.\n> >\n> > Reviewed-by: Hirokazu Honda \n> >\n> > > > > /*\n> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > index 11bdfec8d587..ba3ec8770e11 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -29,6 +29,7 @@\n> > > > > #include \"camera_worker.h\"\n> > > > > #include \"jpeg/encoder.h\"\n> > > > >\n> > > > > +class CameraProps;\n> > > > > class CameraDevice : protected libcamera::Loggable\n> > > > > {\n> > > > > public:\n> > > > > @@ -36,7 +37,7 @@ public:\n> > > > > std::shared_ptr cam);\n> > > > > ~CameraDevice();\n> > > > >\n> > > > > - int initialize();\n> > > > > + int initialize(const CameraProps &cameraProps);\n> > > > >\n> > > > > int open(const hw_module_t *hardwareModule);\n> > > > > void close();\n> > > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > > index bf3fcda75237..a517727ea0b8 100644\n> > > > > --- a/src/android/camera_hal_manager.cpp\n> > > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default;\n> > > > >\n> > > > > int CameraHalManager::init()\n> > > > > {\n> > > > > + int ret = halConfig_.open();\n> > > > > + if (ret)\n> > > > > + return ret;\n> > > > > +\n> > > > > cameraManager_ = std::make_unique();\n> > > > >\n> > > > > /* Support camera hotplug. */\n> > > > > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> > > > > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> > > > >\n> > > > > - int ret = cameraManager_->start();\n> > > > > + ret = cameraManager_->start();\n> > > > > if (ret) {\n> > > > > LOG(HAL, Error) << \"Failed to start camera manager: \"\n> > > > > << strerror(-ret);\n> > > > > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > + /*\n> > > > > + * Get camera properties from the configuration file.\n> > > > > + *\n> > > > > + * The camera properties as recorded in the configuration file\n> > > > > + * supplement information that cannot be retrieved from the\n> > > > > + * libcamera::Camera at run time.\n> > > > > + */\n> > > > > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());\n> > > > > + if (!cameraProps.valid) {\n> > > > > + LOG(HAL, Error) << \"Failed to register camera: \" << cam->id();\n> > > > > + return;\n> > > > > + }\n>\n> What if we have a fully populated libcamera::Camera (with properties\n> retrieved from the firmware) ? Should we still require a configuration\n> file then ? The configuration file will likely be mandatory in the\n> future to provide additional information that the firmware doesn't\n> provide (such as lens-related information for instance), so I suppose\n> it's fine to already make it mandatory.\n>\n> We should however not make the configuration mandatory for external\n> cameras, this would break UVC support.\n>\n> Related to breakages, we need to also ensure that the configuration file\n> will get deployed correctly in Chrome OS builds of libcamera, or the\n> existing tests will start failing. I'm not sure how that's best handled,\n> Hiro, maybe this is something that you could help us with ?\n>\n> > > > > +\n> > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > > std::unique_ptr camera = CameraDevice::create(id, cam);\n> > > > > - int ret = camera->initialize();\n> > > > > + int ret = camera->initialize(cameraProps);\n> > > > > if (ret) {\n> > > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > > return;\n> > > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > > index d9bf27989965..af1581da6579 100644\n> > > > > --- a/src/android/camera_hal_manager.h\n> > > > > +++ b/src/android/camera_hal_manager.h\n> > > > > @@ -19,6 +19,8 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \"camera_hal_config.h\"\n> > > > > +\n> > > > > class CameraDevice;\n> > > > >\n> > > > > class CameraHalManager\n> > > > > @@ -50,6 +52,7 @@ private:\n> > > > > CameraDevice *cameraDeviceFromHalId(unsigned int id);\n> > > > >\n> > > > > std::unique_ptr cameraManager_;\n> > > > > + CameraHalConfig halConfig_;\n> > > > >\n> > > > > const camera_module_callbacks_t *callbacks_;\n> > > > > std::vector> cameras_;\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 1C683C0DA3\n\tfor ;\n\tTue, 6 Apr 2021 13:52:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 915716879F;\n\tTue, 6 Apr 2021 15:52:47 +0200 (CEST)","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 212026084F\n\tfor ;\n\tTue, 6 Apr 2021 15:52:46 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 30EBD40022;\n\tTue, 6 Apr 2021 13:52:44 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 6 Apr 2021 15:53:21 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210406135321.2gwibiku5sjhlplm@uno.localdomain>","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-5-jacopo@jmondi.org>\n\t\n\t<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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":16127,"web_url":"https://patchwork.libcamera.org/comment/16127/","msgid":"","date":"2021-04-06T14:00:32","subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Apr 06, 2021 at 03:53:21PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 03, 2021 at 06:08:48AM +0300, Laurent Pinchart wrote:\n> > On Thu, Apr 01, 2021 at 11:02:30AM +0900, Hirokazu Honda wrote:\n> > > On Wed, Mar 31, 2021 at 8:08 PM Jacopo Mondi wrote:\n> > > > On Wed, Mar 31, 2021 at 06:12:10PM +0900, Hirokazu Honda wrote:\n> > > > > On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Open the HAL configuration file in the Camera HAL manager and get\n> > > > > > the camera properties for each created CameraDevice and initialize it\n> > > > > > with them.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 9 +++------\n> > > > > > src/android/camera_device.h | 3 ++-\n> > > > > > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--\n> > > > > > src/android/camera_hal_manager.h | 3 +++\n> > > > > > 4 files changed, 27 insertions(+), 9 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index eb327978c84e..e59f25c68d6b 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -6,6 +6,7 @@\n> > > > > > */\n> > > > > >\n> > > > > > #include \"camera_device.h\"\n> > > > > > +#include \"camera_hal_config.h\"\n> > > > > > #include \"camera_ops.h\"\n> > > > > > #include \"post_processor.h\"\n> > > > > >\n> > > > > > @@ -351,7 +352,7 @@ std::unique_ptr CameraDevice::create(unsigned int id,\n> > > > > > * Initialize the camera static information.\n> > > > > > * This method is called before the camera device is opened.\n> > > > > > */\n> > > > > > -int CameraDevice::initialize()\n> > > > > > +int CameraDevice::initialize(const CameraProps &cameraProps)\n> > > > > > {\n> > > > > > /* Initialize orientation and facing side of the camera. */\n> > > > > > const ControlList &properties = camera_->properties();\n> > > > > > @@ -370,11 +371,7 @@ int CameraDevice::initialize()\n> > > > > > break;\n> > > > > > }\n> > > > > > } else {\n> > > > > > - /*\n> > > > > > - * \\todo Retrieve the camera location from configuration file\n> > > > > > - * if not available from the library.\n> > > > > > - */\n> > > > > > - facing_ = CAMERA_FACING_FRONT;\n> > > > > > + facing_ = cameraProps.facing;\n> >\n> > Should we guard against the case where the property isn't specified in\n> > the configuration file either ? How about the case where it's specified\n> \n> I think we should assume the configuration file is complete and\n> correct. It's structure is validated at parsing time, we should make\n> sure all mandatory information are there by validating it through a\n> schema ?\n\nThat would then require the property to be present even on platforms\nthat report it correctly in the firmware, which would kind of defeat the\npoint of exposing it in the firmware :-)\n\n> > but has a different value than the one retrieved from the firmware, I\n> > wonder if we should warn the user ?\n> \n> If the information is available from firmware it takes precedence and\n> we don't get here at all\n\nI know it takes precedence, but should we warn the user that the value\nfrom the configuration is different and ignored ?\n\n> > > > > > }\n> > > > > >\n> > > > >\n> > > > > Can you explain the rule of updating facing_ and rotation_?\n> > > > > How do we prioritize camera_->properties() and cameraProps?\n> > > >\n> > > > Do you mean clarifying which one has precendence ?\n> > > >\n> > > > If the information is available from the Camera (which means it has\n> > > > been collected from the firmware interface) then it has prcedence.\n> > > > Otherwise the configuration file is used as a fallback.\n> > >\n> > > Yes, thanks for explaining.\n> > > Shall we add the comment in code about it?\n> >\n> > A comment could be nice indeed.\n> \n> and that's the reason why the code layout is self-explanatory enough\n> that I considered it was not worth a comment. But it's cheap, I can\n> add one.\n> \n> > > Regardless, the code change looks good.\n> > >\n> > > Reviewed-by: Hirokazu Honda \n> > >\n> > > > > > /*\n> > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > > index 11bdfec8d587..ba3ec8770e11 100644\n> > > > > > --- a/src/android/camera_device.h\n> > > > > > +++ b/src/android/camera_device.h\n> > > > > > @@ -29,6 +29,7 @@\n> > > > > > #include \"camera_worker.h\"\n> > > > > > #include \"jpeg/encoder.h\"\n> > > > > >\n> > > > > > +class CameraProps;\n> > > > > > class CameraDevice : protected libcamera::Loggable\n> > > > > > {\n> > > > > > public:\n> > > > > > @@ -36,7 +37,7 @@ public:\n> > > > > > std::shared_ptr cam);\n> > > > > > ~CameraDevice();\n> > > > > >\n> > > > > > - int initialize();\n> > > > > > + int initialize(const CameraProps &cameraProps);\n> > > > > >\n> > > > > > int open(const hw_module_t *hardwareModule);\n> > > > > > void close();\n> > > > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > > > index bf3fcda75237..a517727ea0b8 100644\n> > > > > > --- a/src/android/camera_hal_manager.cpp\n> > > > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > > > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default;\n> > > > > >\n> > > > > > int CameraHalManager::init()\n> > > > > > {\n> > > > > > + int ret = halConfig_.open();\n> > > > > > + if (ret)\n> > > > > > + return ret;\n> > > > > > +\n> > > > > > cameraManager_ = std::make_unique();\n> > > > > >\n> > > > > > /* Support camera hotplug. */\n> > > > > > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> > > > > > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> > > > > >\n> > > > > > - int ret = cameraManager_->start();\n> > > > > > + ret = cameraManager_->start();\n> > > > > > if (ret) {\n> > > > > > LOG(HAL, Error) << \"Failed to start camera manager: \"\n> > > > > > << strerror(-ret);\n> > > > > > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > > }\n> > > > > > }\n> > > > > >\n> > > > > > + /*\n> > > > > > + * Get camera properties from the configuration file.\n> > > > > > + *\n> > > > > > + * The camera properties as recorded in the configuration file\n> > > > > > + * supplement information that cannot be retrieved from the\n> > > > > > + * libcamera::Camera at run time.\n> > > > > > + */\n> > > > > > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());\n> > > > > > + if (!cameraProps.valid) {\n> > > > > > + LOG(HAL, Error) << \"Failed to register camera: \" << cam->id();\n> > > > > > + return;\n> > > > > > + }\n> >\n> > What if we have a fully populated libcamera::Camera (with properties\n> > retrieved from the firmware) ? Should we still require a configuration\n> > file then ? The configuration file will likely be mandatory in the\n> > future to provide additional information that the firmware doesn't\n> > provide (such as lens-related information for instance), so I suppose\n> > it's fine to already make it mandatory.\n> >\n> > We should however not make the configuration mandatory for external\n> > cameras, this would break UVC support.\n> >\n> > Related to breakages, we need to also ensure that the configuration file\n> > will get deployed correctly in Chrome OS builds of libcamera, or the\n> > existing tests will start failing. I'm not sure how that's best handled,\n> > Hiro, maybe this is something that you could help us with ?\n> >\n> > > > > > +\n> > > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > > > std::unique_ptr camera = CameraDevice::create(id, cam);\n> > > > > > - int ret = camera->initialize();\n> > > > > > + int ret = camera->initialize(cameraProps);\n> > > > > > if (ret) {\n> > > > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > > > return;\n> > > > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > > > index d9bf27989965..af1581da6579 100644\n> > > > > > --- a/src/android/camera_hal_manager.h\n> > > > > > +++ b/src/android/camera_hal_manager.h\n> > > > > > @@ -19,6 +19,8 @@\n> > > > > >\n> > > > > > #include \n> > > > > >\n> > > > > > +#include \"camera_hal_config.h\"\n> > > > > > +\n> > > > > > class CameraDevice;\n> > > > > >\n> > > > > > class CameraHalManager\n> > > > > > @@ -50,6 +52,7 @@ private:\n> > > > > > CameraDevice *cameraDeviceFromHalId(unsigned int id);\n> > > > > >\n> > > > > > std::unique_ptr cameraManager_;\n> > > > > > + CameraHalConfig halConfig_;\n> > > > > >\n> > > > > > const camera_module_callbacks_t *callbacks_;\n> > > > > > std::vector> cameras_;","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 11DC2BD695\n\tfor ;\n\tTue, 6 Apr 2021 14:01:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 779AC687A1;\n\tTue, 6 Apr 2021 16:01:18 +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 5ED8E6084F\n\tfor ;\n\tTue, 6 Apr 2021 16:01:17 +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 CCD428E5;\n\tTue, 6 Apr 2021 16:01:16 +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=\"NWUJb4Ht\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617717677;\n\tbh=Dkxp5bKqnnE1QYi2mhobRp6ETIRlGKmXHscaA44qWqg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NWUJb4Ht8shlTrzeUJah75I68iGm+0W6wVhhtfXyxrdNjadNE6DgegPoc2viZap0B\n\t5MYPG08Cg1VNeqNPzSGfbuHDmQqQiJcD4MCS91qdmnhkg/7CKI45VvAqulS6/9Cy9L\n\t5u7b8mtO7vlyQ3AafQkFXxxDufdFq+GxF2ms1rkM=","Date":"Tue, 6 Apr 2021 17:00:32 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-5-jacopo@jmondi.org>\n\t\n\t<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>\n\t\n\t\n\t<20210406135321.2gwibiku5sjhlplm@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210406135321.2gwibiku5sjhlplm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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":16347,"web_url":"https://patchwork.libcamera.org/comment/16347/","msgid":"","date":"2021-04-19T11:50:26","subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","submitter":{"id":72,"url":"https://patchwork.libcamera.org/api/people/72/","name":"Han-lin Chen","email":"hanlinchen@google.com"},"content":"On Sat, Apr 3, 2021 at 11:09 AM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 01, 2021 at 11:02:30AM +0900, Hirokazu Honda wrote:\n> > On Wed, Mar 31, 2021 at 8:08 PM Jacopo Mondi wrote:\n> > > On Wed, Mar 31, 2021 at 06:12:10PM +0900, Hirokazu Honda wrote:\n> > > > On Tue, Mar 30, 2021 at 11:20 PM Jacopo Mondi wrote:\n> > > > >\n> > > > > Open the HAL configuration file in the Camera HAL manager and get\n> > > > > the camera properties for each created CameraDevice and initialize it\n> > > > > with them.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 9 +++------\n> > > > > src/android/camera_device.h | 3 ++-\n> > > > > src/android/camera_hal_manager.cpp | 21 +++++++++++++++++++--\n> > > > > src/android/camera_hal_manager.h | 3 +++\n> > > > > 4 files changed, 27 insertions(+), 9 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index eb327978c84e..e59f25c68d6b 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -6,6 +6,7 @@\n> > > > > */\n> > > > >\n> > > > > #include \"camera_device.h\"\n> > > > > +#include \"camera_hal_config.h\"\n> > > > > #include \"camera_ops.h\"\n> > > > > #include \"post_processor.h\"\n> > > > >\n> > > > > @@ -351,7 +352,7 @@ std::unique_ptr CameraDevice::create(unsigned int id,\n> > > > > * Initialize the camera static information.\n> > > > > * This method is called before the camera device is opened.\n> > > > > */\n> > > > > -int CameraDevice::initialize()\n> > > > > +int CameraDevice::initialize(const CameraProps &cameraProps)\n> > > > > {\n> > > > > /* Initialize orientation and facing side of the camera. */\n> > > > > const ControlList &properties = camera_->properties();\n> > > > > @@ -370,11 +371,7 @@ int CameraDevice::initialize()\n> > > > > break;\n> > > > > }\n> > > > > } else {\n> > > > > - /*\n> > > > > - * \\todo Retrieve the camera location from configuration file\n> > > > > - * if not available from the library.\n> > > > > - */\n> > > > > - facing_ = CAMERA_FACING_FRONT;\n> > > > > + facing_ = cameraProps.facing;\n>\n> Should we guard against the case where the property isn't specified in\n> the configuration file either ? How about the case where it's specified\n> but has a different value than the one retrieved from the firmware, I\n> wonder if we should warn the user ?\n>\n> > > > > }\n> > > > >\n> > > >\n> > > > Can you explain the rule of updating facing_ and rotation_?\n> > > > How do we prioritize camera_->properties() and cameraProps?\n> > >\n> > > Do you mean clarifying which one has precendence ?\n> > >\n> > > If the information is available from the Camera (which means it has\n> > > been collected from the firmware interface) then it has prcedence.\n> > > Otherwise the configuration file is used as a fallback.\n> >\n> > Yes, thanks for explaining.\n> > Shall we add the comment in code about it?\n>\n> A comment could be nice indeed.\n>\n> > Regardless, the code change looks good.\n> >\n> > Reviewed-by: Hirokazu Honda \n> >\n> > > > > /*\n> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > > index 11bdfec8d587..ba3ec8770e11 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -29,6 +29,7 @@\n> > > > > #include \"camera_worker.h\"\n> > > > > #include \"jpeg/encoder.h\"\n> > > > >\n> > > > > +class CameraProps;\n> > > > > class CameraDevice : protected libcamera::Loggable\n> > > > > {\n> > > > > public:\n> > > > > @@ -36,7 +37,7 @@ public:\n> > > > > std::shared_ptr cam);\n> > > > > ~CameraDevice();\n> > > > >\n> > > > > - int initialize();\n> > > > > + int initialize(const CameraProps &cameraProps);\n> > > > >\n> > > > > int open(const hw_module_t *hardwareModule);\n> > > > > void close();\n> > > > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > > > > index bf3fcda75237..a517727ea0b8 100644\n> > > > > --- a/src/android/camera_hal_manager.cpp\n> > > > > +++ b/src/android/camera_hal_manager.cpp\n> > > > > @@ -39,13 +39,17 @@ CameraHalManager::~CameraHalManager() = default;\n> > > > >\n> > > > > int CameraHalManager::init()\n> > > > > {\n> > > > > + int ret = halConfig_.open();\n> > > > > + if (ret)\n> > > > > + return ret;\n> > > > > +\n> > > > > cameraManager_ = std::make_unique();\n> > > > >\n> > > > > /* Support camera hotplug. */\n> > > > > cameraManager_->cameraAdded.connect(this, &CameraHalManager::cameraAdded);\n> > > > > cameraManager_->cameraRemoved.connect(this, &CameraHalManager::cameraRemoved);\n> > > > >\n> > > > > - int ret = cameraManager_->start();\n> > > > > + ret = cameraManager_->start();\n> > > > > if (ret) {\n> > > > > LOG(HAL, Error) << \"Failed to start camera manager: \"\n> > > > > << strerror(-ret);\n> > > > > @@ -115,9 +119,22 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > + /*\n> > > > > + * Get camera properties from the configuration file.\n> > > > > + *\n> > > > > + * The camera properties as recorded in the configuration file\n> > > > > + * supplement information that cannot be retrieved from the\n> > > > > + * libcamera::Camera at run time.\n> > > > > + */\n> > > > > + const CameraProps &cameraProps = halConfig_.cameraProps(cam->id());\n> > > > > + if (!cameraProps.valid) {\n> > > > > + LOG(HAL, Error) << \"Failed to register camera: \" << cam->id();\n> > > > > + return;\n> > > > > + }\n>\n> What if we have a fully populated libcamera::Camera (with properties\n> retrieved from the firmware) ? Should we still require a configuration\n> file then ? The configuration file will likely be mandatory in the\n> future to provide additional information that the firmware doesn't\n> provide (such as lens-related information for instance), so I suppose\n> it's fine to already make it mandatory.\n>\n> We should however not make the configuration mandatory for external\n> cameras, this would break UVC support.\n>\n> Related to breakages, we need to also ensure that the configuration file\n> will get deployed correctly in Chrome OS builds of libcamera, or the\n> existing tests will start failing. I'm not sure how that's best handled,\n> Hiro, maybe this is something that you could help us with ?\n\nUsually the configuration file is submitted to the device overlay (or\nit's parent overlay).\nAn example (poppy is the parent overlay of soraka):\nhttps://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/camera3_profiles.xml\nin the case, libcamera may load from a static file path, but the\nactual files are installed by different device overlays when built,\nand thus they have different contents.\n\n>\n> > > > > +\n> > > > > /* Create a CameraDevice instance to wrap the libcamera Camera. */\n> > > > > std::unique_ptr camera = CameraDevice::create(id, cam);\n> > > > > - int ret = camera->initialize();\n> > > > > + int ret = camera->initialize(cameraProps);\n> > > > > if (ret) {\n> > > > > LOG(HAL, Error) << \"Failed to initialize camera: \" << cam->id();\n> > > > > return;\n> > > > > diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h\n> > > > > index d9bf27989965..af1581da6579 100644\n> > > > > --- a/src/android/camera_hal_manager.h\n> > > > > +++ b/src/android/camera_hal_manager.h\n> > > > > @@ -19,6 +19,8 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \"camera_hal_config.h\"\n> > > > > +\n> > > > > class CameraDevice;\n> > > > >\n> > > > > class CameraHalManager\n> > > > > @@ -50,6 +52,7 @@ private:\n> > > > > CameraDevice *cameraDeviceFromHalId(unsigned int id);\n> > > > >\n> > > > > std::unique_ptr cameraManager_;\n> > > > > + CameraHalConfig halConfig_;\n> > > > >\n> > > > > const camera_module_callbacks_t *callbacks_;\n> > > > > std::vector> cameras_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 04461BD814\n\tfor ;\n\tMon, 19 Apr 2021 11:50:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6392668821;\n\tMon, 19 Apr 2021 13:50:40 +0200 (CEST)","from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com\n\t[IPv6:2a00:1450:4864:20::42b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A6006880C\n\tfor ;\n\tMon, 19 Apr 2021 13:50:38 +0200 (CEST)","by mail-wr1-x42b.google.com with SMTP id w4so29973154wrt.5\n\tfor ;\n\tMon, 19 Apr 2021 04:50:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"vkylhjAi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=M8XCQXlCJItSp1YjNbvLYNOzJV1CRaQ7UCOJdSPPhuA=;\n\tb=vkylhjAiT1/BsUIhfhPqzalRcKYvUslNG4ZEyQOBI9+Y4hGVWT+zthW+DlQKhqwPwO\n\thp0dD0IbfEG4rW2KDk202SkAEeOC/86gdF016INHK4CT9SZmabo/VN9eEvqv+YeebrBi\n\tHCAQOU0iAJTfl+LAoMqVMfWnWAQVEkTnGe+Wj/GDpLa5ZZ8MDYykt+fyAawhJZyPsYIJ\n\tQjDUECIhdeoFkqI63TMXFdoPYmgSHJoHsFVeXxUhhUwmxgM8IujdMswygCPneQcm2nqN\n\tcQDFYhiVoLOMim3OspquxWGo+cI06S+DU/O5p+xhdaucMl9DJDJPswsvk940VP1/MVz3\n\t10AQ==","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=M8XCQXlCJItSp1YjNbvLYNOzJV1CRaQ7UCOJdSPPhuA=;\n\tb=Joa5jJS73OSx6fNgAtYnuZO76pVvCrV1h5uwcMT7qgA11XoYyAxXLCdz5cJRL8+IK3\n\tQyZpd4QFQXtqMifBjyIVhY3wduKmKY7COrrztRlJ42QXJII3AiFCIl8voXU67KkUG3rQ\n\tpBNXKmgVvY/rmb65GJsk10A+vYlJtoCMRVtS7iD7+9EAzyx9TEjkdYHY3dwUwI5uDs1B\n\tWJvBX0YNmOBCuQaGY0WrT8L88U7g+T1a4BUJcu7iPuHvJuq5rvtXTPBy3nzmV+6ud5HZ\n\t64TMYiyJMGsyFrnxRaomrVqifsk3/2jbKSpoRSLxslUSW6/kOC3KsUKqqTxGgz2rG9Z7\n\tlnGA==","X-Gm-Message-State":"AOAM532ghzKTlll1cbShekKQ+K2P+yNP6JjyE+VA1xofIvfMlw2gPo0W\n\t8QcFeJbvfpZuVwErltXSz/gCyKaW+y4nkx7/UrtWDw==","X-Google-Smtp-Source":"ABdhPJxCHzBQ3lpa3hOuaG6hE+ZTeLBS60uq4znKvX9Ce3VJLkv9tZEWQgUJJ39yPZLwoHbUiDgchjmPZgpSg0InimU=","X-Received":"by 2002:a5d:4884:: with SMTP id\n\tg4mr13869977wrq.191.1618833037661; \n\tMon, 19 Apr 2021 04:50:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210330142113.37457-1-jacopo@jmondi.org>\n\t<20210330142113.37457-5-jacopo@jmondi.org>\n\t\n\t<20210331110851.4dprztcq3eoe4u6v@uno.localdomain>\n\t\n\t","In-Reply-To":"","From":"Han-lin Chen ","Date":"Mon, 19 Apr 2021 19:50:26 +0800","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] android: camera_device: Get\n\tproperties from configuration","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\" "}}]