Make the Opus decoder a build option and require flac or pcm in player formats by marcelveldt · Pull Request #126 · Sendspin/sendspin-cpp

marcelveldt · 2026-09-15T22:20:33Z

Sendspin/spec#238 changed the codec rules: servers must support flac and pcm and may support opus; players must list flac or pcm and may add opus. A player is not told which codecs a server has, so one that lists only opus can end up with nothing to play. Opus is also patent-encumbered, and some products need a build without it.

Changes

PlayerRoleConfig::audio_formats must contain a flac or pcm entry. A non-empty list without one makes start() log an error and return false, and the client stays stopped (the same rollback path the visualizer uses for an invalid ring size). Rejecting instead of appending a pcm entry follows the fail-closed validation rule in docs/conventions.md and keeps the advertised list equal to what the consumer configured.
New SENDSPIN_ENABLE_OPUS build option (CMake option and Kconfig entry, on by default, so existing builds are unchanged). With it off, micro-opus is not fetched, required, or linked; the Opus branches in the decoder are compiled out; opus entries in audio_formats are refused at start(); and an opus stream/start takes the unsupported-codec path.
The host examples guard their opus usage on the option, and CI gains a job that builds and tests with Opus off (under ASan/UBSan, since one test only exists in that configuration).
Docs updated: README, integration guide, internals, conventions, CLAUDE.md.

Note for the ESPHome component

The ESPHome Sendspin component supplies the audio_formats list. It may want to put flac/pcm first and leave opus out when it builds with CONFIG_SENDSPIN_ENABLE_OPUS=n. That is a change in the esphome repository, not here.

The ESP-IDF side (Kconfig entry, requires list, manifest rule) was not compiled locally. The Kconfig entry was checked with Espressif's kconfiglib (defaults to on, reads as off when the player role is off) and the manifest rule with the component manager's own if parser on 2.2.0, 2.5.2 and 3.1.2 (true only with both options on; no error when the player role is off and the Opus symbol is absent).

Sendspin/spec#238 makes flac and pcm the only codecs a server must support. A player is not told which other codecs a server has, so one that lists only opus can end up with no playable format. The player role now refuses to start when its non-empty audio_formats list has neither, following the fail-closed validation posture in docs/conventions.md instead of appending a pcm entry the consumer did not configure. - PlayerRole::Impl::start() logs the rejected rule and returns false, so SendspinClient::start() rolls back and the client stays stopped - Documents the rule on PlayerRoleConfig::audio_formats and in the integration guide - Adds a lifecycle test with flac-only and pcm-only controls

Sendspin/spec#238 makes opus optional for servers and notes that it is covered by third-party patents, so some products need a player without it. SENDSPIN_ENABLE_OPUS (a CMake option on host, a Kconfig entry on ESP-IDF, on by default) now controls the decoder; existing builds are unchanged. - With it off, micro-opus is not fetched, required, or linked (host.cmake, CMakeLists.txt, idf_component.yml), the Opus branches in decoder.h/.cpp are compiled out, opus entries in audio_formats are refused at start(), and an opus stream/start takes the unsupported-codec path - The host examples guard their opus usage on the option - CI gains a job that builds the library, examples, and tests with the option off - Docs: README, integration guide, internals, conventions, CLAUDE.md, and the house-patterns skill

Copilot

🟡 Changes recommended

Add runtime no-Opus stream coverage and correct the documented build and codec conditions.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Makes Opus support optional and validates player codec configurations for FLAC/PCM compatibility.

Changes:

Adds CMake/Kconfig Opus build gates and conditional dependencies.
Validates advertised formats and unsupported Opus streams.
Updates decoder code, examples, documentation, and CI coverage.

File summaries

