-
Notifications
You must be signed in to change notification settings - Fork 318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wasapi: Fixed Pa_ReadStream()/Pa_WriteStream() for 1-channel (Mono) I/O #948
wasapi: Fixed Pa_ReadStream()/Pa_WriteStream() for 1-channel (Mono) I/O #948
Conversation
It's better but not completely free of cracking for some? devices. Here are two examples: Each file has 4 segments of ~3sec:
While (2) is still obviously problematic it's much better now. |
f49366e
to
00770bf
Compare
I found the problematic place, we were writing some trash/silence behind buffer when saving trailing frames into the circular buffer. Now all is fine. I used your test bench in my tests. Would you please check from your side. |
Tested, it works, no problems. 🎉 |
That's great! I hope @RossBencina and @philburk will review this PR soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made various comments. Also:
The patch updates examples/tests with compile-time options to select number of CHANNELS and to find and select the [Loopback]
device for input.
examples/paex_read_write_wire.c
examples/paex_write_sine.c
test/patest_read_record.c
Since the loopback device with this name is a WASAPI-specific thing I'm not sure whether it makes sense to update the generic examples with this feature. No need to change anything yet but this is something to discuss with Phil when he shows up.
…tream, made some examples configurable for 1-channel operation for easier testing of Mono I/O
00770bf
to
6248ea0
Compare
@RossBencina I updated implementation based on your review notes, please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Dmitry, I've resolved a bunch of the old feedback and added some more.
Unfortunately Phil is absent today so I can't discuss some of the remaining issues with him.
This was very time consuming to re-review. You could help by:
-
Please do not force-push during the middle of a review. We need to be able to see what changes you made to resolve our review comments. We can always squash when we merge, or you could force-push once we have agreed to the changes.
-
Please do not fold-in irrelevant changes such as whitespace and variable renaming. Especially when the PR is big like this one. I know that it is more work for you to keep things separate but it will make it much quicker for us to review the PRs.
Thank you!
…y the device. Co-authored-by: Ross Bencina <[email protected]>
Co-authored-by: Ross Bencina <[email protected]>
Added WASAPI-specific patest_read_write_wire_wasapi.c test which allows to also use Loopback devices (USE_LOOPBACK_INPUT must be 1).
…s the lowest value specified by the process.
@philburk I made all changes we discussed, would you please check and approve if all is fine now. |
…use the Loobpack WASAPI device, reverted test 'patest_read_record' back to its original state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Dmitry, I'm not going to hold up the merge for my latest request, more of a nice to have.
…fer's memory succeeds, set it to 0 if memory allocation fails.
Ross, I made proposed change, thank you for pointing to it. |
@philburk waiting for your review... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for doing this.
Just a minor nit about an assert being a NOOP. OK to merge as is.
I've tested the version merged into master and it works for me, thank you! |
The problem is covered by #945 issue. Modified examples can be used to test 1-channel input/output stream, including the loopback.
After the acceptance of this PR I will further address #946 issue in another PR and refactor mono-stereo mixer's implementation names.
Closes #945