Conversation
roderickvd
left a comment
There was a problem hiding this comment.
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 😄
src/host/alsa/mod.rs
Outdated
| 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.
This should probably be fill_typed!(DsdU8)
There was a problem hiding this comment.
now when I removed structs I am not sure what to use here. any suggestion?
There was a problem hiding this comment.
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.
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
}
}
src/host/alsa/mod.rs
Outdated
| 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.
Something like this?
const DSD_SILENCE_BYTE: u8 = 0x69;
SampleFormat::DsdU8 | SampleFormat::DsdU16 | SampleFormat::DsdU32 => buffer.fill(DSD_SILENCE_BYTE),
src/samples_formats.rs
Outdated
| SampleFormat::U64 => u64::BITS, | ||
| SampleFormat::F32 => 32, | ||
| SampleFormat::F64 => 64, | ||
| SampleFormat::DsdU8 => 1, |
There was a problem hiding this comment.
As a matter of style we could reduce this to SampleFormat::DsdU8 | SampleFormat::DsdU16 | SampleFormat::DsdU32 => 1.
src/samples_formats.rs
Outdated
| SampleFormat::U64 => "u64", | ||
| SampleFormat::F32 => "f32", | ||
| SampleFormat::F64 => "f64", | ||
| SampleFormat::DsdU8 => "DsdU8", |
There was a problem hiding this comment.
For consistency, change into lowercase dsdu8.
src/samples_formats.rs
Outdated
| /// `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.
For consistency I suggest /// DSD 1-bit stream in u8 container (8 samples per byte) with 0x69 being the silence pattern.
|
All suggestions applied. Thank you for your patience! |
roderickvd
left a comment
There was a problem hiding this comment.
No problem. Some hopefully final remarks.
src/samples_formats.rs
Outdated
| | SampleFormat::U32 | ||
| // | SampleFormat::U48 | ||
| | SampleFormat::U64 | ||
| | SampleFormat::DsdU8 |
There was a problem hiding this comment.
DSD is not unsigned integer; these lines should be removed here.
src/samples_formats.rs
Outdated
| /// DSD 1-bit stream in u8 container (8 samples per byte) with 0x69 being the silence pattern. | ||
| DsdU8, | ||
|
|
||
| /// DSD 1-bit stream in u16 container (8 samples per byte) with 0x69 being the silence pattern. |
There was a problem hiding this comment.
I know I suggested this before but how about:
"DSD 1-bit stream in u16 container (16 bits = 16 DSD samples) with 0x69 being the silence byte pattern."
CHANGELOG.md
Outdated
| - **WASAPI**: `Send` and `Sync` implementations to `Stream`. | ||
| - **WebAudio**: `Send` and `Sync` implementations to `Stream`. | ||
| - **WebAudio**: `BufferSize::Fixed` validation against supported range. | ||
| - **ALSA**: Add support native DSD playback |
There was a problem hiding this comment.
Add support for native DSD playback
|
Thank you, merged! |
No description provided.