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

Workaround for #1064 #1172

Closed
wants to merge 2 commits into from
Closed

Workaround for #1064 #1172

wants to merge 2 commits into from

Conversation

erfan-khadem
Copy link

@erfan-khadem erfan-khadem commented Feb 16, 2022

I noticed that from probably PipeWire 0.3.34 up to now, cmus started taking a long time before exiting when the music is paused (#1064), otherwise it has no issue. It seems like disabling _pa_stream_drain on PipeWire when exiting cmus fixes the issue and has no apparent setbacks. This seems to be an issue with upstream (as it was once fixed there), but given the simplicity of the workaround, we can have this until upstream fixes the issue.

@erfan-khadem erfan-khadem changed the title Workaround for #1126 Workaround for #1064 Feb 16, 2022
Copy link
Contributor

@gavtroy gavtroy left a comment

Choose a reason for hiding this comment

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

Sure, since it's not obvious what purpose _pa_stream_drain() serves in the first place I wouldn't object to adding this workaround.

op/pulse.c Outdated
if (is_pipewire) {
return OP_ERROR_SUCCESS;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this could be removed as you also guard calling the function.

op/pulse.c Show resolved Hide resolved
@erfan-khadem
Copy link
Author

erfan-khadem commented Jun 26, 2022

Thanks for you review @gavtroy. I'm really busy until Thursday. I will most certainly fix all of the issues you mentioned by the end of the week though.

Update:
I managed to squeeze this in.

@philippemilink
Copy link
Contributor

This patch works fine and I pushed it in the Debian cmus package.

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Sep 6, 2022
ologantr added a commit to ologantr/void-packages that referenced this pull request Sep 19, 2022
This patch fixes cmus freezing on exit using
both cmus-pulseaudio and pipewire-pulse.

Original bug report: cmus/cmus#1184
Patch from: cmus/cmus#1172
Debian: https://salsa.debian.org/multimedia-team/cmus/-/commit/2e20a489bc58a604a0093830d71e43a5ac2b6c51
if (i) {
if (strstr(i->server_name, "PipeWire") != NULL) {
// server is PipeWire
d_print("Pulseaudio server is pipewire. Disabling _pa_stream_drain()\n");
Copy link
Member

Choose a reason for hiding this comment

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

Is there any upstream issue we can reference here, so that we know when we can drop the workaround?

Copy link
Author

Choose a reason for hiding this comment

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

Hi. I really can't investigate this issue right now. I live in Iran and ... nothing good is going on right now. A few weeks ago the police was in our university (Sharif University of Technology. Go look at its wikipedia page) shooting students and beating faculty members (yeah our own professors) and arresting people and ... I just need some time to deal with this shit.

Choose a reason for hiding this comment

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

take care :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Hope all is well.

I don't think there is an existing upstream report other than the original pipewire/-/issues/946, but the change that broke it again (pipewire/-/commit/b9c8f0f6b9) says it's expected behavior:

A corked stream should not complete a pending drain operation

I experimented with uncorking the stream first and it also solves the issue, but just removing the call makes more sense to me. It doesn't seem like we ever had any reason to wait for the stream to drain in the first place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been working on a pipewire op (it's mostly done, but I need to rework the op api to allow for pull-based output plugins to prevent it from being unnecessarily complex and inefficient) and an aaudio one, and I think removing the drain makes sense too.

Copy link
Member

Choose a reason for hiding this comment

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

So, let's go with #1268?

flyingmutant added a commit that referenced this pull request Jul 15, 2023
flyingmutant added a commit that referenced this pull request Jul 15, 2023
@pgaskin pgaskin closed this Jul 15, 2023
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.

None yet

6 participants