wasapi: implement IMMNotificationClient for device change detection

[0x0003]

Closes #1408
Closes #1880

[MaxKellermann]

I didn't understand much of this because I'm not a Windows guy, but some things looked exceptionally ugly.

[0x0003]

Refactored, should address all of your comments.
No more atomic polling, explicit default device bool (don't know why I didn't think of that initially), exception class instead of virtual in Interface.hxx, removed a bunch of unnecessary logging.
Also removed urls from the commit messages to not spam issues.

[0x0003]

Accidentally pushed fork's master to this branch, sorry for the spam.

[0x0003]

The audio pop fixes should be in a separate PR now that I think about it, sorry about that.

[MaxKellermann]

Hey, don't request another review if you havn't addressed all comments from the other review!

[MaxKellermann]

I'm currently reading #1880 again, and I wonder how your change would be able to solve this problem. You added a try/catch block to WasapiMixer::GetVolume(), but this isn't where the crash of that bug report occurs. Are you really sure that your patch solves this crash bug?

[0x0003]

Oh damn, you are right, it doesn't. It solves crashes when the output is transfered to another device, not when there's nothing to output to.
I did test against this case, but turns out I had audio output transferred to my monitor when headphones and speakers are plugged out! It's always muted, so I didn't catch that.

[MaxKellermann]

So it's a different crash bug?
Your try/catch is in a code location that is allowed to throw. If throwing here crashes MPD, then we have a serious bug in a completely different part of MPD, but certainly not in the Wasapi plugin.

[0x0003]

Wait, no, I take that back. Yes, yes it does solve that crash, from the issue too, along with output transfers (which is really the same as trying to output to nothing, if only for a very brief moment, now that I think about it)
I'd just compiled against a branch that didn't have the changes from this PR (facepalm)

I did not properly test it though, the output was silently going to my monitor without me noticing, so it's a good thing you questioned me. I can confirm this really does work.

[0x0003]

Also, I'm anticipating you comment that #1408 has winmm output, not wasapi :)
My rationale is that winmm has been deprecated for years, it's not even mentioned in the docs on the website https://mpd.readthedocs.io/en/latest/plugins.html#output-plugins
This PR brings wasapi up to speed, at least as far as device switching is concerned.

If you think that it doesn't really solve that issue - and to be fair it doesn't technically - feel free to drop it from the description.

[MaxKellermann]

Yes, yes it does solve that crash, from the issue too

Really? Your fix that adds a try/catch to the main thread fixes a crash in the COM worker thread?

[0x0003]

Yes. Literally just tried again using the same repro steps:

- Start playback on WASAPI output
- Pause playback
- Unplug/deactivate audio device
- Resume playback

I tried with

1. several devices connected, in that case plugging out the device that mpd is currently outputting to, on unpause audio starts flowing to another device which windows assigns the default.
2. one device connected, then disconnected, on unpause mpd doesn't start the playback.

No crashes in both instances.

[MaxKellermann]

And you have the same crash backtrace, i.e. termating from inside COMWorker::Work?

This sounds like an impossible situation.

Wait, you wrote no crashes. But your first commit claims it fixes a crash bug! So what crash are we really talking about?

[0x0003]

Wait, you wrote no crashes.

No crashes with the changes from this PR applied.
Without them, there is a crash, reproducible by following the steps from the linked issue.

I can't get gdb to play well on windows, it says no backtrace after the crash (yes I am running a build with debug symbols), so no stack trace, sorry. The crash is reproducible in the exact same steps though, as I said.

[Inferior 1 (process 6316) exited with code 03] <- crash
(gdb) bt
No stack.

It could very well be that the "wasapi: detect default audio device changes" fixes it, but that shouldn't be the case, I worked on the problems separately and fixed the crash first, that's why they're separate commits... I'll try building without it in a moment.

[MaxKellermann]

It could very well be that the "wasapi: detect default audio device changes" fixes it

Then try with just the first commit. The commit message of your first commit claims it fixes a crash bug, and I want know which one and I want to verify whether that is really correct.

[0x0003]

It crashes with either wasapi: detect default audio device changes or wasapi/mixer: gracefully handle device disconnection. No crash with both, so the fix is partly in both commits, guess I didn't add the correct hunks after refactoring the initial mess.
Want me to squash both commits into one, or fix diffs in two?

Let me do more testing actually.

[MaxKellermann]

Squashing makes no sense here. These are distinct changes, but I doubt that the "mixer" commit makese sense at all.

[0x0003]

I'm thinking so too looking it over again. I'll tackle it later.

[0x0003]

You were right, mixer changes were superficial.
During initial planning, it seemed like the most obvious (to me) place to add error handling, and I never bothered to see if it actually did anything.
Default device switch is literally just 2 line diff without the comment, the rest has to do with crashing on disconnect. This is just a very bad case of me misunderstanding what the code does.

Sorry for wasting your time on reviewing irrelevant code.

[MaxKellermann]

Sorry for wasting your time on reviewing irrelevant code.

But it's not a waste of time if you have learned something useful. I'm happy that you decided to improve MPD! Nothing is simple, just don't give up.

[0x0003]

Found a couple edge cases that have to do with setting explicit device:

Device is set to "1" in MPD config.

- Start with only wired headphones plugged in. This is recognized as device "0" by windows.
Starting MPD playback doesn't output audio to it, which is correct. <- or maybe not so correct, it shouldn't Play() at all!
                                                                    1) stems from it
- Connect bluetooth headset, it's going to be recognized as device "1" by windows.

1) If device "1" was connected while MPD playback is active (e.g. not paused/stopped), it
requires a manual pause/play toggle for audio to start flowing.
2) If device "1" is disconnected while MPD is paused, resuming MPD playback crashes the program.
So same exact pattern as "https://github.com/MusicPlayerDaemon/MPD/issues/1880"

Converting to draft while I investigate.

[0x0003]

I renamed the PR and squashed everything into one commit, since this is really more of an all-encompassing addition of the device change notification support that was completely missing before, that also happens to fix a couple of filed issues, rather than a targeted bugfix.

Think this is the best I can do. Gonna run it for a while, see if there are any quirks, then mark for review.

There is still one super niche issue left, in a FIXME comment in DeviceNotification.hxx. I can't figure out how to solve it, but it's more of an annoyance than anything. Maybe if someone smarter than me comes around they'll be able to fix it.

[MaxKellermann]

fyi, I'll be offline for a week or so, so further review of your PRs will be delayed.