Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for PulseAudio maxlength. Fixes #54 #90

Merged
merged 5 commits into from
Jun 18, 2022

Conversation

darkstego
Copy link
Contributor

Allows changing PulseAudio stream buffer maxlength. This puts a limit to how much to buffer audio. This is useful in cases of increasing audio delay due to timer mismatch between source and receiver.

I am unsure whether default values for latency and max_latency should be dictated by scream, or if they should be left to pulse to decide defaults. I think the correct way is the latter unless the pulse defaults don't work for the majority of scream use cases.

@duncanthrax
Copy link
Owner

Interesting! What happens if you set max latency to target latency? If I was a user, I'd wonder what the difference is.

@martinellimarco
Copy link
Collaborator

martinellimarco commented May 7, 2020

I've tested it and it works even with some incredibly low values like maxlength = 100 bytes.
Using pa_usec_to_bytes it does work with any value I set, even 1ms (which is translated to 176 bytes)

The only thing I would change is the default 200ms that it's a bit too high, then I think this can be merged.

That said #54 is not fixed yet. While pulseaudio now doesn't buffer more than the allowed amount the ring buffer in the memory map does.

I'm writing a patch for that but it'll not be ready until tomorrow.

@darkstego
Copy link
Contributor Author

I was thinking it was too high, but then again I wanted it to be larger than the target latency default of 50ms which might be high as well. The Pulse default according to documentation is is whatever the maximum size the pulse server accepts. Again, there might need to be a discussion as to whether the defaults values even need to be set or just have PulseAudio handle it if the flags weren't set.

Also, there is a flag PA_STREAM_ADJUST_LATENCY that might be of use here since it takes into account the sink latency as well the playback stream, so the numbers passed represent the overall latency.

I will play around with it some more and try to find a reasonable default.

@martinellimarco
Copy link
Collaborator

PA_STREAM_ADJUST_LATENCY is already set inside pa_simple_new.

You know what? I was forgetting that now the receivers are merged and in the network version a larger buffer may be a good thing.

@darkstego
Copy link
Contributor Author

@duncanthrax You did mention how this would work from a user perspective. To minimize confusion. One could possibly set all these to be the same amount and have one latency number.

@darkstego
Copy link
Contributor Author

Since #91 got merged I wanted to see if this PR could get merged as well to solve the audio latency issues.

@duncanthrax
Copy link
Owner

OK, please fix conflicts and I'll put it in.

@duncanthrax duncanthrax merged commit f32a640 into duncanthrax:master Jun 18, 2022
@duncanthrax
Copy link
Owner

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants