Add an Opus encode path to the source role - #116
Conversation
c00553e to
1ec53c8
Compare
# Conflicts: # include/sendspin/config.h
There was a problem hiding this comment.
🟡 Changes recommended
Valid 5 ms frames are rejected, defaults conflict with Opus validation, and encoding adds an avoidable persistent buffer and copy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Opus encoding to the source role with timestamp compensation, configuration validation, dependency gating, and round-trip tests.
Changes:
- Implements chunk-based Opus encoding and lookahead handling.
- Adds Opus source configuration and build integration.
- Expands validation, wire-level, and round-trip coverage.
File summaries
| File | Description |
|---|---|
tests/test_source_role.cpp |
Adds Opus validation, encoding, wire, and round-trip tests. |
src/source_task.cpp |
Integrates Opus encoding and packet sizing. |
src/source_role.cpp |
Validates Opus-specific configuration. |
src/source_encoder_opus.h |
Declares the Opus encoder. |
src/source_encoder_opus.cpp |
Implements Opus encoding and lookahead handling. |
src/opus_state_location.h |
Centralizes Opus state placement. |
src/decoder.cpp |
Reuses the shared Opus placement policy. |
include/sendspin/config.h |
Exposes Opus source configuration. |
idf_component.yml |
Enables micro-opus for source builds. |
CMakeLists.txt |
Links micro-opus when source support is enabled. |
cmake/sources.cmake |
Registers the Opus encoder source. |
cmake/host.cmake |
Updates host codec dependency gating. |
Review details
Suppressed comments (4)
idf_component.yml:15
- This changes dependency gating, but
docs/internals.md:17andCLAUDE.md:30still state that micro-opus is fetched/linked only for the player role and that the manifest gates it only onSENDSPIN_ENABLE_PLAYER. Update both descriptions to say micro-opus is required by player or source.
# Opus decode for the player role, Opus encode for the source role.
- if: "$CONFIG{SENDSPIN_ENABLE_PLAYER} == True || $CONFIG{SENDSPIN_ENABLE_SOURCE} == True"
include/sendspin/config.h:328
- The public documentation omits the valid 5 ms Opus frame duration, matching the validation bug rather than the codec and protocol contract. Include 5 ms in this list when correcting validation.
/// [CHUNK_MIN_MS, CHUNK_MAX_MS]. OPUS accepts only 10, 20, 40, or 60 (one
/// chunk is exactly one legal Opus frame), so the PCM default of 25 is rejected for OPUS
tests/test_source_role.cpp:375
- This bound assertion is tautological because
packet.size()is only 3840, so every successful encode is already<= 3840; it cannot validate the advertised 4000-byte packet capacity. Size the packet and assert usingMAX_PACKET_BYTESso the test actually protects the production constant instead of duplicating a dependent literal (docs/conventions.md:69-71).
// Packet bound: nonempty and inside the 4000-byte scratch (libopus's recommended
// maximum) at the default bitrate.
ASSERT_GT(written, 0U);
ASSERT_LE(written, 4000U);
tests/test_source_role.cpp:549
- This expectation locks in rejection of 5 ms even though 5 ms is a legal Opus frame and meets the source protocol's minimum. It should be an accepting boundary case once validation is corrected.
EXPECT_FALSE(advertises_source(*opus_with([](auto& c) { c.chunk_duration_ms = 5; })));
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// @brief Outbound chunk duration in milliseconds, validated against the spec bounds | ||
| /// [CHUNK_MIN_MS, CHUNK_MAX_MS] | ||
| /// [CHUNK_MIN_MS, CHUNK_MAX_MS]. OPUS accepts only 10, 20, 40, or 60 (one | ||
| /// chunk is exactly one legal Opus frame), so the PCM default of 25 is rejected for OPUS | ||
| uint32_t chunk_duration_ms{DEFAULT_CHUNK_MS}; |
There was a problem hiding this comment.
Fixed in 0b51d56 (part 3, where the default lives): the default chunk duration is now 20 ms — a legal Opus frame — so switching codec alone keeps a valid config; the config reference documents the property.
| // `in` sits behind the 9-byte wire header and is not int16-aligned, so copy to the aligned | ||
| // scratch; encoding into the packet scratch (never `out`) is what honors in == out | ||
| memcpy(this->pcm_scratch_.data(), in, in_len); | ||
| const opus_int32 written = | ||
| opus_encode(this->encoder_state_.as<OpusEncoder>(), this->pcm_scratch_.as<opus_int16>(), | ||
| static_cast<int>(in_len / this->bytes_per_frame_), this->packet_scratch_.data(), | ||
| static_cast<opus_int32>(MAX_PACKET_BYTES)); |
There was a problem hiding this comment.
Fixed in e853ac7: the packet encodes straight into the payload area (the aligned PCM copy already satisfies the aliasing contract), dropping the 4 KB scratch and the per-chunk copy. One deliberate semantic: libopus treats the capacity as a hard packet cap and degrades to fit rather than erroring, which the capacity test now pins; the task always offers MAX_PACKET_BYTES, so in-tree streams never degrade.
| // Single legal Opus frames within the spec's chunk bounds | ||
| static constexpr uint32_t OPUS_CHUNK_DURATIONS_MS[] = {10, 20, 40, 60}; |
There was a problem hiding this comment.
Fixed in e853ac7: 5 ms joins the accepted Opus durations (legal frame at the spec minimum), with the error text, config docs, and validation test flipped to an accepting boundary case.
| /// The configured format is the contract for every stream the role opens: there is no | ||
| /// negotiation, and write_audio() bytes are forwarded untouched. An invalid config leaves the | ||
| /// role added but inert (logged at ERROR; the role is not advertised and never streams) -- | ||
| /// spec-invalid values are rejected, never clamped or repaired. | ||
| /// negotiation, and write_audio() consumes PCM in exactly that format (sent untouched for the | ||
| /// PCM codec, encoded chunk-by-chunk for OPUS). An invalid config leaves the role added but | ||
| /// inert (logged at ERROR; the role is not advertised and never streams) -- spec-invalid values | ||
| /// are rejected, never clamped or repaired. |
There was a problem hiding this comment.
Fixed in e853ac7: write_audio's header doc scopes the untouched-bytes promise to PCM and names the Opus encode behavior; the integration guide's config table (part 6) already documents the codec and the bitrate/complexity fields.
Encode straight into the payload area now that the aligned PCM copy already satisfies the in == out contract, dropping the 4 KB packet scratch and its per-chunk copy; a small payload capacity acts as libopus's native hard cap (degrade to fit) and the capacity test pins that contract. 5 ms joins the accepted Opus chunk durations (a legal frame at the spec's minimum), the packet-bound test asserts against MAX_PACKET_BYTES itself, and write_audio's doc scopes the untouched- bytes promise to PCM.
Part 5/6 of the source@v1 stack (tracker: #95).
What it adds: an Opus encode path for the source role via micro-opus (already a dependency — its encoder is compiled in): one RFC 6716 packet per chunk, encoder lookahead reflected in the chunk timestamps, fail-closed validation of opus-legal rates/frame durations, and codec linking gated on player OR source.
How it's used: set
codec = OPUS(plus bitrate/complexity) inSourceRoleConfig; everything else is unchanged. Verified by encode→decode round-trip tests against the library's own decoder.Validation: the Opus path ran end-to-end against Music Assistant's
sendspin_sourceingest on a dev MA server (same interop shims as part 3): one RFC 6716 packet per chunk decoded by the server, timestamp continuity within tens of microseconds across the stream.