File	Reviewed changes
`tests/test_client_lifecycle.cpp`	Adds codec validation lifecycle tests.
`src/player_role.cpp`	Validates formats and gates Opus streams.
`src/decoder.h`	Conditionally exposes Opus support.
`src/decoder.cpp`	Conditionally compiles Opus decoding.
`README.md`	Documents optional Opus dependencies.
`Kconfig`	Adds the ESP-IDF Opus option.
`include/sendspin/config.h`	Documents format requirements.
`idf_component.yml`	Gates the micro-opus dependency.
`examples/tui_client/main.cpp`	Guards Opus configuration.
`examples/basic_client/main.cpp`	Guards Opus configuration.
`docs/internals.md`	Documents codec gating.
`docs/integration-guide.md`	Documents codec requirements and options.
`docs/conventions.md`	Updates compile-gate conventions.
`CMakeLists.txt`	Adds Opus configuration.
`cmake/host.cmake`	Conditionally fetches micro-opus.
`CLAUDE.md`	Updates dependency documentation.
`.github/workflows/ci.yml`	Adds no-Opus CI coverage.
`.claude/skills/house-patterns/SKILL.md`	Updates codec-gate review guidance.

Review details

Suppressed comments (6)

CLAUDE.md:80

This host-build description has the same missing condition as the README: cmake/host.cmake:64-83 skips micro-opus whenever the player role is disabled, regardless of the Opus option. Please state that micro-opus requires both SENDSPIN_ENABLE_PLAYER and SENDSPIN_ENABLE_OPUS so the developer documentation matches the build logic.

- **Host (CMake)**: `cmake -B build && cmake --build build`. Fetches dependencies (ArduinoJson, micro-flac, IXWebSocket, and micro-opus unless `SENDSPIN_ENABLE_OPUS=OFF`) via FetchContent.

docs/integration-guide.md:77

The main player example immediately above unconditionally adds an OPUS entry (lines 62-68). In the supported SENDSPIN_ENABLE_OPUS=OFF configuration, copying that example makes start() return false, despite this paragraph documenting that configuration. Keep the portable example to FLAC/PCM or show the Opus entry as conditional/removable guidance.

List at least one `FLAC` or `PCM` format. Those are the only codecs a server must support, and the server picks only among the formats it can produce, so a list with neither can leave the player with nothing to play; `start()` refuses it. `OPUS` is optional and needs a build with the Opus decoder (`SENDSPIN_ENABLE_OPUS`, on by default); `start()` refuses an `OPUS` entry otherwise.

docs/internals.md:22

With SENDSPIN_ENABLE_OPUS=OFF, the surrounding internals text is still unconditional: it says src/decoder.h pulls in <opus.h> and that LOAD_CHUNK decodes FLAC/Opus/PCM, while the implementation guards both the include and the Opus decode branch. Please update those statements so the no-Opus build is described accurately.

The Opus decoder is a third gate inside the player role. `SENDSPIN_ENABLE_OPUS` (a CMake option on host, a Kconfig entry that depends on `SENDSPIN_ENABLE_PLAYER` on ESP-IDF, `ON` by default) controls whether micro-opus is fetched, required, and linked, and defines `SENDSPIN_ENABLE_OPUS` for the library and its consumers. It cannot be a source-list exclusion because the decoder is one translation unit, so `src/decoder.h` and `src/decoder.cpp` guard the Opus include, state, and branches with `#ifdef SENDSPIN_ENABLE_OPUS`, and `src/player_role.cpp` derives one constexpr from it to refuse opus entries in `audio_formats` at start and to route an opus `stream/start` to the unsupported-codec path. No other file in the library references the macro.

docs/internals.md:22

The earlier conditional-compilation paragraph at line 17 still says the ESP-IDF manifest gates all audio codec dependencies solely on SENDSPIN_ENABLE_PLAYER, but this change makes micro-opus conditional on both player and SENDSPIN_ENABLE_OPUS (idf_component.yml:14-15, CMakeLists.txt:50-57). That leaves the overview contradictory; update the earlier sentence to distinguish micro-flac from micro-opus's second gate.

