-
Notifications
You must be signed in to change notification settings - Fork 144
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
No_std support for symphonia-core #244
base: master
Are you sure you want to change the base?
Conversation
Quick update:
Is there a reason why |
I will push the code later but for now another update, I got the FLAC playing but:
Some more thought is needed to make it work generically which makes this more of a proof of concept. To make it marge-able I will need some guidance on how these can be handled. |
Hi @atoktoto, Very interesting work here and looking forward to seeing how far you can get! I have not done a very thorough read-through of the changes, but I do have some concerns I'd like to bring up before you get in too far. I know this is early days and just proof-of-concept work! First, I think there needs to be a very firm separation between changes required for In terms of changes to
In terms of the changes you're making to
As you can see, changing the decoders to suit small processors can quickly become a major task. For this reason, I think those changes should live in another PR, or a separate set of "micro" versions of the decoders should be created instead. Particularly if you want to use fixed-point instead of floating-point (as the lossy decoders do). I don't think I am willing to compromise on the functionality, correctness, performance, or maintainability of the current set of decoders to better support microcontrollers. However, making such compromises in a separate complementary set of decoders whether, in-tree or out-of-tree, is okay. Finally, to answer your question:
This was a change request made long ago to simplify the effort required to use Symphonia in certain use-cases. It's not a requirement of the library itself, and I will likely be reverting it at the next sem-ver breaking release. I hope this helps guide you! |
Thank you for the response and it all makes a lot of sense for me. What I'm thinking now is:
I changed let predicted = coeffs
.iter()
.zip(&buf[i - N..i])
.map(|(&c, &s)| c * c)
.sum::<i32>(); into let buf_s = &buf[i - N..i];
let mut predicted = 0;
for j in 0..N {
predicted += buf_s[j] * coeffs[j]
} I didn't check for correctness (or overflows), just wanted to finally hear some sound :D and it will stay on a different branch. I will measure the actual impact and share the data. |
…to make the binary smaller
e06fd58
to
7e05c8b
Compare
ErrorsI reworked the Error structure. There is no copied code from PerformanceI run the ffbench vs symphonia-play a couple of times on the MLKDream.mp3 and there seems to be a no measurable impact on performance from those changes comparing to master. I did not manage to run the full suite as it seems to depend on Next up
Open to any feedback :) |
Sounds like you have a pretty solid plan! It looks like the For now, I think you could just continue with your plan. As long as you try to keep the changes as uninvasive as possible it should be workable. |
I decided to not migrate all codecs in this PR. While the changes are straight-forward there is a lot of them and they would make the review of this PR harder. I left no_std versions of WAV and FLAC as an example what no_std compatibility entails but removed all changes that made them micro-controller-ready. |
Following the encouragement from #88 and seeing that #91 got stuck I decided to take a shot myself.
My approach:
#![no_std]
andextern crate alloc;
core::Error
even though it's still behind#![feature(error_in_core)]
, switch to Nightly until stabilized ( Tracking Issue forError
incore
rust-lang/rust#103765 )core
andflac
and get it running on actual hardware to see if a simple conversion is viable.