Clock: use secs for better accuracy#32967
Open
RomanPudashkin wants to merge 6 commits intomusescore:masterfrom
Open
Clock: use secs for better accuracy#32967RomanPudashkin wants to merge 6 commits intomusescore:masterfrom
RomanPudashkin wants to merge 6 commits intomusescore:masterfrom
Conversation
…back position desync
📝 WalkthroughWalkthroughThe PR converts the audio framework's timing API from 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/audio/engine/internal/clock.cpp`:
- Around line 48-56: When m_countDown <= secs, compute the remaining advance as
remainder = secs - m_countDown (capture m_countDown before zeroing it), set
m_countDown = 0, call onAction(ActionType::CountDownEnded, m_currentTime), and
then set newTime = m_currentTime + remainder instead of advancing by the full
secs; this ensures the tick only advances by the post-countdown remainder and
fixes the off-by-one-tick behavior.
- Around line 82-123: The callbacks invoked via onAction() from Clock methods
(start, stop, pause, resume, seek) capture this (SequencePlayer) and can race
with destruction because setOnAction(nullptr) is called in the destructor
without waiting; fix by introducing synchronization around the callback storage
and invocation: protect the internal callback with a mutex (or make it an
atomic/shared_ptr) updated in setOnAction and read under lock in onAction(), or
swap to a refcounted/shared_ptr inside setOnAction so onAction can copy the
shared_ptr and invoke it outside the lock; also ensure the destructor either
clears the callback under the same lock or waits for in-flight invocations to
finish (e.g., via a guard/refcount or condition variable) before returning.
Ensure references to methods: onAction, setOnAction, start, stop, pause, resume,
seek, and the SequencePlayer destructor are updated accordingly.
In `@src/framework/audio/engine/internal/sequenceplayer.cpp`:
- Around line 155-163: In SequencePlayer::setLoop, pass the requested new loop
start (the parameter from) into m_clock->setTimeLoop instead of using the old
m_loopStart, and only update m_loopStart to from when setTimeLoop succeeds;
i.e., call m_clock->setTimeLoop(from, to) and on successful Ret set m_loopStart
= from so the validation uses the requested range and not the previous value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20910562-fd30-476d-a59e-a3e5c17a35ec
📒 Files selected for processing (21)
src/framework/audio/engine/iclock.hsrc/framework/audio/engine/iengineplayback.hsrc/framework/audio/engine/internal/clock.cppsrc/framework/audio/engine/internal/clock.hsrc/framework/audio/engine/internal/engineplayback.cppsrc/framework/audio/engine/internal/engineplayback.hsrc/framework/audio/engine/internal/enginerpccontroller.cppsrc/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/export/soundtrackwriter.hsrc/framework/audio/engine/internal/igetplaybackposition.hsrc/framework/audio/engine/internal/mixer.cppsrc/framework/audio/engine/internal/mixer.hsrc/framework/audio/engine/internal/sequenceplayer.cppsrc/framework/audio/engine/internal/sequenceplayer.hsrc/framework/audio/engine/internal/tracksequence.cppsrc/framework/audio/engine/internal/tracksequence.hsrc/framework/audio/engine/isequenceplayer.hsrc/framework/audio/main/internal/player.cppsrc/framework/audio/main/internal/player.hsrc/framework/audio/main/iplayer.hsrc/playback/internal/playbackcontroller.cpp
💤 Files with no reviewable changes (1)
- src/framework/audio/engine/internal/tracksequence.h
RomanPudashkin
force-pushed
the
clock_use_secs
branch
from
4354514 to
072419e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/framework/audio/engine/internal/clock.cpp (2)
177-186:⚠️ Potential issue | 🔴 CriticalMake callback teardown safe under concurrent dispatch.
setOnAction()mutatesm_onActionFuncwhileonAction()reads and invokes it without synchronization.SequencePlayer::~SequencePlayer()clears the callback, but a proc-thread dispatch can still race and execute the stale lambda afterSequencePlayeris destroyed.#!/bin/bash set -eu echo "=== Clock callback storage/invocation ===" sed -n '172,190p' src/framework/audio/engine/internal/clock.cpp echo echo "=== SequencePlayer callback registration and teardown ===" sed -n '36,78p' src/framework/audio/engine/internal/sequenceplayer.cpp echo echo "=== Synchronization primitives around m_onActionFunc ===" rg -n 'm_onActionFunc|mutex|lock_guard|unique_lock|shared_ptr|condition_variable|atomic' src/framework/audio/engine/internal/clock.*Use shared state that survives in-flight calls, or add a teardown barrier so
setOnAction(nullptr)cannot race withonAction().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/engine/internal/clock.cpp` around lines 177 - 186, The callback race is caused by unsynchronized mutation of m_onActionFunc in Clock::setOnAction while Clock::onAction reads and calls it; fix by making the stored callback reference-counted and copied before invocation (e.g. change storage to std::shared_ptr<OnActionFunc> m_onActionFuncPtr, have setOnAction create/reset that shared_ptr, and have onAction grab a local copy auto cb = std::atomic_load(&m_onActionFuncPtr) or std::shared_ptr copy under a mutex and then call cb if non-null), or alternatively add a teardown barrier so setOnAction(nullptr) waits for in-flight onAction invocations to finish (use a mutex + counter or condition_variable) — update Clock::setOnAction and Clock::onAction accordingly and ensure SequencePlayer::~SequencePlayer() clears the callback via the new safe API.
48-59:⚠️ Potential issue | 🟠 MajorAdvance only by the post-countdown remainder.
When
m_countDown <= secs, Line 59 still advances by the fullsecs. That skips ahead by the portion of the block that should still have been spent waiting; them_countDown == secscase is off by a full tick.Proposed fix
- if (!m_countDown.is_zero()) { - m_countDown -= secs; - - if (m_countDown > 0.) { - return; - } - - m_countDown = 0.; - onAction(ActionType::CountDownEnded, m_currentTime); - } - - secs_t newTime = m_currentTime + secs; + secs_t elapsed = secs; + + if (!m_countDown.is_zero()) { + if (m_countDown >= secs) { + m_countDown -= secs; + if (m_countDown.is_zero()) { + onAction(ActionType::CountDownEnded, m_currentTime); + } + return; + } + + elapsed -= m_countDown; + m_countDown = 0.; + onAction(ActionType::CountDownEnded, m_currentTime); + } + + secs_t newTime = m_currentTime + elapsed;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/engine/internal/clock.cpp` around lines 48 - 59, The countdown handling consumes part of the incoming secs but the code still advances m_currentTime by the full secs; capture the original m_countDown at the start of the block and when m_countDown <= secs compute the leftover time = secs - original_countDown, set m_countDown = 0 and call onAction(ActionType::CountDownEnded, m_currentTime), then advance time using only the leftover instead of the full secs (use leftover when computing newTime = m_currentTime + ...); if m_countDown > secs keep the current subtract-and-return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/audio/engine/internal/clock.cpp`:
- Around line 119-126: Clock::seek currently returns early when newPosition ==
m_currentTime which suppresses same-position seek notifications; remove that
early return so Clock::seek always calls setCurrentTime(newPosition) and then
onAction(ActionType::Seek, m_currentTime) even when position is unchanged.
Ensure the function continues to use setCurrentTime and onAction as-is so
resume() and loop-rewind paths still emit ActionType::Seek to SequencePlayer for
resyncing.
In `@src/framework/audio/engine/internal/sequenceplayer.cpp`:
- Around line 61-63: The current LoopEndReached handler always seeks to
m_loopStart, dropping any overshoot; change it to preserve the remainder by
seeking to m_loopStart + (time - m_loopEnd). To implement this, ensure
SequencePlayer has access to the loop end/time (add m_loopEnd if not present) or
have Clock include the wrapped target time in the LoopEndReached action; then in
the case for IClock::ActionType::LoopEndReached compute wrapped = m_loopStart +
(action.time - m_loopEnd) and call m_clock->seek(wrapped) instead of
m_clock->seek(m_loopStart), keeping all other behavior the same.
---
Duplicate comments:
In `@src/framework/audio/engine/internal/clock.cpp`:
- Around line 177-186: The callback race is caused by unsynchronized mutation of
m_onActionFunc in Clock::setOnAction while Clock::onAction reads and calls it;
fix by making the stored callback reference-counted and copied before invocation
(e.g. change storage to std::shared_ptr<OnActionFunc> m_onActionFuncPtr, have
setOnAction create/reset that shared_ptr, and have onAction grab a local copy
auto cb = std::atomic_load(&m_onActionFuncPtr) or std::shared_ptr copy under a
mutex and then call cb if non-null), or alternatively add a teardown barrier so
setOnAction(nullptr) waits for in-flight onAction invocations to finish (use a
mutex + counter or condition_variable) — update Clock::setOnAction and
Clock::onAction accordingly and ensure SequencePlayer::~SequencePlayer() clears
the callback via the new safe API.
- Around line 48-59: The countdown handling consumes part of the incoming secs
but the code still advances m_currentTime by the full secs; capture the original
m_countDown at the start of the block and when m_countDown <= secs compute the
leftover time = secs - original_countDown, set m_countDown = 0 and call
onAction(ActionType::CountDownEnded, m_currentTime), then advance time using
only the leftover instead of the full secs (use leftover when computing newTime
= m_currentTime + ...); if m_countDown > secs keep the current
subtract-and-return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f1b615a-c161-4ab9-bb36-9f48ef3fa09d
📒 Files selected for processing (19)
src/framework/audio/engine/iclock.hsrc/framework/audio/engine/iengineplayback.hsrc/framework/audio/engine/internal/clock.cppsrc/framework/audio/engine/internal/clock.hsrc/framework/audio/engine/internal/engineplayback.cppsrc/framework/audio/engine/internal/engineplayback.hsrc/framework/audio/engine/internal/enginerpccontroller.cppsrc/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/export/soundtrackwriter.hsrc/framework/audio/engine/internal/igetplaybackposition.hsrc/framework/audio/engine/internal/mixer.cppsrc/framework/audio/engine/internal/mixer.hsrc/framework/audio/engine/internal/sequenceplayer.cppsrc/framework/audio/engine/internal/sequenceplayer.hsrc/framework/audio/engine/isequenceplayer.hsrc/framework/audio/main/internal/player.cppsrc/framework/audio/main/internal/player.hsrc/framework/audio/main/iplayer.hsrc/playback/internal/playbackcontroller.cpp
Comment on lines
+119
to
+126
| void Clock::seek(const secs_t newPosition) | ||
| { | ||
| if (m_currentTime == msecs) { | ||
| if (m_currentTime == newPosition) { | ||
| return; | ||
| } | ||
|
|
||
| setCurrentTime(msecs); | ||
| m_seekOccurred.notify(); | ||
| setCurrentTime(newPosition); | ||
| onAction(ActionType::Seek, m_currentTime); |
There was a problem hiding this comment.
Do not suppress same-position seek notifications.
Line 121 turns seek(m_currentTime) into a no-op. That breaks the explicit resync in resume(), and loop rewinds can hit the same path when the loop starts at the current position, so SequencePlayer never receives ActionType::Seek to realign tracks.
Proposed fix
void Clock::seek(const secs_t newPosition)
{
- if (m_currentTime == newPosition) {
- return;
+ if (m_currentTime != newPosition) {
+ setCurrentTime(newPosition);
}
-
- setCurrentTime(newPosition);
- onAction(ActionType::Seek, m_currentTime);
+ onAction(ActionType::Seek, newPosition);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void Clock::seek(const secs_t newPosition) | |
| { | |
| if (m_currentTime == msecs) { | |
| if (m_currentTime == newPosition) { | |
| return; | |
| } | |
| setCurrentTime(msecs); | |
| m_seekOccurred.notify(); | |
| setCurrentTime(newPosition); | |
| onAction(ActionType::Seek, m_currentTime); | |
| void Clock::seek(const secs_t newPosition) | |
| { | |
| if (m_currentTime != newPosition) { | |
| setCurrentTime(newPosition); | |
| } | |
| onAction(ActionType::Seek, newPosition); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/engine/internal/clock.cpp` around lines 119 - 126,
Clock::seek currently returns early when newPosition == m_currentTime which
suppresses same-position seek notifications; remove that early return so
Clock::seek always calls setCurrentTime(newPosition) and then
onAction(ActionType::Seek, m_currentTime) even when position is unchanged.
Ensure the function continues to use setCurrentTime and onAction as-is so
resume() and loop-rewind paths still emit ActionType::Seek to SequencePlayer for
resyncing.
Comment on lines
+61
to
+63
| case IClock::ActionType::LoopEndReached: | ||
| m_clock->seek(m_loopStart); | ||
| break; |
There was a problem hiding this comment.
Preserve the loop remainder on wraparound.
Line 62 ignores the time carried by LoopEndReached and always snaps back to m_loopStart. If a processing block crosses the loop end by a partial chunk, that overshoot is dropped instead of wrapped, so every loop becomes shorter by up to one block. Store the loop end in SequencePlayer as well, or have Clock dispatch the wrapped target directly, then seek to m_loopStart + (time - m_loopEnd).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/engine/internal/sequenceplayer.cpp` around lines 61 - 63,
The current LoopEndReached handler always seeks to m_loopStart, dropping any
overshoot; change it to preserve the remainder by seeking to m_loopStart + (time
- m_loopEnd). To implement this, ensure SequencePlayer has access to the loop
end/time (add m_loopEnd if not present) or have Clock include the wrapped target
time in the LoopEndReached action; then in the case for
IClock::ActionType::LoopEndReached compute wrapped = m_loopStart + (action.time
- m_loopEnd) and call m_clock->seek(wrapped) instead of
m_clock->seek(m_loopStart), keeping all other behavior the same.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on: #32598