-
Notifications
You must be signed in to change notification settings - Fork 234
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
Test fixes, upgrade quickcheck #640
base: master
Are you sure you want to change the base?
Conversation
`cargo test --all-targets` excludes doc tests. Experimental features are sometimes mutually exclusive with the stable code.
Overflows chould have distort sound without triggering any errors.
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.
Nice fixes and improvements 👍 , thank you very much :) . This must have taken a while!
Few things I am unsure about as you'll see in the discussion. Its a bit too late for me to check on the performance impact of the try_from. Ill see if I can add a new interpolate benchmark to rodio tomorrow.
Love the work here, can not look at it the next few days unfortunately. After the weekend I'll see if I can come up with a benchmark for the resampler. We need one anyway (I want to introduce a highfy resampler in the future). Ill do a detailed review and answer all the questions that came up then. |
Looking a other filters in As I understand rodio in most cases can use generic parameters to avoid dyn references (but Sync and Mixer still use them). That probably can be slightly more efficient. Any other reason rodio cannot use some existing external libraries for sound processing graph? |
Rodio and dasp have fundamentally different goals, that makes it hard and maybe unwise to merge them or require users to be familiar with both.
Rodio predates dasp, so its easy to turn this around and ask, why do they not depend on rodio. Again its a difference in goals, dasp wants to have zero dependencies, rodio just wants to be easy to use (so no C-deps if possible).
I also think rodio has most features dasp has since #602 landed. I might be mistaken there. And a Source that makes it easy to use dasp from rodio might be interesting. The sinc interpolator in dasp is interesting, rodio needs an option for a slower but more hifi interpolator. See #584. |
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.
Only AGC example and what is possibly a superfluous --doc argument in ci.yml left and then we can merge this 👍
Edit: oh and the extra asserts you added might need a message, if only to make reading the code easier.
Still have not found the time for the benchmark, as soon as I've done that ill post the results here and we can see what we do based on them. |
benchmarks results from main on my machine: If you rebase on (or merge with) main ill rerun them on the PR and we can see if something changed.
|
`experimental` flag removes some API, just keep the example minimally functional in that case.
benchmarks this pr on my machine:
main branch:
So that's a 5 to 10% slowdown. A bit too much. Lets see what causes it |
with the try from in sample.rs replaced with a cast performance increases a tiny bit however it does not go back to what it was.
this pr with changes to sample.rs reverted, again matches main branch in perf:
The switch from i32 to i64 is probably to blame. Using i32 might enable the compiler to emit better/any SIMD. We won't know without looking at the assembly. We could keep the |
quickcheck
1.x input values vary more than in 0.9 which finds overflow cases and excessive memory allocation problems inSample::lerp
. Calculations now use f64 to avoid that. Conversion back to 16 bit resolution is done explicitly sinceas
conversion may silently discard most significant bits. Alternatively, interpolation coefficient (numerator/dividor
) may use e.g.u16
instead ofu32
, or maybe even floating point. I believeu32
precision is unnecessary there. But that requires updating sample rate converter which at the moment is the only user of this interpolation.Sample::lerp
was incorrect. It said that calculations should followc * first + (c - 1) * second
. which givesfirst
atc==1
andsecond
atc==0
but actual implementations use the oppositefirst
atc==0
andsecond
atc==1
.no_run
since they require audio devices and may actually make sounds whilecargo test
command is running.--all-targets
switch excludes doc tests, so those are executed separately.experimental
builds exclude some code and in some code parts use mutually exclusive implementations, so I included those in CI too to keep experimental code compilable.