wasapi: make render thread exist for the lifetime of the stream object

[ligfx]

Took a crack at this to see what it might look like. See #534 for why I think this could be a good idea.

• Converts cubeb_stream into a reference-counted object, and gets rid of
  emergency_bailout logic
• When reconfiguring, only restart a stream if it was already started
• Removes timeout inside the render thread, as we can't guarantee events
  while a stream is stopped

[kinetiknz]

Thanks for doing this! The approach generally looks good to me. Do you want me to go ahead and review this as-is?

I suspect we'll need to retain part of the emergency bailout logic. Specifically, we need be sure we never execute callbacks after a stream_stop/stream_destroy.

[ligfx]

@kinetiknz

After stream_destroy, the only way I see that a callback would be executed is if WaitForMultipleObjects stalls, and then returns a refill/input_available event even though a shutdown event is also set. Do we know if WaitForMultipleObjects ever stalls, or if the stalls are just inside WASAPI?

After stream_stop is complicated. Even after calling IAudioClient::Stop, the events can still be triggered. I'm seeing usually one more event to fill up the buffer (which executes a callback), and then events with 0 frames left to fill (which doesn't execute a callback, but might unnecessarily add CPU load/thread context switches). How strict is the requirement not to execute callbacks after stream_stop? Thinking through this, I think the only way to accomplish this in every case would be to move the stream start/stop logic inside the render loop thread. Not that complex to code but increases the probability of encountering a stalled thread.

[kinetiknz]

After stream_destroy, the only way I see that a callback would be executed is if WaitForMultipleObjects stalls, and then returns a refill/input_available event even though a shutdown event is also set. Do we know if WaitForMultipleObjects ever stalls, or if the stalls are just inside WASAPI?

Just WASAPI, so this case is fine and I'm just wrong about it. stream_destroy is the important case, since we expect the caller to free resources after the stream is destroyed.

After stream_stop is complicated. Even after calling IAudioClient::Stop, the events can still be triggered. I'm seeing usually one more event to fill up the buffer (which executes a callback), and then events with 0 frames left to fill (which doesn't execute a callback, but might unnecessarily add CPU load/thread context switches). How strict is the requirement not to execute callbacks after stream_stop? Thinking through this, I think the only way to accomplish this in every case would be to move the stream start/stop logic inside the render loop thread. Not that complex to code but increases the probability of encountering a stalled thread.

I think this is fine for now, since it's no different from the behaviour before your changes. Eventually we'll want to tighten this up for all backends, I think.

[ligfx]

@kinetiknz Apologies for the delay on this. Rebased to current master (taking extra care around the changes to the thread/bailout logic), and based on the comments above this is ready for review as-is.

[kinetiknz]

Thanks! This looks good. I'll merge this soon.

[ligfx]

@kinetiknz Are you still interested in this? Happy to rebase and fix conflicts if so.

[danilaml]

@kinetiknz any reason this wasn't merged?

[padenot]

Sorry about that, I think we forgot. I've rebased your initial patch, because the underlying code has changed a lot.

https://github.com/mozilla/cubeb/tree/wasapi_render_loop_lifecycle_rebase is my rebase. I'll change this branch so that it's cleaner, there are various things in a single commit at the moment and CI changes unrelated to your fix.

We now have CI for Windows (as of last week): https://github.com/mozilla/cubeb/actions/runs/4862284679/jobs/8668439986, and I'm trying the patch on Firefox's CI, running most of our media tests, we'll see.

[padenot]

I'm trying the patch on Firefox's CI, running most of our media tests, we'll see

https://treeherder.mozilla.org/jobs?repo=try&revision=254552db12c035f9a53c5985e4c837b35d4e6cf7. Our CI is having an outage at the moment, but I'm told ops will restart jobs once it's fixed, so we'll have results there.

[padenot]

@danilaml are you up to review my rebase, since you wrote the code in the first place, and it was already reviewed by @Kinetik (and I had to re-read it for the rebase)? It's at 3e12930. If I haven't screwed up, I'll separate what was your patch orignally from my other fixes, put the right author on the commit and merge.

I've started more tests on Firefox's CI (same link), running the same test suite in opt / non-debug and ASAN enabled, since we're really dealing about lifetimes of resources and such here, can't be too careful. cubeb's CI doesn't run ASAN on Windows yet, and certainly hammers the code less than the full media test suite run of Firefox.

[danilaml]

@padenot I think you've made a typo. The original author of this code is @ligfx , not me. I've just discovered it when root causing the bug I was experiencing.

[padenot]

Ha yes that's what I get for only reading the end of the thread, apologies.

[danilaml]

@padenot for the record, I've been running with the rebased branch for a few days so far and haven't encountered any issues (and it did indeed fix the problem I was having). Granted, that's even less coverage than mozilla CI provides, but at least I can confirm that this solves something ;P

[padenot]

Granted, that's even less coverage than mozilla CI provides

Yes, but the workloads are less synthetic, so it's useful to know, thanks for giving it a try! I'm going to re-read the patch myself and probably merge it.

[padenot]

@kinetiknz I'm merging this, it's green all around on treeherder and here. I've added a couple commits to be able to have clang-format that has the same results locally and on the CI but it's the same as the original patch (rebased) otherwise.