[{"id":15073,"web_url":"https://patchwork.libcamera.org/comment/15073/","msgid":"","date":"2021-02-09T01:35:33","subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: camera_device:\n\tCompute frame durations","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 Tue, Jan 26, 2021 at 06:30:05PM +0100, Jacopo Mondi wrote:\n> Use the FrameDuration control reported by pipeline handlers to register\n> the Auto-Exposure routine FPS range, the minimum stream frame durations\n> and the sensor maximum frame duration.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 72 +++++++++++++++++++++++++----------\n> 1 file changed, 52 insertions(+), 20 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 1a384460974c..e3d43bea4700 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,6 +9,7 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -682,10 +683,10 @@ std::tuple CameraDevice::calculateStaticMetadataSize()\n> {\n> \t/*\n> \t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 53 entries, 782 bytes of static metadata\n> +\t * Currently: 54 entries, 802 bytes of static metadata\n> \t */\n> -\tuint32_t numEntries = 53;\n> -\tuint32_t byteSize = 782;\n> +\tuint32_t numEntries = 54;\n> +\tuint32_t byteSize = 802;\n> \n> \t/*\n> \t * Calculate space occupation in bytes for dynamically built metadata\n> @@ -760,12 +761,34 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\t\t\t aeAvailableModes.data(),\n> \t\t\t\t aeAvailableModes.size());\n> \n> -\tstd::vector availableAeFpsTarget = {\n> -\t\t15, 30,\n> -\t};\n> -\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,\n> -\t\t\t\t availableAeFpsTarget.data(),\n> -\t\t\t\t availableAeFpsTarget.size());\n> +\tint64_t minFrameDurationUsec = -1;\n> +\tint64_t maxFrameDurationUsec = -1;\n\nAs Android reports frame durations in nanoseconds, how about storing\nthem in nanoseconds in these two variables as well (and naming them\nminFrameDurationNsec and maxFrameDurationNsec) ?\n\n> +\tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurations);\n> +\tif (frameDurationsInfo != controlsInfo.end()) {\n> +\t\tminFrameDurationUsec = frameDurationsInfo->second.min().get();\n> +\t\tmaxFrameDurationUsec = frameDurationsInfo->second.max().get();\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\nDo we ? :-) It's fine for now, we'll likely rework this later in any\ncase.\n\n> +\t\t */\n> +\t\tint32_t minFps = static_cast(::round(1000000.0f /\n\nI'd use std::round(), for the reason explained in the coding style\ndocument. Do you need the cast, isn't it implicit ? Maybe 1e6 would be\nmore readable ? Up to you.\n\n> +\t\t\t\t\t\t\t maxFrameDurationUsec));\n> +\t\tminFps = std::max(1, minFps);\n> +\t\tint maxFps = static_cast(::round(1000000.0f /\n> +\t\t\t\t\t\t\t minFrameDurationUsec));\n\nSame here.\n\nMaybe int32_t, as minFps is an int32_t too ?\n\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\nThis may not be enough, there's a comment for camcorder profiles in the\ndocumentation, but we can care about this later.\n\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, 4);\n> +\t}\n> \n> \tstd::vector aeCompensationRange = {\n> \t\t0, 0,\n> @@ -929,6 +952,12 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,\n> \t\t\t\t ×tampSource, 1);\n> \n> +\tif (maxFrameDurationUsec > 0) {\n> +\t\tint64_t maxFrameDuration = maxFrameDurationUsec * 1000;\n\nYou'll be able to drop this variable if you turn maxFrameDurationUsec\ninto maxFrameDurationNsec.\n\n> +\t\tstaticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION,\n> +\t\t\t\t\t &maxFrameDuration, 1);\n> +\t}\n> +\n> \t/* Statistics static metadata. */\n> \tuint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> \tstaticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,\n> @@ -1063,18 +1092,20 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\t\t\t availableStallDurations.data(),\n> \t\t\t\t availableStallDurations.size());\n> \n> -\t/* \\todo Collect the minimum frame duration from the camera. */\n> -\tstd::vector minFrameDurations;\n> -\tminFrameDurations.reserve(streamConfigurations_.size() * 4);\n> -\tfor (const auto &entry : streamConfigurations_) {\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(33333333);\n> +\t/* Use the minimum frame duration for all the YUV/RGB formats. */\n> +\tif (minFrameDurationUsec > 0) {\n> +\t\tstd::vector minFrameDurations;\n> +\t\tminFrameDurations.reserve(streamConfigurations_.size() * 4);\n> +\t\tfor (const auto &entry : streamConfigurations_) {\n> +\t\t\tminFrameDurations.push_back(entry.androidFormat);\n> +\t\t\tminFrameDurations.push_back(entry.resolution.width);\n> +\t\t\tminFrameDurations.push_back(entry.resolution.height);\n> +\t\t\tminFrameDurations.push_back(minFrameDurationUsec * 1000);\n> +\t\t}\n> +\t\tstaticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> +\t\t\t\t\t minFrameDurations.data(),\n> +\t\t\t\t\t minFrameDurations.size());\n> \t}\n> -\tstaticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,\n> -\t\t\t\t minFrameDurations.data(),\n> -\t\t\t\t minFrameDurations.size());\n> \n> \tuint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;\n> \tstaticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);\n> @@ -1156,6 +1187,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> \t\tANDROID_SENSOR_INFO_SENSITIVITY_RANGE,\n> \t\tANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,\n> \t\tANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,\n> +\t\tANDROID_SENSOR_INFO_MAX_FRAME_DURATION,\n> \t\tANDROID_SENSOR_ORIENTATION,\n> \t\tANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,\n> \t\tANDROID_SENSOR_INFO_PHYSICAL_SIZE,","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 5BB34BD160\n\tfor ;\n\tTue, 9 Feb 2021 01:36:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD0AB6160B;\n\tTue, 9 Feb 2021 02:35:59 +0100 (CET)","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 54A77613F3\n\tfor ;\n\tTue, 9 Feb 2021 02:35:58 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2408583;\n\tTue, 9 Feb 2021 02:35:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"u6sDKm5t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612834557;\n\tbh=MWT+XSxvzdjIMjuUC2hryoKqF+2g2YTyFuoADAIxvBs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=u6sDKm5tjXCq3jvrxtr5ltEAp7grAapn71LukANYmAtjfrnLS4uVWLANgZVR2JKl1\n\tIXaouYgt2AET4T7K+Q4/4Mu/2IPiRyrjXMGeT7+mmtn07rF/eHtfqR6bq0ap0n23So\n\tOPFHs+Z9TUwHjDOU8d73pzMdqDTRi790nfrZmWWQ=","Date":"Tue, 9 Feb 2021 03:35:33 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210126173008.446321-1-jacopo@jmondi.org>\n\t<20210126173008.446321-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126173008.446321-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] android: camera_device:\n\tCompute 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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]