[Merged by Bors] - Expose symphonia features from rodio in bevy_audio and bevy by ong-yy · Pull Request #6388 · bevyengine/bevy

ong-yy · 2022-10-27T15:43:58Z

Objective

Fix #6301

Solution

Add new features in bevy_audio to use symphonia sound format from rodio
Also add in bevy

alice-i-cecile · 2022-10-27T15:47:27Z

Ping @harudagondi.

harudagondi

Should we add the description of the features here?

Otherwise, this LGTM.

alice-i-cecile · 2022-10-28T00:49:18Z

We should, yes.

james7132 · 2022-10-28T07:49:25Z

Cargo.toml

+symphonia-all = ["bevy_internal/symphonia-all"]
+symphonia-flac = ["bevy_internal/symphonia-flac"]
+symphonia-isomp4 = ["bevy_internal/symphonia-isomp4"]
+symphonia-mp3 = ["bevy_internal/symphonia-mp3"]


Why expose this when there's another mp3 feature already? Same with wav and vorbis.

Symphonia is an alternative backend to what rodio uses, which depends on the features used (flac = claxon, vorbis = lewton, wav = hound, mp3 = minimp3).

imo these features should be available because:

For completeness sake.

When you want to support formats, where is only supported by symphonia (for example aac and wav) you'll have to compile both hound and symphonia. I think it's better as a user to just enable symphonia's wav feature instead of using hound to reduce compile times.

ong-yy · 2022-10-29T09:20:27Z

Hi. Regarding the CI Dependencies / check-bans that is failing, the issue is on error[duplicate]: found 2 duplicate entries for crate 'nix'. I am not entirely sure how to fix this. Can please guide me on fixing this?

harudagondi · 2022-10-29T10:06:30Z

It's probably because of the alsa issue (diwic/alsa-rs#90) and #6359. Your code is currently fine, however someone else needs to simply just update the ignore file for nix versions.

mockersf · 2022-10-29T18:28:46Z

docs/cargo_features.md

 |wav|WAV audio format support.|
+|symphonia-aac|AAC audio format support by Symphonia.|
+|symphonia-all|AAC, FLAC, MP4, MP3, Vorbis, WAV support by Symphonia.|
+|symphonia-flac|FLAC audio format support by Symphonia.|


It would be nice to have a short explanation of how this is different than the flac feature (same for mp3 and wav)

Do I put the explanation in this cargo_features file as well? Besides, AFAIK they are just different libraries. Both can be used to process audio files. A benefit I could think of using Symphonia is easily change audio formats used by games since it can probe the media source and decode accordingly instead of changing bunch of code when you need to change audio file from wav = hound to mp3 = minimp3.

Bevy completely abstracts over the file loaded, so changing from wav to Flac would just mean changing the feature and the file extension.

I would like to see in this file why someone would enable a feature, and I'm not sure "support by Symphonia" is enough of a difference with the flac feature to help users decide which they need.

Internally, rodio parses audio files using the following libraries:

Format Library

flac claxon

vorbis lewton

wav hound

mp3 minimp3

Symphonia support parsing all of the mentioned file formats and more, however it is much "heavier" due to having support of more file formats. Therefore symphonia is not enabled by default.

Symphonia is usually enabled if:

You have audio files not supported by any of the four audio files mentioned. (I don't think there's even a library for parsing aac files that is widely used and tested)

You support a multitude of audio file formats and would not like to compile five different crates that essentially do the same thing (well, more than five, since symphonia have its own subcrates)

In this scenario, symphonia is just an implementation detail. Perhaps we can indicate that when enabling symphonia, they should disable other features to avoid redundancies. (also when you enable both mp3 and symphonia-mp3 features, the mp3 will actually be disable and symphonia will take priority. see here)

