use set_buffer_size_near to calculate fixed alsa buffer size

[attackgoat]

Fixes #743
Closes #917

Hardware parameters cannot be set to some arbitrary values within the supported range.

Two alternate fixes are:

• Expose the "near" ALSA function to the user
• Return the actual buffer size used

I am not familiar enough with CPAL usage to know if the alternate fixes are needed, but for my use cases they are not.

[roderickvd]

Agreed, this is the right thing to do. This highly related commit in librespot actually combines the two approaches you mention: librespot-org/librespot@d5efb8a

[roderickvd]

@attackgoat as I became a maintainer today, and agree with your approach, I would like to work with you to getting to a mergable state. Could you update it to:

• take the device default when the "near" function fails? (look at the librespot example for inspiration)
• add a changelog entry

Thanks!

[roderickvd]

I see that #917 also does this (and more).

[attackgoat]

I see that #917 also does this (and more).

That PR seems to be the better direction, although it causes me the same issue with large buffers. For now I've addressed the feedback but we may want to close this PR if #917 is cleaned up.

[roderickvd]

Thanks a lot @attackgoat. The author of #917 expressed that he is no longer working on it. Would you be willing to merge the two approaches?

[roderickvd]

Thanks a lot, this is really going in the right direction. Some points to make it even more robust - I've got prior experience where I know that ALSA drivers can be really fickle.

At any time just let me know if and how I can help.

[roderickvd]

Well implemented with one final bit of polish! 👏

[roderickvd]

As final polish this would be even more resilient:

// Try to set desired period size
let period_size = if let Ok(size) = hw_params.set_period_size_near(period_size as _, alsa::ValueOr::Greater) {
    size  // Use the size we successfully set
} else {
    // If setting fails, try to get whatever period size the device currently has
    if let Ok(current_size) = hw_params.get_period_size() {
        current_size  // Work with device's current period size
    } else {
        return false;  // Only fail if we can't even query the current size
    }
};

[roderickvd]

Actually, though I marked the review as “changes requested”, keen to know what you think. It has the upside of pretty much always working, but the downside of potentially ignoring the user’s request.

[attackgoat]

This was been incorporated in b710e04, but after rethinking it I don't think this check is required.

Reasoning: The sub-function set_hw_params_buffer_size takes a buffer size and attempts to fit the desired number of periods. If this fails, the fallback in the outer function already gets the period size and attempts to set valid parameters using a period-time approach, thereby handling the proposed check.

Also:

• I realized snd_pcm_hw_params_set_periods was only being called in the absolute fallback case, so the desired count of two was never actually respected (fixed)
• It's possible to make set_hw_params_periods infallible: if it fails it is already ignored anyways (not fixed)

[roderickvd]

I've been thinking about this as well - thanks for sticking through this difficult undertaking of getting the period / buffer system right.

We need to have a behavior contract that's clear to what a user would reasonable expect:

• BufferSize::Fixed(n) → "I need around n frames, fail if you can't get close"
• BufferSize::Default → "Give me something reasonable for this hardware"

Right now we might be too accommodating for Fixed(n) and succeed even when we request 128 frames and fall back to 2048. By analogy, we also fail opening a 48 kHz stream when a device only supports 44.1 or 96 kHz.

Should be easy to change, right?

  if let BufferSize::Fixed(val) = buffer_size {
      if set_hw_params_buffer_size(hw_params, period_count, val) {
          return true;
      }
      // Currently falls through to fallback
  }

⬇️

  if let BufferSize::Fixed(val) = buffer_size {
      if set_hw_params_buffer_size(hw_params, period_count, val) {
          return true;
      }
      return false; // Fail immediately - don't use fallback
  }

[attackgoat]

Sounds good - the code has been adjusted to return BackendSpecificError in case it cannot set the requested buffer size (default or fixed).

In the future I think it would be helpful to return a better error type that programs could inspect and use to make decisions, because the string-based error is only helpful for the programmer. The log crate would be handy here.

[roderickvd]

Thanks for sticking through this difficult undertaking of getting the period / buffer system right.

We need to have a behavior contract that's clear to what a user would reasonable expect:

BufferSize::Fixed(n) → "I need around n frames, fail if you can't get close"
BufferSize::Default → "Give me something reasonable for this hardware"

There's a one-line proposal for that in the comments.

[roderickvd]

I've been thinking about this as well - thanks for sticking through this difficult undertaking of getting the period / buffer system right.

We need to have a behavior contract that's clear to what a user would reasonable expect:

• BufferSize::Fixed(n) → "I need around n frames, fail if you can't get close"
• BufferSize::Default → "Give me something reasonable for this hardware"

Right now we might be too accommodating for Fixed(n) and succeed even when we request 128 frames and fall back to 2048. By analogy, we also fail opening a 48 kHz stream when a device only supports 44.1 or 96 kHz.

Should be easy to change, right?

  if let BufferSize::Fixed(val) = buffer_size {
      if set_hw_params_buffer_size(hw_params, period_count, val) {
          return true;
      }
      // Currently falls through to fallback
  }

⬇️

  if let BufferSize::Fixed(val) = buffer_size {
      if set_hw_params_buffer_size(hw_params, period_count, val) {
          return true;
      }
      return false; // Fail immediately - don't use fallback
  }

[roderickvd]

Great. Going to merge this now. If you've got further improvements, we can add them later. This already is a big improvement, thanks so much.