removed AudioQueue and added direct IO callback#26
Open
konradglas wants to merge 5 commits into
Open
Conversation
konradglas
force-pushed
the
direct-core-audio-callback
branch
10 times, most recently
from
aaf16e5 to
ffcd540
Compare
Contributor
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThis PR introduces a complete realtime audio task scheduling system alongside a new macOS CoreAudio direct driver. It adds type-erased callable wrappers (stdext::inplace_function and move-only variants), audio workgroup abstractions wrapping OS primitives, a lightweight semaphore with cross-platform implementations, a blocking concurrent queue, a realtime thread pool with worker coordination, and an audio task scheduler interface. The mixer is updated to use the new scheduler for multithreaded track processing. The AudioDriverController and AudioModule register the scheduler, and a new CoreAudioDirect driver option is available on macOS. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 20
♻️ Duplicate comments (1)
framework/audio/common/signpost.h (1)
22-25:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftVariable redefinition prevents multiple
MSS_SIGNPOST_FUNCTIONinvocations in the same scope.Same shadowing issue as
MSS_SIGNPOST_SCOPE. The macro declaresspidandgwithout unique naming.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audio/common/signpost.h` around lines 22 - 25, The macro MSS_SIGNPOST_FUNCTION defines non-unique identifiers (spid and g) causing redefinition/shadowing when used multiple times; change it to generate unique identifiers using token-pasting helpers (e.g., define MSS_CONCAT/ MSS_UNIQUE using __COUNTER__ or __LINE__) and replace spid and g with MSS_UNIQUE(spid) and MSS_UNIQUE(g) so the generated variables (the os_signpost_id from os_signpost_id_generate and the muse::Defer instance) are uniquely named; keep the calls to os_signpost_interval_begin/end and muse::Defer but bind them to the MSS_UNIQUE names to avoid collisions with MSS_SIGNPOST_SCOPE.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/audio/common/audiotaskscheduler.h`:
- Around line 25-26: The includes for sibling headers are inconsistent: replace
the local includes "common/audioworkgroup.h" and "common/realtimethreadpool.h"
with the local, sibling-style includes "audioworkgroup.h" and
"realtimethreadpool.h" in framework/audio/common/audiotaskscheduler.h so they
match the other includes ("iaudiodriver.h", "iaudiotaskscheduler.h",
"audiosanitizer.h") and the project's existing usage in realtimethreadpool.h and
audioworkgroup.cpp; update the two include lines accordingly to use the direct
header names.
In `@framework/audio/common/CMakeLists.txt`:
- Around line 36-39: The CMakeLists target_sources() in framework/audio/common
is missing two headers; add iaudiotaskscheduler.h and realtimethreadpool.h to
the list alongside audioworkgroup.h, audioworkgroup.cpp, audiotaskscheduler.h,
and inplace_function_mv.h so IDEs and dependency tracking pick them up; update
the target_sources entry in framework/audio/common/CMakeLists.txt to include
those two header filenames.
In `@framework/audio/common/realtimethreadpool.h`:
- Around line 23-25: Guard the RealtimeThreadPool constructor's int parameter
num_of_workers against negative values before converting to size_t: in
RealtimeThreadPool(std::string name, int num_of_workers ...) validate/clamp
num_of_workers (e.g., compute a size_t workers = num_of_workers > 0 ?
static_cast<size_t>(num_of_workers) : 0 or fallback to
std::thread::hardware_concurrency()) and then iterate using workers in the for
loop instead of directly casting num_of_workers; optionally log or throw if a
negative value was provided to surface the bad input.
- Around line 25-59: Reserve m_workers capacity before the loop and make thread
startup exception-safe by wrapping the push_back and muse::setThreadPriority
call in a try/catch; if an exception occurs, set this->m_should_stop = true,
unblock worker threads (e.g., signal m_queue/m_inflightSemaphore as needed),
join any already-started threads by iterating m_workers and joining each
worker->m_thread if joinable, then rethrow the exception; ensure you still
create Worker via std::make_unique<Worker>(), start the thread (assign to
worker->m_thread) and only push_back the worker into m_workers after the
operations that can throw are protected by the try/catch so no joinable
std::thread is left unjoined (refer to Worker, m_thread, m_workers,
muse::setThreadPriority, m_queue, m_inflightSemaphore, and m_should_stop).
In `@framework/audio/common/signpost.h`:
- Around line 17-20: The MSS_SIGNPOST_SCOPE macro declares non-unique local
variables (spid and g) causing redefinition errors when invoked multiple times
in the same scope; update the macro to generate unique identifiers via
token-pasting (e.g., add MSS_CONCAT_IMPL and MSS_CONCAT helpers and use
MSS_CONCAT(spid, __LINE__) / MSS_CONCAT(g, __LINE__) or __COUNTER__) and adjust
the muse::Defer lambda to capture the generated spid by value so the macro can
be used multiple times in one function without name collisions; keep the
os_signpost_id_generate, os_signpost_interval_begin, and
os_signpost_interval_end calls but reference the new unique variable names.
- Around line 1-27: Add an `#else` branch for the non-Apple case that provides
no-op fallback macro definitions so cross-platform builds compile: under the
existing `#ifdef` __APPLE__ ... `#endif` block add an `#else` branch and define
MSS_SIGNPOST_PREPARE, MSS_SIGNPOST_BEGIN(name), MSS_SIGNPOST_END(name),
MSS_SIGNPOST_SCOPE(name), and MSS_SIGNPOST_FUNCTION as empty macros (no-op),
mirroring the Apple names used in the file (MSS_SIGNPOST_PREPARE,
MSS_SIGNPOST_BEGIN, MSS_SIGNPOST_END, MSS_SIGNPOST_SCOPE,
MSS_SIGNPOST_FUNCTION); keep the final `#endif` to close the conditional.
- Around line 7-9: Update the comment above the MSS_SIGNPOST_PREPARE macro to
clarify where it should be invoked: replace "call this macro after you include
this header file" with wording like "Call this macro in your source file (at
function or file/namespace scope) to initialize the signpost log." Keep the
macro name MSS_SIGNPOST_PREPARE and the explanatory note adjacent to the macro
definition so callers know to invoke it in their .cpp/.m files after including
signpost.h.
- Around line 12-16: The macros MSS_SIGNPOST_BEGIN / MSS_SIGNPOST_END are
fragile because MSS_SIGNPOST_BEGIN defines a local auto spid and
MSS_SIGNPOST_END expects it still in scope; replace this pair with an RAII
helper to avoid scope issues: add a small class (e.g., SignpostGuard) that
stores an os_signpost_id_t spid, calls os_signpost_id_generate(sn_log) and
os_signpost_interval_begin(sn_log, spid, name) in its constructor and calls
os_signpost_interval_end(sn_log, spid, name) in its destructor, and then change
MSS_SIGNPOST_BEGIN to instantiate a uniquely named SignpostGuard (use
__COUNTER__ to form the variable name) so the interval is automatically ended
when the guard goes out of scope; ensure the implementation uses sn_log,
os_signpost_id_generate, os_signpost_interval_begin, and
os_signpost_interval_end and remove the MSS_SIGNPOST_END macro.
In `@framework/audio/driver/CMakeLists.txt`:
- Around line 90-92: Fix the inconsistent indentation by aligning the closing
parenthesis of the target_sources(...) block and the subsequent
set_source_files_properties(...) call with their corresponding opening
statements; adjust the extra leading whitespace so target_sources and
set_source_files_properties start at the same column as their matching opening
keywords to match the CMake formatting convention.
In `@framework/audio/driver/platform/osx/osxdirectaudiodriver.mm`:
- Around line 198-199: The call to AudioDeviceStop is incorrectly dereferencing
data->procId; procId is an opaque AudioDeviceIOProcID and must be passed
directly. Update the conditional handling around data->stopPending so that
AudioDeviceStop(data->deviceId, data->procId) is used (matching how
AudioDeviceStart and AudioDeviceDestroyIOProcID are invoked), ensuring you pass
the procId value itself rather than *data->procId.
- Around line 111-114: The destructor
OSXDirectAudioDriver::~OSXDirectAudioDriver currently only calls doClose() but
never removes the CoreAudio property listener registered by
initDeviceMapListener(), which leaves onDeviceListChanged() pointing at a
destroyed object; fix by unregistering the listener during teardown—either add a
removeDeviceMapListener() call from the destructor or ensure doClose() calls the
code that removes the CoreAudio property listener (the inverse of
initDeviceMapListener()) so the property listener is removed before the object
is destroyed.
- Around line 594-595: doClose() currently sets m_data->stopPending and
unconditionally calls AudioDeviceDestroyIOProcID and m_data->clear() after a
bounded spin, which can destroy shared state while coreAudioIOProc() is still
running; change doClose() so it waits reliably for m_data->stopped before
destroying the IO proc or clearing m_data (extend wait or return an error if
stop didn't complete, but do NOT call AudioDeviceDestroyIOProcID or
m_data->clear() unless m_data->stopped is true), and ensure you also clear and
publish the invalidation of m_audioWorkGroup (set m_audioWorkGroup to null/empty
and call m_currentWorkgroupChanged.notify()) so downstream scheduler can detect
the workgroup is gone; reference functions/fields: doClose(),
m_data->stopPending, m_data->stopped, coreAudioIOProc(),
AudioDeviceDestroyIOProcID, m_data->clear(), m_audioWorkGroup,
createAudioWorkgroup, m_currentWorkgroupChanged.notify().
- Around line 704-705: The allocated AudioBufferList is created with malloc but
wrapped in std::unique_ptr with the default delete deleter; change bufferList to
use a free-based deleter so the malloc'd memory is freed with free() rather than
delete. Locate the allocation/reinterpret_cast and creation of bufferList
(symbol: bufferList, type: AudioBufferList) used before calling
AudioObjectGetPropertyData and replace the unique_ptr instantiation with a
unique_ptr that uses free (e.g., a deleter function pointer or small lambda that
calls free) to correctly release the malloc'd memory.
- Around line 815-826: In OSXDirectAudioDriver::osxDeviceId() the inner "auto
deviceId = defaultDeviceId(&logError)" shadows the outer AudioDeviceID variable
so the resolved default is never used; change the inner variable name (e.g. auto
defaultId = defaultDeviceId(&logError)), check defaultId, assign *defaultId to
the existing m_data->format.deviceId / outer deviceId, and then return the
numeric AudioDeviceID (avoid QString::fromStdString conversion) so osxDeviceId()
returns the actual resolved device ID; use the functions/variables
DEFAULT_DEVICE_ID, defaultDeviceId(&logError), logError, m_data->format.deviceId
and the osxDeviceId() method to locate and apply the fix.
In `@framework/audio/engine/internal/mixer.cpp`:
- Around line 195-197: The queued lambdas in m_trackTasks capture the loop
variable by reference (e.g., [&t, processChannel] in the range-for over
m_tracks), causing dangling references when the loop iteration ends; change the
capture to store a stable pointer or address by value (e.g., capture trackPtr =
&t) so each lambda uses a stored TrackData* (or
std::reference_wrapper<TrackData>) and call processChannel(*trackPtr) inside the
lambda; update the lambda creation in the loop that builds m_trackTasks (the
same place that later calls
audioTaskScheduler()->submitRealtimeTasksAndWait(m_trackTasks)) to capture the
track address by value instead of &t.
In `@framework/audio/main/internal/audiodrivercontroller.h`:
- Around line 25-26: Remove the concrete include "common/audiotaskscheduler.h"
from audiodrivercontroller.h because the header only uses the
IAudioTaskSchedulerPtr type; instead add an explicit include for the concrete
class in audiodrivercontroller.cpp where you instantiate it with
std::make_shared<AudioTaskScheduler>(). Update references so the header only
forward-declares or includes the IAudiotaskScheduler interface and the .cpp
includes "common/audiotaskscheduler.h" to satisfy the AudioTaskScheduler symbol
used in the std::make_shared<AudioTaskScheduler>() call.
In `@framework/audio/thirdparty/moodycamel/blockingconcurrentqueue.h`:
- Around line 450-458: The blocking bulk dequeue overloads (e.g.,
wait_dequeue_bulk(It itemFirst, size_t max) and the other blocking bulk
overloads around the same area) must short-circuit when max == 0 instead of
calling LightweightSemaphore::waitMany(0); add an early check at the start of
each blocking wait_dequeue_bulk to return 0 immediately if max is zero,
preserving existing behavior of looping into inner.template try_dequeue_bulk and
avoiding the LightweightSemaphore debug assertion from waitMany(0).
In `@framework/audio/thirdparty/moodycamel/lightweightsemaphore.h`:
- Around line 8-12: The header lightweightsemaphore.h is not self-contained: it
uses std::uint64_t/std::int64_t, errno/EINTR, and MOODYCAMEL_DELETE_FUNCTION
without including the proper headers or defining the macro. Fix by adding
`#include` <cstdint> and `#include` <cerrno> to the top of lightweightsemaphore.h so
std::uint64_t/std::int64_t and errno/EINTR are available, and either include
"concurrentqueue.h" or locally define the MOODYCAMEL_DELETE_FUNCTION macro
(matching its definition in concurrentqueue.h) so symbols used in this header
compile when included standalone.
In `@framework/audio/thirdparty/sg14/inplace_function.h`:
- Around line 235-252: The constructor inplace_function(T&& closure) must
enforce that the stored callable type C is nothrow-move-constructible because
relocate_ptr and operations (move ctor, operator=, swap) assume noexcept moves;
add a static_assert(std::is_nothrow_move_constructible<C>::value,
"inplace_function requires nothrow move-constructible callable") alongside the
existing static_asserts in the inplace_function(T&&) constructor to prevent
constructing from types whose move constructor may throw (reference symbols:
inplace_function(T&& closure), relocate_ptr, swap, move constructor).
---
Duplicate comments:
In `@framework/audio/common/signpost.h`:
- Around line 22-25: The macro MSS_SIGNPOST_FUNCTION defines non-unique
identifiers (spid and g) causing redefinition/shadowing when used multiple
times; change it to generate unique identifiers using token-pasting helpers
(e.g., define MSS_CONCAT/ MSS_UNIQUE using __COUNTER__ or __LINE__) and replace
spid and g with MSS_UNIQUE(spid) and MSS_UNIQUE(g) so the generated variables
(the os_signpost_id from os_signpost_id_generate and the muse::Defer instance)
are uniquely named; keep the calls to os_signpost_interval_begin/end and
muse::Defer but bind them to the MSS_UNIQUE names to avoid collisions with
MSS_SIGNPOST_SCOPE.
🪄 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: a6d45d2f-42e3-4616-81d9-ba7ffcf9a421
📒 Files selected for processing (21)
framework/audio/common/CMakeLists.txtframework/audio/common/audiotaskscheduler.hframework/audio/common/audioworkgroup.cppframework/audio/common/audioworkgroup.hframework/audio/common/iaudiotaskscheduler.hframework/audio/common/inplace_function_mv.hframework/audio/common/realtimethreadpool.hframework/audio/common/signpost.hframework/audio/driver/CMakeLists.txtframework/audio/driver/platform/osx/osxdirectaudiodriver.hframework/audio/driver/platform/osx/osxdirectaudiodriver.mmframework/audio/engine/internal/mixer.cppframework/audio/engine/internal/mixer.hframework/audio/iaudiodriver.hframework/audio/main/audiomodule.cppframework/audio/main/internal/audiodrivercontroller.cppframework/audio/main/internal/audiodrivercontroller.hframework/audio/thirdparty/moodycamel/blockingconcurrentqueue.hframework/audio/thirdparty/moodycamel/concurrentqueue.hframework/audio/thirdparty/moodycamel/lightweightsemaphore.hframework/audio/thirdparty/sg14/inplace_function.h
Comment on lines
+145
to
+154
| tokenProvider.reset(); | ||
| AudioWorkgroupTokenProvider provider(m_workgroup); | ||
| if (!provider.isValid()) { | ||
| return false; | ||
| } | ||
|
|
||
| tokenProvider.join([provider = std::move(provider)]() { | ||
| return &provider; | ||
| }); | ||
| return true; |
There was a problem hiding this comment.
Keep the old token alive until the replacement join succeeds.
tokenProvider.reset() happens before the new AudioWorkgroupTokenProvider is known-good. If os_workgroup_join() fails here, the thread drops its existing membership and stays detached.
💡 Suggested change
- tokenProvider.reset();
AudioWorkgroupTokenProvider provider(m_workgroup);
if (!provider.isValid()) {
return false;
}
-
+ tokenProvider.reset();
tokenProvider.join([provider = std::move(provider)]() {
return &provider;
});📝 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
| tokenProvider.reset(); | |
| AudioWorkgroupTokenProvider provider(m_workgroup); | |
| if (!provider.isValid()) { | |
| return false; | |
| } | |
| tokenProvider.join([provider = std::move(provider)]() { | |
| return &provider; | |
| }); | |
| return true; | |
| AudioWorkgroupTokenProvider provider(m_workgroup); | |
| if (!provider.isValid()) { | |
| return false; | |
| } | |
| tokenProvider.reset(); | |
| tokenProvider.join([provider = std::move(provider)]() { | |
| return &provider; | |
| }); | |
| return true; |
Contributor
Author
There was a problem hiding this comment.
I think this is OK. If the workgroup is no longer the current one the thread has to leave it.
If no new workgroup exists (yet) so be it. It will be joined in the next call.
There was a problem hiding this comment.
@konradglas, that makes sense — leaving a stale workgroup eagerly is the right thing to do, and the transient detached state is acceptable since the next call will re-join. I'll defer to your design intent here.
🐇
| @@ -0,0 +1,27 @@ | |||
| #pragma once | |||
Contributor
There was a problem hiding this comment.
This header should be outside of the audio module (in global; perhaps we should add a new folder ("profiling") and move to it other tools for profiling)
|
|
||
| // call this macro after you include this header file | ||
| #define MSS_SIGNPOST_PREPARE \ | ||
| os_log_t sn_log = os_log_create("com.muse.MuseScoreStudio", OS_LOG_CATEGORY_POINTS_OF_INTEREST); |
Contributor
There was a problem hiding this comment.
It would be great to use MUSE_APP_NAME_MACHINE_READABLE here
| #include "common/audiotypes.h" | ||
| #include "common/audioworkgroup.h" | ||
| #include "common/signpost.h" | ||
| #include "thirdparty/kors_logger/src/log_base.h" |
Contributor
There was a problem hiding this comment.
log.h is already included below. Also, we should avoid these includes and include their wrappers from global
| #include <mutex> | ||
|
|
||
| namespace muse::audio { | ||
| class AudioTaskScheduler : public IAudioTaskScheduler, public kors::async::Asyncable |
Contributor
There was a problem hiding this comment.
muse::async::Asyncable
Contributor
There was a problem hiding this comment.
(same for other places where kors:: is used instead of muse::)
| */ | ||
| #pragma once | ||
|
|
||
| #include "audio/thirdparty/sg14/inplace_function.h" |
Contributor
There was a problem hiding this comment.
There are a number of thirdparty includes in .h files. Ideally, we should not include thirdparty headers in other headers to avoid transitive dependencies
Contributor
Author
There was a problem hiding this comment.
Yeah... In that case, since AudioWorkGroup is never retrieved in a realtime context, we could even use a normal pimpl instead.
| const size_t workerIndex = i; | ||
| worker->m_thread = std::make_unique<std::thread>([this, workerPtr, workerIndex, name] { | ||
| { | ||
| #if defined __linux__ |
Contributor
There was a problem hiding this comment.
It's better to use runtime::setThreadName here
| { | ||
| setWorkgroup(audioDriver->getAudioWorkGroup()); | ||
| audioDriver->currentWorkgroupChanged().onNotify( | ||
| this, [this, audioDriver]() { |
Contributor
There was a problem hiding this comment.
I'm pretty sure there is a bug here and audioDriver will never be destroyed due to a reference cycle (or at least AudioDriverController no longer controls the lifetime of the driver)
konradglas
force-pushed
the
direct-core-audio-callback
branch
2 times, most recently
from
2cb585e to
87c5cb1
Compare
- remove latency introduced by rebuffering
- moodycamel's mpmc queues and semaphores - SG14 inplace function
konradglas
force-pushed
the
direct-core-audio-callback
branch
from
87c5cb1 to
c77fbe6
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/audio/common/audiotaskscheduler.h`:
- Around line 47-55: The setAudioDriver(...) implementation dereferences the
nullable IAudioDriverPtr unconditionally; guard against a null driver by
checking if audioDriver is non-null at the top of setAudioDriver and handle the
null case by calling setWorkgroup({}) or an empty/reset workgroup, otherwise
proceed to setWorkgroup(audioDriver->getAudioWorkGroup()) and attach the
currentWorkgroupChanged() notifier using a weak_ptr (audioDriverWeak) as done
now; ensure the notifier lambda also handles the driver possibly being null
before calling getAudioWorkGroup().
In `@framework/audio/common/audioworkgroup.h`:
- Around line 47-55: AudioWorkgroupToken currently exposes the provider hook via
getProvider() and join(ErasedAudioWorkgroupTokenProvider), allowing external
code to install or observe the raw provider; move/hide that plumbing behind
AudioWorkGroup so tokens remain pure join handles. Remove or make private
AudioWorkgroupToken::getProvider() and AudioWorkgroupToken::join(), and instead
add corresponding methods on AudioWorkGroup (e.g.,
AudioWorkGroup::installProvider(...) and
AudioWorkGroup::getProviderForToken(...) or make AudioWorkGroup a friend that
calls a private setter on AudioWorkgroupToken) so only AudioWorkGroup can set or
access the ErasedAudioWorkgroupTokenProvider; update usages to call the new
AudioWorkGroup methods and ensure no external code references getProvider() or
join().
- Around line 41-42: The public move constructor/operator= on
AudioWorkgroupToken allows transferring a joined token across threads which can
cause os_workgroup_leave() to run on the wrong thread; either remove the move
operations (make AudioWorkgroupToken non-movable) or add thread-affinity
tracking and assertions in AudioWorkgroupToken so it records the joining thread
when AudioWorkgroupTokenProvider calls os_workgroup_join() and asserts (or
prevents) any move or destruction from a different thread before calling
os_workgroup_leave() in the destructor; update or delete the
AudioWorkgroupToken(AudioWorkgroupToken&&) noexcept and AudioWorkgroupToken&
operator=(AudioWorkgroupToken&&) noexcept definitions accordingly and add checks
around the destructor and any move logic to reference the recorded thread id.
In `@framework/audio/common/iaudiotaskscheduler.h`:
- Around line 25-27: This header declares/uses std::vector<Task> in the public
interface but does not include <vector>, relying on transitive includes; add
`#include` <vector> to framework/audio/common/iaudiotaskscheduler.h (alongside the
existing includes such as "global/modularity/imoduleinterface.h" and
"common/inplace_function.h") so the symbol std::vector is available for the
exposed type (Task) and callers that include this header directly won't break.
In `@framework/audio/common/lightweightsemaphore.h`:
- Around line 6-7: Replace the identity template alias LightweightSemaphore
(template<typename T = moodycamel::LightweightSemaphore> using
LightweightSemaphore = T;) with a concrete type alias to
moodycamel::LightweightSemaphore so callers can write LightweightSemaphore (no
<>), and to prevent invalid instantiations like LightweightSemaphore<int>;
update any usages such as in realtimethreadpool.h that currently use
LightweightSemaphore<> to the new concrete alias name.
In `@framework/audio/common/realtimethreadpool.h`:
- Around line 52-53: The call to task() followed by m_inflightSemaphore.signal()
can leak the semaphore if task() throws; create an RAII scope guard (e.g., a
small local struct or use C++ scope_exit) that calls
m_inflightSemaphore.signal() in its destructor, instantiate it immediately
before invoking task(), and remove the explicit m_inflightSemaphore.signal()
calls so the permit is always released even on exceptions; apply this change to
both occurrences around task() (the spots currently calling task();
m_inflightSemaphore.signal()) in realtimethreadpool.h.
- Around line 73-76: The enqueue(const Task& func) implementation currently
waits on m_inflightSemaphore then calls m_queue.enqueue(func) but ignores its
boolean result; if enqueue fails you must release the permit and surface the
failure to the caller. Change enqueue to check the return value of
m_queue.enqueue(func): on success return true (or void success), and on failure
call m_inflightSemaphore.notify()/release() (the correct method on the
semaphore) to restore the permit and then propagate the error (return false or
throw) to the caller so callers can handle dropped tasks; update the
enqueue(const Task& func) signature/return path accordingly.
- Around line 141-143: The BlockingConcurrentQueue m_queue is
default-constructed with its small internal capacity, causing allocations when
enqueueing beyond ~192 items; initialize m_queue with the advertised
maxTaskCount capacity so the queue is pre-sized and enqueue() remains
allocation-free. Modify the RealTimeThreadPool/constructor that owns m_queue to
construct muse::BlockingConcurrentQueue<Task> with capacity maxTaskCount (or set
it in the member initializer) so it matches the m_inflightSemaphore/maxTaskCount
expectation and prevents runtime allocations on the real-time submission thread.
In `@framework/audio/driver/platform/osx/osxdirectaudiodriver.h`:
- Around line 25-32: The header declares std::optional<int> (look for the
optional usage around the declaration of std::optional<int> in
osxdirectaudiodriver.h) but never includes <optional>; make the header
self-contained by adding `#include` <optional> at the top with the other standard
includes so the declaration of std::optional<int> compiles regardless of include
order.
In `@framework/global/signpost.h`:
- Around line 16-20: The macros MSS_SIGNPOST_BEGIN and MSS_SIGNPOST_END
currently declare a fixed local variable spid which prevents nesting; change
their signatures to accept a token identifier (e.g., MSS_SIGNPOST_BEGIN(name,
token) and MSS_SIGNPOST_END(name, token)) and use that token to create/use a
token-specific spid variable (e.g., spid_##token) so each begin/end pair
references its own generated os_signpost_id; update both macros to generate the
id via os_signpost_id_generate(sn_log) and call os_signpost_interval_begin/end
using the tokened spid variable so nested intervals in the same scope compile
and pair correctly.
🪄 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: f45c92a2-453a-4ab2-a2ef-2c5730d10ac1
📒 Files selected for processing (25)
framework/audio/common/CMakeLists.txtframework/audio/common/audiotaskscheduler.hframework/audio/common/audioworkgroup.cppframework/audio/common/audioworkgroup.hframework/audio/common/concurrentqueue.hframework/audio/common/iaudiotaskscheduler.hframework/audio/common/inplace_function.hframework/audio/common/inplace_function_mv.hframework/audio/common/lightweightsemaphore.hframework/audio/common/realtimethreadpool.hframework/audio/driver/CMakeLists.txtframework/audio/driver/platform/osx/osxdirectaudiodriver.hframework/audio/driver/platform/osx/osxdirectaudiodriver.mmframework/audio/engine/internal/mixer.cppframework/audio/engine/internal/mixer.hframework/audio/iaudiodriver.hframework/audio/main/audiomodule.cppframework/audio/main/internal/audiodrivercontroller.cppframework/audio/main/internal/audiodrivercontroller.hframework/audio/thirdparty/moodycamel/blockingconcurrentqueue.hframework/audio/thirdparty/moodycamel/concurrentqueue.hframework/audio/thirdparty/moodycamel/lightweightsemaphore.hframework/audio/thirdparty/sg14/inplace_function.hframework/global/CMakeLists.txtframework/global/signpost.h
Comment on lines
+47
to
+55
| void setAudioDriver(const IAudioDriverPtr& audioDriver) | ||
| { | ||
| setWorkgroup(audioDriver->getAudioWorkGroup()); | ||
| audioDriver->currentWorkgroupChanged().onNotify( | ||
| this, [this, audioDriverWeak = std::weak_ptr<IAudioDriver>(audioDriver)]() { | ||
| if (auto audioDriver = audioDriverWeak.lock()) { | ||
| setWorkgroup(audioDriver->getAudioWorkGroup()); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Guard setAudioDriver() against a null driver.
Line 49 dereferences audioDriver unconditionally. IAudioDriverPtr is nullable, so a failed driver switch or teardown path will crash here instead of resetting the scheduler to an empty workgroup.
Proposed fix
void setAudioDriver(const IAudioDriverPtr& audioDriver)
{
+ if (!audioDriver) {
+ setWorkgroup({});
+ return;
+ }
+
setWorkgroup(audioDriver->getAudioWorkGroup());
audioDriver->currentWorkgroupChanged().onNotify(
this, [this, audioDriverWeak = std::weak_ptr<IAudioDriver>(audioDriver)]() {
if (auto audioDriver = audioDriverWeak.lock()) {
setWorkgroup(audioDriver->getAudioWorkGroup());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/audio/common/audiotaskscheduler.h` around lines 47 - 55, The
setAudioDriver(...) implementation dereferences the nullable IAudioDriverPtr
unconditionally; guard against a null driver by checking if audioDriver is
non-null at the top of setAudioDriver and handle the null case by calling
setWorkgroup({}) or an empty/reset workgroup, otherwise proceed to
setWorkgroup(audioDriver->getAudioWorkGroup()) and attach the
currentWorkgroupChanged() notifier using a weak_ptr (audioDriverWeak) as done
now; ensure the notifier lambda also handles the driver possibly being null
before calling getAudioWorkGroup().
d9c4d06 to
269557c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
framework/audio/common/audiotaskscheduler.h (1)
50-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
setAudioDriver()against a null driver.Line 52 dereferences
audioDriverunconditionally. If a nullIAudioDriverPtris passed (e.g., during teardown), this will crash. Add a null check and reset the workgroup appropriately.Proposed fix
void setAudioDriver(const IAudioDriverPtr& audioDriver) { + if (!audioDriver) { + setWorkgroup({}); + return; + } + setWorkgroup(audioDriver->getAudioWorkGroup()); audioDriver->currentWorkgroupChanged().onNotify(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audio/common/audiotaskscheduler.h` around lines 50 - 58, The setAudioDriver method currently dereferences audioDriver unconditionally; modify setAudioDriver(IAudioDriverPtr const& audioDriver) to first check if audioDriver is null and, if so, clear/reset the workgroup via setWorkgroup(nullptr) (or the appropriate empty workgroup value) and unsubscribe any previous notifications; otherwise proceed to call setWorkgroup(audioDriver->getAudioWorkGroup()) and attach the currentWorkgroupChanged().onNotify handler using a weak_ptr (audioDriverWeak) as shown so the callback only executes when audioDriverWeak.lock() succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/audio/common/audiotaskscheduler.h`:
- Around line 70-78: The getIdealThreadCount(AudioWorkGroup workGroup)
implementation can return 0 when bestThreadHint is 1 because of integer division
by hardwareToRealtimeRatio; change the return logic in getIdealThreadCount to
ensure it always returns at least 1 (e.g., compute the candidate as
bestThreadHint / hardwareToRealtimeRatio and return std::max(1, candidate) or
similar), keeping the existing bestThreadHint determination (using
workGroup.getProvider() and getMaxParallelThreadCount()) and the
hardwareToRealtimeRatio heuristic.
- Around line 42-46: The fallback logic is inverted: change the condition so the
fallback runs only when enqueuing fails by calling IF_ASSERT_FAILED with
m_threadPool->enqueue(task) (not its negation); update the loop that iterates
tasks to call IF_ASSERT_FAILED(m_threadPool->enqueue(task)) { task(); } so that
tasks are executed in the thread pool on success and only run inline when
enqueue() on m_threadPool returns false, leaving the IF_ASSERT_FAILED macro,
m_threadPool, and enqueue(task) identifiers intact.
In `@framework/audio/common/realtimethreadpool.h`:
- Around line 67-69: The code calls task() without catching exceptions (both in
the path that uses InflightSemaphoreRelease and the other caller-thread path),
which can cause std::terminate if a task throws; wrap each invocation of task()
in a try { task(); } catch (...) { /* swallow or log error here without
rethrowing */ } so exceptions don't escape into thread code, and ensure the
InflightSemaphoreRelease and any cleanup still run (keep the
InflightSemaphoreRelease scope unchanged so the semaphore is released even if
task() throws). Reference the invocations that use InflightSemaphoreRelease and
the alternate call-site that directly calls task().
In `@framework/audio/driver/platform/osx/osxdirectaudiodriver.mm`:
- Around line 704-708: When leaking m_data in the early return path (the block
that does [[maybe_unused]] auto _leaked = m_data.release(); and recreates m_data
= std::make_unique<Data>()), also clear the current workgroup and emit the same
notification as the normal stop path: reset/clear m_audioWorkGroup and call the
m_currentWorkgroupChanged notifier (the same call sequence used when stopping
normally) so downstream users (e.g., AudioTaskScheduler) don't retain a stale
workgroup reference to the leaked device; mirror the cleanup performed in the
regular stop/cleanup branch around the Data lifecycle to ensure consistency.
In `@framework/audio/main/internal/audiodrivercontroller.cpp`:
- Around line 202-206: The call passes m_audioDriver (which may be null during
teardown) into audioWorkgroupSource->setAudioDriver causing a crash; either
update AudioTaskScheduler::setAudioDriver to handle a nullptr safely or add a
null guard here in setNewDriver: check that m_audioDriver is non-null before
calling audioWorkgroupSource->setAudioDriver(m_audioDriver), otherwise call a
safe-reset path (e.g., audioWorkgroupSource->setAudioDriver(nullptr) only if
setAudioDriver is updated) or skip the call—refer to m_audioDriver,
audioWorkgroupSource, setAudioDriver and setNewDriver when making the change.
In `@framework/global/functional/inplace_function_mv.h`:
- Around line 24-27: The header uses placement new in two places (the
placement-new calls around line 66 and line 127) but doesn't include <new>,
relying on transitive includes; make the header self-contained by adding an
explicit `#include` <new> at the top of
framework/global/functional/inplace_function_mv.h so placement new is properly
declared for the placement-new usages.
---
Duplicate comments:
In `@framework/audio/common/audiotaskscheduler.h`:
- Around line 50-58: The setAudioDriver method currently dereferences
audioDriver unconditionally; modify setAudioDriver(IAudioDriverPtr const&
audioDriver) to first check if audioDriver is null and, if so, clear/reset the
workgroup via setWorkgroup(nullptr) (or the appropriate empty workgroup value)
and unsubscribe any previous notifications; otherwise proceed to call
setWorkgroup(audioDriver->getAudioWorkGroup()) and attach the
currentWorkgroupChanged().onNotify handler using a weak_ptr (audioDriverWeak) as
shown so the callback only executes when audioDriverWeak.lock() succeeds.
🪄 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: 6b1c6a5c-be6e-4a13-8f96-752d0d0b82a3
📒 Files selected for processing (22)
framework/audio/common/CMakeLists.txtframework/audio/common/audiotaskscheduler.hframework/audio/common/audioworkgroup.cppframework/audio/common/audioworkgroup.hframework/audio/common/concurrentqueue.hframework/audio/common/iaudiotaskscheduler.hframework/audio/common/lightweightsemaphore.hframework/audio/common/realtimethreadpool.hframework/audio/driver/CMakeLists.txtframework/audio/driver/platform/osx/osxdirectaudiodriver.hframework/audio/driver/platform/osx/osxdirectaudiodriver.mmframework/audio/engine/internal/mixer.cppframework/audio/engine/internal/mixer.hframework/audio/iaudiodriver.hframework/audio/main/audiomodule.cppframework/audio/main/internal/audiodrivercontroller.cppframework/audio/main/internal/audiodrivercontroller.hframework/global/CMakeLists.txtframework/global/functional/inplace_function.hframework/global/functional/inplace_function_mv.hframework/global/signpost.hframework/global/thirdparty/sg14/inplace_function.h
💤 Files with no reviewable changes (1)
- framework/global/thirdparty/sg14/inplace_function.h
konradglas
force-pushed
the
direct-core-audio-callback
branch
from
269557c to
0e2b192
Compare
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
2 participants
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.
MacOS Only:
Core Audio callback
Notes/Assumptions:
isStoppedflag is set, the callback will not be called anymore and the used Data can be cleared.OSX Audio Work Groups
Realtime auxiliary threads running on Apple silicon hardware need to be synchronized using Audio Workgroups.
Otherwise they might be scheduled falsely on Efficiency-Cores and miss the hard deadline.
I introduced a new class
AudioWorkGroupthat holds the details in a type-erased function (so the class header isn't exposing macos specifics on Windows/Linux)Added a new class/interface
AudioTaskScheduleras a global inject.It is global because it has a hard restriction on the number of workers.
Currently it is set to half the performance cores. This might be too conservative. But the worst that can happen is one of those workers being scheduled but waiting for an available core.
The realtime thread itself also participates in task execution.
If we find that this is ever the bottleneck (which I doubt) we can also make the
RealtimeThreadPoolimplement job stealing and dedicated worker task lists. But for now this should suffice.