Skip to content

Encapsulate transport state in TransportState struct#961

Draft
sbooth wants to merge 22 commits into
mainfrom
transportstate
Draft

Encapsulate transport state in TransportState struct#961
sbooth wants to merge 22 commits into
mainfrom
transportstate

Conversation

@sbooth

@sbooth sbooth commented Jul 26, 2026

Copy link
Copy Markdown
Owner

This attempts to minimize the use of locks on UI-facing methods.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a detail::TransportSnapshot to provide a consistent, UI-friendly view of transport/playback state without taking activeDecodersMutex_, aiming to reduce lock contention on UI-facing methods.

Changes:

  • Adds detail::TransportSnapshot and routes playback position/time queries through a snapshot loaded via a seqlock-like mechanism.
  • Refactors seeking to use event-driven seekRequest / seekComplete commands and decouples seek requests from direct decoder-state mutation.
  • Publishes transport snapshots at key points in decoder lifecycle and render-event processing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Sources/CSFBAudioEngine/Player/AudioPlayer.mm Implements snapshot publish/load logic and refactors seeking/event processing to rely on snapshots and new seek events.
Sources/CSFBAudioEngine/Player/AudioPlayer.h Introduces detail::TransportSnapshot, adds snapshot storage fields, and inlines playback position/time methods to read snapshots.
Comments suppressed due to low confidence (3)

Sources/CSFBAudioEngine/Player/AudioPlayer.mm:2305

  • loadTransportSnapshot() relies on copying currentSnapshot_ non-atomically and then uses an acquire fence followed by a relaxed sequence reload. This doesn't provide the usual seqlock acquire semantics and (combined with the plain struct copy) can allow torn/stale reads. Switching the snapshot copy to std::atomic_ref and making the second sequence load memory_order_acquire gives the intended synchronization with the writer’s final memory_order_release store.
        const auto seq = snapshotSequence_.load(std::memory_order_acquire);

        // If seq is odd, a writer is currently updating the snapshot
        if ((seq & 1) != 0) [[unlikely]] {
#if DEBUG
            os_log_debug(log_, "Unable to load playback snapshot: write in progress");
#endif /* DEBUG */
            cpuPause();

Sources/CSFBAudioEngine/Player/AudioPlayer.mm:1203

  • This log message still says "decoder seek event" even though the enqueued command is now EventCommand::seekComplete. Updating the wording will make fault reports easier to interpret.

This issue also appears on line 1256 of the same file.

            if (events_.enqueue(EventCommand::seekComplete, decoderState->sequenceNumber_, framePosition.value())) {
                eventSemaphore_.signal();
            } else {
                os_log_fault(log_, "Error writing decoder seek event");
            }

Sources/CSFBAudioEngine/Player/AudioPlayer.mm:1261

  • This log message still says "decoder seek event" even though the enqueued command is now EventCommand::seekComplete. Updating the wording will make fault reports easier to interpret.
                                if (events_.enqueue(EventCommand::seekComplete, nextDecoderState->sequenceNumber_,
                                                    framePosition.value())) {
                                    eventSemaphore_.signal();
                                } else {
                                    os_log_fault(log_, "Error writing decoder seek event");
                                }

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

Comment thread Sources/CSFBAudioEngine/Player/AudioPlayer.mm
Comment thread Sources/CSFBAudioEngine/Player/AudioPlayer.mm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment on lines +516 to +518
#if DEBUG
assert(position != nullptr || time != nullptr);
#endif /* DEBUG */
Comment on lines +1819 to +1823
} else {
os_log_error(log_, "Decoder state with sequence number %llu missing for seek request event",
sequenceNumber);
return false;
}
Comment on lines +2313 to +2315
#if DEBUG
os_log_debug(log_, "Unable to load playback snapshot: write in progress");
#endif /* DEBUG */
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants