refactor: Update symphonia from 0.5.5 to 0.6.1 - #920
refactor: Update symphonia from 0.5.5 to 0.6.1#920UnknownSuperficialNight wants to merge 1 commit into
0.5.5 to 0.6.1#920Conversation
- Bump symphonia to version 0.6.1 in `Cargo.toml` - Migrate codec registry API from `register_all::<D>()` to `register_audio_decoder::<D>()` - Replace `Decoder` and `SignalSpec` with new `AudioDecoder` and `AudioSpec` types - Introduce lifetime parameters on `Decoder`, `LoopedDecoder`, `SymphoniaDecoder`, and `DecoderImpl` - Refactor `SymphoniaDecoder` to handle EOF via `AudioDecoder`'s `last_decoded()` method - Update test code to use the new lifetime-based `Decoder<'static, R>` types
| fn samples_from_time_f64( | ||
| t: symphonia::core::units::Time, | ||
| sample_rate: u32, | ||
| channels: u32, | ||
| ) -> usize { | ||
| let (secs_i64, nanos_u32) = t.parts(); | ||
| if secs_i64 < 0 { | ||
| return 0; | ||
| } | ||
| let secs = secs_i64 as f64 + (nanos_u32 as f64) / 1e9_f64; | ||
| (secs * sample_rate as f64 * channels as f64).ceil() as usize | ||
| } |
There was a problem hiding this comment.
This is probably not a good idea but just a plaster/band-aid solution to get it compiling
|
Also found this probably would be worth investigating seeing if its fixed in the new symphonia Lines 195 to 203 in c5f1e94 |
|
I like to use these opportunities to point out that in #786 and https://github.com/RustAudio/rodio/tree/feat/comprehensive-seeking-and-bit-depth I had fixed a gazillion of these difficult classes of bugs. Would be great if that one could be updated. That would ideally go two ways: first checking if in |
| @@ -215,9 +249,20 @@ impl Source for SymphoniaDecoder { | |||
| } | |||
|
|
|||
| fn try_seek(&mut self, pos: Duration) -> Result<(), source::SeekError> { | |||
There was a problem hiding this comment.
This is what i need input on
Think that would be up-to (@yara-blue) as im not really to up to date on all the commits in master
To clarify you saying basically check if anything in master should be merged into comprehensive-seeking-and-bit-depth then once that is completed take comprehensive-seeking-and-bit-depth and merge to main or use as a basis for this PR? I'm kind of confused on the intent of your wording? |
|
Yes. It was some time ago so I imagine they have diverged. Personally, I would go from |
Do you mean make a new branch and and rebase comprehensive-seeking-and-bit-depth into master get it fixed up then make a pr then after than consider what im doing on this branch? or do you mean since this current branch is based on the current master rebasing comprehensive-seeking-and-bit-depth onto migrate-symphonia-0.5.5-to-0.6.1 |
|
Would be great if we could get those improvements in, but I do not want to scope creep beyond what @UnknownSuperficialNight has time for. Keep in mind that the mentioned PR could not get reviewed due to size. So if you bring it up to the point it can be merged to master it would still need to be split up*. At that point it might be easier to take some of the fixes move those in here and then follow up with more PR's moving in the rest bit by bit. Please aim for no more then 500L per PR. If you chose to go that route Roderick can maybe suggest what would be good points to split it up at. [*]: though there is an argument to be made that at that point the decoder code has been thoroughly reviewed by you and it can therefore be merged as a whole. Keep in mind though that it is about 7k lines. That is about a week of work to review. I am not able to split review work across multiple sessions since I tend to forget a lot in between, that limits things. |
|
I understand it grew beyond review capability. Neither of us fancies the time to put into review or to put into splitting. I could point Claude at it to automate the splitting, and it could take the Symphonia upgrade along in one swoop. The test suite is a blessing here. But we should prioritize our efforts. Probably the new audio pipeline should come first? |
Yes, I think there are about two efforts/PR's that are soft blocked on that since it makes their work far easier. (An effect that then no longer needs to account for spans, and something else I forgot...)
I'm fine with you (specifically not other contributors) using LLM based tools to automate the splitting but ideally the upgrade happens non-agentically. Maybe the best order here is:
|
That makes logical sense to me |
3 participants
This is a very rough draft (basically got it compiling and it works in the libopus example): @yara-blue
Seeking is especially rough. I would like some input on the best way to handle that.
Closes #919
Here are test results as of 47e574b: