ao_coreaudio: stop audio unit after timeout to allow sleep

[lhc70000]

This commit attempts to fix the problem that mpv prevents sleep on Mac while paused (#11617). The problem also affects libmpv clients.

This problem was introduced by commit 39f7f83, where AudioOutputUnitStop was changed to AudioUnitReset to prevent Bluetooth audio lag during seeking. However, coreaudiod keeps putting an assertion to prevent sleep when the audio unit is active.

The proposed solution is to call AudioOutputUnitStop after a timeout. If the audio unit is reset and not restarted during the timeout, it indicates that the user paused the playback (rather than seeking), and sleep should be allowed.

A serial dispatch queue is created to reset, start and stop the audio unit to avoid race conditions. start, reset, and uninit will run synchronously on this queue. check_stop will run asynchronously on this queue, after the timeout. The ao is put into the queue's metadata and removed on unit, so the scheduled check_stop tasks will know if ao still exists.

Thanks @low-batt for testing this patch.

[low-batt]

I have built mpv with the proposed changes and tested the fix.

Using this command to monitor powerd assertions:

while true; do pmset -g assertions | grep 'coreaudiod\|mpv' | grep -v grep; sleep 1; done

Shows mpv releases its assertion the moment playback is paused and coreaudiod now releases the assertion it created on behalf of mpv after a few seconds. In the following Terminal session output stopped at the point where you see me interrupting the command:

low-batt@gag ~$ while true; do pmset -g assertions | grep 'coreaudiod\|mpv' | grep -v grep; sleep 1; done
   pid 9037(mpv): [0x000767b500059117] 00:00:00 PreventUserIdleDisplaySleep named: "io.mpv.video_playing_back"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:00 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
   pid 9037(mpv): [0x000767b500059117] 00:00:01 PreventUserIdleDisplaySleep named: "io.mpv.video_playing_back"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:01 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:02 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:03 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:04 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:05 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:06 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
   pid 190(coreaudiod): [0x000767b500019111] 00:00:07 PreventUserIdleSystemSleep named: "com.apple.audio.BuiltInSpeakerDevice.context.preventuseridlesleep"  
^C
low-batt@gag ~$

Monitoring coreaudiod CPU usage shows it going back to zero with this fix.

Adapting the command to monitor the behavior of QuickTime when pausing playback also shows a similar pattern where coreaudiod releases its assertion on behalf of QuickTime after several seconds has passed, suggesting QuickTime is delaying calling stop.

@LiJChang, @sfan5 Thoughts on issue #11617 and this proposed fix?

[krackers]

With the implementation, could there be an issue in the following sequence of events?

• Pause, starting reset timer
• Unpause
• Track ends. Ao uninit called.
• New track begins. Start called. Value of start_counts is 1.
• Reset timer fires, --start_counts becomes zero, so AO is erroneously stopped

Using 1 static dispatch queue instead of creating a new dispatch queue per AO-init would help avoid this, since then the start_count state is maintained across track boundary. Alternatively need to check not only whether the ao key is set, but also that it hasn't changed since block enqueue. Or maybe another way is using a dispatch timer source, which can be cancelled when uniniting.

[low-batt]

@krackers Glad you are thinking about such scenarios and asking questions.

We will have to see that the mpv developers who really know this code think. Here is what I believe will happen in the described scenario…

The uninit method in ao_coreaudio.c is called by the ao_uninit method in buffer.c and as seen here, it frees the ao struct. If there are any check_stop tasks in the queue then the queue will remain around until the last of those has run. Those tasks will try and get a pointer to the ao struct from the queue's context dictionary. They will find uninit has set the value of the key to be NULL and the tasks will immediately finish. Once the last has finished the queue will be freed.

The next track will get a fresh ao and its own queue. I don't think there is any problem with stale information being carried over?

The data must be kept separate as when libmpv is being used more than one file can be opened and played at the same time.

Kept thinking about how to break this code and asking questions!

[krackers]

Yes you are right, thank you for the explanation! I misunderstood how dispatch_queue_set_specific worked.

[low-batt]

I'm happy to explain things. Questioning the code like this is exactly what should happen during review. I really appreciate you taking the time to read and think about the proposed changes. Any other questions, speak up!

[orion1vi]

I believe debouncing using dispatch work item would be cleaner: orion1vi@6240364, if it must be serialized: orion1vi@4fa23cf.

[low-batt]

I agree, canceling a dispatch block is cleaner. Serialization is still required to properly coordinate with the queue'd block.

[krackers]

Note dispatch_block_cancel was only added in 10.10, but mpv claims to support down to 10.8. I agree it looks much cleaner though. So probably also bump minimum version to 10.10, or surround the debounce with (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_10) so we can still compile on older version?

This will impact e.g. macports version of mpv: https://ports.macports.org/port/mpv/details/ which enables CoreAudio for anything >= 10.6.

[low-batt]

@krackers Thanks for pointing that out.

Always important to remember that as a result of the high cost of Apple hardware there are users who are forced to use old versions of macOS due to their ancient hardware. A list of vintage and obsolete Macs can be found here.

I've been confused about mpv's system requirements for macOS ever since I read a response to an issue that indicated mpv was following Apple's convention of only supporting the 3 latest versions of macOS. I've heard that is Apple's policy, but I've been unable to find an actual statement from Apple on that. Most Apple web site pages only list macOS Ventura.

The Apple page Find out which macOS your Mac is using still has a link for macOS Catalina (10.15). The Apple page Apple security updates shows the last security update for Catalina was on 20 Jul 2022. After that date macOS Big Sur is the oldest macOS release listed.

Officially mpv's System requirements says this:

• A not too ancient Linux, Windows 7 or later, or OSX 10.8 or later.

Is that out of date?

The current release of IINA has a minimum requirement of OS X El Capitan (10.11).

[krackers]

OSX 10.8 or later.
Is that out of date?

It probably might be, although it's not strictly wrong. I recall

diff --git a/osdep/macosx_menubar.m b/osdep/macosx_menubar.m
index c424df255c6..b51b3738e78 100644
--- a/osdep/macosx_menubar.m
+++ b/osdep/macosx_menubar.m
@@ -715,7 +715,7 @@ - (void)openPlaylist
 
     if ([panel runModal] == NSModalResponseOK){
         NSString *pl = [NSString stringWithFormat:@"loadlist \"%@\"",
-                                                  [panel URLs][0].path];
+                                                  [[panel URLs][0] path]];
         [(Application *)NSApp queueCommand:(char *)[pl UTF8String]];
     }
 }