The Opus decoder is a third gate inside the player role. `SENDSPIN_ENABLE_OPUS` (a CMake option on host, a Kconfig entry that depends on `SENDSPIN_ENABLE_PLAYER` on ESP-IDF, `ON` by default) controls whether micro-opus is fetched, required, and linked, and defines `SENDSPIN_ENABLE_OPUS` for the library and its consumers. It cannot be a source-list exclusion because the decoder is one translation unit, so `src/decoder.h` and `src/decoder.cpp` guard the Opus include, state, and branches with `#ifdef SENDSPIN_ENABLE_OPUS`, and `src/player_role.cpp` derives one constexpr from it to refuse opus entries in `audio_formats` at start and to route an opus `stream/start` to the unsupported-codec path. No other file in the library references the macro.

include/sendspin/config.h:139

This field's new comment says every list must include FLAC or PCM, but the implementation intentionally permits an empty list to omit the player from client/hello (src/player_role.cpp:211-214). Describe the rule as applying to non-empty lists and document the empty-list behavior so the public contract matches start() and the existing default {}.

    /// @brief Audio formats the player can play, advertised to the server in preference order.
    /// Must include a flac or pcm entry: those are the only codecs every server supports, and a
    /// player is not told which others a server has (roles/player/v1.md). Opus may be listed in
    /// addition, but only in a build with the Opus decoder (SENDSPIN_ENABLE_OPUS, on by default).
    /// SendspinClient::start() refuses a non-empty list that breaks either rule.

tests/test_client_lifecycle.cpp:451

The no-Opus branch here only checks that start() rejects a configuration containing OPUS; it never sends a stream/start with codec:"opus" through PlayerRole::Impl::handle_stream_start(). A removal or inversion of the new OPUS_DECODER_ENABLED gate at src/player_role.cpp:306-307 would therefore still pass while an Opus stream is incorrectly accepted in this build. Add a no-Opus case that drives that message and asserts the unsupported-codec/end behavior.

#ifdef SENDSPIN_ENABLE_OPUS
    ASSERT_TRUE(client.start());
    client.stop();
#else
    EXPECT_FALSE(client.start());
    EXPECT_FALSE(client.is_started());

Files reviewed: 18/18 changed files
Comments generated: 2
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…docs - A loopback test in the opus-off build sends an opus stream/start to a pcm-only player and checks it takes the unsupported-codec path while the pcm stream/start behind it starts normally - README, CLAUDE.md, and the integration guide state that micro-opus needs both the player role and SENDSPIN_ENABLE_OPUS, the guide's example guards its opus entry, internals.md mentions the second gate where it lists the codec dependencies, and the audio_formats doc names the empty-list behaviour

Copilot

🔵 Needs a closer look

Unresolved moderate findings remain, including unsupported-format validation, test coverage, conditional example output, and ESP-IDF validation.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

examples/tui_client/main.cpp:355

This new no-Opus branch rejects opus, but the same example still advertises opus in print_usage() and in the unknown-codec message. A binary built with SENDSPIN_ENABLE_OPUS=OFF therefore tells users that a format it will reject is supported; make those user-facing lists conditional on the same build option.

docs/internals.md:22

This update leaves the state-machine description at docs/internals.md:362 saying that LOAD_CHUNK decodes via a FLAC/Opus/PCM decoder unconditionally. With SENDSPIN_ENABLE_OPUS=OFF, only FLAC and PCM are compiled into the decoder, so update that summary to describe Opus as conditional as well.

The Opus decoder is a third gate inside the player role. `SENDSPIN_ENABLE_OPUS` (a CMake option on host, a Kconfig entry that depends on `SENDSPIN_ENABLE_PLAYER` on ESP-IDF, `ON` by default) controls whether micro-opus is fetched, required, and linked, and defines `SENDSPIN_ENABLE_OPUS` for the library and its consumers. It cannot be a source-list exclusion because the decoder is one translation unit, so `src/decoder.h` and `src/decoder.cpp` guard the Opus include, state, and branches with `#ifdef SENDSPIN_ENABLE_OPUS`, and `src/player_role.cpp` derives one constexpr from it to refuse opus entries in `audio_formats` at start and to route an opus `stream/start` to the unsupported-codec path. No other file in the library references the macro.