As such, enabling all the aformentioned features plus symphonia-all will simply render the above four features useless, while still compiling the corresponding libraries (i think. haven't delved deep into how rust compile dependencies).

Maybe the best thing to do is to link symphonia to its docs.rs page, and mention this comment (or put it) on symphonia-all's description.

I'd be very happy to add a modified version of this comment to the feature list.

Just a few more info to chip in after researching a bit.

On the codec mp3, the rodio is most likely also switching to symphonia due to cross compilations, potentially unsafe code and perhaps ios rejection as discussed here.

And also over here, it seems symphonia has more momentums compared to lewton for vorbis and therefore, could reiterate more often and provide more features/fixes perhaps?

alice-i-cecile · 2022-10-31T16:10:05Z

Waiting to merge until @mockersf's concerns above are addressed a bit more. @harudagondi, perhaps you can help out?

harudagondi · 2022-12-24T09:34:08Z

Hey @Ian-Yy can you update this PR? Would love to see this merged for Bevy 0.10

ong-yy · 2022-12-24T12:57:18Z

Hi @harudagondi, sorry not sure if I get it. Do you mean pulling the latest and re-apply the changes?

harudagondi · 2022-12-24T14:12:57Z

Hi @harudagondi, sorry not sure if I get it. Do you mean pulling the latest and re-apply the changes?

Yes, and add a comment in the Cargo.toml or the cargo_features.md regarding the difference between the default decoder and symphonia.

ong-yy · 2023-01-06T15:46:33Z

Hi, I have merged and add more details in the cargo_features.md. Please let me know if any correction is needed.

docs/cargo_features.md

alice-i-cecile · 2023-01-09T18:51:55Z

bors r+

# Objective Fix #6301 ## Solution Add new features in `bevy_audio` to use `symphonia` sound format from `rodio` Also add in `bevy`

bors · 2023-01-09T19:24:10Z

Pull request successfully merged into main.

Build succeeded:

…ne#6388) # Objective Fix bevyengine#6301 ## Solution Add new features in `bevy_audio` to use `symphonia` sound format from `rodio` Also add in `bevy`

Add symphonia features in bevy_audio and bevy

af4401e

alice-i-cecile added A-Audio Sounds playback and modification C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Oct 27, 2022

alice-i-cecile approved these changes Oct 27, 2022

View reviewed changes

harudagondi approved these changes Oct 28, 2022

View reviewed changes

james7132 reviewed Oct 28, 2022

View reviewed changes

Add symphony features into optional feature docs

865258b

alice-i-cecile approved these changes Oct 29, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 29, 2022

mockersf reviewed Oct 29, 2022

View reviewed changes

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 31, 2022

ong-yy added 2 commits January 6, 2023 23:20

Merge branch 'main' into expose-symphonia-from-rodio

0d5f0f8

Add more details on cargo_features

e50eb17

harudagondi requested a review from mockersf January 6, 2023 16:13

harudagondi reviewed Jan 6, 2023

View reviewed changes

docs/cargo_features.md Outdated Show resolved Hide resolved

docs/cargo_features.md Outdated Show resolved Hide resolved

Add correction on features descriptions

5908dfa

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 9, 2023

bors bot pushed a commit that referenced this pull request Jan 9, 2023

Expose symphonia features from rodio in bevy_audio and bevy (#6388)

a41e869

# Objective Fix #6301 ## Solution Add new features in `bevy_audio` to use `symphonia` sound format from `rodio` Also add in `bevy`

bors bot changed the title ~~Expose symphonia features from rodio in bevy_audio and bevy~~ [Merged by Bors] - Expose symphonia features from rodio in bevy_audio and bevy Jan 9, 2023

bors bot closed this Jan 9, 2023

Uh oh!

Conversation

ong-yy commented Oct 27, 2022

Objective

Solution

Uh oh!

alice-i-cecile commented Oct 27, 2022

Uh oh!

harudagondi left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Oct 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ong-yy commented Oct 29, 2022

Uh oh!

harudagondi commented Oct 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Oct 31, 2022

Uh oh!

harudagondi commented Dec 24, 2022

Uh oh!

ong-yy commented Dec 24, 2022

Uh oh!

harudagondi commented Dec 24, 2022

Uh oh!

ong-yy commented Jan 6, 2023

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Jan 9, 2023

Uh oh!

bors bot commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants