Move potentially blocking operations outside locks

[sbooth]

This PR delays decoder opening until necessary in the decoding thread and also moves the initialization of decoder state properties (which could potentially block) outside the locks.

[github-actions]

Cpp-linter Review

Used clang-format v22.1.6

Click here for the full clang-format patch

diff --git a/Sources/CSFBAudioEngine/Player/AudioPlayer.mm b/Sources/CSFBAudioEngine/Player/AudioPlayer.mm
index 156f152..722fca9 100644
--- a/Sources/CSFBAudioEngine/Player/AudioPlayer.mm
+++ b/Sources/CSFBAudioEngine/Player/AudioPlayer.mm
@@ -292,2 +292 @@ uint64_t AudioPlayer::DecoderState::sequenceCounter_ = 1;
-inline AudioPlayer::DecoderState::DecoderState(Decoder _Nonnull decoder) noexcept
-    : decoder_{decoder} {
+inline AudioPlayer::DecoderState::DecoderState(Decoder _Nonnull decoder) noexcept : decoder_{decoder} {
@@ -350,3 +349 @@ inline AVAudioFramePosition AudioPlayer::DecoderState::framePosition() const noe
-inline AVAudioFramePosition AudioPlayer::DecoderState::frameLength() const noexcept {
-    return frameLength_;
-}
+inline AVAudioFramePosition AudioPlayer::DecoderState::frameLength() const noexcept { return frameLength_; }
@@ -1330 +1327,2 @@ void sfb::AudioPlayer::processDecoders(std::stop_token stoken) noexcept {
-                if (NSError *error = nil; !decoderState->decoder_.isOpen && ![decoderState->decoder_ openReturningError:&error]) {
+                if (NSError *error = nil;
+                    !decoderState->decoder_.isOpen && ![decoderState->decoder_ openReturningError:&error]) {

Have any feedback or feature suggestions? Share it here.

outdated suggestion

[Copilot]

Pull request overview

This PR aims to reduce lock contention in AudioPlayer by deferring decoder opening and other potentially blocking decoder-state initialization work to the decoding thread, rather than performing it while holding queue/active decoder locks.

Changes:

• Defers openReturningError: from enqueueDecoder() to the decoding thread (processDecoders()).
• Moves initialization of decoder-derived state (sampleRate_, frameLength_) out of DecoderState construction and into DecoderState::allocate().
• Adjusts DecoderState storage for frame length/sample rate accordingly.

Comments suppressed due to low confidence (1)

Sources/CSFBAudioEngine/Player/AudioPlayer.mm:606

• enqueueDecoder(..., NSError **error) no longer attempts to open the decoder, so open/validation failures can’t be reported via the synchronous return value / error out-parameter anymore (they’re now deferred to the decoding thread). This is a behavioral/API change for callers like -playURL:error: that expect NO + NSError on failure. Consider either documenting this clearly and removing/repurposing the error parameter, or performing the potentially-blocking open outside queuedDecodersMutex_ before enqueueing so synchronous failure reporting remains accurate.

#endif /* DEBUG */

    // Ensure only one decoder can be enqueued at a time
    std::lock_guard lock{queuedDecodersMutex_};

    if (forImmediatePlayback) {
        queuedDecoders_.clear();
    }

    try {
        queuedDecoders_.push_back(decoder);
    } catch (const std::exception &e) {
        os_log_error(log_, "Error pushing %{public}@ to queuedDecoders_: %{public}s", decoder, e.what());

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sbooth]

This approach is not currently sound- once a decoder state is added to activeDecoders_ it can be used from multiple threads (normally returned via firstActiveDecoderState()) so setting properties after object construction is incorrect.