Conversation
With a storage size that was not a multiple of the 8-byte item alignment, the wrap-around dummy filler stored its raw tail size while the reader skipped the aligned size, desynchronizing the free-byte accounting (data corruption); a tail smaller than ItemHeader made try_acquire fail forever even on an empty buffer (permanent stall). Round the usable size down to the alignment in create() so unaligned tails can never exist, and reject storage too small for a single item. The default audio_buffer_capacity happened to be a multiple of 8, so default configs were unaffected; the ESP FreeRTOS-backed build was never affected. (cherry picked from commit 35a3fe8b5e5ce249c473deafd12c67f26d5c33c8) (cherry picked from commit 45c484b767382925e3d4f064b8c7c6d9b7218ee7)
A player role with empty audio_formats or no listener constructs a SyncTask but never calls init(), so its event flags are never created. handle_stream_end/clear, cleanup(), and drain_events() still signal and query the task unconditionally; on ESP those paths call xEventGroupSetBits/GetBits with a null handle and crash on the first disconnect or stream/end. Guard all signal/query entry points on is_initialized(). Also remove CHUNK_TYPE_DECODED_AUDIO: nothing ever produced it, and decode_chunk() fell through both the header and decode branches for it, silently discarding the chunk while reporting SUCCESS. (cherry picked from commit eb1307a37d7034f778450d5057948bed5e834c3c) (cherry picked from commit cc61d8c4ec6757dd5cfb9cb1826b3a0629aeb089)
- Defer the playback high-performance acquire from the network thread's handle_stream_start to the main loop's STREAM_START drain, making high_performance_requested_for_playback main-thread-only and making the documented listener threading contract true for on_request_high_performance. - Replace the load-then-fetch_sub in release_high_performance with a compare-exchange loop so two concurrent releases at count 1 cannot underflow the counter. - Track stream_active across drained STREAM_START/STREAM_END events so on_stream_end() fires exactly once per on_stream_start(): cleanup() after a stream already ended (and a failed stream start) no longer produce spurious/duplicate on_stream_end callbacks. - Invalidate the pre-loop sync_idle snapshot after processing a STREAM_START so a STREAM_END later in the same drain batch waits for the just-started sync task instead of firing immediately. - Log when the stream/state/time event queues overflow instead of dropping events silently (a lost STREAM_START parks the sync task forever). (cherry picked from commit de24d77ac801b9913b273b9853ff7d3b6a8e1e35) (cherry picked from commit c735c5adeb73946714671d12b1fa11a13ea56299)
- get_volume() documented 0-255; the wire range is 0-100. - The controller usage example sent VOLUME 128, outside the protocol's 0-100 range (the reference server rejects it). - Document that on_audio_write() partial writes must return whole-frame byte counts: the sync task derives the playtime estimate from the returned count, so a mid-frame value drifts sync and misaligns the next write. (cherry picked from commit dd086d4d2ce40f6f386ec8761038279fed71e1e1) (cherry picked from commit 925c82fa188508736fea724aee98b6b3a635ad30)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses correctness and threading/lifecycle issues across the audio playback pipeline, including SPSC ring-buffer wrap accounting, high-performance networking refcounting, and stream start/end callback pairing.
Changes:
- Fix SPSC ring buffer usable-size alignment (round down to 8-byte multiples) and add regression tests covering wrap-around across off-by-N storage sizes.
- Eliminate a potential
release_high_performance()refcount underflow via a compare-exchange loop, and move high-performance acquisition into the main-loop stream-start drain. - Add SyncTask initialization guards to prevent null-handle crashes on ESP; improve queue-full logging; remove dead
CHUNK_TYPE_DECODED_AUDIO; and update public docs for volume and PCM frame write requirements.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_spsc_ring_buffer.cpp | Adds fill/drain regression tests targeting wrap-around accounting and minimum storage handling. |
| tests/CMakeLists.txt | Registers the new SPSC ring buffer test in the test binary. |
| src/sync_task.h | Guards is_running() for pre-init calls to avoid ESP event-flag crashes. |
| src/sync_task.cpp | Adds init guards for stream signals and removes dead chunk-type checks in decode/drain logic. |
| src/player_role.cpp | Defers high-performance acquire to main loop, enforces 1:1 stream start/end callback pairing, and adds queue-full logging. |
| src/player_role_impl.h | Adds stream_active flag and declares enqueue_stream_event(). |
| src/platform/spsc_ring_buffer.h | Rounds down storage size to alignment and rejects too-small post-rounded buffers. |
| src/client.cpp | Fixes potential high-performance refcount underflow; adds time-queue-full logging. |
| src/audio_types.h | Removes CHUNK_TYPE_DECODED_AUDIO from internal chunk tags. |
| include/sendspin/player_role.h | Documents PCM-frame-aligned partial writes and corrects volume range to 0–100. |
| include/sendspin/controller_role.h | Updates volume example to the documented 0–100 range. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Fixes a set of correctness and threading issues in the audio player path, plus a host ring-buffer alignment bug.
storage_size_ desyncedthe writer/reader dummy-filler accounting at wrap points, causing data corruption or a permanent stall.create()now masks the usable size to an 8-byte multiple and rejects storage below one header + one aligned byte. Addstest_spsc_ring_buffer.cpp(fill/drain cycling across aligned and off-by-N sizes).release_high_performance()underflow. Replaced the load-then-fetch_subwith a compare-exchange loop so two concurrent releases at count 1 can't both pass the zero-check and underflow the counter.STREAM_STARTdrain, keeping the pairing flag and listener callbacks main-thread-only. Added astream_activeguard soon_stream_end()stays paired 1:1 withon_stream_start()(no spurious end fromcleanup()or a failed start), and invalidated the stalesync_idlesnapshot after a just-started stream so a same-batchSTREAM_ENDwaits for the task to drain.signal_stream_*andis_running()now early-return wheninit()never ran, preventing a null-handle crash from touching uncreated event flags on ESP.CHUNK_TYPE_DECODED_AUDIO, simplifying the decode-path chunk-type checks.on_audio_writepartial writes must be a whole number of PCM frames.