Second round of correctness fixes on multiple backends - #1335
Second round of correctness fixes on multiple backends#1335roderickvd wants to merge 14 commits into
Conversation
|
sure, but might have to wait until monday |
|
I think it's great if we can share knowledge, absolutely no objections from me! |
No worries, let me know when it's done, many of these things we can still backport to |
LastExceed
left a comment
LastExceed
left a comment
There was a problem hiding this comment.
Static review only so far, haven't tested anything yet. Also can't speak on the changes to non-windows backends, as I am not familiar with them
| let frames = get_available_frames(&run_context.stream)?; | ||
| if frames > 0 { | ||
| write_silence(render_client, &run_context.stream, frames)?; | ||
| } |
There was a problem hiding this comment.
Here too, shouldn't we use the data callback instead of generating silence?
There was a problem hiding this comment.
Yeah, not sure if I got this right. As matter of fact, CamillaDSP doesn't write silence at all, but moves start_stream() to after the first pass of the fill loop. This proposal of mine writes silence because process_commands() has no access to the data callback, at the cost of one period of silence at the start. That's smaller than the real fix would be, mirroring what CamillaDSP did. What do you think?
There was a problem hiding this comment.
I guess it is fine for now, not worth refactoring everything just for this. But probably worth creating an issue for future reference, I could imagine similar situations existing on the other backends as well (I haven't checked)
There was a problem hiding this comment.
I'll see how much effort it takes to refactor to move start_stream() to after the first pass of the loop. I like correctness, if it doesn't cost too much.
There was a problem hiding this comment.
FYI I plan to fully rewrite the WASAPI backend once we got the 0.19 API figured out, as there are quite a few things I'd like to change (e.g. upgrading to IAudioClient3 for lower latency), so if fixing this requires some refactoring, then don't bother.
There was a problem hiding this comment.
I have something cooking locally that’s not all too difficult. Will push and ping you soon.
There was a problem hiding this comment.
It's force pushed and in ca13908 if you could do a sanity check on that.
There was a problem hiding this comment.
just 1 nitpick, rest lgtm
|
ran a couple examples on hardware via ASIO and WASAPI, no problems to report |
c8c6e53 to
f69d62f
Compare
f69d62f to
ca13908
Compare
| if run_context.stream.playback_state == PlaybackState::Stopped { | ||
| // PlayStream also fires on resume from pause, where the buffer wasn't reset | ||
| // and may already hold real, unplayed data. | ||
| let cold_start = match run_context.stream.client_flow { |
There was a problem hiding this comment.
Maybe rename cold_start to needs_priming, since capture streams setting cold_start = false is semantically incorrect
There was a problem hiding this comment.
Yes, can do that. If you can confirm it works, I'll update the naming and merge after.
There was a problem hiding this comment.
Hmm, pausing immediately after starting always results in 1 callback invocation slipping through, but stopping immedately after starting does not. This isn't really a problem, but it seems strange, and I can't find an explanation for this difference
There was a problem hiding this comment.
Hmm. Are you calling pause() immediately after start(), with no delay in between? Or does the stream play for a bit first before you pause it?
There was a problem hiding this comment.
immediately. its simply
stream.start().unwrap();
stream.pause().unwrap();vs
stream.start().unwrap();
stream.stop().unwrap();There was a problem hiding this comment.
Welp, out of ideas then too. A WASAPI thing? May as well merge this.
3 participants
This is the result from a second pass comparing cpal against CamillaDSP's backends for correctness and robustness
gaps (first one landed in 0.18.0). Their backends moved on since (
next5), so I went through it again.In full transparency of licensing: CamillaDSP is MPL-2.0, cpal is Apache 2.0. That should be OK: MPL's copyleft covers licensed source files but not ideas or bugs found while reading them. Nothing was copied, rather, every fix was reimplemented against cpal's conventions. @HEnquist let me know if you have any objections.
@LastExceed would you verify the ASIO and WASAPI changes? You'll note that in WASAPI I made some small changes to pave the way for exclusive mode later.