Math Errors in alsa backend?

[JasonLG1979]

Here the intention seems to be to find the period size:

    let mut period_size = ((SAMPLES_PER_SECOND * format.size() as u32) as f32
        * (BUFFERED_LATENCY.as_secs_f32() / BUFFERED_PERIODS as f32))
        as Frames;

Putting this after it yields Period Size: 5512 for S16:

println!("Period Size: {}", period_size as usize);

But it yields 8268 for S24_3, 11025 for S24 and 11025 for S32.

The scale used seems to be 1 byte is assumed to be 2756 for some reason? At least that's what the above equation works out to be.

S16 = 2 bytes, 2 * 2756 = 5512, S24_3 = 3 bytes, 3 * 2756 = 8268, S24 and S32 = 4 bytes, 4 * 2756 = 11025.

An ALSA Frame = a sample, so 44100 audio has 44100 Frames per sec, it has nothing to do with the format, given the same sampling rate it should not change. If you want a period time of 125ms that would mean 5512.5 (44100/8) (ALSA will round that up to 5513) so the math kinda checks out for S16 but it should not grow as you up the format.

The end result is that the period_size and buffer_size (as per cat /proc/asound/card*/pcm*p/sub*/hw_params) are not set to correct values on formats other than S16 and self.buffer in alsa.rs ends up being up to twice the size as it really needs to be the wrong size.

I have a wip branch I'm working on that I think is a better solution. @roderickvd if you could take a look at it and tell me that you think that would be great?

[JasonLG1979]

cat /proc/asound/card*/pcm*p/sub*/hw_params Should look like this given a buffer consisting of 4 125 ms periods with a sampling rate of 44100 regardless of format.

access: RW_INTERLEAVED
format: S32_LE
subformat: STD
channels: 2
rate: 44100 (44100/1)
period_size: 5513
buffer_size: 22052

The above period_size and buffer_size values are in Frames.

Currently it looks like this with S32 for example:

access: RW_INTERLEAVED
format: S32_LE
subformat: STD
channels: 2
rate: 44100 (44100/1)
period_size: 11026
buffer_size: 44104

[roderickvd]

The old code wasn't perfect and the comment did have a slight error as well as setting the latency 4x too high... (it's the buffer size that matters there, rather than the period size, right?) so maybe that confused things when it all got changed.

Yes the error is mine, and that is probably what got me on the wrong foot.

@JasonLG1979 I just read through your code, will you open a PR? Checking the period size is a nice improvement too. As well as no longer requiring the Frames struct.

Only question I have is to change PERIOD_TIME into a Duration::from_millis() and later PERIOD_TIME.as_micros() as u32. As we discussed in the other PR, we're trying to make all the time constants a bit more Rust idiomatic. Thanks!

[JasonLG1979]

I can do that. I used u32 because it was easier to wrap my non-corroded mind around (see what I did there,lol?) I will open a PR when I get it a presentable state.

[JasonLG1979]

@kingosticks:

it's the buffer size that matters there, rather than the period size, right?

The buffer size is the total audio latency in Frames. Latency for playback is not really a big deal so long as it's under 1 sec (we shoot for about 0.5 sec). Network latency is more of a wildcard and maybe larger then the audio latency. A really high playback latency would just mean you might notice a delay when manually changing tracks or pausing and un-pausing.

Period size is the size of the periods, basically chunks of the buffer. The buffer is basically a segmented ring buffer. Period size is buffer size / periods. Not enough periods and you risk buffer underruns, to many periods and you hammer the CPU. Each period is an interrupt. 4 periods is the default for ALSA and seems to be what everyone uses as it seems like a happy medium between not getting underruns and not hammering the CPU.

[JasonLG1979]

@roderickvd:

Checking the period size is a nice improvement too

Especially since period_size, buffer_size, buffer_time, period_time, and periods can all be manually forced in /etc/asound.conf.

[JasonLG1979]

Only question I have is to change PERIOD_TIME into a Duration::from_millis() and later PERIOD_TIME.as_micros() as u32. As we discussed in the other PR, we're trying to make all the time constants a bit more Rust idiomatic. Thanks!

Done.

[roderickvd]

Quick reply here while I’m gardening 😄

I think you might be onto something here. One thing that crosses my mind is that in Rust Alsa, the Frames type is defined as a c_long which is i32 or i64 depending on the platform. How do you see this factoring in?

In the end the buffer should be big enough to hold the number of bytes that we’re writing.

[JasonLG1979]

I think that usize makes sense for format size maybe? But not Frames. I think maybe that's one reason for the whole Frames abstraction? Any place in alsa.rs that takes plain number takes a u32? I don't know yet? I'll have to look at how big self.buffer actually is?

[roderickvd]

Yeah I thought Frames was pretty hawkish too just to mimic whatever isize is, when in fact you're not dealing with Frames at all. On my RPi I first changed it to some integer too, then saw that the CI failed on 64-bit hosts. I think your code fixes this because it no longer uses hwp.set_period_size_near. That function took Frames but the ones you're using now are using u32, much saner.

[JasonLG1979]

I just checked and the buffer size is 44096 bytes for S16 (if I have the unit correct?) in my code. To me that would be about right for a mono file for a full buffer (500ms) but I think that we might need to * NUM_CHANNELS?

Something like this maybe?

Edit: periods is already u32.

(period_frames as u32 * periods * NUM_CHANNELS as u32) as usize * format.size()

[JasonLG1979]

the Frames type is defined as a c_long which is i32 or i64 depending on the platform. How do you see this factoring in?

My type conversion foo is not the greatest but I can't imagine frames being negative or being larger than u32 I think it's pretty safe to convert period_frames u32 as periods is already a u32?

[roderickvd]

the Frames type is defined as a c_long which is i32 or i64 depending on the platform. How do you see this factoring in?

My type conversion foo is not the greatest but I can't imagine frames being negative or being larger than u32 I think it's pretty safe to convert period_frames u32 as periods is already a u32?

Casting behavior well-defined in Rust: https://doc.rust-lang.org/nomicon/casts.html
In this situation I agree that it should be safe.

[JasonLG1979]

Dealing strictly in periods and time also opens up the possibility for a --buffer-size option in secs, 0.5 being the default? Although I can see it being used incorrectly. As people have it in their head that a really small buffer means really low latency when really low latency really doesn't matter that much for playback and can actually lead to more issues than it would solve if you go below 0.5.

[roderickvd]

I think we should keep this KISS and let people override it in their backend, if so supported.

[JasonLG1979]

Thinking about it I tend to agree.

[kingosticks]

Agreed. People are obsessed with trying to minimise buffers like these and normally they have no idea what they are doing. Plus, less user options is better.

[roderickvd]

Closing this in favor of #789.