-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Update Rodio to 0.21 #20323
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
base: main
Are you sure you want to change the base?
Update Rodio to 0.21 #20323
Conversation
|
Outstanding items:
|
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
Symphonia released 0.5.5 last week, and bevy_audio picks it up by default after cargo update. However, the combination of rodio 0.20 and symphonia 0.5.5 causes a panic when decoding MPEG-4 streams. This bug can be verified by running this rodio example directly I suggest either merging this PR or pinning the symphonia dependency to version 0.5.4 in Cargo.toml. |
|
@nyfair this PR is quite broken so I wouldn't merge it yet. We could do the version pinning as a stopgap and leave a comment on said change to fix it later. I might have some time to update this PR in a couple of days so I'll try my best to finalize it soon. @alice-i-cecile in the meantime, would you be able to help me figuring out the 3rd, 4th and 5th items that I left in my comment? |
7111175 to
51b8311
Compare
| /// which iterates over samples of type [`rodio::Sample`]. | ||
| /// Must be a [`rodio::Source`] so that it can provide information on the audio it is iterating over. | ||
| type Decoder: rodio::Source + Send + Iterator<Item = Self::DecoderItem>; | ||
| type Decoder: rodio::Source + Send + Iterator<Item = rodio::Sample>; |
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.
rodio::Source has Iterator<Item = rodio::Sample> as a bound as far as I can see, should we drop this redundant bound or should we keep it?
|
There's an unreleased change in EDIT: If someone has any suggestions on how to proceed from here please let me know. Related issue: RustAudio/cpal#1038 |
b85a940 to
6248b2e
Compare
alice-i-cecile
left a comment
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.
We shouldn't be exposing minimp3 at all, per #20183
6248b2e to
9ef8982
Compare
Rebased, removed the unwanted feature and fixed some wrongly set internal feature flags for the fallbacks. I've also cleaned up my own messages as there was too much noise in the PR. Later I will try pushing a temporary commit to see if I can fix the outstanding issues and we can finally test this, analyze what options we have and make a decision for this upgrade. |
|
You added a new feature but didn't update the readme. Please run |
|
Can you please resolve merge conflicts and get CI passing? |
Hey @alice-i-cecile, I can fix the conflicts but I don't think I can fix the CI, we'd need a new version of |
|
Can you track down the |
Relevant: RustAudio/cpal#1065 😄 Please also check this question I had:
It seems like a PR that fixed that "got too big" and was just closed, and I didn't see any other PRs pushed to fix that. So as a follow up question:
As far as I understand, symphonia is an opt-in feature right now. Thanks @alice-i-cecile! |
--- updated-dependencies: - dependency-name: cpal dependency-version: 0.16.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
This version depends on cpal 0.16
e230b93 to
210a0bb
Compare
|
Rebased and added a migration guide. I just realized that this will probably become a bump to |
9c9e8fb to
fa00c43
Compare
Objective
Solution
cpalandrodioto their latest versions.rodio's breaking changes.symphoniabecomes the default audio backend except for .ogg files, where we will still default tolewton.aarch64-apple-ios-simtarget witharm64-apple-ios-simulator.Testing