Make ChannelCount and SampleRate NonZero #709
Conversation
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. Performance might drop in decoders, the current implementation makes the bound check every time `channels` is called which is once per span. This could be cached to alleviate that.
dvdsk
force-pushed
the
NonZero
branch
3 times, most recently
from
415ecc3 to
67e759a
Compare
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.
dvdsk
changed the title
|
Yes, maybe. |
| /// short macro to generate a `NonZero`. It panics during compile if the | ||
| /// passed in literal is zero. Used for `ChannelCount` and `Samplerate` | ||
| /// constants | ||
| macro_rules! nz { |
There was a problem hiding this comment.
A similar macro nonzero is provided by nonzero_ext. I understand it may be too heavy-weight to pull in a dependency just for that, but maybe we could use the same name for recognizability and ease of switching if we ever want to use more of nonzero_ext in the future.
There was a problem hiding this comment.
I had a look its not a transitive dependencies of rodio so for now lets keep it out. (Another argument is that nz! is nice in short, though that might be bad taste on my end).
If you ever need something from nonzero_ext do not hesitate to include it though!
|
I support this for reasons of clarity and code robustness.
I agree, for performance-critical paths it's worth spending a few bytes of cache in the struct. |
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
channelsorsample_rateis called which should be about once per span. This could be cached to alleviate that.