Skip to content

Conversation

@ljufa
Copy link

@ljufa ljufa commented Dec 17, 2025

No description provided.

Copy link
Member

@roderickvd roderickvd left a 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),
Copy link
Member

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)

Copy link
Author

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?

Copy link
Member

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),

@ljufa
Copy link
Author

ljufa commented Dec 20, 2025

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 😄

Thank you for you review! I am not alsa expert but I've tested it in my rsplayer repo.
I hope that change is not too specific for my use case and it will be generaly useful.

@ljufa ljufa marked this pull request as ready for review December 20, 2025 22:47
Copy link
Member

@roderickvd roderickvd left a 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:

  1. Please add a changelog entry.
  2. In alsa/mod.rs we 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);
}
  1. 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),
Copy link
Member

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,
Copy link
Member

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",
Copy link
Member

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)
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants