Skip to content

wasapi: prevent audio pops on startup, resume, and seek#2545

Draft
0x0003 wants to merge 3 commits into
MusicPlayerDaemon:masterfrom
0x0003:win-wasapi-pop
Draft

wasapi: prevent audio pops on startup, resume, and seek#2545
0x0003 wants to merge 3 commits into
MusicPlayerDaemon:masterfrom
0x0003:win-wasapi-pop

Conversation

@0x0003

@0x0003 0x0003 commented Jul 15, 2026

Copy link
Copy Markdown

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.

0x0003 added 3 commits July 15, 2026 18:20
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.
Comment on lines +349 to +351
bool SupportsHardwarePause() const noexcept override {
return true;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@0x0003 0x0003 Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

"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?


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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why? The commit message doesn't say.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@0x0003 0x0003 Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

bool playing = false;

bool started = false;
bool primed = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@0x0003 0x0003 Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why?

/* 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@0x0003 0x0003 Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@0x0003 0x0003 Jul 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

if (!primed) {
const UINT32 needed = buffer_size_in_frames * frame_size;
if (ring_buffer.ReadAvailable() < needed) {
continue;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What will the device do if we skip the rest of the loop and don't write anything to the device's buffer?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nothing, it hasn't started. Start() is at 501, just below this block.

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

0x0003 commented Jul 15, 2026

Copy link
Copy Markdown
Author

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.

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.

2 participants