Fix #32568: Sound glitches with Loop playback enabled

[RomanPudashkin]

Resolves: #32568
Ports: #32573

[coderabbitai]

📝 Walkthrough

Walkthrough

IClock's event-specific notification APIs (seekOccurred(), statusChanged(), countDownEnded()) were removed and replaced with a unified action callback API: an ActionType enum and setOnAction(OnActionFunc) to receive events with timestamps. Clock implementation now stores PlaybackStatus directly, emits actions via a stored callback, and no longer exposes notification/channel accessors. SequencePlayer now creates and owns an internal Clock (constructor no longer accepts an external IClockPtr) and handles clock events via the unified callback. TrackSequence no longer holds a clock member.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The PR description provides issue resolution links but lacks detail about changes, motivation, and missing checklist completion. The template requires more comprehensive documentation.	Complete the description template by explaining the technical approach (unified action callback system, clock ownership refactoring) and checking off applicable checklist items.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing sound glitches with loop playback by refactoring the audio event system from callback-based notifications to a unified action handler.
Linked Issues check	✅ Passed	The changes directly address the root cause of the sound glitches by refactoring the event notification system and fixing loop boundary behavior to emit LoopEndReached instead of seeking, which prevents duplicate seeks and dropped notes.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the loop playback issue by refactoring the audio clock event system and loop handling logic. No unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/iclock.h`:
- Around line 69-70: The header declares OnActionFunc using std::function but
doesn't include <functional>, risking breakage; open iclock.h and add an
`#include` for <functional> near the other includes so the declaration of
OnActionFunc and the pure virtual setOnAction method reliably compile regardless
of include order.

In `@src/framework/audio/engine/internal/sequenceplayer.cpp`:
- Around line 164-166: The SequencePlayer currently assigns m_loopStart before
calling m_clock->setTimeLoop(...), which can leave SequencePlayer and Clock out
of sync if Clock::setTimeLoop rejects the range; change the order in the
setLoop/setTimeLoop call: first call m_clock->setTimeLoop(fromMsec * 1000,
toMsec * 1000) and check its success/return value, and only on success update
m_loopStart (and any other loop boundary members) so SequencePlayer mirrors the
Clock exactly; reference symbols: m_loopStart, m_clock->setTimeLoop(...),
SequencePlayer, Clock::setTimeLoop.

In `@src/framework/audio/engine/internal/sequenceplayer.h`:
- Around line 73-79: m_playbackStatusChanged is default-constructed but is sent
from a realtime clock callback; change its construction to disable waiting on
subscribers to avoid blocking in realtime (same pattern used for
Clock::m_timeChangedInSecs). In sequenceplayer.cpp, initialize the member
async::Channel m_playbackStatusChanged with options that call
disableWaitPendingsOnSend() (or use the Channel factory/constructor overload
that sets that option) so sends from the clock action callback cannot wait on
subscribers; update the constructor/initializer of the SequencePlayer class
where m_playbackStatusChanged is created accordingly.

🪄 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: cb625103-dc11-41c5-863b-a317060f8fe3

📥 Commits

Reviewing files that changed from the base of the PR and between 3923aab and 9673833.

📒 Files selected for processing (7)

• src/framework/audio/engine/iclock.h
• src/framework/audio/engine/internal/clock.cpp
• src/framework/audio/engine/internal/clock.h
• src/framework/audio/engine/internal/sequenceplayer.cpp
• src/framework/audio/engine/internal/sequenceplayer.h
• src/framework/audio/engine/internal/tracksequence.cpp
• src/framework/audio/engine/internal/tracksequence.h

💤 Files with no reviewable changes (1)

• src/framework/audio/engine/internal/tracksequence.h

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/framework/audio/engine/internal/sequenceplayer.cpp (1)

160-170: ⚠️ Potential issue | 🔴 Critical

Bug: setTimeLoop called with wrong start boundary.

Line 164 passes m_loopStart (the old/initial value, which is 0 on first call) instead of fromMsec * 1000 to m_clock->setTimeLoop(). This causes the clock's internal loop boundaries to be incorrect until after the first successful call, and even then the loop start in the clock won't match the requested fromMsec.

Additionally, as noted in a prior review, m_loopStart should only be updated after setTimeLoop succeeds.

🐛 Proposed fix

 Ret SequencePlayer::setLoop(const msecs_t fromMsec, const msecs_t toMsec)
 {
     ONLY_AUDIO_ENGINE_THREAD;

-    Ret ret = m_clock->setTimeLoop(m_loopStart, toMsec * 1000);
+    msecs_t loopStartMsecs = fromMsec * 1000;
+    Ret ret = m_clock->setTimeLoop(loopStartMsecs, toMsec * 1000);
     if (ret) {
-        m_loopStart = fromMsec * 1000;
+        m_loopStart = loopStartMsecs;
     }

     return ret;
 }

🤖 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 160 -
170, In SequencePlayer::setLoop, setTimeLoop is called with the old m_loopStart
instead of the requested new start; change the call to
m_clock->setTimeLoop(fromMsec * 1000, toMsec * 1000) and only assign m_loopStart
= fromMsec * 1000 after setTimeLoop returns success (Ret indicates success),
ensuring the clock receives the correct start boundary and m_loopStart is
updated only on success.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/framework/audio/engine/internal/sequenceplayer.cpp`:
- Around line 160-170: In SequencePlayer::setLoop, setTimeLoop is called with
the old m_loopStart instead of the requested new start; change the call to
m_clock->setTimeLoop(fromMsec * 1000, toMsec * 1000) and only assign m_loopStart
= fromMsec * 1000 after setTimeLoop returns success (Ret indicates success),
ensuring the clock receives the correct start boundary and m_loopStart is
updated only on success.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8efc18bd-d97a-4c83-96d9-066022c67a46

📥 Commits

Reviewing files that changed from the base of the PR and between 9673833 and 0427b6b.

📒 Files selected for processing (7)

• src/framework/audio/engine/iclock.h
• src/framework/audio/engine/internal/clock.cpp
• src/framework/audio/engine/internal/clock.h
• src/framework/audio/engine/internal/sequenceplayer.cpp
• src/framework/audio/engine/internal/sequenceplayer.h
• src/framework/audio/engine/internal/tracksequence.cpp
• src/framework/audio/engine/internal/tracksequence.h

💤 Files with no reviewable changes (1)

• src/framework/audio/engine/internal/tracksequence.h

[DmitryArefiev]

Tested on Win10. There is a regression with metronome in loop mode:

bandicam.2026-04-14.00-21-34-357.mp4