-
Notifications
You must be signed in to change notification settings - Fork 268
feat: change default decoder to Symphonia and update feature flags #753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Switch default decoder to Symphonia and revise feature flags
- Change default format decoder to Symphonia; retain previous decoders (
claxon
,lewton
,hound
, etc.) as optional features - Clean and document
Cargo.toml
with explicit feature definitions, including separatewav_output
for WAV writing - Update tests, source code, documentation, CHANGELOG, and CI to reflect renamed features
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/wav_test.rs | Updated #[cfg(feature = "wav")] to #[cfg(feature = "hound")] |
tests/total_duration.rs | Swapped wav →hound and flac →claxon in test templates |
tests/seek.rs | Replaced wav →hound and flac →claxon feature flags |
tests/flac_test.rs | Changed #[cfg(feature = "flac")] to #[cfg(feature = "claxon")] |
src/source/mod.rs | Renamed SeekError #[cfg(feature = "wav")] to #[cfg(feature = "hound")] |
src/lib.rs | Moved WAV output module under wav_output instead of wav |
src/decoder/mp3.rs | Adjusted sample type import from DecoderSample to Sample |
src/decoder/mod.rs | Renamed feature gates: wav →hound , flac →claxon , vorbis →lewton |
src/decoder/builder.rs | Applied same feature renames in DecoderBuilder |
README.md | Updated bullet list to reflect default Symphonia decoders and optional backends |
Cargo.toml | Overhauled [features] section; added wav_output , renamed decoders |
CHANGELOG.md | Documented new default decoders and feature flag changes |
.github/workflows/ci.yml | Extended CI to test alternative decoder feature sets |
Comments suppressed due to low confidence (5)
src/source/mod.rs:652
- [nitpick] The error message refers to "wav source" but this variant is gated by the
hound
feature; consider updating it to mention "Hound decoder" or "WAV (Hound)" for clearer, consistent messaging.
SeekError::HoundDecoder(err) => write!(f, "Error seeking in wav source: {}", err),
README.md:11
- [nitpick] This bullet appears misaligned compared to the surrounding list items; ensure consistent indentation and prefix so it renders properly as part of the bullet list.
- FLAC by [claxon](https://github.com/ruuda/claxon).
Cargo.toml:14
- The default features include
flac
andwav
, which now map to Symphonia backends; verify that this matches the intention, as the new optionalclaxon
andhound
features may cause confusion.
default = ["playback", "flac", "mp3", "mp4", "vorbis", "wav"]
tests/wav_test.rs:1
- The WAV test now only runs under the
hound
feature; consider adding a parallel test or adjusting the cfg so it also covers WAV decoding via thesymphonia-wav
feature, since that's now the default backend.
#[cfg(feature = "hound")]
src/lib.rs:175
- The new
wav_output
module is gated by thewav_output
feature but currently lacks dedicated tests; consider adding tests foroutput_to_wav
to validate WAV file generation.
#[cfg(feature = "wav_output")]
We should check with Bevy and some other Rodio users. @will3942 whats your opinion on this change? |
Thanks for flagging. A less permissive license is always less desirable for us. MPL isn't a problem though and we already use Symphonia. |
Bevy runs with default features false so it should not be an issue for them (as they have zero decoders enabled by default) |
Could you add a section to the upgrade guide? Do we need to say something in the release announcement? (see outreach folder for the current draft in the 0.21 release branch). |
Sure - you mean you want to target it for v0.21? |
Why not :) |
Add CI check for minimp3 feature and update mp3 decoder to use Sample type instead of DecoderSample.
Switch default decoding features to use Symphonia for all formats. Rename alternative decoder features: use `claxon` for FLAC, `lewton` for Vorbis, and `hound` for WAV. Update documentation, tests, and code to match new feature flag names and defaults. Add Symphonia CAF and MKV format dependencies.
8e0d18d
to
dd2a49b
Compare
Done in dd2a49b. I think we can skip it in the release announcement; it should be all the same to 99.9% of existing users. |
@PetrGlad would you agree with these changes? (no need for full review though one is always welcome). |
Bevy maintainer here: yep, this won't cause any problems for our users as a result. |
Not sure how controversial this may be? Probably for a next version.
Pros:
Cons:
To counter that licensing point, the other decoders are still available as optional features.
Further changes:
Cargo.toml
symphonia-wav
).