Repository navigation
Decode Opus at 16 bits whatever stream/start's bit_depth says - #197
Merged
Merged
Conversation
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.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The public header and integration guide must document the new Opus bit_depth semantics.
1 open finding
What changed in this PR
Normalizes Opus playback parameters to the decoder’s fixed 16-bit output format.
Changes:
- Adds a shared Opus decoded-depth constant.
- Normalizes codec headers and published stream parameters.
- Tests Opus normalization with PCM as a control.
| File | Description |
|---|---|
src/decoder.h |
Defines the 16-bit Opus output depth. |
src/player_role.cpp |
Normalizes Opus stream parameters. |
src/player_role_impl.h |
Updates the handler signature. |
tests/test_player_role.cpp |
Tests normalized and control behavior. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
roles/player/v1.md "stream/start player object" says
bit_depthis ignored for opus, so a compliant server may send any value.handle_stream_start()copied it into the Opus codec header and the published stream params. The decoder always writesopus_decode()'s 16-bit samples, but sizes frames and output bytes from the header, so any other depth broke playback (reproduced with a 20 ms stereo packet):bytes_to_frames().The consumer also configured its output from the wire depth through
get_current_stream_params().handle_stream_start()setsbit_depthto the newOPUS_DECODED_BITS_PER_SAMPLE(16, indecoder.h) before it builds the codec header and publishes the params. The player is the only producer of the Opus header, so this is the one place.handle_stream_start()takes its param by value so it can adjust it and move it into the params slot (same one copy as before).sendspin-server and aiosendspin both send 16 for Opus today, so no known server hits this.
Tests
New table test
PlayerStreamStart.BitDepthIsTheDecodedDepth:stream/startwithbit_depth: 24puts 16 in both the codec header and the published params for Opus, and keeps 24 for PCM (control). Mutation-checked: dropping the override, or building the header from the wire value, fails it. 701/701 under ASan.