renamed sequence player to engine player. Step 6#33045
renamed sequence player to engine player. Step 6#33045RomanPudashkin merged 2 commits intomusescore:masterfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughSequence-related public types and interfaces were removed or renamed. 🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/framework/audio/main/internal/playback.h (1)
22-23: 🧹 Nitpick | 🔵 TrivialNit: stale header guard name.
The header guard
MUSE_AUDIO_SEQUENCER_His a leftover from the sequencer-era naming. Consider renaming toMUSE_AUDIO_PLAYBACK_H(or switching to#pragma oncefor consistency with other headers in this PR likeiengineplayer.h) while you're cleaning up sequence terminology.Also applies to: 123-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/internal/playback.h` around lines 22 - 23, Header guard name MUSE_AUDIO_SEQUENCER_H is stale; change it to MUSE_AUDIO_PLAYBACK_H (or replace with `#pragma` once for consistency with iengineplayer.h). Update the opening macro (MUSE_AUDIO_SEQUENCER_H -> MUSE_AUDIO_PLAYBACK_H), the matching `#endif` comment (if any), and any other occurrences in playback.h (including the one noted around line 123) so the macro name is consistent throughout the file.src/framework/audio/main/internal/playback.cpp (1)
601-625:⚠️ Potential issue | 🔴 CriticalCritical:
initedis never set totrue— theGetSaveSoundTrackProgressRPC is re-sent on every call.The guard at line 604 is ineffective because nothing ever flips
inited. Each invocation ofsaveSoundTrackProgressChanged()will dispatch anotherMethod::GetSaveSoundTrackProgressmessage to the engine; the in-callbackm_saveSoundTrackProgressStreamId == 0check only prevents duplicate stream registrations — it does not avoid the redundant RPC round-trips (and theassert(m_saveSoundTrackProgressStreamId == streamId)on line 620 assumes the engine returns the same id every time, which is an implementation coupling worth avoiding).Additionally, note the lifecycle mismatch with
deinitPlayback()(line 139): it resetsm_saveSoundTrackProgressStreambut leavesm_saveSoundTrackProgressStreamIdstale. Onceinitedis properly flipped, a re-init cycle would never re-register the stream. Both pieces of state should be reset together.🐛 Proposed fix
SaveSoundTrackProgress Playback::saveSoundTrackProgressChanged() const { - static bool inited = false; - if (!inited) { + if (m_saveSoundTrackProgressStreamId == 0) { Msg msg = rpc::make_request(Method::GetSaveSoundTrackProgress); channel()->send(msg, [this](const Msg& res) { ONLY_AUDIO_MAIN_THREAD; StreamId streamId = 0; IF_ASSERT_FAILED(RpcPacker::unpack(res.data, streamId)) { return; } if (m_saveSoundTrackProgressStreamId == 0) { m_saveSoundTrackProgressStreamId = streamId; channel()->addReceiveStream(StreamName::SaveSoundTrackProgressStream, m_saveSoundTrackProgressStreamId, m_saveSoundTrackProgressStream); } assert(m_saveSoundTrackProgressStreamId == streamId); }); } return m_saveSoundTrackProgressStream; }And in
deinitPlayback():void Playback::deinitPlayback() { ONLY_AUDIO_MAIN_THREAD; + m_saveSoundTrackProgressStreamId = 0; m_saveSoundTrackProgressStream = SaveSoundTrackProgress(); }Using the
m_saveSoundTrackProgressStreamId == 0member as the guard (instead of a function-localstatic) also removes the cross-instance coupling that a function-localstaticintroduces if more than onePlaybackis ever constructed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/internal/playback.cpp` around lines 601 - 625, The function-local static "inited" is never flipped so the RPC in saveSoundTrackProgressChanged() is re-sent every call; replace that static guard with a member-based guard by checking m_saveSoundTrackProgressStreamId (send the rpc only when m_saveSoundTrackProgressStreamId == 0), and keep the existing callback logic to set m_saveSoundTrackProgressStreamId and register the stream via channel()->addReceiveStream(StreamName::SaveSoundTrackProgressStream, m_saveSoundTrackProgressStreamId, m_saveSoundTrackProgressStream); also update deinitPlayback() to reset both m_saveSoundTrackProgressStream and m_saveSoundTrackProgressStreamId back to 0 so re-initialization works correctly and removes the cross-instance coupling introduced by the function-local static.
🤖 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/main/internal/playback.cpp`:
- Around line 601-625: The function-local static "inited" is never flipped so
the RPC in saveSoundTrackProgressChanged() is re-sent every call; replace that
static guard with a member-based guard by checking
m_saveSoundTrackProgressStreamId (send the rpc only when
m_saveSoundTrackProgressStreamId == 0), and keep the existing callback logic to
set m_saveSoundTrackProgressStreamId and register the stream via
channel()->addReceiveStream(StreamName::SaveSoundTrackProgressStream,
m_saveSoundTrackProgressStreamId, m_saveSoundTrackProgressStream); also update
deinitPlayback() to reset both m_saveSoundTrackProgressStream and
m_saveSoundTrackProgressStreamId back to 0 so re-initialization works correctly
and removes the cross-instance coupling introduced by the function-local static.
In `@src/framework/audio/main/internal/playback.h`:
- Around line 22-23: Header guard name MUSE_AUDIO_SEQUENCER_H is stale; change
it to MUSE_AUDIO_PLAYBACK_H (or replace with `#pragma` once for consistency with
iengineplayer.h). Update the opening macro (MUSE_AUDIO_SEQUENCER_H ->
MUSE_AUDIO_PLAYBACK_H), the matching `#endif` comment (if any), and any other
occurrences in playback.h (including the one noted around line 123) so the macro
name is consistent throughout the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 792f7d53-f59f-45da-a307-1a08bc80a5a2
📒 Files selected for processing (13)
src/framework/audio/common/audiotypes.hsrc/framework/audio/engine/CMakeLists.txtsrc/framework/audio/engine/iengineplayer.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/enginerpccontroller.hsrc/framework/audio/engine/itracksequence.hsrc/framework/audio/main/internal/playback.cppsrc/framework/audio/main/internal/playback.hsrc/framework/audio/main/iplayback.h
💤 Files with no reviewable changes (2)
- src/framework/audio/common/audiotypes.h
- src/framework/audio/engine/itracksequence.h
igorkorsukov
force-pushed
the
w/audio/remove_seq_6
branch
from
80a6295 to
27803f2
Compare
There was a problem hiding this comment.
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/main/internal/playback.cpp (1)
601-627:⚠️ Potential issue | 🟠 Major
static bool initedshould be a per-instance member — breaks multi-context and re-init.Playback is a context-scoped service (one instance per window, per the class comment in
playback.h), butinitedis a function-localstatic, so it is shared process-wide across allPlaybackinstances. Only the first instance will sendGetSaveSoundTrackProgress; every other instance will skip the request and return its ownm_saveSoundTrackProgressStreamthat is never attached viaaddReceiveStream, so consumers in those contexts will silently receive no progress updates.The same flag also persists across
deinitPlayback()→ re-init cycles. SincedeinitPlayback()(Line 139) replacesm_saveSoundTrackProgressStreamwith a freshSaveSoundTrackProgress()but leavesinited == trueandm_saveSoundTrackProgressStreamId != 0, the next call to this method won't resubscribe and will hand out a disconnected channel. Note also thatm_saveSoundTrackProgressStreamIdis never reset indeinitPlayback(), which makes theif (m_saveSoundTrackProgressStreamId == 0)guard in the lambda incorrect on re-init.🛠️ Suggested fix
In
playback.h, add a member flag next tom_saveSoundTrackProgressStreamId:mutable bool m_saveSoundTrackProgressInited = false;In
playback.cpp:void Playback::deinitPlayback() { ONLY_AUDIO_MAIN_THREAD; m_saveSoundTrackProgressStream = SaveSoundTrackProgress(); + m_saveSoundTrackProgressStreamId = 0; + m_saveSoundTrackProgressInited = false; }SaveSoundTrackProgress Playback::saveSoundTrackProgressChanged() const { - static bool inited = false; - if (!inited) { + if (!m_saveSoundTrackProgressInited) { Msg msg = rpc::make_request(Method::GetSaveSoundTrackProgress); channel()->send(msg, [this](const Msg& res) { ... }); - inited = true; + m_saveSoundTrackProgressInited = true; } return m_saveSoundTrackProgressStream; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/internal/playback.cpp` around lines 601 - 627, Replace the function-local static inited with a per-instance mutable flag (e.g. add mutable bool m_saveSoundTrackProgressInited = false next to m_saveSoundTrackProgressStreamId in Playback) and use that member inside saveSoundTrackProgressChanged() instead of the static; also ensure deinitPlayback() resets m_saveSoundTrackProgressInited to false and resets m_saveSoundTrackProgressStreamId to 0 so re-init will re-subscribe, and keep the lambda logic that assigns m_saveSoundTrackProgressStreamId and calls channel()->addReceiveStream when the id is 0 (or when changed) so each Playback instance gets its own receive stream.
🤖 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/main/internal/playback.cpp`:
- Around line 601-627: Replace the function-local static inited with a
per-instance mutable flag (e.g. add mutable bool m_saveSoundTrackProgressInited
= false next to m_saveSoundTrackProgressStreamId in Playback) and use that
member inside saveSoundTrackProgressChanged() instead of the static; also ensure
deinitPlayback() resets m_saveSoundTrackProgressInited to false and resets
m_saveSoundTrackProgressStreamId to 0 so re-init will re-subscribe, and keep the
lambda logic that assigns m_saveSoundTrackProgressStreamId and calls
channel()->addReceiveStream when the id is 0 (or when changed) so each Playback
instance gets its own receive stream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59a987ff-2fea-461e-99cc-0bcbb31838cb
📒 Files selected for processing (7)
src/framework/audio/common/audiotypes.hsrc/framework/audio/engine/internal/engineplayback.hsrc/framework/audio/engine/internal/enginerpccontroller.cppsrc/framework/audio/engine/internal/enginerpccontroller.hsrc/framework/audio/main/internal/playback.cppsrc/framework/audio/main/internal/playback.hsrc/framework/audio/main/iplayback.h
💤 Files with no reviewable changes (1)
- src/framework/audio/common/audiotypes.h
igorkorsukov
force-pushed
the
w/audio/remove_seq_6
branch
from
27803f2 to
6915efc
Compare
2 participants
No description provided.