-
Notifications
You must be signed in to change notification settings - Fork 466
add native dsd support #1078
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
base: master
Are you sure you want to change the base?
add native dsd support #1078
Conversation
roderickvd
left a comment
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.
Really nice add. I know it's still a draft, but I took the opportunity to a quick review anyway. Also don't forget to add a changelog entry - worth it 😄
| SampleFormat::U64 => fill_typed!(u64), | ||
| SampleFormat::F32 => fill_typed!(f32), | ||
| SampleFormat::F64 => fill_typed!(f64), | ||
| SampleFormat::DsdU8 => fill_typed!(u8), |
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.
This should probably be fill_typed!(DsdU8)
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.
now when I removed structs I am not sure what to use here. any suggestion?
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.
Something like this?
const DSD_SILENCE_BYTE: u8 = 0x69;
SampleFormat::DsdU8 | SampleFormat::DsdU16 | SampleFormat::DsdU32 => buffer.fill(DSD_SILENCE_BYTE),
Thank you for you review! I am not alsa expert but I've tested it in my rsplayer repo. |
roderickvd
left a comment
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.
Thanks for the update. In addition to the included review comments, I see the following:
- Please add a changelog entry.
- In
alsa/mod.rswe need to expand the check to correctly fill buffers with DSD silence:
if sample_format.is_uint() || sample_format.is_dsd() {
fill_with_equilibrium(&mut silence_template, sample_format);
}- Then in
examples/record_wav.rs, bail out when the input is DSD:
fn sample_format(format: cpal::SampleFormat) -> hound::SampleFormat {
if format.is_float() {
hound::SampleFormat::Float
} else if format.is_dsd() {
panic!("DSD formats cannot be written to WAV files");
// Or handle gracefully
} else {
hound::SampleFormat::Int
}
}| SampleFormat::U64 => fill_typed!(u64), | ||
| SampleFormat::F32 => fill_typed!(f32), | ||
| SampleFormat::F64 => fill_typed!(f64), | ||
| SampleFormat::DsdU8 => fill_typed!(u8), |
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.
Something like this?
const DSD_SILENCE_BYTE: u8 = 0x69;
SampleFormat::DsdU8 | SampleFormat::DsdU16 | SampleFormat::DsdU32 => buffer.fill(DSD_SILENCE_BYTE),| SampleFormat::U64 => u64::BITS, | ||
| SampleFormat::F32 => 32, | ||
| SampleFormat::F64 => 64, | ||
| SampleFormat::DsdU8 => 1, |
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.
As a matter of style we could reduce this to SampleFormat::DsdU8 | SampleFormat::DsdU16 | SampleFormat::DsdU32 => 1.
| SampleFormat::U64 => "u64", | ||
| SampleFormat::F32 => "f32", | ||
| SampleFormat::F64 => "f64", | ||
| SampleFormat::DsdU8 => "DsdU8", |
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.
For consistency, change into lowercase dsdu8.
| /// `f64` with a valid range of `-1.0..=1.0` with `0.0` being the origin. | ||
| F64, | ||
|
|
||
| /// DSD stream (U8) |
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.
For consistency I suggest /// DSD 1-bit stream in u8 container (8 samples per byte) with 0x69 being the silence pattern.
No description provided.