wasapi: prevent audio pops on startup, resume, and seek by 0x0003 · Pull Request #2545 · MusicPlayerDaemon/MPD

0x0003 · 2026-07-15T09:38:23Z

Fixes annoying audio pops in the wasapi output caused by sample waveform discontinuities. Most of them, at least. The remaining ones can only be masked by something like fade in/out, which MPD doesn't have.

Call IAudioClient::Reset() when resuming playback after a pause to flush stale buffered audio and prevent audible pops.

shared mode Stop the IAudioClient before discarding the ring buffer on cancel, then continue back to the wait loop so the hardware gets a full cycle before Restart on the next iteration. The audio will still pop on seek occasionally, but that's a known issue with wasapi itself.

Gate the first Start() on having a full buffer of real audio data in the ring buffer. Push() signals the event during this priming phase so the worker wakes as new data arrives.

MaxKellermann · 2026-07-15T11:43:03Z

+	bool SupportsHardwarePause() const noexcept override {
+		return true;
+	}


I don't understand this change; the commit message doesn't explain it. Does this really have anything to do with the rest of this commit?

Yes, it is needed. Without it Cancel() fires on pause and discards the ring buffer, which makes the Reset fix on resume useless. I did test without this block compiled in, the pops were in.

This sounds like trial'n'error - i.e. you did not understand what you were doing, and randomly changed things until they worked.
So, do you want to discard buffers or not? The other part of this commit deals with clearing buffers, but here you disabled it. This sounds like a contradiction to me, not a proper fix.

MaxKellermann · 2026-07-15T11:45:08Z

+		/* Reset to clear residual state that could
+		   cause stale samples. */
+		if (!started) {
+			Reset(client);
+		}


I don't understand this either. Where did stale data really come from?
And why do we need to call Reset() here (adding overhead to every iteration) and not at the start of the pause state?

"Stale data" is misleading indeed, I'll update the comment so it's clearer.

Reset() must run between Stop() and Start(), not at pause time, because the cancel (seek) path also calls Stop() independently.
This:

- /* stop the IAudioClient while paused; it will - be restarted as soon as we're asked to - resume playback */ + /* stop and reset the IAudioClient while + paused; it will be restarted as soon as + we're asked to resume playback */ Stop(client); + Reset(client); started = false; continue;

creates pops on seek.

Overhead -> it is guarded by if (!started), so it should only run once per pause/play cycle. Unless I'm misunderstanding something?

