Test fixes, upgrade quickcheck #640
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.
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. |
|
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 |
|
@dvdsk Security-wise there is no strict need in checked cast but u32 * u32 may overflow in some cases. AFAIK overlapped value will wrap over zero in release build, and panic in debug mode. I'd expected some clicks or distortion in case of overflows and panics in debug mode (which was triggered by quickcheck). The final interpolated value should be in range, though. One could use smaller integers for the ratios (I doubt u32 precision is actually necessary there), or use f32 as interpolation coefficient. I think smaller integers, like u16 can be an approximation, maybe even something crude, like dropping some least significant bits in the u32 numerator and divider when one of those has significant bits beyond 16 position. Or finding some other approximate value pair. |
|
@dvdsk Security-wise there is no strict need in checked cast but u32 * u32 may overflow in some cases. AFAIK overlapped value will wrap over zero in release build, and panic in debug mode. I'd expected some clicks or distortion in case of overflows and panics in debug mode (which was triggered by quickcheck). The final interpolated value should be in range, though. One could use smaller integers for the ratios (I doubt u32 precision is actually necessary there), or use f32 as interpolation coefficient. I think smaller integers, like u16 can be an approximation, maybe even something crude, like dropping some least significant bits in the u32 numerator and divider when one of those has significant bits beyond 16 position. Or finding some other approximate value pair. let ratio = num::rational::Rational32::approximate_float(2.3f32).unwrap();
dbg!(ratio.numer(), ratio.denom());Maybe making this more reliable can be a separate task... |
quickcheck1.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 sinceasconversion may silently discard most significant bits. Alternatively, interpolation coefficient (numerator/dividor) may use e.g.u16instead ofu32, or maybe even floating point. I believeu32precision is unnecessary there. But that requires updating sample rate converter which at the moment is the only user of this interpolation.Sample::lerpwas incorrect. It said that calculations should followc * first + (c - 1) * second. which givesfirstatc==1andsecondatc==0but actual implementations use the oppositefirstatc==0andsecondatc==1.no_runsince they require audio devices and may actually make sounds whilecargo testcommand is running.--all-targetsswitch excludes doc tests, so those are executed separately.experimentalbuilds exclude some code and in some code parts use mutually exclusive implementations, so I included those in CI too to keep experimental code compilable.