Clock: use secs for better accuracy#32967
Open
RomanPudashkin wants to merge 1 commit intomusescore:masterfrom
Open
Clock: use secs for better accuracy#32967RomanPudashkin wants to merge 1 commit intomusescore:masterfrom
RomanPudashkin wants to merge 1 commit intomusescore:masterfrom
Conversation
📝 WalkthroughWalkthroughThis pull request converts the audio framework's timing system from millisecond-based to second-based units. Type signatures across clock, player, engine, and mixer interfaces change 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
RomanPudashkin
force-pushed
the
clock_use_secs
branch
from
072419e to
c77ec18
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.h`:
- Line 54: The override declaration for setTimeLoop in the clock implementation
uses parameter names "from" and "to" which drift from the IClock::setTimeLoop
parameter names; update the override signature Ret setTimeLoop(const secs_t
from, const secs_t to) override to use the same parameter identifiers as the
interface (rename to fromSec and toSec) so the header aligns with
IClock::setTimeLoop, or alternatively change the interface to remove the "Sec"
suffix across both declarations—ensure the parameter names match exactly between
IClock::setTimeLoop and this override.
In `@src/framework/audio/engine/internal/engineplayer.cpp`:
- Around line 118-123: The duration() method inconsistently null-checks m_clock
while the EnginePlayer constructor and sibling methods (setDuration, setLoop,
resetLoop, seek, play, pause, stop, resume, playbackPosition,
playbackPositionChanged, playbackStatus, playbackStatusChanged) assume m_clock
is non-null; remove the redundant guard in EnginePlayer::duration() and directly
call m_clock->timeDuration() so behavior matches the constructor and other
methods (ensure ONLY_AUDIO_ENGINE_THREAD remains).
🪄 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: fe39d6e9-8f5a-4960-b238-ccfe5f3f3a2b
📒 Files selected for processing (18)
src/framework/audio/engine/iclock.hsrc/framework/audio/engine/iengineplayback.hsrc/framework/audio/engine/iengineplayer.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/engineplayer.cppsrc/framework/audio/engine/internal/engineplayer.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/main/internal/player.cppsrc/framework/audio/main/internal/player.hsrc/framework/audio/main/iplayer.h
| Ret setTimeLoop(const msecs_t fromMsec, const msecs_t toMsec) override; | ||
| secs_t timeDuration() const override; | ||
| void setTimeDuration(const secs_t duration) override; | ||
| Ret setTimeLoop(const secs_t from, const secs_t to) override; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: parameter-name drift from IClock.
IClock::setTimeLoop declares parameters as fromSec, toSec (see src/framework/audio/engine/iclock.h), while the override here uses from, to. Legal, but aligning parameter names across interface and implementation reduces confusion when reading headers/docs side-by-side. Either rename here or drop the Sec suffix in the interface (the latter is more consistent with the rest of this PR, which drops unit suffixes from identifiers).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/engine/internal/clock.h` at line 54, The override
declaration for setTimeLoop in the clock implementation uses parameter names
"from" and "to" which drift from the IClock::setTimeLoop parameter names; update
the override signature Ret setTimeLoop(const secs_t from, const secs_t to)
override to use the same parameter identifiers as the interface (rename to
fromSec and toSec) so the header aligns with IClock::setTimeLoop, or
alternatively change the interface to remove the "Sec" suffix across both
declarations—ensure the parameter names match exactly between
IClock::setTimeLoop and this override.
Comment on lines
+118
to
123
| secs_t EnginePlayer::duration() const | ||
| { | ||
| ONLY_AUDIO_ENGINE_THREAD; | ||
|
|
||
| if (!m_clock) { | ||
| return 0; | ||
| } | ||
|
|
||
| return m_clock->timeDuration(); | ||
| return m_clock ? m_clock->timeDuration() : secs_t { 0. }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Null-check in duration() is inconsistent with sibling methods.
The constructor unconditionally dereferences m_clock (lines 37-54), and every other method on EnginePlayer (setDuration, setLoop, resetLoop, seek, play, pause, stop, resume, playbackPosition, playbackPositionChanged, playbackStatus, playbackStatusChanged) dereferences m_clock without a guard. The class contract therefore already requires a non-null clock. Either drop the guard here (preferred, for consistency) or extend the same guard to the other methods if a null m_clock is genuinely reachable.
♻️ Proposed fix (drop the redundant guard)
secs_t EnginePlayer::duration() const
{
ONLY_AUDIO_ENGINE_THREAD;
- return m_clock ? m_clock->timeDuration() : secs_t { 0. };
+ return m_clock->timeDuration();
}📝 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
| secs_t EnginePlayer::duration() const | |
| { | |
| ONLY_AUDIO_ENGINE_THREAD; | |
| if (!m_clock) { | |
| return 0; | |
| } | |
| return m_clock->timeDuration(); | |
| return m_clock ? m_clock->timeDuration() : secs_t { 0. }; | |
| } | |
| secs_t EnginePlayer::duration() const | |
| { | |
| ONLY_AUDIO_ENGINE_THREAD; | |
| return m_clock->timeDuration(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/engine/internal/engineplayer.cpp` around lines 118 - 123,
The duration() method inconsistently null-checks m_clock while the EnginePlayer
constructor and sibling methods (setDuration, setLoop, resetLoop, seek, play,
pause, stop, resume, playbackPosition, playbackPositionChanged, playbackStatus,
playbackStatusChanged) assume m_clock is non-null; remove the redundant guard in
EnginePlayer::duration() and directly call m_clock->timeDuration() so behavior
matches the constructor and other methods (ensure ONLY_AUDIO_ENGINE_THREAD
remains).
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