docs/internals.md:24

Add the missing space after the semicolon in headers;e.g..

As a consequence, role-only headers;e.g., `src/decoder.h`, which pulls in `<micro_flac/flac_decoder.h>` and, with `SENDSPIN_ENABLE_OPUS`, `<opus.h>`, must only be reachable through role-only sources or through `#ifdef`-guarded includes in `client.cpp`. Public role headers in `include/sendspin/` must remain free of codec dependencies so that core files like `src/transfer_buffer.cpp` and `src/protocol_messages.h` can include them unconditionally.

idf_component.yml:15

This adds the ESP-IDF dependency/build gate, but .github/workflows/ci.yml only configures host CMake builds and the PR notes that the ESP-IDF side was not compiled locally. A malformed manifest rule, Kconfig interaction, or ESP compile-definition problem can therefore pass CI unnoticed; add an ESP-IDF configure/build check or require an explicit manual ESP validation before merging.

      - if: "$CONFIG{SENDSPIN_ENABLE_PLAYER} == True && $CONFIG{SENDSPIN_ENABLE_OPUS} == True"

src/player_role.cpp:84

The validation falls through for SendspinCodecFormat::UNSUPPORTED (and any future enum value), so {FLAC, UNSUPPORTED} is accepted. build_hello_fields() serializes that entry as codec "unsupported" (src/protocol.cpp:230-240,845-850), allowing the server to select a format this player cannot decode. Fail closed for every codec other than FLAC, PCM, and an enabled OPUS.

        } else if (format.codec == SendspinCodecFormat::OPUS) {
            has_opus = true;
        }

src/player_role.cpp:307