MaxKellermann · 2026-07-15T11:46:30Z


 		if (cancel.load()) {
+			if (started && !is_exclusive) {
+				Stop(client);


Why? The commit message doesn't say.

In shared mode, after Cancel(), the framework will close and reopen the output, creating a new thread with a fresh IAudioClient. If the old client is still running when the new one is created, both clients briefly compete for the same endpoint. One is playing discarded data, and new one starting fresh. This will cause a pop.

!is_exclusive guard is there because the audio simply doesn't play in exclusive mode with that fix in. I don't use exclusive mode, so I don't know the reason why it happens honestly. I just threw it in, seemed to work.

Oh, is this true, two can exist at the same time? That sounds dangerous, and we should ensure that the old threads finishes before reporting completion. In this context, your change sounds like a bad workaround.

No, I was wrong. That was ass-backwards train of though, going from the symptom (pop on seek), and I just assumed it's two threads competing. Adding a change, compiling and testing got rid of the pop, so it "confirmed" my hypothesis, but that's not really what's happening there. Gotta be something way simpler.

MaxKellermann · 2026-07-15T11:47:06Z

 			cancel.store(false);
 			empty.store(true);
 			InterruptWaiter();
+			continue;


What's the point of "getting a full cycle" and what does "getting a full cycle" mean? The commit message does't say.

Without it, execution falls through to the PLAY processing below, which would call GetBuffer() on the just-stopped client, fill it with zeros and call Start() -> pop.

But the client was only stopped if it's not exclusive.
How will zeroes produce a pop? Zero is full silence.

Pop isn't the zeroes, it's the transition from silence to audio, and continue prevents the worker from rendering a 0filled buffer in the first place.
At least that's how I intuited it, fact of the matter is that it does help with eliminating pops.
I'm sorry I can't give a better explanation. Let's call it another trial and error :)

Also should probably reset primed here somewhere.

Trail'n'error is not a good strategy, because it leads to an incomprehensible and fragile mountain of spaghetti code. We need a full understanding of what happens, or else we can't find a solid fix.

Where's the code that fills zeroes, anyway?

WasapiOutputPlugin.cxx:519

So the flow is, as I understand it

1. Cancel handler: Stop(), started = false, ring_buffer.Discard() -> 2. Falls through to PLAY -> 3. !started, Reset(client) 4. (primed == true, my code) -> 5. ReadTo() reads 0 bytes (ring buffer empty) -> 6. std::fill_n fills entire buffer with zeros -> 7. ReleaseBuffer(buffer_size_in_frames, 0) -> 8. Start(), DAC plays full buffer of zeros -> 9. Next render, real audio, waveform discontinuity if the zeroes are behind

continue prevents steps 5-8.

It prevents that by ignoring the device's request for more data (which is not good), and if you ignore that, what will lead to waking up the next loop iteration?

MaxKellermann · 2026-07-15T11:50:22Z

 	bool playing = false;

 	bool started = false;
+	bool primed = false;


What does this field mean and how it it protected across threads?

The field means the device has been primed, obviously. It's on line 476. I stole the idea from MPV, they do something similar in there so that there's no pop on startup.

I'll make it atomic.

I feared you'd say that, but atomic isn't a general-purpose magic spell that will solve all threading bugs. Think twice before using it. It's too easy to introduce new concurrency bugs with it.

I think atomic is the correct solution here after all.

The alternatives are:
Removing !primed guard from Push() - it's gonna wake the worker even after priming is done which is not only wasteful, it also reintroduces pops. So not really an alternative, even.
Or a separate signaling mechanism, that's just extra complexity for no real benefit.

MaxKellermann · 2026-07-15T11:51:16Z

 		if (!playing) {
 			playing = true;
 			Play();
+		} else if (!primed) {


This field is written by another thread - without protection, you must not read it here.

MaxKellermann · 2026-07-15T11:51:20Z

 			playing = true;
 			Play();
+		} else if (!primed) {
+			event.Set();


MaxKellermann · 2026-07-15T11:55:21Z

+		/* On cold start, wait for enough data to fill
+		   the entire endpoint buffer to avoid a pop. */
+		if (!primed) {
+			const UINT32 needed = buffer_size_in_frames * frame_size;


I believe waiting until the buffer is completely full is excessive, adds too much delay. We should only wait long enough to prevent the pop, but not more. How much do we need for that?

A full buffer is definitely needed to prevent the pop entirely. I didn't ~~see~~ hear any perceptible delay during testing. I'll experiment with only filling up to a point though.

Is this behavior documented?
If this is true, then we can implement a big code simplification: then we don't need a ring buffer, and instead only send blocks of full buffers to the API.

It is documented, yes.
https://learn.microsoft.com/en-us/windows/win32/coreaudio/rendering-a-stream

In its initial calls to the IAudioRenderClient::GetBuffer and IAudioRenderClient::ReleaseBuffer methods, the function fills the entire buffer before calling the IAudioClient::Start method to begin playing the buffer.

https://learn.microsoft.com/en-us/windows/win32/api/audioclient/nf-audioclient-iaudioclient-initialize

Each time the thread awakens, it should call IAudioClient::GetCurrentPadding to determine how much data to write to a rendering buffer

I don't believe getting rid of the ring buffer is at all feasible. How is output gonna call wasapi? It doesn't even have COM context. Among other things. Is this a tongue-in-cheek statement?
primed check only gates Start()

I fed chromium wasapi implementation to my local qwen llm (too lazy to dig through all of it myself, not like I'm working on chromium code anyway), and it said there's no ring buffer in there, so maybe you are right after all. I don't know how much of that is "simplification" though, looks like a massive overhaul to me. Definitely beyond my abilities.

OK, it's good that we have documentation, and you should add a code comment explaining why you're waiting for the ring buffer to become full before starting the device (with a link to MSDN).
But it could be so much simpler, without the "primed" flag: just do the "full" check from inside Push() and inhibit the Play() call until the buffer is full. No new code in Work() at all. Simpler and more robust.

I think we still need the ring buffer, after all. Just wait for it to initially become full before starting the device.

0x0003 · 2026-07-15T23:16:35Z

Converting to draft until I can come up with something better with clearer explanations. Most of the current state was done by "throw shit at the wall and see what sticks" principle, and even though it works, I don't understand most of what's going on behind the scenes really.

MaxKellermann · 2026-07-16T06:43:41Z

Throwing shit is a good approach for the first iteration, but before merging, we need a better understanding of the situation.

Certainly the existing code isn't as good as it should be, and suffers from the same problem; I guess the author didn't have a full understanding either. But new code solves problems, and never introduces new problems. But when I merge changes to existing code, that may introduce new problems, therefore merging code changes needs more thorough review. Because likely you will be gone when the bug reports start coming in, and then it's my problem.

I suggest you try to finish the "primed" patch first, but a simpler version as I described in the comment. It is easy to see that this needs to be improved and how.

0x0003 added 3 commits July 15, 2026 18:20

wasapi: reset audio client on resume from pause

4f9e9f6

Call IAudioClient::Reset() when resuming playback after a pause to flush stale buffered audio and prevent audible pops.

wasapi: wait for full buffer on cold start to prevent pop

8d45d99

Gate the first Start() on having a full buffer of real audio data in the ring buffer. Push() signals the event during this priming phase so the worker wakes as new data arrives.

MaxKellermann requested changes Jul 15, 2026

View reviewed changes

0x0003 marked this pull request as draft July 15, 2026 23:15

Uh oh!

Conversation

0x0003 commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x0003 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x0003 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x0003 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x0003 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x0003 Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0x0003 commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxKellermann commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

0x0003 commented Jul 15, 2026 •

edited

Loading

0x0003 Jul 15, 2026 •

edited

Loading

0x0003 Jul 15, 2026 •

edited

Loading

0x0003 Jul 15, 2026 •

edited

Loading

0x0003 Jul 15, 2026 •

edited

Loading

0x0003 Jul 16, 2026 •

edited

Loading

0x0003 commented Jul 15, 2026 •

edited

Loading