[{"id":35971,"web_url":"https://patchwork.libcamera.org/comment/35971/","msgid":"","date":"2025-09-25T11:15:24","subject":"Re: [PATCH 3/4] libipa: Add SyncHelper","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 29. 11:10 keltezéssel, Paul Elder írta:\n> The sync layer allows any Camera to get sync support as long as it\n> implements:\n> - SyncAdjustment control\n> - FrameDurationLimits control\n> - SensorTimestamp metadata\n> \n> While FrameDurationLimits is usually implemented by AGC and\n> SensorTimestamp is already implemented by all pipeline handlers, only\n> SyncAdjustment is left that needs to be implemented for all platforms.\n> It naturally would be a good idea to implement it in libipa so all IPAs\n> can easily be supported.\n> \n> Add SyncHelper that wraps handling of the SyncAdjustment control.\n> Although it still needs to be plumbed into each IPA, it should mitigate\n> a lot of future code duplication.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/libipa/meson.build | 2 +\n> src/ipa/libipa/sync_helper.cpp | 99 ++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/sync_helper.h | 36 +++++++++++++\n> 3 files changed, 137 insertions(+)\n> create mode 100644 src/ipa/libipa/sync_helper.cpp\n> create mode 100644 src/ipa/libipa/sync_helper.h\n> \n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 660be94054fa..f48d66425e52 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -17,6 +17,7 @@ libipa_headers = files([\n> 'lux.h',\n> 'module.h',\n> 'pwl.h',\n> + 'sync_helper.h',\n> ])\n> \n> libipa_sources = files([\n> @@ -36,6 +37,7 @@ libipa_sources = files([\n> 'lux.cpp',\n> 'module.cpp',\n> 'pwl.cpp',\n> + 'sync_helper.cpp',\n> ])\n> \n> libipa_includes = include_directories('..')\n> diff --git a/src/ipa/libipa/sync_helper.cpp b/src/ipa/libipa/sync_helper.cpp\n> new file mode 100644\n> index 000000000000..ceab3866cd6b\n> --- /dev/null\n> +++ b/src/ipa/libipa/sync_helper.cpp\n> @@ -0,0 +1,99 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025 Ideas on Board Oy\n> + *\n> + * Helper class that handles sync\n> + */\n> +#include \"sync_helper.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +/**\n> + * \\file sync_helper.h\n> + * \\brief Helper class that encapsulates handling sync\n> + */\n> +\n> +namespace libcamera {\n> +\n> +using namespace std::literals::chrono_literals;\n\nIs this needed?\n\n\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class SyncHelper\n> + * \\brief Class for handling sync\n> + *\n> + * In order for a Camera to support the sync algorithm (via the sync layer), it\n> + * needs to implement the FrameDurationLimits control, the SyncAdjustment\n> + * control, and the SensorTimestamp metadata. The first is handled by AGC, the\n> + * last is handled by the IPA cores, and the second one is handled by this\n> + * helper. It must be plumbed into IPAs, however.\n> + */\n> +\n> +/**\n> + * \\fn SyncHelper::SyncHelper()\n> + * \\brief Construct an SyncHelper instance\n> + */\n> +\n> +/**\n> + * \\brief Return an entry for the ControlInfoMap of the IPA for SyncAdjustment\n> + * \\param[in] maxFrameDuration The maximum value for FrameDurationLimits\n> + *\n> + * This function creates an entry for SyncAdjustment that can be insterted\n> + * directly into the ControlInfoMap of the IPA.\n> + *\n> + * The SyncAdjustment limits are computed based on the \\a maxFrameDuration.\n> + * Technically the limits of SyncAdjustment depend on the currently set\n> + * FrameDurationLimits, but since they can be set in the same request this\n> + * doesn't really work. Instead report half of the maximum FrameDurationLimits\n> + * as the SyncAdjustment limits.\n> + *\n> + * \\return Entry for ControlInfoMap for SyncAdjustment\n> + */\n> +ControlInfo SyncHelper::controlInfo(int64_t maxFrameDuration) const\n> +{\n> +\treturn ControlInfo(static_cast(-maxFrameDuration / 2),\n> +\t\t\t static_cast(maxFrameDuration / 2), 0);\n> +}\n> +\n> +/**\n> + * \\brief Set the sync adjustment value\n> + * \\param[in] sync The SyncAdjustment value as passed in by the application, in microseconds\n> + * \\param[in] minFrameDuration The minimum frame duration as set by FrameDurationLimits\n> + *\n> + * This function takes the value of SyncAdjustment as passed in by the\n> + * application, and saves it to be later read by getSync(). The \\a sync value\n> + * will be clamped by \\a minFrameDuration. This\n> + * function is meant to be called by the IPA at queueRequest time.\n> + */\n> +void SyncHelper::setSync(int32_t sync, utils::Duration minFrameDuration)\n> +{\n> +\tutils::Duration value = std::chrono::microseconds(sync);\n> +\tframeDurationOffset_ = std::clamp(value,\n> +\t\t\t\t\t -minFrameDuration, minFrameDuration);\n> +}\n> +\n> +/**\n> + * \\fn SyncHelper::getSync()\n> + * \\brief Retrieve the set sync adjustment value\n> + *\n> + * This function returns the SyncAdjustment value stored in setSync(). It is\n> + * meant to be read out by the IPA when the computing the frame duration to\n> + * set, usually by AGC.\n> + *\n> + * \\return The amount of time to adjust the frame duration by for sync\n> + */\n> +\n> +/**\n> + * \\fn SyncHelper::resetSync()\n> + * \\brief Reset the stored sync adjustment value\n> + *\n> + * This function resets the state of the sync helper, that is it zeros the\n> + * stored frame duration offset.\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/sync_helper.h b/src/ipa/libipa/sync_helper.h\n> new file mode 100644\n> index 000000000000..41850a53043a\n> --- /dev/null\n> +++ b/src/ipa/libipa/sync_helper.h\n> @@ -0,0 +1,36 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025 Ideas on Board Oy\n> + *\n> + * Helper class that handles sync\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class SyncHelper\n> +{\n> +public:\n> +\tSyncHelper() = default;\n> +\t~SyncHelper() = default;\n\nI would drop these.\n\n\n> +\n> +\tControlInfo controlInfo(int64_t maxFrameDuration) const;\n\nI think this function can be static.\n\n\n> +\n> +\tvoid setSync(int32_t sync, utils::Duration minFrameDuration);\n\nI feel like the above two functions are short enough to be inline.\n\n\nRegards,\nBarnabás Pőcze\n\n> +\tutils::Duration getSync() const { return frameDurationOffset_; }\n> +\tvoid resetSync() { frameDurationOffset_ = utils::Duration(0); }\n> +\n> +private:\n> +\tutils::Duration frameDurationOffset_ = utils::Duration(0);\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */","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 30A85BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 11:15:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4CD46B5F3;\n\tThu, 25 Sep 2025 13:15:30 +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 A260E69318\n\tfor ;\n\tThu, 25 Sep 2025 13:15:28 +0200 (CEST)","from [192.168.33.22] (185.221.140.70.nat.pool.zt.hu\n\t[185.221.140.70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D2A32D0;\n\tThu, 25 Sep 2025 13:14: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=\"EAlKjgNQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758798844;\n\tbh=uISjXcj7t1kU/6JeTmvfy6ZKkc2bwnR5bDV3dH2EAmM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=EAlKjgNQk2uKwTLBQFaUf5VCihBAvwgDXbXyt/jh8EWe/o2lzG9MawvBaDcYRBXaB\n\trHZDkujn1u5ik5zjm4sCwM0NCtcNe9IYY+oBXYsHEEDZQ+SJtr3GfC7dD5no/qy+Z8\n\tu4BrJ8pboSsx/5x9e/Py0kbve9H4o3wjkonkSNRE=","Message-ID":"","Date":"Thu, 25 Sep 2025 13:15:24 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/4] libipa: Add SyncHelper","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"david.plowman@raspberrypi.com, naush@raspberrypi.com,\n\tkieran.bingham@ideasonboard.com, stefan.klug@ideasonboard.com","References":"<20250829091011.2628954-1-paul.elder@ideasonboard.com>\n\t<20250829091011.2628954-4-paul.elder@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250829091011.2628954-4-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]