[{"id":19196,"web_url":"https://patchwork.libcamera.org/comment/19196/","msgid":"<20210831021707.GF20879@jade.amanokami.net>","date":"2021-08-31T02:17:07","subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote:\n> Now that building the list of supported stream configuration requires\n> applying a configuration to the Camera, re-initialize the camera\n> controls by applying a configuration generated for the Viewfinder stream\n> role before building the list of static metadata.\n> \n> Signed-off-by: Jacopo Mondi \n\nLooks good, but I have a question.\n\n> ---\n> src/android/camera_capabilities.cpp | 18 +++++++++++++++---\n> 1 file changed, 15 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index fdda90379ce2..723a4fd5a880 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n> \t}\n> \n> \tret = initializeStreamConfigurations();\n> -\tcamera_->release();\n> -\tif (ret)\n> +\tif (ret) {\n> +\t\tcamera_->release();\n> \t\treturn ret;\n> +\t}\n> \n> -\treturn initializeStaticMetadata();\n> +\tret = initializeStaticMetadata();\n> +\tcamera_->release();\n> +\treturn ret;\n> }\n> \n> std::vector\n> @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\treturn -EINVAL;\n> \t}\n> \n> +\tstd::unique_ptr cameraConfig =\n> +\t\tcamera_->generateConfiguration({ StreamRole::Viewfinder });\n\nFrom what I see, initializeStreamConfigurations() generates a\nconfiguration for the StillCapture role. Is it fine that this uses a\ndifferent role? (Or was the other one changed to Viewfinder where I\nwasn't paying attention?)\n\n\nPaul\n\n> +\tint ret = camera_->configure(cameraConfig.get());\n> +\tif (ret) {\n> +\t\tLOG(HAL, Error) << \"Failed to initialize the camera state\";\n> +\t\tstaticMetadata_.reset();\n> +\t\treturn ret;\n> +\t}\n> +\n> \tconst ControlInfoMap &controlsInfo = camera_->controls();\n> \tconst ControlList &properties = camera_->properties();\n> \n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 60D0EBD87D\n\tfor ;\n\tTue, 31 Aug 2021 02:17:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0FC16916A;\n\tTue, 31 Aug 2021 04:17:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E900D68891\n\tfor ;\n\tTue, 31 Aug 2021 04:17:18 +0200 (CEST)","from jade.amanokami.net (KD027085206055.au-net.ne.jp\n\t[27.85.206.55])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C461F323;\n\tTue, 31 Aug 2021 04:17:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lvLY1Mk3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630376238;\n\tbh=pl7og7ZkZvt9bCkGMWx1EXTB8tgyBvBmCKQu1+rk2Po=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lvLY1Mk32lVybqs5KS+Xea2YNTFwI6XFMql1SLLY7m+zIz91n6ZGfH7pTNl/ryn5u\n\tm5GLwQ3ufHIsdG7EwyB4VtM/sWIvJ24OnDXEch8MufnfuXQrWDasYZKAfnLQQzLk0T\n\t9oGCiObMETps6syKtUKFojWmOPIUF8H1PvVGoUWg=","Date":"Tue, 31 Aug 2021 11:17:07 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Message-ID":"<20210831021707.GF20879@jade.amanokami.net>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210827120757.110615-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19197,"web_url":"https://patchwork.libcamera.org/comment/19197/","msgid":"<20210831022102.GH20879@jade.amanokami.net>","date":"2021-08-31T02:21:02","subject":"Re: [libcamera-devel] [PATCH 06/16] android: capabilties: Assume\n\tcontrols::FrameDurationLimits is supported","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Aug 27, 2021 at 02:07:47PM +0200, Jacopo Mondi wrote:\n> As we now collect the per-stream frame durations at\n> initializeStreamConfigurations() times, the Camera is now guaranteed to\n> support the controls::FrameDurationLimits control.\n> \n> Remove the check for its presence when populating the\n> ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES static metadata.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_capabilities.cpp | 83 ++++++++++++++---------------\n> 1 file changed, 41 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 723a4fd5a880..c45d74189aaa 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -858,55 +858,54 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> \t\t\t\t aeAvailableModes);\n> \n> +\t/* Initialize the AE frame duration limits. */\n> \tint64_t minFrameDurationNsec = -1;\n> \tint64_t maxFrameDurationNsec = -1;\n> \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> -\tif (frameDurationsInfo != controlsInfo.end()) {\n> -\t\tminFrameDurationNsec = frameDurationsInfo->second.min().get() * 1000;\n> -\t\tmaxFrameDurationNsec = frameDurationsInfo->second.max().get() * 1000;\n> +\tminFrameDurationNsec = frameDurationsInfo->second.min().get() * 1000;\n> +\tmaxFrameDurationNsec = frameDurationsInfo->second.max().get() * 1000;\n\nWhy not just initialize these directly?\n\n\nOtherwise, looks good.\n\nReviewed-by: Paul Elder \n\n> \n> -\t\t/*\n> -\t\t * Adjust the minimum frame duration to comply with Android\n> -\t\t * requirements. The camera service mandates all preview/record\n> -\t\t * streams to have a minimum frame duration < 33,366 milliseconds\n> -\t\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> -\t\t * implementation).\n> -\t\t *\n> -\t\t * If we're close enough (+ 500 useconds) to that value, round\n> -\t\t * the minimum frame duration of the camera to an accepted\n> -\t\t * value.\n> -\t\t */\n> -\t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97;\n> -\t\tif (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS &&\n> -\t\t minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000)\n> -\t\t\tminFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000;\n> +\t/*\n> +\t * Adjust the minimum frame duration to comply with Android\n> +\t * requirements. The camera service mandates all preview/record\n> +\t * streams to have a minimum frame duration < 33,366 milliseconds\n> +\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> +\t * implementation).\n> +\t *\n> +\t * If we're close enough (+ 500 useconds) to that value, round\n> +\t * the minimum frame duration of the camera to an accepted\n> +\t * value.\n> +\t */\n> +\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97;\n> +\tif (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS &&\n> +\t minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000)\n> +\t\tminFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000;\n> \n> -\t\t/*\n> -\t\t * The AE routine frame rate limits are computed using the frame\n> -\t\t * duration limits, as libcamera clips the AE routine to the\n> -\t\t * frame durations.\n> -\t\t */\n> -\t\tint32_t maxFps = std::round(1e9 / minFrameDurationNsec);\n> -\t\tint32_t minFps = std::round(1e9 / maxFrameDurationNsec);\n> -\t\tminFps = std::max(1, minFps);\n> +\t/*\n> +\t * The AE routine frame rate limits are computed using the frame\n> +\t * duration limits, as libcamera clips the AE routine to the\n> +\t * frame durations.\n> +\t */\n> +\tint32_t maxFps = std::round(1e9 / minFrameDurationNsec);\n> +\tint32_t minFps = std::round(1e9 / maxFrameDurationNsec);\n> +\tminFps = std::max(1, minFps);\n> \n> -\t\t/*\n> -\t\t * Force rounding errors so that we have the proper frame\n> -\t\t * durations for when we reuse these variables later\n> -\t\t */\n> -\t\tminFrameDurationNsec = 1e9 / maxFps;\n> -\t\tmaxFrameDurationNsec = 1e9 / minFps;\n> +\t/*\n> +\t * Force rounding errors so that we have the proper frame\n> +\t * durations for when we reuse these variables later\n> +\t */\n> +\tminFrameDurationNsec = 1e9 / maxFps;\n> +\tmaxFrameDurationNsec = 1e9 / minFps;\n> \n> -\t\t/*\n> -\t\t * Register to the camera service {min, max} and {max, max}\n> -\t\t * intervals as requested by the metadata documentation.\n> -\t\t */\n> -\t\tint32_t availableAeFpsTarget[] = {\n> -\t\t\tminFps, maxFps, maxFps, maxFps\n> -\t\t};\n> -\t\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> -\t\t\t\t\t availableAeFpsTarget);\n> -\t}\n> +\t/*\n> +\t * Register to the camera service {min, max} and {max, max}\n> +\t * intervals as requested by the metadata documentation.\n> +\t */\n> +\tint32_t availableAeFpsTarget[] = {\n> +\t\tminFps, maxFps, maxFps, maxFps\n> +\t};\n> +\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> +\t\t\t\t availableAeFpsTarget);\n> \n> \tstd::vector aeCompensationRange = {\n> \t\t0, 0,\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 33FC8BDC71\n\tfor ;\n\tTue, 31 Aug 2021 02:21:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BF366916A;\n\tTue, 31 Aug 2021 04:21:14 +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 DDC7368891\n\tfor ;\n\tTue, 31 Aug 2021 04:21:12 +0200 (CEST)","from jade.amanokami.net (KD027085206055.au-net.ne.jp\n\t[27.85.206.55])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3556323;\n\tTue, 31 Aug 2021 04:21:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i6dB/Gfy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630376472;\n\tbh=t/CSSsUYHXTlWhmJJJAT+RhuKqzXv6lJrqarvZNieOM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i6dB/Gfy4tUk1EPIQHhv9CiGFYXq4r6sOel7XdC2kvMrIEks1LO7wxKdxHQMpxFZa\n\tCI+nxZ6/QdTz2v2cI7rXEUemkRGDURTaPIJCBW1BFv1+EbniZajGgd+krNHS/ol8Ze\n\tqCFD+KuX7gki9wVpwHdhb9Vwe+c6E3cu+e4rHZjc=","Date":"Tue, 31 Aug 2021 11:21:02 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Message-ID":"<20210831022102.GH20879@jade.amanokami.net>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210827120757.110615-7-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 06/16] android: capabilties: Assume\n\tcontrols::FrameDurationLimits is supported","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19199,"web_url":"https://patchwork.libcamera.org/comment/19199/","msgid":"<20210831022657.GJ20879@jade.amanokami.net>","date":"2021-08-31T02:26:57","subject":"Re: [libcamera-devel] [PATCH 09/16] android: capabilities: Collect\n\tabsolute max frame durations","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Aug 27, 2021 at 02:07:50PM +0200, Jacopo Mondi wrote:\n> While building the list of supported stream configurations also collet\n\ns/collet/collect\n\n> the absolute max frame durations to be used to populate the sensor\n> maximum frame duration.\n> \n> Signed-off-by: Jacopo Mondi \n\nLooks good.\n\nReviewed-by: Paul Elder \n\n> ---\n> src/android/camera_capabilities.cpp | 9 ++++++---\n> src/android/camera_capabilities.h | 1 +\n> 2 files changed, 7 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 23a9a3fbec50..388552963c47 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -385,6 +385,7 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n> \torientation_ = orientation;\n> \tfacing_ = facing;\n> \trawStreamAvailable_ = false;\n> +\tmaxFrameDuration_ = 0;\n> \n> \t/* Acquire the camera and initialize available stream configurations. */\n> \tint ret = camera_->acquire();\n> @@ -659,6 +660,9 @@ int CameraCapabilities::initializeStreamConfigurations()\n> \t\t\t\t});\n> \t\t\t\tmaxJpegSize = std::max(maxJpegSize, res);\n> \t\t\t}\n> +\n> +\t\t\tmaxFrameDuration_ = std::max(maxFrameDuration_,\n> +\t\t\t\t\t\t maxFrameDuration);\n> \t\t}\n> \n> \t\t/*\n> @@ -1132,9 +1136,8 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> \t\t\t\t timestampSource);\n> \n> -\tif (maxFrameDurationNsec > 0)\n> -\t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION,\n> -\t\t\t\t\t maxFrameDurationNsec);\n> +\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION,\n> +\t\t\t\t maxFrameDuration_);\n> \n> \t/* Statistics static metadata. */\n> \tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> index 6e55ddab445e..2cf97ae80095 100644\n> --- a/src/android/camera_capabilities.h\n> +++ b/src/android/camera_capabilities.h\n> @@ -71,6 +71,7 @@ private:\n> \tint facing_;\n> \tint orientation_;\n> \tbool rawStreamAvailable_;\n> +\tint64_t maxFrameDuration_;\n> \tcamera_metadata_enum_android_info_supported_hardware_level hwLevel_;\n> \tstd::set capabilities_;\n> \n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id ECA0CBD87D\n\tfor ;\n\tTue, 31 Aug 2021 02:27:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63E716916A;\n\tTue, 31 Aug 2021 04:27:10 +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 73B7068891\n\tfor ;\n\tTue, 31 Aug 2021 04:27:08 +0200 (CEST)","from jade.amanokami.net (KD027085206055.au-net.ne.jp\n\t[27.85.206.55])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 495BA323;\n\tTue, 31 Aug 2021 04:27:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Y12eiQ+R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630376828;\n\tbh=No3tbmfjsS4VmxmnFYXh+oX58EiwLZCUtv3/xjikBjM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y12eiQ+RIY/hnYJIEJJEWbciQBpNuT+ZyWmUf8ylbHVfssSzXI7Ooilz6yGNOHpZ3\n\t4RgJpjX2S+kYBW9LUvkH3C73pxdlnmfJNzi7N3ouaHbGKSUU/tvWkFSCYF0Z0K9/Va\n\t5iWZ/J9IJIKC7+hzFexwwjg9pIeY1BRSO8Rmohjw=","Date":"Tue, 31 Aug 2021 11:26:57 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Message-ID":"<20210831022657.GJ20879@jade.amanokami.net>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-10-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210827120757.110615-10-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 09/16] android: capabilities: Collect\n\tabsolute max frame durations","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19210,"web_url":"https://patchwork.libcamera.org/comment/19210/","msgid":"<20210831094138.zfmli7cpxwxgmmgu@uno.localdomain>","date":"2021-08-31T09:41:38","subject":"Re: [libcamera-devel] [PATCH 06/16] android: capabilties: Assume\n\tcontrols::FrameDurationLimits is supported","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Aug 31, 2021 at 11:21:02AM +0900, Paul Elder wrote:\n> Hi Jacopo,\n>\n> On Fri, Aug 27, 2021 at 02:07:47PM +0200, Jacopo Mondi wrote:\n> > As we now collect the per-stream frame durations at\n> > initializeStreamConfigurations() times, the Camera is now guaranteed to\n> > support the controls::FrameDurationLimits control.\n> >\n> > Remove the check for its presence when populating the\n> > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES static metadata.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_capabilities.cpp | 83 ++++++++++++++---------------\n> > 1 file changed, 41 insertions(+), 42 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 723a4fd5a880..c45d74189aaa 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -858,55 +858,54 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > \t\t\t\t aeAvailableModes);\n> >\n> > +\t/* Initialize the AE frame duration limits. */\n> > \tint64_t minFrameDurationNsec = -1;\n> > \tint64_t maxFrameDurationNsec = -1;\n> > \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> > -\tif (frameDurationsInfo != controlsInfo.end()) {\n> > -\t\tminFrameDurationNsec = frameDurationsInfo->second.min().get() * 1000;\n> > -\t\tmaxFrameDurationNsec = frameDurationsInfo->second.max().get() * 1000;\n> > +\tminFrameDurationNsec = frameDurationsInfo->second.min().get() * 1000;\n> > +\tmaxFrameDurationNsec = frameDurationsInfo->second.max().get() * 1000;\n>\n> Why not just initialize these directly?\n\nYou're right, I'll do so!\n\nThanks\n j\n\n>\n>\n> Otherwise, looks good.\n>\n> Reviewed-by: Paul Elder \n>\n> >\n> > -\t\t/*\n> > -\t\t * Adjust the minimum frame duration to comply with Android\n> > -\t\t * requirements. The camera service mandates all preview/record\n> > -\t\t * streams to have a minimum frame duration < 33,366 milliseconds\n> > -\t\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> > -\t\t * implementation).\n> > -\t\t *\n> > -\t\t * If we're close enough (+ 500 useconds) to that value, round\n> > -\t\t * the minimum frame duration of the camera to an accepted\n> > -\t\t * value.\n> > -\t\t */\n> > -\t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97;\n> > -\t\tif (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS &&\n> > -\t\t minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000)\n> > -\t\t\tminFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000;\n> > +\t/*\n> > +\t * Adjust the minimum frame duration to comply with Android\n> > +\t * requirements. The camera service mandates all preview/record\n> > +\t * streams to have a minimum frame duration < 33,366 milliseconds\n> > +\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> > +\t * implementation).\n> > +\t *\n> > +\t * If we're close enough (+ 500 useconds) to that value, round\n> > +\t * the minimum frame duration of the camera to an accepted\n> > +\t * value.\n> > +\t */\n> > +\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97;\n> > +\tif (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS &&\n> > +\t minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000)\n> > +\t\tminFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000;\n> >\n> > -\t\t/*\n> > -\t\t * The AE routine frame rate limits are computed using the frame\n> > -\t\t * duration limits, as libcamera clips the AE routine to the\n> > -\t\t * frame durations.\n> > -\t\t */\n> > -\t\tint32_t maxFps = std::round(1e9 / minFrameDurationNsec);\n> > -\t\tint32_t minFps = std::round(1e9 / maxFrameDurationNsec);\n> > -\t\tminFps = std::max(1, minFps);\n> > +\t/*\n> > +\t * The AE routine frame rate limits are computed using the frame\n> > +\t * duration limits, as libcamera clips the AE routine to the\n> > +\t * frame durations.\n> > +\t */\n> > +\tint32_t maxFps = std::round(1e9 / minFrameDurationNsec);\n> > +\tint32_t minFps = std::round(1e9 / maxFrameDurationNsec);\n> > +\tminFps = std::max(1, minFps);\n> >\n> > -\t\t/*\n> > -\t\t * Force rounding errors so that we have the proper frame\n> > -\t\t * durations for when we reuse these variables later\n> > -\t\t */\n> > -\t\tminFrameDurationNsec = 1e9 / maxFps;\n> > -\t\tmaxFrameDurationNsec = 1e9 / minFps;\n> > +\t/*\n> > +\t * Force rounding errors so that we have the proper frame\n> > +\t * durations for when we reuse these variables later\n> > +\t */\n> > +\tminFrameDurationNsec = 1e9 / maxFps;\n> > +\tmaxFrameDurationNsec = 1e9 / minFps;\n> >\n> > -\t\t/*\n> > -\t\t * Register to the camera service {min, max} and {max, max}\n> > -\t\t * intervals as requested by the metadata documentation.\n> > -\t\t */\n> > -\t\tint32_t availableAeFpsTarget[] = {\n> > -\t\t\tminFps, maxFps, maxFps, maxFps\n> > -\t\t};\n> > -\t\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> > -\t\t\t\t\t availableAeFpsTarget);\n> > -\t}\n> > +\t/*\n> > +\t * Register to the camera service {min, max} and {max, max}\n> > +\t * intervals as requested by the metadata documentation.\n> > +\t */\n> > +\tint32_t availableAeFpsTarget[] = {\n> > +\t\tminFps, maxFps, maxFps, maxFps\n> > +\t};\n> > +\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> > +\t\t\t\t availableAeFpsTarget);\n> >\n> > \tstd::vector aeCompensationRange = {\n> > \t\t0, 0,\n> > --\n> > 2.32.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5DA40BDC71\n\tfor ;\n\tTue, 31 Aug 2021 09:40:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E41126916A;\n\tTue, 31 Aug 2021 11:40:50 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A351660288\n\tfor ;\n\tTue, 31 Aug 2021 11:40:49 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 1239C1BF20B;\n\tTue, 31 Aug 2021 09:40:48 +0000 (UTC)"],"Date":"Tue, 31 Aug 2021 11:41:38 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210831094138.zfmli7cpxwxgmmgu@uno.localdomain>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-7-jacopo@jmondi.org>\n\t<20210831022102.GH20879@jade.amanokami.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210831022102.GH20879@jade.amanokami.net>","Subject":"Re: [libcamera-devel] [PATCH 06/16] android: capabilties: Assume\n\tcontrols::FrameDurationLimits is supported","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19211,"web_url":"https://patchwork.libcamera.org/comment/19211/","msgid":"<20210831095750.3pjo6ddk2xdsi6wz@uno.localdomain>","date":"2021-08-31T09:57:50","subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Aug 31, 2021 at 11:17:07AM +0900, Paul Elder wrote:\n> Hi Jacopo,\n>\n> On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote:\n> > Now that building the list of supported stream configuration requires\n> > applying a configuration to the Camera, re-initialize the camera\n> > controls by applying a configuration generated for the Viewfinder stream\n> > role before building the list of static metadata.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> Looks good, but I have a question.\n>\n> > ---\n> > src/android/camera_capabilities.cpp | 18 +++++++++++++++---\n> > 1 file changed, 15 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index fdda90379ce2..723a4fd5a880 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n> > \t}\n> >\n> > \tret = initializeStreamConfigurations();\n> > -\tcamera_->release();\n> > -\tif (ret)\n> > +\tif (ret) {\n> > +\t\tcamera_->release();\n> > \t\treturn ret;\n> > +\t}\n> >\n> > -\treturn initializeStaticMetadata();\n> > +\tret = initializeStaticMetadata();\n> > +\tcamera_->release();\n> > +\treturn ret;\n> > }\n> >\n> > std::vector\n> > @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata()\n> > \t\treturn -EINVAL;\n> > \t}\n> >\n> > +\tstd::unique_ptr cameraConfig =\n> > +\t\tcamera_->generateConfiguration({ StreamRole::Viewfinder });\n>\n> From what I see, initializeStreamConfigurations() generates a\n> configuration for the StillCapture role. Is it fine that this uses a\n\nIt does so only to retrieve the maximum resolution.\nA todo note reminds us that we should get that from a camera property\n\n\t * Get the maximum output resolutions\n\t * \\todo Get this from the camera properties once defined\n\t */\n\tstd::unique_ptr cameraConfig =\n\t\tcamera_->generateConfiguration({ StillCapture });\n\n> different role? (Or was the other one changed to Viewfinder where I\n> wasn't paying attention?)\n\nWell, the choice of the Viewfinder role is pretty arbitrary, but we\nhave to define a 'mode' the camera should be set to when initializing\nthe static metadata. We don't have the luxury of assuming much from\nthe underlying camera, and we have to construct the static metadata\nfrom the camera properties (static) and the camera controls (change\ndepending on the configuration).\n\nBefore this series the Camera::controls where static as well, so the\nCamera didn't get configured at all during the static metadata\ninitialization, and we relied on the PH-initialized ControlInfoMap.\n\nAs we now need to apply a Configuration to the Camera to collect\nper-configuration control limits, we need to re-initialize them to a\nknown default before inspecting them.\n\nI considered Viewfinder as a good default, but I'm open to other\nproposals.\n\n>\n>\n> Paul\n>\n> > +\tint ret = camera_->configure(cameraConfig.get());\n> > +\tif (ret) {\n> > +\t\tLOG(HAL, Error) << \"Failed to initialize the camera state\";\n> > +\t\tstaticMetadata_.reset();\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > \tconst ControlInfoMap &controlsInfo = camera_->controls();\n> > \tconst ControlList &properties = camera_->properties();\n> >\n> > --\n> > 2.32.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D4FCBBD87D\n\tfor ;\n\tTue, 31 Aug 2021 09:57:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FD716916A;\n\tTue, 31 Aug 2021 11:57:03 +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 E20B460288\n\tfor ;\n\tTue, 31 Aug 2021 11:57:01 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 6AE1F40015;\n\tTue, 31 Aug 2021 09:57:01 +0000 (UTC)"],"Date":"Tue, 31 Aug 2021 11:57:50 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210831095750.3pjo6ddk2xdsi6wz@uno.localdomain>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-6-jacopo@jmondi.org>\n\t<20210831021707.GF20879@jade.amanokami.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210831021707.GF20879@jade.amanokami.net>","Subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19243,"web_url":"https://patchwork.libcamera.org/comment/19243/","msgid":"<20210901072402.GS968527@pyrite.rasen.tech>","date":"2021-09-01T07:24:02","subject":"Re: [libcamera-devel] [PATCH 12/16] android: Populate streams and\n\tduration in the same loop","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Aug 27, 2021 at 02:07:53PM +0200, Jacopo Mondi wrote:\n> The ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and\n> ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS static metadata are\n> populated by looping on the streamConfigurations_ vector.\n> \n> Unify them in a single loop to avoid repeating it.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Paul Elder \n\n> ---\n> src/android/camera_capabilities.cpp | 23 ++++++++++-------------\n> 1 file changed, 10 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 484242d0ad81..8bde06e824ef 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -1255,7 +1255,9 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\t\t\t maxZoom);\n> \n> \tstd::vector availableStreamConfigurations;\n> +\tstd::vector minFrameDurations;\n> \tavailableStreamConfigurations.reserve(streamConfigurations_.size() * 4);\n> +\tminFrameDurations.reserve(streamConfigurations_.size() * 4);\n> \tfor (const auto &entry : streamConfigurations_) {\n> \t\t/*\n> \t\t * Filter out YUV streams not capable of running at 30 FPS.\n> @@ -1272,12 +1274,19 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\tif (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)\n> \t\t\tcontinue;\n> \n> +\t\t/* Stream configuration map. */\n> \t\tavailableStreamConfigurations.push_back(entry.androidFormat);\n> \t\tavailableStreamConfigurations.push_back(entry.resolution.width);\n> \t\tavailableStreamConfigurations.push_back(entry.resolution.height);\n> \t\tavailableStreamConfigurations.push_back(\n> \t\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT);\n> \n> +\t\t/* Per-stream durations. */\n> +\t\tminFrameDurations.push_back(entry.androidFormat);\n> +\t\tminFrameDurations.push_back(entry.resolution.width);\n> +\t\tminFrameDurations.push_back(entry.resolution.height);\n> +\t\tminFrameDurations.push_back(entry.minFrameDurationNsec);\n> +\n> \t\tLOG(HAL, Debug)\n> \t\t\t<< \"Output Stream:\" << utils::hex(entry.androidFormat)\n> \t\t\t<< \"(\" << entry.resolution.width << \"x\"\n> @@ -1285,22 +1294,10 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\t\t<< entry.minFrameDurationNsec << \"]\"\n> \t\t\t<< \"@\" << fps;\n> \t}\n> +\n> \tstaticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> \t\t\t\t availableStreamConfigurations);\n> \n> -\tstd::vector minFrameDurations;\n> -\tminFrameDurations.reserve(streamConfigurations_.size() * 4);\n> -\tfor (const auto &entry : streamConfigurations_) {\n> -\t\tunsigned int fps = static_cast\n> -\t\t\t\t (floor(1e9 / entry.minFrameDurationNsec + 0.05f));\n> -\t\tif (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)\n> -\t\t\tcontinue;\n> -\n> -\t\tminFrameDurations.push_back(entry.androidFormat);\n> -\t\tminFrameDurations.push_back(entry.resolution.width);\n> -\t\tminFrameDurations.push_back(entry.resolution.height);\n> -\t\tminFrameDurations.push_back(entry.minFrameDurationNsec);\n> -\t}\n> \tstaticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> \t\t\t\t minFrameDurations);\n> \n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5BA2CBD87D\n\tfor ;\n\tWed, 1 Sep 2021 07:24:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D34CC60255;\n\tWed, 1 Sep 2021 09:24:11 +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 03C0760253\n\tfor ;\n\tWed, 1 Sep 2021 09:24:10 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7BF1B3D7;\n\tWed, 1 Sep 2021 09:24:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jOJoRwKJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630481049;\n\tbh=Anvpw0eR69WxvXhYhGqkYqtBYvmBue9K4QETTEy6iCg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jOJoRwKJ8mlEKFlW4C7UbRb7HPKWhFlgLAa49WGoon3DOsX43JmSqgwNZogO6DCKf\n\tVqaORNBUSxgtNZU8UcqQQJk7+kxZXCcB9s9r17lQFG7O1DVF2bNxCVBaoeCWBzrKi3\n\tdKYTge7mqZuAM3ehu0Y+mHu8wwqjaAnGr59ZgLOo=","Date":"Wed, 1 Sep 2021 16:24:02 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210901072402.GS968527@pyrite.rasen.tech>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-13-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210827120757.110615-13-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 12/16] android: Populate streams and\n\tduration in the same loop","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19245,"web_url":"https://patchwork.libcamera.org/comment/19245/","msgid":"<3954354a-5285-4ecd-4526-ebc181869554@ideasonboard.com>","date":"2021-09-01T07:44:20","subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\n\nOn 8/31/21 3:27 PM, Jacopo Mondi wrote:\n> Hi Paul,\n>\n> On Tue, Aug 31, 2021 at 11:17:07AM +0900, Paul Elder wrote:\n>> Hi Jacopo,\n>>\n>> On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote:\n>>> Now that building the list of supported stream configuration requires\n>>> applying a configuration to the Camera, re-initialize the camera\n>>> controls by applying a configuration generated for the Viewfinder stream\n>>> role before building the list of static metadata.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>> Looks good, but I have a question.\n>>\n>>> ---\n>>> src/android/camera_capabilities.cpp | 18 +++++++++++++++---\n>>> 1 file changed, 15 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n>>> index fdda90379ce2..723a4fd5a880 100644\n>>> --- a/src/android/camera_capabilities.cpp\n>>> +++ b/src/android/camera_capabilities.cpp\n>>> @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n>>> \t}\n>>>\n>>> \tret = initializeStreamConfigurations();\n>>> -\tcamera_->release();\n>>> -\tif (ret)\n>>> +\tif (ret) {\n>>> +\t\tcamera_->release();\n>>> \t\treturn ret;\n>>> +\t}\n>>>\n>>> -\treturn initializeStaticMetadata();\n>>> +\tret = initializeStaticMetadata();\n>>> +\tcamera_->release();\n>>> +\treturn ret;\n>>> }\n>>>\n>>> std::vector\n>>> @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata()\n>>> \t\treturn -EINVAL;\n>>> \t}\n>>>\n>>> +\tstd::unique_ptr cameraConfig =\n>>> +\t\tcamera_->generateConfiguration({ StreamRole::Viewfinder });\n>> From what I see, initializeStreamConfigurations() generates a\n>> configuration for the StillCapture role. Is it fine that this uses a\n> It does so only to retrieve the maximum resolution.\n> A todo note reminds us that we should get that from a camera property\n>\n> \t * Get the maximum output resolutions\n> \t * \\todo Get this from the camera properties once defined\n> \t */\n> \tstd::unique_ptr cameraConfig =\n> \t\tcamera_->generateConfiguration({ StillCapture });\n>\n>> different role? (Or was the other one changed to Viewfinder where I\n>> wasn't paying attention?)\n> Well, the choice of the Viewfinder role is pretty arbitrary, but we\n> have to define a 'mode' the camera should be set to when initializing\n> the static metadata. We don't have the luxury of assuming much from\n> the underlying camera, and we have to construct the static metadata\n> from the camera properties (static) and the camera controls (change\n> depending on the configuration).\n>\n> Before this series the Camera::controls where static as well, so the\n> Camera didn't get configured at all during the static metadata\n> initialization, and we relied on the PH-initialized ControlInfoMap.\n>\n> As we now need to apply a Configuration to the Camera to collect\n> per-configuration control limits, we need to re-initialize them to a\n> known default before inspecting them.\n>\n> I considered Viewfinder as a good default, but I'm open to other\n> proposals.\n\n\nI agree with the reasoning but also feels to me that there should be \ncomment placed in the file to address this.\n\nUp to you.\n\nReviewed-by: Umang Jain \n\n>\n>>\n>> Paul\n>>\n>>> +\tint ret = camera_->configure(cameraConfig.get());\n>>> +\tif (ret) {\n>>> +\t\tLOG(HAL, Error) << \"Failed to initialize the camera state\";\n>>> +\t\tstaticMetadata_.reset();\n>>> +\t\treturn ret;\n>>> +\t}\n>>> +\n>>> \tconst ControlInfoMap &controlsInfo = camera_->controls();\n>>> \tconst ControlList &properties = camera_->properties();\n>>>\n>>> --\n>>> 2.32.0\n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 973B9BDC71\n\tfor ;\n\tWed, 1 Sep 2021 07:44:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17D206916A;\n\tWed, 1 Sep 2021 09:44:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BE6E60253\n\tfor ;\n\tWed, 1 Sep 2021 09:44:26 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0FF363D7;\n\tWed, 1 Sep 2021 09:44:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GbwPcszm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630482266;\n\tbh=0El1iXNMR+m0l1jCf3NSVuqq02sJOrPgZBK8QegOGMI=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=GbwPcszmNt9Bw0pVaXrVWFCq0FRYpLucD7LInz8CZ8EHJOR5hEGWgQ+Jp3RftADnV\n\t+30fTY7Rdm2OjJML3pW2hAZoLR3JTzgySw363qfG15MuQXQrh6YdB1Q9fdq4m1cz79\n\thVXr9bDE3RBja801s5af7oAyivB/cJBCvFxaEzvI=","To":"Jacopo Mondi ,\n\tPaul Elder ","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-6-jacopo@jmondi.org>\n\t<20210831021707.GF20879@jade.amanokami.net>\n\t<20210831095750.3pjo6ddk2xdsi6wz@uno.localdomain>","From":"Umang Jain ","Message-ID":"<3954354a-5285-4ecd-4526-ebc181869554@ideasonboard.com>","Date":"Wed, 1 Sep 2021 13:14:20 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210831095750.3pjo6ddk2xdsi6wz@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19246,"web_url":"https://patchwork.libcamera.org/comment/19246/","msgid":"<9cb15fbd-9676-84fe-229e-8fec2dd4334d@ideasonboard.com>","date":"2021-09-01T07:47:45","subject":"Re: [libcamera-devel] [PATCH 06/16] android: capabilties: Assume\n\tcontrols::FrameDurationLimits is supported","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nThanks for the patch.\n\nOn 8/27/21 5:37 PM, Jacopo Mondi wrote:\n> As we now collect the per-stream frame durations at\n> initializeStreamConfigurations() times, the Camera is now guaranteed to\n> support the controls::FrameDurationLimits control.\n>\n> Remove the check for its presence when populating the\n> ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES static metadata.\n>\n> Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Umang Jain \n\n> ---\n> src/android/camera_capabilities.cpp | 83 ++++++++++++++---------------\n> 1 file changed, 41 insertions(+), 42 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 723a4fd5a880..c45d74189aaa 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -858,55 +858,54 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> \t\t\t\t aeAvailableModes);\n> \n> +\t/* Initialize the AE frame duration limits. */\n> \tint64_t minFrameDurationNsec = -1;\n> \tint64_t maxFrameDurationNsec = -1;\n> \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> -\tif (frameDurationsInfo != controlsInfo.end()) {\n> -\t\tminFrameDurationNsec = frameDurationsInfo->second.min().get() * 1000;\n> -\t\tmaxFrameDurationNsec = frameDurationsInfo->second.max().get() * 1000;\n> +\tminFrameDurationNsec = frameDurationsInfo->second.min().get() * 1000;\n> +\tmaxFrameDurationNsec = frameDurationsInfo->second.max().get() * 1000;\n> \n> -\t\t/*\n> -\t\t * Adjust the minimum frame duration to comply with Android\n> -\t\t * requirements. The camera service mandates all preview/record\n> -\t\t * streams to have a minimum frame duration < 33,366 milliseconds\n> -\t\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> -\t\t * implementation).\n> -\t\t *\n> -\t\t * If we're close enough (+ 500 useconds) to that value, round\n> -\t\t * the minimum frame duration of the camera to an accepted\n> -\t\t * value.\n> -\t\t */\n> -\t\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97;\n> -\t\tif (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS &&\n> -\t\t minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000)\n> -\t\t\tminFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000;\n> +\t/*\n> +\t * Adjust the minimum frame duration to comply with Android\n> +\t * requirements. The camera service mandates all preview/record\n> +\t * streams to have a minimum frame duration < 33,366 milliseconds\n> +\t * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service\n> +\t * implementation).\n> +\t *\n> +\t * If we're close enough (+ 500 useconds) to that value, round\n> +\t * the minimum frame duration of the camera to an accepted\n> +\t * value.\n> +\t */\n> +\tstatic constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97;\n> +\tif (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS &&\n> +\t minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000)\n> +\t\tminFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000;\n> \n> -\t\t/*\n> -\t\t * The AE routine frame rate limits are computed using the frame\n> -\t\t * duration limits, as libcamera clips the AE routine to the\n> -\t\t * frame durations.\n> -\t\t */\n> -\t\tint32_t maxFps = std::round(1e9 / minFrameDurationNsec);\n> -\t\tint32_t minFps = std::round(1e9 / maxFrameDurationNsec);\n> -\t\tminFps = std::max(1, minFps);\n> +\t/*\n> +\t * The AE routine frame rate limits are computed using the frame\n> +\t * duration limits, as libcamera clips the AE routine to the\n> +\t * frame durations.\n> +\t */\n> +\tint32_t maxFps = std::round(1e9 / minFrameDurationNsec);\n> +\tint32_t minFps = std::round(1e9 / maxFrameDurationNsec);\n> +\tminFps = std::max(1, minFps);\n> \n> -\t\t/*\n> -\t\t * Force rounding errors so that we have the proper frame\n> -\t\t * durations for when we reuse these variables later\n> -\t\t */\n> -\t\tminFrameDurationNsec = 1e9 / maxFps;\n> -\t\tmaxFrameDurationNsec = 1e9 / minFps;\n> +\t/*\n> +\t * Force rounding errors so that we have the proper frame\n> +\t * durations for when we reuse these variables later\n> +\t */\n> +\tminFrameDurationNsec = 1e9 / maxFps;\n> +\tmaxFrameDurationNsec = 1e9 / minFps;\n> \n> -\t\t/*\n> -\t\t * Register to the camera service {min, max} and {max, max}\n> -\t\t * intervals as requested by the metadata documentation.\n> -\t\t */\n> -\t\tint32_t availableAeFpsTarget[] = {\n> -\t\t\tminFps, maxFps, maxFps, maxFps\n> -\t\t};\n> -\t\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> -\t\t\t\t\t availableAeFpsTarget);\n> -\t}\n> +\t/*\n> +\t * Register to the camera service {min, max} and {max, max}\n> +\t * intervals as requested by the metadata documentation.\n> +\t */\n> +\tint32_t availableAeFpsTarget[] = {\n> +\t\tminFps, maxFps, maxFps, maxFps\n> +\t};\n> +\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> +\t\t\t\t availableAeFpsTarget);\n> \n> \tstd::vector aeCompensationRange = {\n> \t\t0, 0,","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 49308BD87D\n\tfor ;\n\tWed, 1 Sep 2021 07:47:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C4EA96916C;\n\tWed, 1 Sep 2021 09:47:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E5D560253\n\tfor ;\n\tWed, 1 Sep 2021 09:47:50 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F3803D7;\n\tWed, 1 Sep 2021 09:47:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FY9bq/8D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630482469;\n\tbh=8UU7H+D5Jc5fbdr7wbZDyni/+qIopSNs8zY4CDBKLF8=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=FY9bq/8DOOxKukvHuF+OR6muv8LbTjsUICGdJ/hSXlmD4GK/C/kU3tDMFDl3jhgR7\n\tUzzAYY2j1FAEzrb6Rn7Rg1kPWW74GPJRw2HkBYYcssjE7a0yyStdPHUq1rTu7v1tHY\n\ty7Do07WO6+ih0N1N2WkOo8Ia8xeuqdjqkhkKcK5o=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-7-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"<9cb15fbd-9676-84fe-229e-8fec2dd4334d@ideasonboard.com>","Date":"Wed, 1 Sep 2021 13:17:45 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210827120757.110615-7-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 06/16] android: capabilties: Assume\n\tcontrols::FrameDurationLimits is supported","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19250,"web_url":"https://patchwork.libcamera.org/comment/19250/","msgid":"","date":"2021-09-01T08:17:55","subject":"Re: [libcamera-devel] [PATCH 09/16] android: capabilities: Collect\n\tabsolute max frame durations","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch\n\nOn 8/27/21 5:37 PM, Jacopo Mondi wrote:\n> While building the list of supported stream configurations also collet\n> the absolute max frame durations to be used to populate the sensor\n> maximum frame duration.\n>\n> Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Umang Jain \n\n> ---\n> src/android/camera_capabilities.cpp | 9 ++++++---\n> src/android/camera_capabilities.h | 1 +\n> 2 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 23a9a3fbec50..388552963c47 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -385,6 +385,7 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n> \torientation_ = orientation;\n> \tfacing_ = facing;\n> \trawStreamAvailable_ = false;\n> +\tmaxFrameDuration_ = 0;\n> \n> \t/* Acquire the camera and initialize available stream configurations. */\n> \tint ret = camera_->acquire();\n> @@ -659,6 +660,9 @@ int CameraCapabilities::initializeStreamConfigurations()\n> \t\t\t\t});\n> \t\t\t\tmaxJpegSize = std::max(maxJpegSize, res);\n> \t\t\t}\n> +\n> +\t\t\tmaxFrameDuration_ = std::max(maxFrameDuration_,\n> +\t\t\t\t\t\t maxFrameDuration);\n> \t\t}\n> \n> \t\t/*\n> @@ -1132,9 +1136,8 @@ int CameraCapabilities::initializeStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> \t\t\t\t timestampSource);\n> \n> -\tif (maxFrameDurationNsec > 0)\n> -\t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION,\n> -\t\t\t\t\t maxFrameDurationNsec);\n> +\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION,\n> +\t\t\t\t maxFrameDuration_);\n> \n> \t/* Statistics static metadata. */\n> \tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h\n> index 6e55ddab445e..2cf97ae80095 100644\n> --- a/src/android/camera_capabilities.h\n> +++ b/src/android/camera_capabilities.h\n> @@ -71,6 +71,7 @@ private:\n> \tint facing_;\n> \tint orientation_;\n> \tbool rawStreamAvailable_;\n> +\tint64_t maxFrameDuration_;\n> \tcamera_metadata_enum_android_info_supported_hardware_level hwLevel_;\n> \tstd::set capabilities_;\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 D8306BD87D\n\tfor ;\n\tWed, 1 Sep 2021 08:18:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 522BE60255;\n\tWed, 1 Sep 2021 10:18:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6251B60253\n\tfor ;\n\tWed, 1 Sep 2021 10:18:00 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7AA478F;\n\tWed, 1 Sep 2021 10:17:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qw+jlj9V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630484280;\n\tbh=1Jv07hfnDbIKSjByap+WkHwlP5kbpYozgfhXgtlr0DE=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=qw+jlj9Vp+7OFZAI/v9sDxLaYX9lVlcdUNyRMLg/GUYvhhKEJYesN+r9U99FRcYIN\n\t8fbgYbGlgbIIWQDAfdddkIX5uUT6dvFhh9pHgTQ13uNjlpHcEqEWbP7iPl2YxKobJP\n\tgsuphbRof7Ukc9lzyKFJwMdc6YcS+cD9Oh3ux+ks=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-10-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 1 Sep 2021 13:47:55 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210827120757.110615-10-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 09/16] android: capabilities: Collect\n\tabsolute max frame durations","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19255,"web_url":"https://patchwork.libcamera.org/comment/19255/","msgid":"<20210901084258.GY968527@pyrite.rasen.tech>","date":"2021-09-01T08:42:58","subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Aug 31, 2021 at 11:57:50AM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Tue, Aug 31, 2021 at 11:17:07AM +0900, Paul Elder wrote:\n> > Hi Jacopo,\n> >\n> > On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote:\n> > > Now that building the list of supported stream configuration requires\n> > > applying a configuration to the Camera, re-initialize the camera\n> > > controls by applying a configuration generated for the Viewfinder stream\n> > > role before building the list of static metadata.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> >\n> > Looks good, but I have a question.\n> >\n> > > ---\n> > > src/android/camera_capabilities.cpp | 18 +++++++++++++++---\n> > > 1 file changed, 15 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > index fdda90379ce2..723a4fd5a880 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr camera,\n> > > \t}\n> > >\n> > > \tret = initializeStreamConfigurations();\n> > > -\tcamera_->release();\n> > > -\tif (ret)\n> > > +\tif (ret) {\n> > > +\t\tcamera_->release();\n> > > \t\treturn ret;\n> > > +\t}\n> > >\n> > > -\treturn initializeStaticMetadata();\n> > > +\tret = initializeStaticMetadata();\n> > > +\tcamera_->release();\n> > > +\treturn ret;\n> > > }\n> > >\n> > > std::vector\n> > > @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata()\n> > > \t\treturn -EINVAL;\n> > > \t}\n> > >\n> > > +\tstd::unique_ptr cameraConfig =\n> > > +\t\tcamera_->generateConfiguration({ StreamRole::Viewfinder });\n> >\n> > From what I see, initializeStreamConfigurations() generates a\n> > configuration for the StillCapture role. Is it fine that this uses a\n> \n> It does so only to retrieve the maximum resolution.\n> A todo note reminds us that we should get that from a camera property\n> \n> \t * Get the maximum output resolutions\n> \t * \\todo Get this from the camera properties once defined\n> \t */\n> \tstd::unique_ptr cameraConfig =\n> \t\tcamera_->generateConfiguration({ StillCapture });\n> \n> > different role? (Or was the other one changed to Viewfinder where I\n> > wasn't paying attention?)\n> \n> Well, the choice of the Viewfinder role is pretty arbitrary, but we\n> have to define a 'mode' the camera should be set to when initializing\n> the static metadata. We don't have the luxury of assuming much from\n> the underlying camera, and we have to construct the static metadata\n> from the camera properties (static) and the camera controls (change\n> depending on the configuration).\n> \n> Before this series the Camera::controls where static as well, so the\n> Camera didn't get configured at all during the static metadata\n> initialization, and we relied on the PH-initialized ControlInfoMap.\n> \n> As we now need to apply a Configuration to the Camera to collect\n> per-configuration control limits, we need to re-initialize them to a\n> known default before inspecting them.\n> \n> I considered Viewfinder as a good default, but I'm open to other\n> proposals.\n\nAh, okay, I see.\n\n\nReviewed-by: Paul Elder \n\n> \n> >\n> > > +\tint ret = camera_->configure(cameraConfig.get());\n> > > +\tif (ret) {\n> > > +\t\tLOG(HAL, Error) << \"Failed to initialize the camera state\";\n> > > +\t\tstaticMetadata_.reset();\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > \tconst ControlInfoMap &controlsInfo = camera_->controls();\n> > > \tconst ControlList &properties = camera_->properties();\n> > >\n> > > --\n> > > 2.32.0\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D8F30BDC71\n\tfor ;\n\tWed, 1 Sep 2021 08:43:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E34C6916A;\n\tWed, 1 Sep 2021 10:43:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A626C60253\n\tfor ;\n\tWed, 1 Sep 2021 10:43:05 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E3DB3D7;\n\tWed, 1 Sep 2021 10:43:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Wlab8Lbp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630485785;\n\tbh=VlKgJqfBbKAptKWsB8agbGacHQ/G18PxVOG0i6G3WT8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wlab8Lbpu6JX7LBy2BKbHHyIKODexeho1JbZjZblqWtFLyQGiQBYe8suSiTwAX/Ko\n\tbD4XmtH4INjfXiqbXGVXEIkj0A2JJdBnCi1BWfFuho5VAJUPk6hx334wm+Ew/2TVzx\n\t3+GkYAlwTLpQcMF9COQWJ0IDVRUeM7RYBUGs+niM=","Date":"Wed, 1 Sep 2021 17:42:58 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210901084258.GY968527@pyrite.rasen.tech>","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-6-jacopo@jmondi.org>\n\t<20210831021707.GF20879@jade.amanokami.net>\n\t<20210831095750.3pjo6ddk2xdsi6wz@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210831095750.3pjo6ddk2xdsi6wz@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 05/16] android: capabilities:\n\tInitialize camera state when building properties","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19312,"web_url":"https://patchwork.libcamera.org/comment/19312/","msgid":"","date":"2021-09-03T05:43:30","subject":"Re: [libcamera-devel] [PATCH 12/16] android: Populate streams and\n\tduration in the same loop","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 8/27/21 5:37 PM, Jacopo Mondi wrote:\n> The ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and\n> ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS static metadata are\n> populated by looping on the streamConfigurations_ vector.\n>\n> Unify them in a single loop to avoid repeating it.\n>\n> Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Umang Jain \n\n> ---\n> src/android/camera_capabilities.cpp | 23 ++++++++++-------------\n> 1 file changed, 10 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 484242d0ad81..8bde06e824ef 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -1255,7 +1255,9 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\t\t\t maxZoom);\n> \n> \tstd::vector availableStreamConfigurations;\n> +\tstd::vector minFrameDurations;\n> \tavailableStreamConfigurations.reserve(streamConfigurations_.size() * 4);\n> +\tminFrameDurations.reserve(streamConfigurations_.size() * 4);\n> \tfor (const auto &entry : streamConfigurations_) {\n> \t\t/*\n> \t\t * Filter out YUV streams not capable of running at 30 FPS.\n> @@ -1272,12 +1274,19 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\tif (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)\n> \t\t\tcontinue;\n> \n> +\t\t/* Stream configuration map. */\n> \t\tavailableStreamConfigurations.push_back(entry.androidFormat);\n> \t\tavailableStreamConfigurations.push_back(entry.resolution.width);\n> \t\tavailableStreamConfigurations.push_back(entry.resolution.height);\n> \t\tavailableStreamConfigurations.push_back(\n> \t\t\tANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT);\n> \n> +\t\t/* Per-stream durations. */\n> +\t\tminFrameDurations.push_back(entry.androidFormat);\n> +\t\tminFrameDurations.push_back(entry.resolution.width);\n> +\t\tminFrameDurations.push_back(entry.resolution.height);\n> +\t\tminFrameDurations.push_back(entry.minFrameDurationNsec);\n> +\n> \t\tLOG(HAL, Debug)\n> \t\t\t<< \"Output Stream:\" << utils::hex(entry.androidFormat)\n> \t\t\t<< \"(\" << entry.resolution.width << \"x\"\n> @@ -1285,22 +1294,10 @@ int CameraCapabilities::initializeStaticMetadata()\n> \t\t\t<< entry.minFrameDurationNsec << \"]\"\n> \t\t\t<< \"@\" << fps;\n> \t}\n> +\n> \tstaticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,\n> \t\t\t\t availableStreamConfigurations);\n> \n> -\tstd::vector minFrameDurations;\n> -\tminFrameDurations.reserve(streamConfigurations_.size() * 4);\n> -\tfor (const auto &entry : streamConfigurations_) {\n> -\t\tunsigned int fps = static_cast\n> -\t\t\t\t (floor(1e9 / entry.minFrameDurationNsec + 0.05f));\n> -\t\tif (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)\n> -\t\t\tcontinue;\n> -\n> -\t\tminFrameDurations.push_back(entry.androidFormat);\n> -\t\tminFrameDurations.push_back(entry.resolution.width);\n> -\t\tminFrameDurations.push_back(entry.resolution.height);\n> -\t\tminFrameDurations.push_back(entry.minFrameDurationNsec);\n> -\t}\n> \tstaticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> \t\t\t\t minFrameDurations);\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 C62A6BDC71\n\tfor ;\n\tFri, 3 Sep 2021 05:43:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BDDF6916B;\n\tFri, 3 Sep 2021 07:43:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DAF1869166\n\tfor ;\n\tFri, 3 Sep 2021 07:43:34 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 13D3CBBE;\n\tFri, 3 Sep 2021 07:43:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hutqSdPk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630647814;\n\tbh=x2fkaWdWDMb50ew5uZ2OcCzThlEmiJEEHhrP9z8zKuo=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=hutqSdPkndiMWjw9SE704o9GmKuqUlI8ZbYk+eUi4l8xcIzaXgUOvJs3Y/4rpslyo\n\tq8ha3jlbLtM37wSGI4rbz/a6sf+M61hrIJid5+8KinrOTaOMVJHB8bfxAJ3S+jTVwu\n\tr8ERq78UI3C1kbHNt6WOePRiXUPYHDNa9guf7dDg=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210827120757.110615-1-jacopo@jmondi.org>\n\t<20210827120757.110615-13-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"","Date":"Fri, 3 Sep 2021 11:13:30 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210827120757.110615-13-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 12/16] android: Populate streams and\n\tduration in the same loop","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]