The new no-Opus loopback test does not distinguish this guard from the old behavior: if the condition still enqueues an Opus header, decoder.cpp's no-Opus switch rejects that header and the following PCM stream still produces the same single start callback. Add a white-box assertion or another observable that the Opus stream/start enqueues no codec header, while retaining the PCM control.

        if (codec == SendspinCodecFormat::PCM ||
            (OPUS_DECODER_ENABLED && codec == SendspinCodecFormat::OPUS)) {

Files reviewed: 18/18 changed files
Comments generated: 0 new
Review effort level: Lite

- The opus-off test port is gated like the test that uses it, so the default build compiles without an unused-constant warning - The opus-off CI job runs the suite under ASan/UBSan, since one player test only exists in that configuration - The opus stream/start test pumps until the pcm params are current, so the start count is decisive on its own; the refused player in the flac/pcm test has no listener, which pins that validation covers what the hello advertises - The tui example derives its codec list from the build option - CLAUDE.md, CONTRIBUTING.md, tests/README.md, the integration guide, internals.md, and the helper's spec citation

Copilot

🔵 Needs a closer look

Moderate findings remain in player-format validation, the TUI no-Opus guard, and the lifecycle test.

Review details

Suppressed comments (7)

CONTRIBUTING.md:31

This says it is the “third CI job,” but the workflow places test-no-opus after pre-commit, lint, build, and test (and before test-tsan). Please call it an additional CI job so this contributor guidance stays accurate.

A third CI job builds the library, the examples, and the tests with the Opus decoder off

docs/internals.md:22

The sentence "No other file in the library references the macro" is literally false: CMakeLists.txt and cmake/host.cmake use SENDSPIN_ENABLE_OPUS, while Kconfig and idf_component.yml use its configuration symbol. Scope this claim to C++ source files (or otherwise distinguish the build/configuration files) so the documented gate boundary is accurate.

The Opus decoder is a third gate inside the player role. `SENDSPIN_ENABLE_OPUS` (a CMake option on host, a Kconfig entry that depends on `SENDSPIN_ENABLE_PLAYER` on ESP-IDF, `ON` by default) controls whether micro-opus is fetched, required, and linked, and defines `SENDSPIN_ENABLE_OPUS` for the library and its consumers. It cannot be a source-list exclusion because the decoder is one translation unit, so `src/decoder.h` and `src/decoder.cpp` guard the Opus include, state, and branches with `#ifdef SENDSPIN_ENABLE_OPUS`, and `src/player_role.cpp` derives one constexpr from it to refuse opus entries in `audio_formats` at start and to route an opus `stream/start` to the unsupported-codec path. No other file in the library references the macro.

docs/internals.md:24

The separator before the example is still a semicolon (headers; e.g.); use a comma so this new documentation sentence is grammatically correct.

As a consequence, role-only headers; e.g., `src/decoder.h`, which pulls in `<micro_flac/flac_decoder.h>` and, with `SENDSPIN_ENABLE_OPUS`, `<opus.h>`, must only be reachable through role-only sources or through `#ifdef`-guarded includes in `client.cpp`. Public role headers in `include/sendspin/` must remain free of codec dependencies so that core files like `src/transfer_buffer.cpp` and `src/protocol_messages.h` can include them unconditionally.

examples/tui_client/main.cpp:503

This guard removes Opus from CODECS, but the same example still compiles the Opus-specific filter at line 514 when SENDSPIN_ENABLE_OPUS is off. The consumer-gating convention requires codec usage in examples to be guarded (docs/conventions.md:50-55); wrap this condition and its comment in the same #ifdef so no-Opus builds do not retain the dead Opus branch.

#ifdef SENDSPIN_ENABLE_OPUS
            SendspinCodecFormat::OPUS,
#endif

src/decoder.h:40

The final sentence is inaccurate for the supported Opus path: an Opus dummy header initializes the Opus decoder, and subsequent packets are decoded rather than copied directly. Keep the PCM behavior separate from the Opus dummy-header behavior so this API documentation matches process_header() and decode_audio_chunk().

 * otherwise. PCM and dummy headers bypass decoding and copy data directly.

src/player_role.cpp:84

This validation silently accepts SendspinCodecFormat::UNSUPPORTED whenever a valid FLAC/PCM entry is also present, and build_hello_fields() then serializes that entry as codec "unsupported" (src/protocol.cpp:845-848). That is not a playable format and can make the advertised list invalid; reject any codec other than FLAC, PCM, or (when enabled) OPUS instead of ignoring unknown enum values.

    for (const auto& format : formats) {
        if (format.codec == SendspinCodecFormat::FLAC || format.codec == SendspinCodecFormat::PCM) {
            has_baseline = true;
        } else if (format.codec == SendspinCodecFormat::OPUS) {
            has_opus = true;
        }

tests/test_client_lifecycle.cpp:477

The new loopback peer is constructed immediately after start(), but start() only schedules the WebSocket server; the first client.loop() binds it. Because FakeServer disables automatic reconnection and pump_until() runs only after construction, this can lose the initial connection and hang. Call client.loop() once before constructing FakeServer, as the connection lifecycle tests do.

    FakeServer server(server_url(OPUS_STREAM_TEST_PORT), "server-a");

Files reviewed: 20/20 changed files
Comments generated: 0 new
Review effort level: Lite

…doc wording - The opus stream/start test ticks loop() once before constructing its FakeServer, as the connection lifecycle tests do: IXWebSocket makes one connect attempt with reconnection disabled, so the listener has to exist first - decoder.h describes PCM chunks, not dummy headers, as the copied path - internals.md scopes the "no other file references the macro" claim to source files and names the build files that do, CONTRIBUTING.md stops calling the opus-off job the third one

Copilot

🟡 Changes recommended

The manifest dependency rule and validator testability need correction, with documentation scope inconsistencies also unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

CLAUDE.md:30

The updated repository guidance says the codec gate lives only in the three library implementation files, while tests/test_client_lifecycle.cpp:69,454,463 now intentionally uses the same macro for build-specific tests. Please qualify this as the implementation gate (or explicitly exempt test scaffolding), otherwise the documented convention conflicts with the new test code.

Roles can be disabled at compile time via `SENDSPIN_ENABLE_*` cmake options (host build) or Kconfig entries (ESP-IDF build). When a role is disabled, its source files are not compiled and its `add_*()` declaration, accessor, and `unique_ptr` member are removed from `client.h`. Role `#ifdef` guards live in exactly two places in the library: `cmake/sources.cmake` (source lists) and `include/sendspin/client.h` / `src/client.cpp` (dispatch points); the codec gate `SENDSPIN_ENABLE_OPUS` (cmake option / Kconfig entry, default on) lives only in `src/decoder.h`, `src/decoder.cpp`, and `src/player_role.cpp`. Examples guard their own role and opus usage like any consumer. Audio codec dependencies (micro-flac, micro-opus) are only linked when the player role is enabled, micro-opus also only with `SENDSPIN_ENABLE_OPUS`.

src/player_role.cpp:95

This new pure admission predicate contains two independent build/configuration rules, but it is only exercised indirectly through SendspinClient::start() and threaded lifecycle tests. The established pure helper decode_visualizer_message is declared in src/visualizer_role_impl.h:58-70 and tested directly in tests/test_visualizer_role.cpp; expose this validator through the private impl header (or an equivalent testable helper) and add direct control/rejection cases so mutations to either condition cannot be masked by lifecycle setup.

static bool audio_formats_valid(const std::vector<AudioSupportedFormatObject>& formats) {
    bool has_baseline = false;
    bool has_opus = false;
    for (const auto& format : formats) {
        if (format.codec == SendspinCodecFormat::FLAC || format.codec == SendspinCodecFormat::PCM) {
            has_baseline = true;
        } else if (format.codec == SendspinCodecFormat::OPUS) {
            has_opus = true;
        }
    }
    if (!has_baseline) {
        SS_LOGE(TAG,
                "audio_formats has no flac or pcm entry; servers need not support any other codec");
        return false;
    }
    if (has_opus && !OPUS_DECODER_ENABLED) {
        SS_LOGE(TAG, "audio_formats lists opus but this build has no Opus decoder");
        return false;
    }
    return true;

Files reviewed: 20/20 changed files
Comments generated: 2
Review effort level: Lite

The macro-scope sentences in docs/internals.md and CLAUDE.md now say the build files, the examples, and the configuration-specific tests are the references outside src/.

marcelveldt added 2 commits September 16, 2026 00:11

marcelveldt added the enhancement New feature or request label Sep 15, 2026

marcelveldt requested a lite review from Copilot September 15, 2026 22:20

Copilot started reviewing on behalf of marcelveldt September 15, 2026 22:21 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread src/player_role.cpp

Comment thread README.md Outdated

marcelveldt requested a lite review from Copilot September 15, 2026 22:32

Copilot started reviewing on behalf of marcelveldt September 15, 2026 22:32 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

marcelveldt requested a lite review from Copilot September 15, 2026 22:44

Copilot started reviewing on behalf of marcelveldt September 15, 2026 22:44 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

marcelveldt requested a lite review from Copilot September 15, 2026 22:53

Copilot started reviewing on behalf of marcelveldt September 15, 2026 22:54 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread idf_component.yml

Comment thread docs/internals.md Outdated

Name the tests in the SENDSPIN_ENABLE_OPUS scope statements

f8cec0e

The macro-scope sentences in docs/internals.md and CLAUDE.md now say the build files, the examples, and the configuration-specific tests are the references outside src/.

marcelveldt marked this pull request as ready for review September 15, 2026 23:07

Navigation Menu

Make the Opus decoder a build option and require flac or pcm in player formats - #126

Make the Opus decoder a build option and require flac or pcm in player formats#126
marcelveldt wants to merge 6 commits into
mainfrom
marcelveldt/interesting-heisenberg-c65a9c

marcelveldt commented Sep 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

marcelveldt commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Note for the ESPHome component

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (6)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (6)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (7)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marcelveldt commented Sep 15, 2026 •

edited

Loading