Closed bramtayl closed 3 years ago
Merging #79 (9ce4cc1) into master (6a018cf) will increase coverage by
1.52%
. The diff coverage is92.23%
.
@@ Coverage Diff @@
## master #79 +/- ##
==========================================
+ Coverage 90.00% 91.52% +1.52%
==========================================
Files 2 2
Lines 250 342 +92
==========================================
+ Hits 225 313 +88
- Misses 25 29 +4
Impacted Files | Coverage Ξ | |
---|---|---|
src/libportaudio.jl | 87.87% <91.93%> (+11.82%) |
:arrow_up: |
src/PortAudio.jl | 92.39% <92.30%> (-3.14%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Ξ = absolute <relative> (impact)
,ΓΈ = not affected
,? = missing data
Powered by Codecov. Last update 6a018cf...9ce4cc1. Read the comment docs.
Ok, so, I figured as long as I'm doing a rewrite for #63, might as well implement it as well. This could probably use an extra set of eyes.
Note: I'm guessing this will tank coverage because of all the new automatically generated and unused wrappers
Looking good @bramtayl. Just a few requests.
I appreciate all the love you've been giving this package recently. Awesome stuff.
Can you tag a new version before and after each PR?
Sure! I'll tag before merging, if we decide to merge
What is clang? Why would we switch to it? What's the benefit? What's the negative?
The clang generators will automatically generate wrappers for a C library based on its headers. So everything you see in libportaudio.jl is automatically generated from the package. We would presumably switch if the benefits outweigh the costs, namely:
Benefits:
Costs:
get_default_input_device
in this PR for an example.Maybe @gnimuc who did the clang stuff might have something to add
What's the potential complications this issue may raise that I should look for
This is entirely a rearrangement of the existing code, so there shouldn't be any performance or functionality changes at all. If there is, it would be due to a mistake on my part.
So looking at the code, I realized that there were new threads being spawned during each buffer read and write. This takes time, instead, we can just maintain two demons, one for reading, one for writing, and communicate with them via channels. Since I'm making so many other changes, I've gone ahead and done that too. This seems to have totally eliminated xruns on my end, so I've set xrun warnings to true by default.
I've also tried to pepper in many more comments and make the code more self documenting to aid in review
Lots of detailed work here π want me to take a listen yet? Or you're still going?
Still going...sorry I'll label it as WIP. I think the last commit might have reduced performance, and I could simply it a bit too.
@rob-luke ok I think I'm ready now!
Hi @bramtayl I pulled your branch to run a few tests and have been receiving overflow errors. I do not get these overflow errors on the master branch.
Here is my terminal demonstrating the overflow warnings:
And here is the terminal demonstrating no warnings on master:
Is this expected? It may be that you have changed the warning system in this PR, I haven't read the code yet. But you don't describe this change above so I expect its undesired. It may also be that I am installing the new version wrong?
This is being run on MacOS with an external usb microphone. Once we understand this warning then I can test the audio output using my RME FireFace sound card.
Yes, I changed warn_xruns
to true
by default as described here: https://github.com/JuliaAudio/PortAudio.jl/pull/79#issuecomment-866295761. The xrun warnings are disappointing though; I had hoped they might have disappeared. What results do you get if you add warn_xruns = true
on the master branch? Do you get the same number of xruns?
Thanks for clarifying @bramtayl. Could you summarise all the changes (specifically ones affecting end users like the warnings that I observed) at the top of the pull request in the initial comment. This makes it easier to review, whereas comments scattered throughout commits get easily lost.
Summarising the changes at the top also allows me to more easily go through and check that all the changes work when I test them locally. As it stands now, its hard for me to know what to test.
What results do you get if you add warn_xruns = true on the master branch? Do you get the same number of xruns?
Using the USB microphone with master I get one overflow. With this branch I get two overflows.
Using my RME I get to overflows on each branch.
I dont use audio input very often, so I cant comment if this is expected or not, but I imagine its not good.
Hmm, could you try again, but run the full test suite, adding warn_xruns = true
to each call to PortAudioStream on master? I get 5 xruns on master and 1 on this branch.
I tried my hand at making performance improvements; maybe it helped a little? Also, I edited the initial comment to summarize the changes here.
Ok, no more xruns on my end
Wow! Ok I'll run the test suite and send you my results today.
Running this branch I still get a few over/underflows. I have pasted the output below for you incase that helps.
For such a major rewrite as this it would be good to get some input and testing from others. Once you are happy with this PR lets ping a few people for a review.
Yeah, the 4 xruns is very concerning. I don't see any on master, did you add warn_xruns=true
to the PortAudioStream
calls?
The show and domain errors are also mildly concerning
It's also interesting that all four of your errors are from the input. When googling fireface I get "the Fireface UC provides revolutionary ultra-low latencies". Maybe the xrun errors would go away if you bumped up the latency a bit?
Ok, the show errors might have been caused by a source/sink issue I noticed and just fixed. But there's still something funny going on. Is it possible that the devices changed mid-test?
did you add warn_xruns=true to the PortAudioStream calls?
No π’, but now I did. See the output below.
Is it possible that the devices changed mid-test?
I am quite sure it didn't, unless the test code makes it. I have run the tests multiple times and always get the same output.
When googling fireface
I think the tests are actually using the "Built-in Input" 2β0
which is some motherboard device ive never used. So I dont think the test are using the Fireface. I would prefer the tests do use the Fireface as its a very standard piece of audio equipment in labs. Is there a way I can force the tests to use that device? I tried setting the input_index
variable so that the Fireface was used, but then I got stacks of new errors.
and just fixed.
Yes you did!! And that works great. Here is the new output of the test on my pc incase that helps. I am still getting the constructor error.
Am I right in counting three xruns each on master and the PR? Not great but not terrible then.
Is there a way I can force the tests to use that device? I tried setting the input_index variable so that the Fireface was used, but then I got stacks of new errors.
The tests should be using the default input and output devices as given by PortAudio. I don't know where PortAudio gets that information from? I could add a line at the start like testing_input_device_index = [default]
so you could edit the script more easily.
I tried setting the
input_index
variable so that the Fireface was used, but then I got stacks of new errors.
What errors?
With the domain error, maybe we could add an adjust = false
keyword that if enabled would set the requested device count to the max if the max is exceeded? Then we could enable it in all the tests?
Ok, I added an adjust_channels = false
to enable the tests not to exceed the max, and pulled out the device names to make them easier to hardcode into the tests
Updates: I added real docs (both to document new features, and also cause we didn't really have them before). The doctests don't seem to run, I think it has something to do with https://github.com/JuliaIO/Suppressor.jl/issues/28
Well, darn, didn't mean to merge this but guess it's too late now...I do think it should be pretty good quality code though, I've been working on it long enough. I don't think there's anything we can do about the doctest issue; I'm guessing the problem is that redirect_stderr and documenter are competing for access to stderr
Please use squash and merge when merging. The commit history is now becoming littered with junk like "fix".
@bramtayl Could you try and look up some git magic to retrospectively fix your commit history from this PR? I am sure it can be done.
Ok, sorry. I've squashed all the commits and forced pushed, so hopefully that should be better?
Hey, and look at the new docs! https://juliaaudio.github.io/PortAudio.jl/dev/
@JuliaRegistrator register
Comments on pull requests will not trigger Registrator, as it is disabled. Please try commenting on a commit or issue.
This is near total rewrite of the existing code. However, the front end interface remains the same so there should be no breaking changes. The three main changes are:
1) Use clang wrappers (described in the new readme in the gen folder) 2) Reduce thread spawning: instead of spawning a new thread for each read and write, have a single thread for reads and a single thread for writes 3) Add an internal "scribe" interface that allows users to opt out of SampledSignals if they want to access the buffers directly