[{"id":35623,"web_url":"https://patchwork.libcamera.org/comment/35623/","msgid":"","date":"2025-08-29T10:10:16","subject":"Re: [PATCH 2/4] layer: Add layer that implements the sync algorithm","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 08. 29. 11:10 keltezéssel, Paul Elder írta:\n> Implement the sync layer, which implements the sync algorithm. Any\n> Camera that supports the SyncAdjustment and FrameDurationLimits\n> controls, and that reports the SensorTimestamp metadata will\n> automatically be supported by this layer.\n> \n> This code is heavily based on Raspberry Pi's sync algorithm\n> implementation, from the following files in commit [1]:\n> - src/ipa/rpi/controller/sync_algorithm.h\n> - src/ipa/rpi/controller/sync_status.h\n> - src/ipa/rpi/controller/rpi/sync.cpp\n> - src/ipa/rpi/controller/rpi/sync.h\n> \n> [1] https://github.com/raspberrypi/libcamera/commit/d1a712060dcb0aab8564e0d1d86efe9ffcfee6b9\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/layer/meson.build | 1 +\n> src/layer/sync/meson.build | 16 ++\n> src/layer/sync/sync.cpp | 465 +++++++++++++++++++++++++++++++++++++\n> src/layer/sync/sync.h | 96 ++++++++\n> 4 files changed, 578 insertions(+)\n> create mode 100644 src/layer/sync/meson.build\n> create mode 100644 src/layer/sync/sync.cpp\n> create mode 100644 src/layer/sync/sync.h\n> \n> diff --git a/src/layer/meson.build b/src/layer/meson.build\n> index 3d8b70ad2cd2..24012b239eb0 100644\n> --- a/src/layer/meson.build\n> +++ b/src/layer/meson.build\n> @@ -14,3 +14,4 @@ layers_env.set('LIBCAMERA_LAYER_PATH', meson.current_build_dir())\n> meson.add_devenv(layers_env)\n> \n> subdir('inject_controls')\n> +subdir('sync')\n> diff --git a/src/layer/sync/meson.build b/src/layer/sync/meson.build\n> new file mode 100644\n> index 000000000000..a8af2d2d3e40\n> --- /dev/null\n> +++ b/src/layer/sync/meson.build\n> @@ -0,0 +1,16 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +layer_name = 'sync'\n> +\n> +sync_sources = files([\n> + 'sync.h',\n\nsync.h should not be necessary here.\n\n\n> + 'sync.cpp',\n> +])\n> +\n> +mod = shared_module(layer_name, [sync_sources, libcamera_internal_headers],\n\nMaybe `libcamera_private` should be modified to include `libcamera_internal_headers`?\n\n\n> + name_prefix : '',\n> + include_directories : layer_includes,\n> + dependencies : [libcamera_public, libcamera_private],\n\n`libcamera_private` already includes `libcamera_public`, so I am not sure if\nwe want to explicitly list it.\n\n\n> + gnu_symbol_visibility: 'hidden',\n> + install : true,\n> + install_dir : layer_install_dir)\n> diff --git a/src/layer/sync/sync.cpp b/src/layer/sync/sync.cpp\n> new file mode 100644\n> index 000000000000..7ce70eedceaf\n> --- /dev/null\n> +++ b/src/layer/sync/sync.cpp\n> @@ -0,0 +1,465 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Raspberry Pi Ltd\n> + * Copyright (C) 2025, Ideas On Board Oy\n> + *\n> + * Layer implementation for sync algorithm\n> + */\n> +\n> +#include \"sync.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(SyncLayer)\n> +\n> +} /* namespace libcamera */\n> +\n> +using namespace libcamera;\n> +using namespace std::chrono_literals;\n> +\n> +void *init([[maybe_unused]] const std::string &id)\n> +{\n> +\tSyncLayerData *data = new SyncLayerData;\n> +\tmemset(static_cast(data), 0, sizeof(*data));\n\nThis memset is not correct. The constructor should already take care of\ninitialization, so it should be removed.\n\n\n> +\tdata->socket = -1;\n> +\n> +\tLOG(SyncLayer, Info) << \"Initializing sync layer\";\n> +\n> +\tconst char *kDefaultGroup = \"239.255.255.250\";\n> +\tconstexpr unsigned int kDefaultPort = 10000;\n> +\tconstexpr unsigned int kDefaultSyncPeriod = 30;\n> +\tconstexpr unsigned int kDefaultReadyFrame = 100;\n> +\tconstexpr unsigned int kDefaultMinAdjustment = 50;\n> +\n> +\t/* \\todo load these from configuration file */\n> +\tdata->group = kDefaultGroup;\n> +\tdata->port = kDefaultPort;\n> +\tdata->syncPeriod = kDefaultSyncPeriod;\n> +\tdata->readyFrame = kDefaultReadyFrame;\n> +\tdata->minAdjustment = kDefaultMinAdjustment;\n> +\n> +\treturn static_cast(data);\n\nNo need to cast to void explicitly.\n\n\n> +}\n> +\n> +void terminate(void *closure)\n> +{\n> +\tSyncLayerData *data = static_cast(closure);\n> +\n> +\tif (data->socket >= 0)\n> +\t\tclose(data->socket);\n> +\n> +\tdelete data;\n> +}\n> +\n> +void requestCompleted(void *closure, Request *request)\n> +{\n> +\tSyncLayerData *data = static_cast(closure);\n> +\n> +\tControlList &metadata = request->metadata();\n> +\n> +\t/* SensorTimestamp is required for sync */\n> +\t/* \\todo Document this requirement (along with SyncAdjustment) */\n> +\tauto sensorTimestamp = metadata.get(controls::SensorTimestamp);\n> +\tif (sensorTimestamp) {\n> +\t\tdata->clockRecovery.addSample();\n> +\t\tuint64_t frameWallClock = data->clockRecovery.getOutput(*sensorTimestamp);\n> +\t\tmetadata.set(controls::FrameWallClock, static_cast(frameWallClock));\n> +\t\tif (data->mode != Mode::Off && data->frameDuration)\n> +\t\t\tprocessFrame(data, frameWallClock, metadata);\n> +\t}\n> +\n> +\treturn;\n> +}\n> +\n> +ControlInfoMap::Map updateControls(void *closure, ControlInfoMap &controls)\n> +{\n> +\tSyncLayerData *data = static_cast(closure);\n> +\n> +\t/*\n> +\t * If the SyncAdjustment control is unavailable then the Camera does\n> +\t * not support Sync adjustment\n> +\t */\n> +\tauto it = controls.find(&controls::SyncAdjustment);\n> +\tdata->syncAvailable = it != controls.end();\n> +\tif (!data->syncAvailable) {\n> +\t\tLOG(SyncLayer, Warning)\n> +\t\t\t<< \"Sync layer is not supported: SyncAdjustment control is not available\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/*\n> +\t * Save the default FrameDurationLimits. If it's not available then we\n> +\t * have to wait until one is supplied in a request. We cannot use the\n> +\t * FrameDuration returned from the first frame as the\n> +\t * FrameDurationLimits has to be explicitly set by the application, as\n> +\t * this is the frame rate target that the cameras will sync to.\n> +\t * \\todo Document that FrameDurationLimits is a required control\n> +\t */\n> +\tit = controls.find(&controls::FrameDurationLimits);\n> +\tif (it != controls.end())\n> +\t\tdata->frameDuration = std::chrono::microseconds(it->second.min().get());\n> +\n> +\treturn {\n> +\t\t{ &controls::rpi::SyncMode,\n> +\t\t ControlInfo(controls::rpi::SyncModeValues) },\n> +\t\t{ &controls::rpi::SyncFrames,\n> +\t\t ControlInfo(1, 1000000, 100) },\n> +\t\t{ &controls::SyncInterface,\n> +\t\t ControlInfo(1, 40) }\n> +\t};\n> +}\n> +\n> +void queueRequest(void *closure, Request *request)\n> +{\n> +\tSyncLayerData *data = static_cast(closure);\n> +\tif (!data->syncAvailable)\n> +\t\treturn;\n> +\n> +\tprocessControls(data, request->controls());\n> +\n> +\tif (data->mode == Mode::Client) {\n> +\t\trequest->controls().set(controls::SyncAdjustment,\n> +\t\t\t\t\tdata->frameDurationOffset.count() / 1000);\n> +\t\tdata->frameDurationOffset = utils::Duration(0);\n> +\t}\n> +}\n> +\n> +void start(void *closure, ControlList &controls)\n> +{\n> +\tSyncLayerData *data = static_cast(closure);\n> +\tif (!data->syncAvailable)\n> +\t\treturn;\n> +\n> +\treset(data);\n> +\tdata->clockRecovery.addSample();\n> +\tprocessControls(data, controls);\n> +}\n> +\n> +void reset(SyncLayerData *data)\n> +{\n> +\tdata->syncReady = false;\n> +\tdata->frameCount = 0;\n> +\tdata->serverFrameCountPeriod = 0;\n> +\tdata->serverReadyTime = 0;\n> +\tdata->clientSeenPacket = false;\n> +}\n> +\n> +void initializeSocket(SyncLayerData *data)\n> +{\n> +\tint ret;\n> +\tstruct ip_mreq mreq{};\n> +\tsocklen_t addrlen;\n> +\tSyncPayload payload;\n> +\tunsigned int en = 1;\n> +\n> +\tif (data->socket >= 0) {\n> +\t\tLOG(SyncLayer, Debug)\n> +\t\t\t<< \"Socket already exists; not recreating\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!data->networkInterface.empty())\n> +\t\tmreq.imr_interface.s_addr = inet_addr(data->networkInterface.c_str());\n> +\telse\n> +\t\tmreq.imr_interface.s_addr = htonl(INADDR_ANY);\n> +\n> +\tdata->socket = socket(AF_INET, SOCK_DGRAM, 0);\n\nSad ipv6 :(\n\n\n\n> +\tif (data->socket < 0) {\n> +\t\tLOG(SyncLayer, Error) << \"Unable to create socket\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tstruct sockaddr_in &addr = data->addr;\n> +\tmemset(&addr, 0, sizeof(addr));\n> +\taddr.sin_family = AF_INET;\n> +\taddr.sin_addr.s_addr = data->mode == Mode::Client ? htonl(INADDR_ANY) : inet_addr(data->group.c_str());\n> +\taddr.sin_port = htons(data->port);\n> +\n> +\tif (data->mode != Mode::Client)\n> +\t\treturn;\n> +\n> +\t/* Set to non-blocking. */\n> +\tint flags = fcntl(data->socket, F_GETFL, 0);\n> +\tif (fcntl(data->socket, F_SETFL, flags | O_NONBLOCK) < 0) {\n\nInstead of this, I think we should pass `SOCK_CLOEXEC | SOCK_NONBLOCK` when\ncreating the socket.\n\n\n> +\t\tLOG(SyncLayer, Error) << \"Unable to set socket to no-blocking: \" << strerror(errno);\n> +\t\tgoto err;\n> +\t}\n> +\n> +\tif (setsockopt(data->socket, SOL_SOCKET, SO_REUSEADDR, &en, sizeof(en)) < 0) {\n> +\t\tLOG(SyncLayer, Error) << \"Unable to set socket options: \" << strerror(errno);\n> +\t\tgoto err;\n> +\t}\n> +\n> +\tmreq.imr_multiaddr.s_addr = inet_addr(data->group.c_str());\n> +\tif (setsockopt(data->socket, IPPROTO_IP, IP_ADD_MEMBERSHIP, &mreq, sizeof(mreq)) < 0) {\n> +\t\tLOG(SyncLayer, Error) << \"Unable to set socket options: \" << strerror(errno);\n> +\t\tgoto err;\n> +\t}\n> +\n> +\tif (bind(data->socket, (struct sockaddr *)&addr, sizeof(addr)) < 0) {\n> +\t\tLOG(SyncLayer, Error) << \"Unable to bind client socket: \" << strerror(errno);\n> +\t\tgoto err;\n> +\t}\n> +\n> +\t/*\n> +\t * For the client, flush anything in the socket. It might be stale from a previous sync run,\n> +\t * or we might get another packet in a frame to two before the adjustment caused by this (old)\n> +\t * packet, although correct, had taken effect. So this keeps things simpler.\n> +\t */\n> +\taddrlen = sizeof(addr);\n> +\tret = 0;\n> +\twhile (ret >= 0) {\n> +\t\tret = recvfrom(data->socket,\n> +\t\t\t &payload, sizeof(payload), 0,\n> +\t\t\t (struct sockaddr *)&addr, &addrlen);\n> +\t}\n> +\n> +\treturn;\n> +\n> +err:\n> +\tclose(data->socket);\n> +\tdata->socket = -1;\n> +}\n> +\n> +void processControls(SyncLayerData *data, ControlList &controls)\n> +{\n> +\tauto intf = controls.get(controls::SyncInterface);\n> +\tif (intf)\n> +\t\tdata->networkInterface = *intf;\n> +\n> +\tauto mode = controls.get(controls::rpi::SyncMode);\n> +\tif (mode) {\n> +\t\tdata->mode = static_cast(*mode);\n> +\t\tif (data->mode == Mode::Off) {\n> +\t\t\treset(data);\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * This goes here instead of init() because we need the control\n> +\t\t\t * to tell us whether we're server or client\n> +\t\t\t */\n> +\t\t\tinitializeSocket(data);\n> +\t\t}\n> +\t}\n> +\n> +\tauto syncFrames = controls.get(controls::rpi::SyncFrames);\n> +\tif (syncFrames && *syncFrames > 0)\n> +\t\tdata->readyFrame = *syncFrames;\n> +\n> +\tauto frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> +\tif (frameDurationLimits)\n> +\t\tdata->frameDuration = std::chrono::microseconds((*frameDurationLimits)[0]);\n> +\n> +\t/*\n> +\t * \\todo Should we just let SyncAdjustment through as-is if the\n> +\t * application provides it? Maybe it wants to do sync itself without\n> +\t * the layer, but the layer has been loaded anyway\n> +\t */\n> +}\n> +\n> +/*\n> + * Camera sync algorithm.\n> + * Server - there is a single server that sends framerate timing information over the network to any\n> + * clients that are listening. It also signals when it will send a \"everything is synchronised, now go\"\n> + * message back to the algorithm.\n> + * Client - there may be many clients, either on the same Pi or different ones. They match their\n\n Pi -> device\n\n?\n\n\n> + * framerates to the server, and indicate when to \"go\" at the same instant as the server.\n> + */\n> +void processFrame(SyncLayerData *data, uint64_t frameWallClock, ControlList &metadata)\n> +{\n> +\t/* frameWallClock has already been de-jittered for us. Convert from ns into us. */\n> +\tuint64_t wallClockFrameTimestamp = frameWallClock / 1000;\n> +\n> +\t/*\n> +\t * This is the headline frame duration in microseconds as programmed into the sensor. Strictly,\n> +\t * the sensor might not quite match the system clock, but this shouldn't matter for the calculations\n> +\t * we'll do with it, unless it's a very very long way out!\n> +\t */\n> +\tuint32_t frameDuration = data->frameDuration.get();\n> +\n> +\t/* Timestamps tell us if we've dropped any frames, but we still want to count them. */\n> +\tunsigned int droppedFrames = 0;\n> +\t/* Count dropped frames into the frame counter */\n> +\tif (data->frameCount) {\n> +\t\twallClockFrameTimestamp =\n> +\t\t\tstd::max(wallClockFrameTimestamp,\n> +\t\t\t\t\t data->lastWallClockFrameTimestamp + frameDuration / 2);\n> +\t\t/*\n> +\t\t * Round down here, because data->frameCount gets incremented\n> +\t\t * at the end of the function.\n> +\t\t */\n> +\t\tdroppedFrames =\n> +\t\t\t(wallClockFrameTimestamp - data->lastWallClockFrameTimestamp - frameDuration / 2) / frameDuration;\n> +\t\tdata->frameCount += droppedFrames;\n> +\t}\n> +\n> +\tif (data->mode == Mode::Server)\n> +\t\tprocessFrameServer(data, wallClockFrameTimestamp, metadata, droppedFrames);\n> +\telse if (data->mode == Mode::Client)\n> +\t\tprocessFrameClient(data, wallClockFrameTimestamp, metadata);\n> +\n> +\tdata->lastWallClockFrameTimestamp = wallClockFrameTimestamp;\n> +\n> +\tmetadata.set(controls::rpi::SyncReady, data->syncReady);\n> +\n> +\tdata->frameCount++;\n> +}\n> +\n> +void processFrameServer(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> +\t\t\tControlList &metadata, unsigned int droppedFrames)\n> +{\n> +\tuint32_t frameDuration = data->frameDuration.get();\n> +\n> +\t/*\n> +\t * Server sends a packet every syncPeriod frames, or as soon after as possible (if any\n> +\t * frames were dropped).\n> +\t */\n> +\tdata->serverFrameCountPeriod += droppedFrames;\n> +\n> +\tif (data->frameCount == 0) {\n> +\t\tdata->frameDurationEstimated = frameDuration;\n> +\t} else {\n> +\t\tdouble diff = (wallClockFrameTimestamp - data->lastWallClockFrameTimestamp) / (1 + droppedFrames);\n> +\t\tint N = std::min(data->frameCount, 99U);\n\nunsigned int / auto ?\n\n\n> +\t\t/*\n> +\t\t * Smooth out the variance of the frame duration estimation to\n> +\t\t * get closer to the true frame duration\n> +\t\t */\n> +\t\tdata->frameDurationEstimated = data->frameCount == 1 ? diff\n> +\t\t\t\t\t : (N * data->frameDurationEstimated + diff) / (N + 1);\n> +\t}\n> +\n> +\t/* Calculate frames remaining, and therefore \"time left until ready\". */\n> +\tint framesRemaining = data->readyFrame - data->frameCount;\n> +\tuint64_t wallClockReadyTime = wallClockFrameTimestamp + (int64_t)framesRemaining * data->frameDurationEstimated;\n> +\n> +\tif (data->serverFrameCountPeriod >= data->syncPeriod) {\n> +\t\t/* It's time to sync */\n> +\t\tdata->serverFrameCountPeriod = 0;\n> +\n> +\t\tSyncPayload payload;\n> +\t\t/* round to nearest us */\n> +\t\tpayload.frameDuration = data->frameDurationEstimated + .5;\n> +\t\tpayload.wallClockFrameTimestamp = wallClockFrameTimestamp;\n> +\t\tpayload.wallClockReadyTime = wallClockReadyTime;\n> +\n> +\t\tLOG(SyncLayer, Debug) << \"Send packet (frameNumber \" << data->frameCount << \"):\";\n> +\t\tLOG(SyncLayer, Debug) << \" frameDuration \" << payload.frameDuration;\n> +\t\tLOG(SyncLayer, Debug) << \" wallClockFrameTimestamp \" << wallClockFrameTimestamp\n> +\t\t\t\t << \" (\" << wallClockFrameTimestamp - data->lastWallClockFrameTimestamp << \")\";\n> +\t\tLOG(SyncLayer, Debug) << \" wallClockReadyTime \" << wallClockReadyTime;\n> +\n> +\t\tif (sendto(data->socket, &payload, sizeof(payload), 0, (const sockaddr *)&data->addr, sizeof(data->addr)) < 0)\n> +\t\t\tLOG(SyncLayer, Error) << \"Send error! \" << strerror(errno);\n> +\t}\n> +\n> +\tint64_t timerValue = static_cast(wallClockReadyTime - wallClockFrameTimestamp);\n> +\tif (!data->syncReady && wallClockFrameTimestamp + data->frameDurationEstimated / 2 > wallClockReadyTime) {\n> +\t\tdata->syncReady = true;\n> +\t\tLOG(SyncLayer, Info) << \"*** Sync achieved! Difference \" << timerValue << \"us\";\n> +\t}\n> +\n> +\t/* Server always reports this */\n> +\tmetadata.set(controls::rpi::SyncTimer, timerValue);\n> +\n> +\tdata->serverFrameCountPeriod += 1;\n> +}\n> +\n> +void processFrameClient(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> +\t\t\tControlList &metadata)\n> +{\n> +\tuint64_t serverFrameTimestamp = 0;\n> +\tSyncPayload payload;\n> +\n> +\tbool packetReceived = false;\n> +\twhile (true) {\n> +\t\tsocklen_t addrlen = sizeof(data->addr);\n> +\t\tint ret = recvfrom(data->socket, &payload, sizeof(payload), 0, (struct sockaddr *)&data->addr, &addrlen);\n\n`recvfrom()` returns `ssize_t`, so I would use that, even if makes no difference here.\nAlso, why update `data->addr`?\n\n\n> +\n> +\t\tif (ret < 0)\n> +\t\t\tbreak;\n\n if (ret != sizeof(payload)) continue;\n\nis something along these lines not necessary?\n\n\n> +\t\tpacketReceived = (ret > 0);\n> +\t\tdata->clientSeenPacket = true;\n> +\n> +\t\tdata->frameDurationEstimated = payload.frameDuration;\n> +\t\tserverFrameTimestamp = payload.wallClockFrameTimestamp;\n> +\t\tdata->serverReadyTime = payload.wallClockReadyTime;\n> +\t}\n> +\n> +\tif (packetReceived) {\n> +\t\tuint64_t clientFrameTimestamp = wallClockFrameTimestamp;\n> +\t\tint64_t clientServerDelta = clientFrameTimestamp - serverFrameTimestamp;\n> +\t\t/* \"A few frames ago\" may have better matched the server's frame. Calculate when it was. */\n> +\t\tint framePeriodErrors = (clientServerDelta + data->frameDurationEstimated / 2) / data->frameDurationEstimated;\n> +\t\tint64_t clientFrameTimestampNearest = clientFrameTimestamp - framePeriodErrors * data->frameDurationEstimated;\n> +\t\t/* We must shorten a single client frame by this amount if it exceeds the minimum: */\n> +\t\tint32_t correction = clientFrameTimestampNearest - serverFrameTimestamp;\n> +\t\tif (std::abs(correction) < data->minAdjustment)\n> +\t\t\tcorrection = 0;\n> +\n> +\t\tLOG(SyncLayer, Debug) << \"Received packet (frameNumber \" << data->frameCount << \"):\";\n> +\t\tLOG(SyncLayer, Debug) << \" serverFrameTimestamp \" << serverFrameTimestamp;\n> +\t\tLOG(SyncLayer, Debug) << \" serverReadyTime \" << data->serverReadyTime;\n> +\t\tLOG(SyncLayer, Debug) << \" clientFrameTimestamp \" << clientFrameTimestamp;\n> +\t\tLOG(SyncLayer, Debug) << \" clientFrameTimestampNearest \" << clientFrameTimestampNearest\n> +\t\t\t\t << \" (\" << framePeriodErrors << \")\";\n> +\t\tLOG(SyncLayer, Debug) << \" correction \" << correction;\n> +\n> +\t\tdata->frameDurationOffset = correction * 1us;\n> +\t}\n> +\n> +\tint64_t timerValue = static_cast(data->serverReadyTime - wallClockFrameTimestamp);\n> +\tif (data->clientSeenPacket && !data->syncReady && wallClockFrameTimestamp + data->frameDurationEstimated / 2 > data->serverReadyTime) {\n> +\t\tdata->syncReady = true;\n> +\t\tLOG(SyncLayer, Info) << \"*** Sync achieved! Difference \" << timerValue << \"us\";\n> +\t}\n> +\n> +\t/* Client reports this once it receives it from the server */\n> +\tif (data->clientSeenPacket)\n> +\t\tmetadata.set(controls::rpi::SyncTimer, timerValue);\n> +}\n> +\n> +namespace libcamera {\n> +\n> +extern \"C\" {\n> +\n> +[[gnu::visibility(\"default\")]]\n> +struct LayerInfo layerInfo{\n> +\t.name = \"sync\",\n> +\t.layerAPIVersion = 1,\n> +};\n> +\n> +[[gnu::visibility(\"default\")]]\n> +struct LayerInterface layerInterface{\n> +\t.init = init,\n> +\t.terminate = terminate,\n> +\t.bufferCompleted = nullptr,\n> +\t.requestCompleted = requestCompleted,\n> +\t.disconnected = nullptr,\n> +\t.acquire = nullptr,\n> +\t.release = nullptr,\n> +\t.controls = updateControls,\n> +\t.properties = nullptr,\n> +\t.configure = nullptr,\n> +\t.createRequest = nullptr,\n> +\t.queueRequest = queueRequest,\n> +\t.start = start,\n> +\t.stop = nullptr,\n> +};\n> +\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/layer/sync/sync.h b/src/layer/sync/sync.h\n> new file mode 100644\n> index 000000000000..141ec0cbf801\n> --- /dev/null\n> +++ b/src/layer/sync/sync.h\n> @@ -0,0 +1,96 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board Oy\n> + *\n> + * Layer implementation for sync algorithm\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/clock_recovery.h\"\n> +\n> +enum class Mode {\n> +\tOff,\n> +\tServer,\n> +\tClient,\n> +};\n> +\n> +struct SyncPayload {\n> +\t/* Frame duration in microseconds. */\n> +\tuint32_t frameDuration;\n> +\t/* Server wall clock version of the frame timestamp. */\n> +\tuint64_t wallClockFrameTimestamp;\n> +\t/* Server wall clock version of the sync time. */\n> +\tuint64_t wallClockReadyTime;\n> +};\n> +\n> +struct SyncLayerData {\n> +\tbool syncAvailable;\n> +\n> +\t/* Sync algorithm parameters */\n> +\t/* IP group address for sync messages */\n> +\tstd::string group;\n> +\t/* port number for messages */\n> +\tuint16_t port;\n> +\t/* send a sync message every this many frames */\n> +\tuint32_t syncPeriod;\n> +\t/* don't adjust the client frame length by less than this (us) */\n> +\tuint32_t minAdjustment;\n> +\t/* This is the network interface to listen/multicast on */\n> +\tstd::string networkInterface;\n> +\n> +\t/* Sync algorithm controls */\n> +\tMode mode;\n> +\tlibcamera::utils::Duration frameDuration;\n> +\t/* tell the application we're ready after this many frames */\n> +\tuint32_t readyFrame;\n> +\n> +\t/* Sync algorithm state */\n> +\tbool syncReady = false;\n> +\tunsigned int frameCount = 0;\n> +\t/* send the next packet when this reaches syncPeriod */\n> +\tuint32_t serverFrameCountPeriod = 0;\n> +\t/* the client's latest value for when the server will be \"ready\" */\n> +\tuint64_t serverReadyTime = 0;\n> +\t/* whether the client has received a packet yet */\n> +\tbool clientSeenPacket = false;\n> +\n> +\t/* estimate the true frame duration of the sensor (in us) */\n> +\tdouble frameDurationEstimated = 0;\n> +\t/* wall clock timestamp of previous frame (in us) */\n> +\tuint64_t lastWallClockFrameTimestamp;\n> +\n> +\t/* Frame length correction to apply */\n> +\tlibcamera::utils::Duration frameDurationOffset;\n> +\n> +\t/* Infrastructure state */\n> +\tclass libcamera::ClockRecovery clockRecovery;\n\nWhy `class`?\n\n\n> +\tsockaddr_in addr;\n> +\tint socket = -1;\n\nWhy not `libcamera::UniqueFd`?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> +};\n> +\n> +void *init(const std::string &id);\n> +void terminate(void *closure);\n> +void requestCompleted(void *closure, libcamera::Request *request);\n> +libcamera::ControlInfoMap::Map updateControls(void *closure,\n> +\t\t\t\t\t libcamera::ControlInfoMap &controls);\n> +void queueRequest(void *closure, libcamera::Request *request);\n> +void start(void *closure, libcamera::ControlList &controls);\n> +\n> +void reset(SyncLayerData *data);\n> +void initializeSocket(SyncLayerData *data);\n> +void processControls(SyncLayerData *data, libcamera::ControlList &controls);\n> +void processFrame(SyncLayerData *data, uint64_t frameWallClock, libcamera::ControlList &metadata);\n> +void processFrameServer(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> +\t\t\tlibcamera::ControlList &metadata, unsigned int droppedFrames);\n> +void processFrameClient(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> +\t\t\tlibcamera::ControlList &metadata);","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 F3284BD87C\n\tfor ;\n\tFri, 29 Aug 2025 10:10:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72DB16931D;\n\tFri, 29 Aug 2025 12:10:21 +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 2EE3B613AF\n\tfor ;\n\tFri, 29 Aug 2025 12:10:20 +0200 (CEST)","from [192.168.33.19] (185.221.143.232.nat.pool.zt.hu\n\t[185.221.143.232])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B25523BEE;\n\tFri, 29 Aug 2025 12:09:14 +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=\"QK92JLj8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1756462155;\n\tbh=Er0wpr4H4bzsGzglvRWpXMF6hIDwXd7LWps+n9nnlsc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=QK92JLj8ycmhfqVuoH1VvKG49BdhwWixMeGh0lEtmOlg2FH8+JFAq+Qzl1qORQepW\n\tRgVRfVaEYJp0e7B3dsfs9digv4+uH1SsmbM6HNFN1Zy940Tsj0vvkZMIaCIbVvKoRL\n\tFJgktMu/HRYYcvOCZJS3tmP1PH0eCZF9LKX2aj/A=","Message-ID":"","Date":"Fri, 29 Aug 2025 12:10:16 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/4] layer: Add layer that implements the sync algorithm","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-3-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-3-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\" "}},{"id":35969,"web_url":"https://patchwork.libcamera.org/comment/35969/","msgid":"<175879649536.3174118.3950800210660921049@neptunite.rasen.tech>","date":"2025-09-25T10:34:55","subject":"Re: [PATCH 2/4] layer: Add layer that implements the sync algorithm","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Barnabás,\n\nThanks for the review.\n\nQuoting Barnabás Pőcze (2025-08-29 19:10:16)\n> Hi\n> \n> \n> 2025. 08. 29. 11:10 keltezéssel, Paul Elder írta:\n> > Implement the sync layer, which implements the sync algorithm. Any\n> > Camera that supports the SyncAdjustment and FrameDurationLimits\n> > controls, and that reports the SensorTimestamp metadata will\n> > automatically be supported by this layer.\n> > \n> > This code is heavily based on Raspberry Pi's sync algorithm\n> > implementation, from the following files in commit [1]:\n> > - src/ipa/rpi/controller/sync_algorithm.h\n> > - src/ipa/rpi/controller/sync_status.h\n> > - src/ipa/rpi/controller/rpi/sync.cpp\n> > - src/ipa/rpi/controller/rpi/sync.h\n> > \n> > [1] https://github.com/raspberrypi/libcamera/commit/d1a712060dcb0aab8564e0d1d86efe9ffcfee6b9\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/layer/meson.build | 1 +\n> > src/layer/sync/meson.build | 16 ++\n> > src/layer/sync/sync.cpp | 465 +++++++++++++++++++++++++++++++++++++\n> > src/layer/sync/sync.h | 96 ++++++++\n> > 4 files changed, 578 insertions(+)\n> > create mode 100644 src/layer/sync/meson.build\n> > create mode 100644 src/layer/sync/sync.cpp\n> > create mode 100644 src/layer/sync/sync.h\n> > \n> > diff --git a/src/layer/meson.build b/src/layer/meson.build\n> > index 3d8b70ad2cd2..24012b239eb0 100644\n> > --- a/src/layer/meson.build\n> > +++ b/src/layer/meson.build\n> > @@ -14,3 +14,4 @@ layers_env.set('LIBCAMERA_LAYER_PATH', meson.current_build_dir())\n> > meson.add_devenv(layers_env)\n> > \n> > subdir('inject_controls')\n> > +subdir('sync')\n> > diff --git a/src/layer/sync/meson.build b/src/layer/sync/meson.build\n> > new file mode 100644\n> > index 000000000000..a8af2d2d3e40\n> > --- /dev/null\n> > +++ b/src/layer/sync/meson.build\n> > @@ -0,0 +1,16 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +layer_name = 'sync'\n> > +\n> > +sync_sources = files([\n> > + 'sync.h',\n> \n> sync.h should not be necessary here.\n\nack\n\n> \n> \n> > + 'sync.cpp',\n> > +])\n> > +\n> > +mod = shared_module(layer_name, [sync_sources, libcamera_internal_headers],\n> \n> Maybe `libcamera_private` should be modified to include `libcamera_internal_headers`?\n\nI suppose internal headers should be considered as part of the private API.\n\n> \n> \n> > + name_prefix : '',\n> > + include_directories : layer_includes,\n> > + dependencies : [libcamera_public, libcamera_private],\n> \n> `libcamera_private` already includes `libcamera_public`, so I am not sure if\n> we want to explicitly list it.\n\nGood point. I guess we don't need to specify libcamera_public.\n\nack on everything else.\n\n\nThanks,\n\nPaul\n\n> \n> \n> > + gnu_symbol_visibility: 'hidden',\n> > + install : true,\n> > + install_dir : layer_install_dir)\n> > diff --git a/src/layer/sync/sync.cpp b/src/layer/sync/sync.cpp\n> > new file mode 100644\n> > index 000000000000..7ce70eedceaf\n> > --- /dev/null\n> > +++ b/src/layer/sync/sync.cpp\n> > @@ -0,0 +1,465 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2024, Raspberry Pi Ltd\n> > + * Copyright (C) 2025, Ideas On Board Oy\n> > + *\n> > + * Layer implementation for sync algorithm\n> > + */\n> > +\n> > +#include \"sync.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(SyncLayer)\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +using namespace libcamera;\n> > +using namespace std::chrono_literals;\n> > +\n> > +void *init([[maybe_unused]] const std::string &id)\n> > +{\n> > + SyncLayerData *data = new SyncLayerData;\n> > + memset(static_cast(data), 0, sizeof(*data));\n> \n> This memset is not correct. The constructor should already take care of\n> initialization, so it should be removed.\n> \n> \n> > + data->socket = -1;\n> > +\n> > + LOG(SyncLayer, Info) << \"Initializing sync layer\";\n> > +\n> > + const char *kDefaultGroup = \"239.255.255.250\";\n> > + constexpr unsigned int kDefaultPort = 10000;\n> > + constexpr unsigned int kDefaultSyncPeriod = 30;\n> > + constexpr unsigned int kDefaultReadyFrame = 100;\n> > + constexpr unsigned int kDefaultMinAdjustment = 50;\n> > +\n> > + /* \\todo load these from configuration file */\n> > + data->group = kDefaultGroup;\n> > + data->port = kDefaultPort;\n> > + data->syncPeriod = kDefaultSyncPeriod;\n> > + data->readyFrame = kDefaultReadyFrame;\n> > + data->minAdjustment = kDefaultMinAdjustment;\n> > +\n> > + return static_cast(data);\n> \n> No need to cast to void explicitly.\n> \n> \n> > +}\n> > +\n> > +void terminate(void *closure)\n> > +{\n> > + SyncLayerData *data = static_cast(closure);\n> > +\n> > + if (data->socket >= 0)\n> > + close(data->socket);\n> > +\n> > + delete data;\n> > +}\n> > +\n> > +void requestCompleted(void *closure, Request *request)\n> > +{\n> > + SyncLayerData *data = static_cast(closure);\n> > +\n> > + ControlList &metadata = request->metadata();\n> > +\n> > + /* SensorTimestamp is required for sync */\n> > + /* \\todo Document this requirement (along with SyncAdjustment) */\n> > + auto sensorTimestamp = metadata.get(controls::SensorTimestamp);\n> > + if (sensorTimestamp) {\n> > + data->clockRecovery.addSample();\n> > + uint64_t frameWallClock = data->clockRecovery.getOutput(*sensorTimestamp);\n> > + metadata.set(controls::FrameWallClock, static_cast(frameWallClock));\n> > + if (data->mode != Mode::Off && data->frameDuration)\n> > + processFrame(data, frameWallClock, metadata);\n> > + }\n> > +\n> > + return;\n> > +}\n> > +\n> > +ControlInfoMap::Map updateControls(void *closure, ControlInfoMap &controls)\n> > +{\n> > + SyncLayerData *data = static_cast(closure);\n> > +\n> > + /*\n> > + * If the SyncAdjustment control is unavailable then the Camera does\n> > + * not support Sync adjustment\n> > + */\n> > + auto it = controls.find(&controls::SyncAdjustment);\n> > + data->syncAvailable = it != controls.end();\n> > + if (!data->syncAvailable) {\n> > + LOG(SyncLayer, Warning)\n> > + << \"Sync layer is not supported: SyncAdjustment control is not available\";\n> > + return {};\n> > + }\n> > +\n> > + /*\n> > + * Save the default FrameDurationLimits. If it's not available then we\n> > + * have to wait until one is supplied in a request. We cannot use the\n> > + * FrameDuration returned from the first frame as the\n> > + * FrameDurationLimits has to be explicitly set by the application, as\n> > + * this is the frame rate target that the cameras will sync to.\n> > + * \\todo Document that FrameDurationLimits is a required control\n> > + */\n> > + it = controls.find(&controls::FrameDurationLimits);\n> > + if (it != controls.end())\n> > + data->frameDuration = std::chrono::microseconds(it->second.min().get());\n> > +\n> > + return {\n> > + { &controls::rpi::SyncMode,\n> > + ControlInfo(controls::rpi::SyncModeValues) },\n> > + { &controls::rpi::SyncFrames,\n> > + ControlInfo(1, 1000000, 100) },\n> > + { &controls::SyncInterface,\n> > + ControlInfo(1, 40) }\n> > + };\n> > +}\n> > +\n> > +void queueRequest(void *closure, Request *request)\n> > +{\n> > + SyncLayerData *data = static_cast(closure);\n> > + if (!data->syncAvailable)\n> > + return;\n> > +\n> > + processControls(data, request->controls());\n> > +\n> > + if (data->mode == Mode::Client) {\n> > + request->controls().set(controls::SyncAdjustment,\n> > + data->frameDurationOffset.count() / 1000);\n> > + data->frameDurationOffset = utils::Duration(0);\n> > + }\n> > +}\n> > +\n> > +void start(void *closure, ControlList &controls)\n> > +{\n> > + SyncLayerData *data = static_cast(closure);\n> > + if (!data->syncAvailable)\n> > + return;\n> > +\n> > + reset(data);\n> > + data->clockRecovery.addSample();\n> > + processControls(data, controls);\n> > +}\n> > +\n> > +void reset(SyncLayerData *data)\n> > +{\n> > + data->syncReady = false;\n> > + data->frameCount = 0;\n> > + data->serverFrameCountPeriod = 0;\n> > + data->serverReadyTime = 0;\n> > + data->clientSeenPacket = false;\n> > +}\n> > +\n> > +void initializeSocket(SyncLayerData *data)\n> > +{\n> > + int ret;\n> > + struct ip_mreq mreq{};\n> > + socklen_t addrlen;\n> > + SyncPayload payload;\n> > + unsigned int en = 1;\n> > +\n> > + if (data->socket >= 0) {\n> > + LOG(SyncLayer, Debug)\n> > + << \"Socket already exists; not recreating\";\n> > + return;\n> > + }\n> > +\n> > + if (!data->networkInterface.empty())\n> > + mreq.imr_interface.s_addr = inet_addr(data->networkInterface.c_str());\n> > + else\n> > + mreq.imr_interface.s_addr = htonl(INADDR_ANY);\n> > +\n> > + data->socket = socket(AF_INET, SOCK_DGRAM, 0);\n> \n> Sad ipv6 :(\n> \n> \n> \n> > + if (data->socket < 0) {\n> > + LOG(SyncLayer, Error) << \"Unable to create socket\";\n> > + return;\n> > + }\n> > +\n> > + struct sockaddr_in &addr = data->addr;\n> > + memset(&addr, 0, sizeof(addr));\n> > + addr.sin_family = AF_INET;\n> > + addr.sin_addr.s_addr = data->mode == Mode::Client ? htonl(INADDR_ANY) : inet_addr(data->group.c_str());\n> > + addr.sin_port = htons(data->port);\n> > +\n> > + if (data->mode != Mode::Client)\n> > + return;\n> > +\n> > + /* Set to non-blocking. */\n> > + int flags = fcntl(data->socket, F_GETFL, 0);\n> > + if (fcntl(data->socket, F_SETFL, flags | O_NONBLOCK) < 0) {\n> \n> Instead of this, I think we should pass `SOCK_CLOEXEC | SOCK_NONBLOCK` when\n> creating the socket.\n> \n> \n> > + LOG(SyncLayer, Error) << \"Unable to set socket to no-blocking: \" << strerror(errno);\n> > + goto err;\n> > + }\n> > +\n> > + if (setsockopt(data->socket, SOL_SOCKET, SO_REUSEADDR, &en, sizeof(en)) < 0) {\n> > + LOG(SyncLayer, Error) << \"Unable to set socket options: \" << strerror(errno);\n> > + goto err;\n> > + }\n> > +\n> > + mreq.imr_multiaddr.s_addr = inet_addr(data->group.c_str());\n> > + if (setsockopt(data->socket, IPPROTO_IP, IP_ADD_MEMBERSHIP, &mreq, sizeof(mreq)) < 0) {\n> > + LOG(SyncLayer, Error) << \"Unable to set socket options: \" << strerror(errno);\n> > + goto err;\n> > + }\n> > +\n> > + if (bind(data->socket, (struct sockaddr *)&addr, sizeof(addr)) < 0) {\n> > + LOG(SyncLayer, Error) << \"Unable to bind client socket: \" << strerror(errno);\n> > + goto err;\n> > + }\n> > +\n> > + /*\n> > + * For the client, flush anything in the socket. It might be stale from a previous sync run,\n> > + * or we might get another packet in a frame to two before the adjustment caused by this (old)\n> > + * packet, although correct, had taken effect. So this keeps things simpler.\n> > + */\n> > + addrlen = sizeof(addr);\n> > + ret = 0;\n> > + while (ret >= 0) {\n> > + ret = recvfrom(data->socket,\n> > + &payload, sizeof(payload), 0,\n> > + (struct sockaddr *)&addr, &addrlen);\n> > + }\n> > +\n> > + return;\n> > +\n> > +err:\n> > + close(data->socket);\n> > + data->socket = -1;\n> > +}\n> > +\n> > +void processControls(SyncLayerData *data, ControlList &controls)\n> > +{\n> > + auto intf = controls.get(controls::SyncInterface);\n> > + if (intf)\n> > + data->networkInterface = *intf;\n> > +\n> > + auto mode = controls.get(controls::rpi::SyncMode);\n> > + if (mode) {\n> > + data->mode = static_cast(*mode);\n> > + if (data->mode == Mode::Off) {\n> > + reset(data);\n> > + } else {\n> > + /*\n> > + * This goes here instead of init() because we need the control\n> > + * to tell us whether we're server or client\n> > + */\n> > + initializeSocket(data);\n> > + }\n> > + }\n> > +\n> > + auto syncFrames = controls.get(controls::rpi::SyncFrames);\n> > + if (syncFrames && *syncFrames > 0)\n> > + data->readyFrame = *syncFrames;\n> > +\n> > + auto frameDurationLimits = controls.get(controls::FrameDurationLimits);\n> > + if (frameDurationLimits)\n> > + data->frameDuration = std::chrono::microseconds((*frameDurationLimits)[0]);\n> > +\n> > + /*\n> > + * \\todo Should we just let SyncAdjustment through as-is if the\n> > + * application provides it? Maybe it wants to do sync itself without\n> > + * the layer, but the layer has been loaded anyway\n> > + */\n> > +}\n> > +\n> > +/*\n> > + * Camera sync algorithm.\n> > + * Server - there is a single server that sends framerate timing information over the network to any\n> > + * clients that are listening. It also signals when it will send a \"everything is synchronised, now go\"\n> > + * message back to the algorithm.\n> > + * Client - there may be many clients, either on the same Pi or different ones. They match their\n> \n> Pi -> device\n> \n> ?\n> \n> \n> > + * framerates to the server, and indicate when to \"go\" at the same instant as the server.\n> > + */\n> > +void processFrame(SyncLayerData *data, uint64_t frameWallClock, ControlList &metadata)\n> > +{\n> > + /* frameWallClock has already been de-jittered for us. Convert from ns into us. */\n> > + uint64_t wallClockFrameTimestamp = frameWallClock / 1000;\n> > +\n> > + /*\n> > + * This is the headline frame duration in microseconds as programmed into the sensor. Strictly,\n> > + * the sensor might not quite match the system clock, but this shouldn't matter for the calculations\n> > + * we'll do with it, unless it's a very very long way out!\n> > + */\n> > + uint32_t frameDuration = data->frameDuration.get();\n> > +\n> > + /* Timestamps tell us if we've dropped any frames, but we still want to count them. */\n> > + unsigned int droppedFrames = 0;\n> > + /* Count dropped frames into the frame counter */\n> > + if (data->frameCount) {\n> > + wallClockFrameTimestamp =\n> > + std::max(wallClockFrameTimestamp,\n> > + data->lastWallClockFrameTimestamp + frameDuration / 2);\n> > + /*\n> > + * Round down here, because data->frameCount gets incremented\n> > + * at the end of the function.\n> > + */\n> > + droppedFrames =\n> > + (wallClockFrameTimestamp - data->lastWallClockFrameTimestamp - frameDuration / 2) / frameDuration;\n> > + data->frameCount += droppedFrames;\n> > + }\n> > +\n> > + if (data->mode == Mode::Server)\n> > + processFrameServer(data, wallClockFrameTimestamp, metadata, droppedFrames);\n> > + else if (data->mode == Mode::Client)\n> > + processFrameClient(data, wallClockFrameTimestamp, metadata);\n> > +\n> > + data->lastWallClockFrameTimestamp = wallClockFrameTimestamp;\n> > +\n> > + metadata.set(controls::rpi::SyncReady, data->syncReady);\n> > +\n> > + data->frameCount++;\n> > +}\n> > +\n> > +void processFrameServer(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> > + ControlList &metadata, unsigned int droppedFrames)\n> > +{\n> > + uint32_t frameDuration = data->frameDuration.get();\n> > +\n> > + /*\n> > + * Server sends a packet every syncPeriod frames, or as soon after as possible (if any\n> > + * frames were dropped).\n> > + */\n> > + data->serverFrameCountPeriod += droppedFrames;\n> > +\n> > + if (data->frameCount == 0) {\n> > + data->frameDurationEstimated = frameDuration;\n> > + } else {\n> > + double diff = (wallClockFrameTimestamp - data->lastWallClockFrameTimestamp) / (1 + droppedFrames);\n> > + int N = std::min(data->frameCount, 99U);\n> \n> unsigned int / auto ?\n> \n> \n> > + /*\n> > + * Smooth out the variance of the frame duration estimation to\n> > + * get closer to the true frame duration\n> > + */\n> > + data->frameDurationEstimated = data->frameCount == 1 ? diff\n> > + : (N * data->frameDurationEstimated + diff) / (N + 1);\n> > + }\n> > +\n> > + /* Calculate frames remaining, and therefore \"time left until ready\". */\n> > + int framesRemaining = data->readyFrame - data->frameCount;\n> > + uint64_t wallClockReadyTime = wallClockFrameTimestamp + (int64_t)framesRemaining * data->frameDurationEstimated;\n> > +\n> > + if (data->serverFrameCountPeriod >= data->syncPeriod) {\n> > + /* It's time to sync */\n> > + data->serverFrameCountPeriod = 0;\n> > +\n> > + SyncPayload payload;\n> > + /* round to nearest us */\n> > + payload.frameDuration = data->frameDurationEstimated + .5;\n> > + payload.wallClockFrameTimestamp = wallClockFrameTimestamp;\n> > + payload.wallClockReadyTime = wallClockReadyTime;\n> > +\n> > + LOG(SyncLayer, Debug) << \"Send packet (frameNumber \" << data->frameCount << \"):\";\n> > + LOG(SyncLayer, Debug) << \" frameDuration \" << payload.frameDuration;\n> > + LOG(SyncLayer, Debug) << \" wallClockFrameTimestamp \" << wallClockFrameTimestamp\n> > + << \" (\" << wallClockFrameTimestamp - data->lastWallClockFrameTimestamp << \")\";\n> > + LOG(SyncLayer, Debug) << \" wallClockReadyTime \" << wallClockReadyTime;\n> > +\n> > + if (sendto(data->socket, &payload, sizeof(payload), 0, (const sockaddr *)&data->addr, sizeof(data->addr)) < 0)\n> > + LOG(SyncLayer, Error) << \"Send error! \" << strerror(errno);\n> > + }\n> > +\n> > + int64_t timerValue = static_cast(wallClockReadyTime - wallClockFrameTimestamp);\n> > + if (!data->syncReady && wallClockFrameTimestamp + data->frameDurationEstimated / 2 > wallClockReadyTime) {\n> > + data->syncReady = true;\n> > + LOG(SyncLayer, Info) << \"*** Sync achieved! Difference \" << timerValue << \"us\";\n> > + }\n> > +\n> > + /* Server always reports this */\n> > + metadata.set(controls::rpi::SyncTimer, timerValue);\n> > +\n> > + data->serverFrameCountPeriod += 1;\n> > +}\n> > +\n> > +void processFrameClient(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> > + ControlList &metadata)\n> > +{\n> > + uint64_t serverFrameTimestamp = 0;\n> > + SyncPayload payload;\n> > +\n> > + bool packetReceived = false;\n> > + while (true) {\n> > + socklen_t addrlen = sizeof(data->addr);\n> > + int ret = recvfrom(data->socket, &payload, sizeof(payload), 0, (struct sockaddr *)&data->addr, &addrlen);\n> \n> `recvfrom()` returns `ssize_t`, so I would use that, even if makes no difference here.\n> Also, why update `data->addr`?\n> \n> \n> > +\n> > + if (ret < 0)\n> > + break;\n> \n> if (ret != sizeof(payload)) continue;\n> \n> is something along these lines not necessary?\n> \n> \n> > + packetReceived = (ret > 0);\n> > + data->clientSeenPacket = true;\n> > +\n> > + data->frameDurationEstimated = payload.frameDuration;\n> > + serverFrameTimestamp = payload.wallClockFrameTimestamp;\n> > + data->serverReadyTime = payload.wallClockReadyTime;\n> > + }\n> > +\n> > + if (packetReceived) {\n> > + uint64_t clientFrameTimestamp = wallClockFrameTimestamp;\n> > + int64_t clientServerDelta = clientFrameTimestamp - serverFrameTimestamp;\n> > + /* \"A few frames ago\" may have better matched the server's frame. Calculate when it was. */\n> > + int framePeriodErrors = (clientServerDelta + data->frameDurationEstimated / 2) / data->frameDurationEstimated;\n> > + int64_t clientFrameTimestampNearest = clientFrameTimestamp - framePeriodErrors * data->frameDurationEstimated;\n> > + /* We must shorten a single client frame by this amount if it exceeds the minimum: */\n> > + int32_t correction = clientFrameTimestampNearest - serverFrameTimestamp;\n> > + if (std::abs(correction) < data->minAdjustment)\n> > + correction = 0;\n> > +\n> > + LOG(SyncLayer, Debug) << \"Received packet (frameNumber \" << data->frameCount << \"):\";\n> > + LOG(SyncLayer, Debug) << \" serverFrameTimestamp \" << serverFrameTimestamp;\n> > + LOG(SyncLayer, Debug) << \" serverReadyTime \" << data->serverReadyTime;\n> > + LOG(SyncLayer, Debug) << \" clientFrameTimestamp \" << clientFrameTimestamp;\n> > + LOG(SyncLayer, Debug) << \" clientFrameTimestampNearest \" << clientFrameTimestampNearest\n> > + << \" (\" << framePeriodErrors << \")\";\n> > + LOG(SyncLayer, Debug) << \" correction \" << correction;\n> > +\n> > + data->frameDurationOffset = correction * 1us;\n> > + }\n> > +\n> > + int64_t timerValue = static_cast(data->serverReadyTime - wallClockFrameTimestamp);\n> > + if (data->clientSeenPacket && !data->syncReady && wallClockFrameTimestamp + data->frameDurationEstimated / 2 > data->serverReadyTime) {\n> > + data->syncReady = true;\n> > + LOG(SyncLayer, Info) << \"*** Sync achieved! Difference \" << timerValue << \"us\";\n> > + }\n> > +\n> > + /* Client reports this once it receives it from the server */\n> > + if (data->clientSeenPacket)\n> > + metadata.set(controls::rpi::SyncTimer, timerValue);\n> > +}\n> > +\n> > +namespace libcamera {\n> > +\n> > +extern \"C\" {\n> > +\n> > +[[gnu::visibility(\"default\")]]\n> > +struct LayerInfo layerInfo{\n> > + .name = \"sync\",\n> > + .layerAPIVersion = 1,\n> > +};\n> > +\n> > +[[gnu::visibility(\"default\")]]\n> > +struct LayerInterface layerInterface{\n> > + .init = init,\n> > + .terminate = terminate,\n> > + .bufferCompleted = nullptr,\n> > + .requestCompleted = requestCompleted,\n> > + .disconnected = nullptr,\n> > + .acquire = nullptr,\n> > + .release = nullptr,\n> > + .controls = updateControls,\n> > + .properties = nullptr,\n> > + .configure = nullptr,\n> > + .createRequest = nullptr,\n> > + .queueRequest = queueRequest,\n> > + .start = start,\n> > + .stop = nullptr,\n> > +};\n> > +\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/layer/sync/sync.h b/src/layer/sync/sync.h\n> > new file mode 100644\n> > index 000000000000..141ec0cbf801\n> > --- /dev/null\n> > +++ b/src/layer/sync/sync.h\n> > @@ -0,0 +1,96 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2025, Ideas On Board Oy\n> > + *\n> > + * Layer implementation for sync algorithm\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/clock_recovery.h\"\n> > +\n> > +enum class Mode {\n> > + Off,\n> > + Server,\n> > + Client,\n> > +};\n> > +\n> > +struct SyncPayload {\n> > + /* Frame duration in microseconds. */\n> > + uint32_t frameDuration;\n> > + /* Server wall clock version of the frame timestamp. */\n> > + uint64_t wallClockFrameTimestamp;\n> > + /* Server wall clock version of the sync time. */\n> > + uint64_t wallClockReadyTime;\n> > +};\n> > +\n> > +struct SyncLayerData {\n> > + bool syncAvailable;\n> > +\n> > + /* Sync algorithm parameters */\n> > + /* IP group address for sync messages */\n> > + std::string group;\n> > + /* port number for messages */\n> > + uint16_t port;\n> > + /* send a sync message every this many frames */\n> > + uint32_t syncPeriod;\n> > + /* don't adjust the client frame length by less than this (us) */\n> > + uint32_t minAdjustment;\n> > + /* This is the network interface to listen/multicast on */\n> > + std::string networkInterface;\n> > +\n> > + /* Sync algorithm controls */\n> > + Mode mode;\n> > + libcamera::utils::Duration frameDuration;\n> > + /* tell the application we're ready after this many frames */\n> > + uint32_t readyFrame;\n> > +\n> > + /* Sync algorithm state */\n> > + bool syncReady = false;\n> > + unsigned int frameCount = 0;\n> > + /* send the next packet when this reaches syncPeriod */\n> > + uint32_t serverFrameCountPeriod = 0;\n> > + /* the client's latest value for when the server will be \"ready\" */\n> > + uint64_t serverReadyTime = 0;\n> > + /* whether the client has received a packet yet */\n> > + bool clientSeenPacket = false;\n> > +\n> > + /* estimate the true frame duration of the sensor (in us) */\n> > + double frameDurationEstimated = 0;\n> > + /* wall clock timestamp of previous frame (in us) */\n> > + uint64_t lastWallClockFrameTimestamp;\n> > +\n> > + /* Frame length correction to apply */\n> > + libcamera::utils::Duration frameDurationOffset;\n> > +\n> > + /* Infrastructure state */\n> > + class libcamera::ClockRecovery clockRecovery;\n> \n> Why `class`?\n> \n> \n> > + sockaddr_in addr;\n> > + int socket = -1;\n> \n> Why not `libcamera::UniqueFd`?\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n> > +};\n> > +\n> > +void *init(const std::string &id);\n> > +void terminate(void *closure);\n> > +void requestCompleted(void *closure, libcamera::Request *request);\n> > +libcamera::ControlInfoMap::Map updateControls(void *closure,\n> > + libcamera::ControlInfoMap &controls);\n> > +void queueRequest(void *closure, libcamera::Request *request);\n> > +void start(void *closure, libcamera::ControlList &controls);\n> > +\n> > +void reset(SyncLayerData *data);\n> > +void initializeSocket(SyncLayerData *data);\n> > +void processControls(SyncLayerData *data, libcamera::ControlList &controls);\n> > +void processFrame(SyncLayerData *data, uint64_t frameWallClock, libcamera::ControlList &metadata);\n> > +void processFrameServer(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> > + libcamera::ControlList &metadata, unsigned int droppedFrames);\n> > +void processFrameClient(SyncLayerData *data, int64_t wallClockFrameTimestamp,\n> > + libcamera::ControlList &metadata);\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 04A34BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 10:35:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7CAB6B5F3;\n\tThu, 25 Sep 2025 12:35:07 +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 DC22769318\n\tfor ;\n\tThu, 25 Sep 2025 12:35:03 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:d0b0:c301:99de:3de])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 20A941129; \n\tThu, 25 Sep 2025 12:33:37 +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=\"BB4v/a6f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758796419;\n\tbh=byAY0vwd6MGuM6NsiKk0ZeLh9+udePgIjaimOPYn/eg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=BB4v/a6ftjcJ/qBbrC74QMX7nNIiaKi3WgdBfg8I/2yKxLUk2El2PqzK++1sOxTna\n\tPcGQmMYRKNQKR8vnW8r94xGkT9kmCG2mjUle7ntlvAXjkqBAZ3RPiVkfL/0Aw9LS4o\n\tXBR4QcPIczKvMSDjRs4vcsBYW6EPZiwKjt+1YMf8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250829091011.2628954-1-paul.elder@ideasonboard.com>\n\t<20250829091011.2628954-3-paul.elder@ideasonboard.com>\n\t","Subject":"Re: [PATCH 2/4] layer: Add layer that implements the sync algorithm","From":"Paul Elder ","Cc":"david.plowman@raspberrypi.com, naush@raspberrypi.com,\n\tkieran.bingham@ideasonboard.com, stefan.klug@ideasonboard.com","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 25 Sep 2025 19:34:55 +0900","Message-ID":"<175879649536.3174118.3950800210660921049@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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\" "}}]