pulseaudio: Make use of suggestedLatency stream parameter.

[kleinerm]

This will translate the client provided suggestedLatency in seconds into microseconds and pass them to Pulseaudio, so the client has some control over required latency.

Invalid values < 1 microseconds get mapped to the default minimum latency, currently 10 msecs.

This replaces the previous hard-coded latency of ~2 msecs, which turned out to be too low on some tested mid-range systems and caused audio buffer underruns and audio artifacts.

[illuusio]

As look better than hard coding why 1e-6? There should be some comment why this is done this way. I'll test these and comment code after that more.

[kleinerm]

Actually, testing further on Ubuntu 20.04-LTS and 22.04-LTS on two machines, and also looking at pulseaudio server source code, and into pulseaudio log output for higher verbosity levels confirms that passing in a value 0 microseconds is also fine. Too small values get clamped to workable values by the server. Obviously very tiny values could cause audio underruns. The pulseaudio server detects too many underruns and automatically increases latency in that case.

So I simplified the code to only use default minimum latency of 10 msecs if a suggestedLatency < 0 is provided by the client. Also added a comment.

[illuusio]

Changes looks fine. I'll test with Pipewire as it's kind of new normal for Pulseaudio.

[RossBencina]

@illuusio let us know when you'd like us to merge it

[philburk]

why 1e-6?

I like using the exponential notation because it avoid bugs caused by miscounting zeros.

Which of there are wrong? NANOS_PER_SECOND = 100000000; NANOS_PER_SECOND = 1e8;

[illuusio]

Ok I've test this it does not make anything worse. Now it works as expected when one changes latency. As I said in review comment I don't know should zero be handled as it's now? What do you think @philburk?

[RossBencina]

Ok I've test this it does not make anything worse. Now it works as expected when one changes latency. As I said in review comment I don't know should zero be handled as it's now?

@illuusio I think better to clamp to 0, i.e.:

        if (inputParameters->suggestedLatency > 0.0)
        {
            stream->suggestedLatencyUSecs = (unsigned int) (inputParameters->suggestedLatency * 1e6 + 0.5f);
        }
        else
        {
            stream->suggestedLatencyUSecs = 0;
        }

It seems that the <0 case is not handled correctly in other host APIs. I have filed a bug against the documentation and other host APIs: #856

[philburk]

What do you think @philburk?

Ross and I discussed this at length. I agree with Ross's comments above.

[illuusio]

@kleinerm are you willing to update this or should I just commit the change.

[kleinerm]

I agree to the change, but away from my computer atm. I can update it in a couple of hours when I'm back at the keyboard.

In principle we could also add 1.0 instead of 0.5 in the regular if-branch case to always choose a latency at least as high as suggestedlatency. This would be in line with some suggestions on the portaudio wiki. In practice it won't make any difference, just for style bonus points.

Pulseaudio will clamp too low values to the smallest supportable value - not the lowest glitch free value though.

What could make sense in pa_front is to reject any suggestedlatency < 0 with a paInvalidparameter error. Most hostapis seem to do very undefined stuff otherwise as far as I could see. But that's a topic for a separate commit...

[kleinerm]

Ok, done. Btw. I'm not sure if also some build config files or such need to be rebuild? Looking at the CI logs, I don't think the CI currently builds the new pulseaudio hostapi?

[illuusio]

Ok, done. Btw. I'm not sure if also some build config files or such need to be rebuild? Looking at the CI logs, I don't think the CI currently builds the new pulseaudio hostapi?

That should be fixed in another PR as it's github runner stuff. I'll think I'll merge this.

[illuusio]

Please close review stuff and I'll merge this.

[kleinerm]

@illuusio @RossBencina I think all review comments have been addressed in the current pull request, and the code been tested to work properly, so this should be ready for merge.