Repository navigation
Always advertise FLAC or PCM to the server - #179
Open
marcelveldt wants to merge 2 commits into
Open
marcelveldt wants to merge 2 commits into
marcelveldt wants to merge 2 commits into
Conversation
Sendspin/spec#238 makes Opus optional for servers: they must support flac and pcm and may support opus, and players must always list flac or pcm in supported_formats since they are not told what a server can produce. An opus-only codec configuration produced a hello that a server without Opus could not serve at all. The format list now appends pcm as the lowest-priority fallback when neither flac nor pcm is offered, which also retires the "no supported codecs" error since the list can no longer be empty. The server/hello type gains the new source@v1_support object, informational only as this SDK implements no source role.
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes are covered by tests with no blocking issues identified.
Pull request overview
Updates codec negotiation to always advertise FLAC or PCM and adds typed server/hello support data.
Changes:
- Adds PCM fallback and related capacity tests.
- Removes obsolete codec validation.
- Adds
source@v1_supporttyping and coverage.
File summaries
| File | Summary |
|---|---|
tests/unit/protocol-handler.test.ts |
Tests codec advertising and server hello handling. |
tests/unit/codec-support.test.ts |
Tests fallback formats and buffer capacity. |
src/types.ts |
Adds typed server hello fields. |
src/core/core.ts |
Removes obsolete codec validation. |
src/core/codec-support.ts |
Ensures FLAC or PCM is advertised. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
When neither flac nor pcm is requested or survives the browser filter, append both where the browser decodes them, flac first, so an opus-only configuration gets the lower-bandwidth lossless format on a server without Opus instead of raw PCM.
This branch has not been deployed
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.
Sendspin/spec#238 makes Opus optional for servers. Every server must support FLAC and PCM and may support Opus, so every player must list FLAC or PCM in its
supported_formats; a player is not told which codecs the server can produce. An app configured withcodecs: ["opus"]sent a hello that a server without Opus could not serve at all.server/hellois typed per the spec, including the newsource@v1_supportobject a server sends when it accepts source clients. This SDK implements no source role, so the field is informational only.stream/start, and the FLAC and PCM paths never touch the Opus decoders.server/hellocarryingsource@v1_support.