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

ao: don't call driver->set_paused after reset #14096

Merged
merged 1 commit into from May 20, 2024

Conversation

ruihe774
Copy link
Contributor

@ruihe774 ruihe774 commented May 9, 2024

This commit adds a state hw_paused for pull-based AO. driver->set_paused(false) is only called if hw_paused is true. hw_paused is cleared after ao_reset, so set_paused will not be called after a reset; instead, driver->start() will be called, which properly starts the AO.

Fixes #14092.

This commit adds a state `hw_paused` for pull-based AO.
`driver->set_paused(false)` is only called if `hw_paused` is true.
`hw_paused` is cleared after `ao_reset`, so `set_paused` will
not be called after a reset; instead, `driver->start()` will
be called, which properly starts the AO.
Copy link

github-actions bot commented May 9, 2024

Download the artifacts for this pull request:

Windows
macOS

@ruihe774
Copy link
Contributor Author

ruihe774 commented May 9, 2024

@low-batt Could you confirm the fix?

@ruihe774
Copy link
Contributor Author

ruihe774 commented May 9, 2024

@kasper93 BTW, I wonder if we can instead use a recursive mutex to avoid the split code path for pull-based and push-based AO.

@low-batt
Copy link
Contributor

low-batt commented May 9, 2024

Confirmed fixed.

I patched buffer.c with the proposed change. Rebuilt mpv and tested. I tried hard, but was not able to reproduce the problem with the fix applied.

@na-na-hi
Copy link
Contributor

na-na-hi commented May 9, 2024

I wonder if we can instead use a recursive mutex to avoid the split code path for pull-based and push-based AO.

I prefer to not introduce new recursive mutex usage. I removed most recursive mutex in mpv code not long ago in #13818.

Not requiring recursive mutex means mpv can use SRW lock on Windows, which has lower overhead for updating lock state. This could also be true for other platforms.

@ruihe774
Copy link
Contributor Author

ruihe774 commented May 9, 2024

I wonder if we can instead use a recursive mutex to avoid the split code path for pull-based and push-based AO.

I prefer to not introduce new recursive mutex usage. I removed most recursive mutex in mpv code not long ago in #13818.

Not requiring recursive mutex means mpv can use SRW lock on Windows, which has lower overhead for updating lock state. This could also be true for other platforms.

You have reminded me. PThreads mutex on macOS, IIRC, has strong fairness and bad performance. It immediately parks the thread on contention. Maybe we can instead use an unfair and high-performance alternative on macOS. @Akemi What do you think of it?

FYI: https://hacks.mozilla.org/2022/10/improving-firefox-responsiveness-on-macos/

@Akemi
Copy link
Member

Akemi commented May 9, 2024

in general, if there is a (performance) problem because of a platform independent library/code path/etc that could possibly be fixed with a platform dependent implementation, it might be worth investigating and using it.

does the ff blog post suggest using private APIs and flags? then no, we have a policy to not use any private APIs.

So how do you use them? Well, it turns out they’re not documented. They rely on a non-public function and flags which I had to duplicate in Firefox.

The function is os_unfair_lock_with_options() and the options I used are OS_UNFAIR_LOCK_DATA_SYNCHRONIZATION and OS_UNFAIR_LOCK_ADAPTIVE_SPIN.

@ruihe774
Copy link
Contributor Author

ruihe774 commented May 9, 2024

does the ff blog post suggest using private APIs and flags?

I'm afraid so.

ff uses a userspace workaround as fallback (https://bugzilla.mozilla.org/show_bug.cgi?id=1784018)

@low-batt
Copy link
Contributor

low-batt commented May 9, 2024

For the record, this is the up to date link to the os_unfair_lock_lock_with_options lock method in Apple's open source for macOS 14.4:
https://github.com/apple-oss-distributions/libplatform/blob/a00a4cc36da2110578bcf3b8eeeeb93dcc7f4e11/private/os/lock_private.h#L309-L324

It is in the private section of libplatform in the file lock_private.h.

@kasper93 kasper93 merged commit 4d03efb into mpv-player:master May 20, 2024
17 checks passed
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.

Playback becomes stuck using avfoundation audio driver
5 participants