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

Disconnect from pulse temporarily while there are no chunks available #931

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

nkvoll
Copy link
Contributor

@nkvoll nkvoll commented Oct 19, 2021

Disconnect from pulseaudio when no chunk has been received for 5000 ms. (same as the alsa player).

When new chunks arrive, reconnect to pulseaudio and resume playing.

This enables the pulseaudio sink to go into an idle/suspended state for powersaving, and saves cpu cycles on the snapclient when no sound is being played.

Closes: #927

@nkvoll
Copy link
Contributor Author

nkvoll commented Oct 19, 2021

Currently it breaks when changing volume when pa is not is connected (if using --mixer hardware):

Assertion 's' failed at pulse/stream.c:349, function pa_stream_get_index(). Aborting.

This needs to be protected with a mutex to avoid races with concurrent connects/disconnects.
@nkvoll
Copy link
Contributor Author

nkvoll commented Oct 20, 2021

This should be fixed now :) Since the volume is set on the sink-input, which doesn't exist when we're disconnected, setting it when we start receiving chunks again seems like a fair compromise?

I do wish the disconnection timeout could be configurable, but I don't see an easy way to provide players with custom options in the current codebase. Maybe something like --player-opts=query-string-like some time in the future?

@tmigone
Copy link

tmigone commented Oct 20, 2021

This is great! Hope it gets merged soon. Thanks

@badaix
Copy link
Owner

badaix commented Oct 21, 2021

Hi, thanks, I will unfortunately not be available for review before the 2nd of November.

@nkvoll
Copy link
Contributor Author

nkvoll commented Oct 21, 2021

No hurry on the PR review 👍 In the meanwhile I will be testing the branch in my own setup.

@nickalcock
Copy link

Seems to work for me as well, fwiw. Kind of, anyway. The output is still deeply unpleasant: see issue #942. (But this is not related to this pull: it's observable with trunk and 0.25 as well.)

@sibowler
Copy link

sibowler commented Dec 1, 2021

Hi @badaix - Any thoughts of when you might get a chance to review/roll this in. I've also just moved to Pulse and hitting this issue. Apart from the ongoing logging, i found that if you stop a source whilst it's playing via pulse, the speakers will then repeat the last 0.2/0.5 secs ongoing as a kind of stutter/stuck-record type sound, which I believe is related to not closing pulse.

Copy link
Owner

@badaix badaix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, please check my comment

@badaix badaix merged commit d9bdd1b into badaix:develop Dec 9, 2021
@badaix
Copy link
Owner

badaix commented Dec 9, 2021

Thanks!

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.

5 participants