RustAudio
Make ChannelCount and SampleRate NonZero
I ran into a lot of bugs while adding tests that had to do with channel being set to zero somewhere. While this change makes the API slightly less easy to use it prevents very hard to debug crashes/underflows etc.
Similar to the channels situation a lot of bugs seem to be caused by zero sample_rate. Since audio can literally not play at zero speed it makes no sense to have zero sample_rate's.
Neither channel = 0 nor sample_rate = 0 are used as a sentinel value anywhere
Performance might drop in decoders, the current implementation makes the bound check every time channels or sample_rate is called which should be about once per span. This could be cached to alleviate that.
I support this for reasons of clarity and code robustness.
Performance might drop in decoders, the current implementation makes the bound check every time
channelsorsample_rateis called which should be about once per span. This could be cached to alleviate that.
I agree, for performance-critical paths it's worth spending a few bytes of cache in the struct.
Though most of this PR is still quite valuable so lets not delete the branch.
this PR is utterly borked github's diff view does not match what I have locally. I am reopening it in the hope it gets fixed.
Well that was messy :(
I would like to merge this tomorrow. I've carefully gone through the full diff and it looks good now. Performance issues (I do not expect any since branches should be on cold paths) can easily be addressed later. If we leave this though other PR's will land and we will need another rebase/merge conflict.
That must've been quite a chore. Reviewing it, I came across a couple of points that are mostly not specific to this change, but stood out.
Thanks for the fast and thorough review! I think there are two unrelated (but important) issues that should be addressed somewhere else.
We could entertain changing SampleRate and ChannelCount upstream in cpal to be NonZero, too.
Good idea! Seems like cpal is still actively maintained (yay :)) Go for it!
This should be perfect now so I'm going to merge this. If any of you disagree feel free to revert it.