Repository navigation
Make first-use allocations fail gracefully and admit only the requested visualizer stream - #255
Merged
Merged
Conversation
…ed visualizer stream Three first-use allocations still went through operator new, which aborts on ESPHome: the sync task's decode buffer object, the FLAC decoder object, and the visualizer's spectrum bins. The sync task now holds its TransferBuffer by value and allocates only its storage from the platform heap on the first codec header; the FLAC decoder is placed in an internal-RAM-preferring PlatformBuffer like the Opus state, so a failed allocation fails the header instead of aborting; and the visualizer reserves its spectrum bins at start(), only when the config requests spectrum. For Sendspin's validated FLAC header micro-flac itself makes no operator new allocation. The visualizer admits only what roles/visualizer/v1.md "stream/start" allows: served types outside the request are dropped, and spectrum is dropped unless its object matches the request (fewer bins are allowed, the spec's upper bound on n_disp_bins). on_visualizer_stream_start() reports the admitted stream, and one warning per stream/start names what was dropped.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new listener contract lacks callback-level coverage for BEAT admission with tracks_downbeats = true, while other assertions rely on private state and public header documentation remains outdated.
3 open issues
What changed in this PR
Reduces aborting first-use allocations in the player and visualizer paths and tightens visualizer admission to match the client’s request.
Changes:
- Move decode-buffer and FLAC-decoder storage to failure-reporting platform allocations.
- Reserve spectrum bins at startup and drop unrequested types or mismatched spectrum configurations.
- Add allocation and admission tests, with corresponding documentation updates.
| File | Description |
|---|---|
| tests/test_visualizer_role.cpp | Add admission-table and spectrum-reservation tests. |
| tests/test_player_role.cpp | Adapt sync test to the by-value decode buffer. |
| tests/test_decoder.cpp | Update buffer setup and decoder-reuse assertions. |
| tests/test_client_lifecycle.cpp | Clarify the spectrum bin-count limit in a comment. |
| tests/test_allocations.cpp | Measure first FLAC-header and spectrum-frame allocations. |
| tests/README.md | Describe expanded test coverage. |
| tests/CMakeLists.txt | Update the private-access justification comment. |
| src/visualizer_role.cpp | Reserve bins and filter served visualizer configurations. |
| src/visualizer_role_impl.h | Hold reusable spectrum bins in drain-task state. |
| src/transfer_buffer.h | Expose allocation methods for by-value ownership. |
| src/transfer_buffer.cpp | Implement explicit allocation and release. |
| src/sync_task.h | Store the decode buffer by value. |
| src/sync_task.cpp | Adapt decoding and cleanup to by-value storage. |
| src/decoder.h | Replace FLAC pointer ownership with platform storage. |
| src/decoder.cpp | Placement-construct and destroy the FLAC decoder. |
| include/sendspin/visualizer_role.h | Document the admitted-stream callback contract. |
| docs/internals.md | Update allocation and spectrum-reservation details. |
| docs/integration-guide.md | Explain visualizer filtering and spectrum limits. |
| CLAUDE.md | Update the allocation-test reference. |
Review effort: Max
…describe it in the headers
kahrendt
enabled auto-merge (squash)
November 5, 2026 19:05
theseal666
added a commit
to theseal666/sendspin-cpp
that referenced
this pull request
Nov 6, 2026
Sendspin#255 made SyncContext::decode_buffer a TransferBuffer rather than a unique_ptr to one. The merge above kept this branch's -> through, since the two changes do not touch the same lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.


Summary
Three first-use allocations still went through
operator new, which aborts on ESPHome: the sync task's decode buffer object, the FLAC decoder object and the visualizer's spectrum bins. Each now fails gracefully or happens atstart(), without reserving anything for consumers that never use it. Part of Sendspin/backlog#92.While there, the visualizer admits only what roles/visualizer/v1.md "stream/start" allows. Behaviour change: a served type the client did not request, and a spectrum whose object does not match the request, were delivered before and are now dropped.
Change
SyncContextholds itsTransferBufferby value (create()removed;allocate()/deallocate()public). Only its storage is allocated, from the platform heap on the first codec header; a failure drops the stream as before.PlatformBuffer(internal RAM preferred) like the Opus state, so a failed allocation fails the header instead of aborting. For Sendspin's validated header (fLaC+ a last STREAMINFO), micro-flac makes nooperator newallocation of its own: no Ogg demuxer, no stored metadata blocks; its working buffers useFLAC_MALLOC.start()to the configuredn_disp_bins, only when the config requests spectrum.on_spectrum()keeps its signature.n_disp_bins"; more bins or another scale or range drop it).on_visualizer_stream_start()reports the admitted stream (withtracks_downbeatscleared when beats are dropped), and one warning perstream/startnames the dropped types.Tests
Allocations.FirstSpectrumFrame, both 0. Mutants caught: the decoder or theTransferBufferback onoperator new, the spectrum vector back on the drain thread's stack.VisualizerStartReservationpins whatstart()reserves; a nine-row admission table checks both whether frames are admitted and what the consumer is told (mutants: no type mask, no bin cap, no scale or range check, no spectrum reset).Decoder.FlacStreamAfterAResetDecodesOnTheSameDecodercompares addresses, so a free-and-reallocate mutant landing on the same address is caught only by ASan.Stack (ESP-IDF 5.5.4, esp32): sync task 5,072 B -Os / 5,120 B -Og against its 6,192 B default; every other task matches
tools/stack_usage/README.md.