was the only change needed to get 0.31 to compile on 10.8 (since I think generics were not supported in that version of objective-c, so compiler cannot infer type of array element). And if using X11 instead of Cocoa for windowing, then even that change is not necessary.

But since mpv developers do not officially support the legacy vo=opengl backend on OSX anymore, and vo=libmpv requires swift, maybe it is ok to bump the minimum to 10.10? Macports could patch on their end to support lower versions (like they already do for 10.6).

[Akemi]

we or rather i don't go out of my way to support anything other than the latest 3 macOS versions. that doesn't mean i proactively remove support for older versions. i only remove old things if they are in the way.

with that being said, this should not be confused with mpv's system requirements. this only really has implications for bug reports. if the bug occurs on older macOS versions only but not on the latest three versions, we/i won't invest any time in fixing or circumventing it. hence only problems/bugs are relevant that are reproducible on the latest 3 versions.

i think mpv's lowest tested macOS target (by me) was 10.10 and that should still work.

on a side note, the menubar is an optional feature (can be deactivated) that is not needed for mpv's main purpose, and if this is really the only place that needs adjustments for 10.8 than 10.8 is still supported and the readme correct.

[orion1vi]

In that case, dispatch source timer could be used: orion1vi@19a63b1, if timer is reused: orion1vi@85735b2.
Though, just noticed that DISPATCH_TIMER_STRICT is available from 10.9, which can be removed in this case anyway.

[krackers]

Dispatch timer approach LGTM, and seems just as clean as the block_cancel approach. DISPATCH_TIMER_STRICT is likely not needed on < 10.9 anyway since I believe it was only needed with introduction of timer coalescing in 10.9, so can be ifdef'd to 0 on older platforms.

[Akemi]

sorry it took me so long. we should finally get a fix for this merged.

we recently raised the minimum macOS version to 10.15. so we don't need the dispatch source time approach from here #11667 (comment) anymore?

i would say, the cleanest way to deal with this is the second debouncing serialised approach from here #11667 (comment)

@orion1vi could you open a PR for this please? would also be nice to have some explanation of the problem, the source of the problem and the fix in the commit body, similar to the commit here.

[low-batt]

I'm glad to hear this issue will be getting some attention. I too was expecting that with the minimum supported macOS version having been raised to macOS Catalina the commit should be updated.

IINA considered blocking sleep to be a critical problem and applied the changes in this PR when building libmpv. No problems have been reported by users regarding those changes.

As this is touching audio on the Mac, note that IINA is currently unable to function properly with libmpv built from the master branch due to the audio problem in issue #13348. Hopefully that can be fixed as well.

[low-batt]

@Akemi Hopefully we will hear from @orion1vi, but if not you can ask me to prepare the PR you requested.

[Akemi]

lets wait some time (~1week) for them to react/reply.

[low-batt]

Yes, waiting sounds good. Just letting you know I'm willing to help if needed.

[Akemi]

@low-batt i will take you up on the offer. would be nice if you could prepare the PR and preserve the authorship.

[low-batt]

I should have time to prepare a PR tomorrow. I will preserve the authorship.

[low-batt]

I'm in the middle of preparing the new pull request and I have a question about the code. I've pulled this commit from @orion1vi and there is a change compared to the commit in this PR.

The new commit uses dispatch_async_f:

static void reset(struct ao *ao)
{
    struct priv *p = ao->priv;
    dispatch_async_f(p->queue, ao, &_reset);
}

Whereas the commit in this PR uses dispatch_sync_f so the operation is completed before returning to the caller:

static void reset(struct ao *ao)
{
    struct priv *p = ao->priv;
    dispatch_sync_f(p->queue, p->queue, reset_p);
}

The start method also uses dispatch_async_f instead of dispatch_sync_f.

I'm worried the calling code expects the operation to be completed when the reset and start methods return and this represents a thread race condition.

Is it ok for these methods to asynchronously perform the operations?

[low-batt]

I've created PR #13663. I did not want to make any code changes without others approving, so I did not change the code other than to add a couple of comments. If these operations need to be synchronous let me know and I will update that PR.

[Akemi]

closing in favour of #13663.