From 4a19343a068eaa4aa06e8257f6485a0998ff3105 Mon Sep 17 00:00:00 2001 From: Kevin Ahrendt Date: Sun, 11 Oct 2026 14:14:54 -0400 Subject: [PATCH] Decode Opus at 16 bits whatever stream/start's bit_depth says roles/player/v1.md "stream/start player object" ignores bit_depth for opus, but handle_stream_start() copied it into the Opus codec header and the published stream params. The decoder always writes opus_decode()'s 16-bit samples while sizing frames and output bytes from that header, so a server sending another depth broke playback: 24 reported stale buffer bytes as audio, 8 overflowed the decode buffer, and 0 divided by zero. The consumer also configured its output from the wire depth. For an Opus stream, handle_stream_start() now sets bit_depth to OPUS_DECODED_BITS_PER_SAMPLE (16, in decoder.h) before building the codec header and publishing the params. --- src/decoder.h | 5 +++++ src/player_role.cpp | 10 ++++++++-- src/player_role_impl.h | 2 +- tests/test_player_role.cpp | 39 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/decoder.h b/src/decoder.h index 9dc5e1a6..51f2f5af 100644 --- a/src/decoder.h +++ b/src/decoder.h @@ -34,6 +34,11 @@ namespace sendspin { /// Constraints"). static constexpr uint32_t MAX_AUDIO_CHUNK_MS = 150U; +/// @brief Bits per sample of decoded Opus audio, whatever stream/start's bit_depth says +/// (roles/player/v1.md "stream/start player object": bit_depth is ignored for opus). The decoder +/// writes opus_decode()'s 16-bit samples. +static constexpr uint8_t OPUS_DECODED_BITS_PER_SAMPLE = 16U; + /** * @brief Audio decoder wrapper supporting FLAC, raw PCM, and (with SENDSPIN_ENABLE_OPUS) Opus * diff --git a/src/player_role.cpp b/src/player_role.cpp index 9cb06111..9663c90a 100644 --- a/src/player_role.cpp +++ b/src/player_role.cpp @@ -351,7 +351,7 @@ SS_HOT void PlayerRole::Impl::handle_binary(InboundMessage& message) { generation); } -void PlayerRole::Impl::handle_stream_start(const ServerPlayerStreamObject& player_obj) { +void PlayerRole::Impl::handle_stream_start(ServerPlayerStreamObject player_obj) { const uint32_t generation = this->cleanup_generation.load(std::memory_order_acquire); bool header_sent = false; // Numbers the codec header and the STREAM_START, so the sync task starts the stream only on @@ -363,6 +363,12 @@ void PlayerRole::Impl::handle_stream_start(const ServerPlayerStreamObject& playe SS_LOGE(TAG, "Stream start message missing required audio parameters"); } else { auto codec = player_obj.codec.value(); + if (codec == SendspinCodecFormat::OPUS) { + // The codec header below and the params the consumer configures its output from both + // carry the depth the decoder writes, not stream/start's bit_depth, which is ignored + // for opus (roles/player/v1.md "stream/start player object"). + player_obj.bit_depth = OPUS_DECODED_BITS_PER_SAMPLE; + } if (codec == SendspinCodecFormat::PCM || (OPUS_DECODER_ENABLED && codec == SendspinCodecFormat::OPUS)) { @@ -411,7 +417,7 @@ void PlayerRole::Impl::handle_stream_start(const ServerPlayerStreamObject& playe // high_performance_requested_for_playback main-thread-only. Both carry `generation`, so a // teardown that runs before the drain takes them leaves a START the drain discards and params // it never applies (see drain_events()). - this->event_state->stream_params_slot.write(player_obj, generation); + this->event_state->stream_params_slot.write(std::move(player_obj), generation); this->enqueue_stream_event(PlayerStreamCallbackType::STREAM_START, generation, ordinal); } diff --git a/src/player_role_impl.h b/src/player_role_impl.h index 21b8482e..b97f0661 100644 --- a/src/player_role_impl.h +++ b/src/player_role_impl.h @@ -109,7 +109,7 @@ struct PlayerRole::Impl : RoleTeardown { /// `message.item`), otherwise copied into an item the protocol task acquires. /// @param message The decrypted chunk; `data` points at its message type byte. void handle_binary(InboundMessage& message); - void handle_stream_start(const ServerPlayerStreamObject& player_obj); + void handle_stream_start(ServerPlayerStreamObject player_obj); void handle_stream_end() const; void handle_stream_clear(); void handle_server_command(const ServerCommandMessage& cmd) const; diff --git a/tests/test_player_role.cpp b/tests/test_player_role.cpp index 534fd234..f326d00e 100644 --- a/tests/test_player_role.cpp +++ b/tests/test_player_role.cpp @@ -741,6 +741,45 @@ TEST(PlayerHello, TheAdvertisedShareOfTheQuotaHoldsEncodedFrames) { } } +// roles/player/v1.md "stream/start player object" ignores bit_depth for opus, which decodes to +// 16-bit samples whatever the server sent. The codec header the sync task decodes with and the +// params the consumer configures its output from both carry the decoded depth. +TEST(PlayerStreamStart, BitDepthIsTheDecodedDepth) { + struct Row { + const char* name; + SendspinCodecFormat codec; + uint8_t decoded_bit_depth; + }; + const Row rows[] = { + {"Control: pcm keeps the server's depth", SendspinCodecFormat::PCM, 24}, +#ifdef SENDSPIN_ENABLE_OPUS + {"opus publishes the decoder's depth", SendspinCodecFormat::OPUS, 16}, +#endif + }; + for (const Row& row : rows) { + SCOPED_TRACE(row.name); + auto impl = make_impl(); + ServerPlayerStreamObject params = pcm_stream_params(); + params.codec = row.codec; + params.sample_rate = 48000; + params.bit_depth = 24; + impl->handle_stream_start(params); + + void* header_item = impl->sync_task->take_item(0); + ASSERT_NE(header_item, nullptr); + DummyHeader header{}; + ASSERT_EQ(SyncTask::encoded_size(header_item), sizeof(header)); + std::memcpy(&header, SyncTask::encoded_data(header_item), sizeof(header)); + impl->sync_task->inbound().return_item(header_item); + EXPECT_EQ(header.bits_per_sample, row.decoded_bit_depth); + + ServerPlayerStreamObject published; + uint32_t generation = 0; + ASSERT_TRUE(impl->event_state->stream_params_slot.take(published, generation)); + EXPECT_EQ(published.bit_depth, row.decoded_bit_depth); + } +} + // ============================================================================ // Output delay: spec clamp, write avoidance, and the persisted-value range // ============================================================================