Skip to content
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

[scsynth] [supernova] [PortAudio] [Windows] separate input and output devices, better default selection #4009

Open

Conversation

@dyfer
Copy link
Contributor

commented Aug 22, 2018

Purpose and Motivation

On Windows, scsynth was not able to select separate input and output devices (it accepted only single device name and tried to use it as both input and output). This essentially prevented manual device selection for anything other than soundcards with ASIO drivers (AFAICT the only API on Windows that reports devices as full-duplex/both inputs and outputs).
Fixes #3990

edit:
Upon further investigation I decided to change a little default device selection. Currently, PortAudio's default device option on Windows chooses to use the MME API, which is old and inefficient (considerable latency). I added a change so the default device is opened on the WDM-KS API, which should provide a much better performance. If anything fails, it will still revert to the old MME API.

This was problematic on some systems. By default the server is still trying to open the MME device.

Types of changes

  • Bug fix: allow scsynth with PortAudio backend to select separate input and output devices (addresses Windows platform, but might apply to others as well if scsynth is built with PortAudio)
  • New feature (kind of): improve default device selection for scsynth: if no device is specified, try selecting the default device through WDM-KS API; if one device is specified, select another one from the same API
  • New feature (kind of): improve default device selection for supernova: if no device is specified, try selecting the default device through WDM-KS API;

Checklist

  • Allow selection of separate input and output devices on Windows
  • Test Windows build
  • Add automatic device selection on WDM-KS API (scsynth)
  • Add selection of matching input/output device (when only one is provided) on a matching API (scsynth)
  • Test API improvements (scsynth)
  • Add automatic device selection on WDM-KS API (supernova)
  • Test changes to supernova can't be tested as SN doesn't currently build on windows
  • Reconsider default API to use on Windows (MME most likely works, but has big latency, WDM-KS provides low latency but blocks other apps from making sound, WASAPI fails if the sample rate is not matching the hardware)
  • Implement the chosen default API, possibly with sample rate detection revert to selecting MME by default; no sample rate detection
  • Improve help describing audio APIs on Windows
  • Get more testing from the community
  • This PR is ready for review
  • commit history cleanup - to be done after any changes need to be made, before merging

Remaining Work

The commit history needs to be cleaned up. I'm happy to do it once the patch is tested.

~~While this looks like it should just work, I do not have Windows build environment ready ATM. ~~
I vaguely remember someone inquiring about access to automatic PR builds from AppVeyor for testing purposes... is this possible?
I think I see how to access AppVeyor build. I'll use this for testing.
Built and tested on my system. Some community members tested with AppVeyor build.

edit:
Notes on supernova:

  • code is designed in a way so it's harder to make multiple guesses for which device to use; instead it tries one option and fails if the device can't be used. In this case I added selecting default device on Windows to use WDM-KS API, but no matching for another device on the same API if one device is available I abandoned choosing WDM-KS by default, since it was problematic on some systems.
  • more importantly, the Windows build with supernova starts, but the language crashes once supernova boots. I see we don't provide windows builds with supernova, but couldn't find a clear reason in the Issues (there is a report it doesn't build, BUT AppVeyor builds it so...?) my bad, I was running supernova from a different install by accident; there is no supernova in AppVeyor builds
  • supernova doesn't list audio devices with their API, only the names. This, on Windows, sometimes prevents selecting the desired device on desired API (devices can have the same name and appear under multiple APIs). This PR does not address this issue. I added listing APIs on Windows for supernova, as it was necessary for the default device selection on WDM-KS API to work allowing to select a device with a given API on Windows.

edit no 2:
Upon further investigation, it looks like the choice of WDM-KS is not the best, as it prevents the sound card from being used by any other applications. PortAudio's WASAPI implementation is however more error prone, as it does not set the soundcard's sample rate - it works only if the user sets sample rate to the correct one before booting (otherwise fails with a rather uninformative error "Invalid device"). This needs further investigation as to what the best default would be. (I'm guessing WASAPI with sample rate detection, but this might be is beyond the scope of this PR to implement).

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch 2 times, most recently from ec6dea8 to 765e263 Aug 22, 2018

@dyfer dyfer changed the title [scsynth] allow selecting separate input and output devices [scsynth] [PortAudio] allow selecting separate input and output devices Aug 23, 2018

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch from 765e263 to 342b9a8 Aug 23, 2018

@snappizz snappizz added this to the 3.11 milestone Aug 23, 2018

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

@snappizz if this gets tested and works well I'd suggest to possibly cherry pick it to 3.10 - the inability to select devices is a real pain on Windows and the sooner we fix it the better :)

@jamshark70

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

I hope this works -- would be great to fix it!

One suggestion would be to add a check in the JACK backend at least to post a warning if the user specifies separate devices. Separate devices aren't valid for JACK and never will be. But I think you're right not to add the check into ServerOptions if it's possible to build in Linux with PortAudio.

I don't see obvious mistakes in the class library; I'm not qualified to review the c++ changes.

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

@jamshark70 looking quickly at JACK backend, it seems that if two devices are specified, it will use the first (input) device. If one devices is specified (correctly), then it should work as before. Do you think JACK warning is in the scope of this PR?
Also, as you said, checking this on the sclang side is impractical - sclang doesn't know which backend the server uses. I think this issue (i.e. sclang not knowing the synth's backend - and we can have different backends for scsynth and supernova in the same install....) will come back in the future if we try to implement ServerOptions.devices on non-macOS platforms...

@jamshark70

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

Do you think JACK warning is in the scope of this PR?

I think so. The PR opens up the possibility of doing something illegal in the server, that was previously prevented by the ServerOptions platform restriction on devices.

The message could be as simple as "Warning: Two device names were specified, using '...'".

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

I tested this on Windows 10, and seems working properly.
I was able to set in and out devices independently, even between internal and USB soundcard. The only requirement is that they use the same API (DirectSound / WDM-KS etc). Also, WASAPI doesn't seem to work (but I believe WDM-KS should provide good performance). Setting a single ASIO device forks fine as it was before.
BTW I feel like the requirement of choosing matching APIs for in/out devices should be documented somewhere. This is potentially platform specific, however should this go to ServerOptions helpfile?

@jamshark70 I'm working on the note for JACK backend.

@jamshark70

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

I feel like the requirement of choosing matching APIs for in/out devices should be documented somewhere.

I think ServerOptions is fine. It's a totally reasonable and predictable requirement -- users shouldn't be surprised.

The JACK warning looks ok to me at a glance, but I haven't tested. device is seldom used for JACK anyway.

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

@jamshark70 unfortunately I can't figure out how to check whether we're building for JACK, I'm sorry. I pushed these changes since I was moving between machines, but the warning message does not work ATM.
If someone can point me whether there's a proper cmake flag to check (inside scsynth_main.cpp), e.g.

#ifdef JACK_BACKEND

I'd be happy to add the warning message. I'll also clean up commit history then.

I just want to reiterate: to the best of my knowledge, there is no difference in functionality with JACK backend with vs without this patch. Previously, with JACK, if outDevice was specified, it was ignored at the sclang layer. Now, it is still ignored, but inside the server. In both cases (with/without this patch) inDevice is used as the only device for JACK. And there was no warning message if the user tried to set outDevice and inDevice separately when using JACK. Finally, by setting device, the server will be passed a single device name.

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch 2 times, most recently from 3ba414e to 4e7dc7c Aug 23, 2018

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

I removed the (non-working) warning message for now. I can add it back if desired and if I could get some help w/r/t the cmake flag (see my previous comment).

edit:
Here's a gist with the wrong cmake flag

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch 2 times, most recently from 5dad8b7 to 37804e5 Aug 26, 2018

@jamshark70

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2018

I'd like to make one suggestion -- I'm happy to revise this after the fact myself, if you're too busy.

https://github.com/supercollider/supercollider/pull/4009/files#diff-1e6cfc85e382559c6aad7bd356dbdfedR194

I think it's clearer if explanatory text is in a section or subsection, and not a comment in an example. 1. Proportionally spaced body text is typographically easier to read. 2. Comments appear in red, not ideal for reading. 3. If there is a section or subsection heading, you can jump to the text using the table of contents.

A rule of thumb might be: if a comment spans more than two lines, better to pull it out into a paragraph.

Again, I don't mind taking this on.

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

I think it's clearer if explanatory text is in a section or subsection

Where should this subsection go? In the middle of examples? or somewhere earlier, maybe around -device section?

edit:
I moved things around... how does it look now?

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch from 1a70f2a to 2052e58 Aug 26, 2018

@dyfer dyfer changed the title [scsynth] [PortAudio] allow selecting separate input and output devices [scsynth] [supernova] [PortAudio] [Windows] separate input and output devices, better default selection Aug 26, 2018

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

This should be good to go - it would be great if others could test this on Windows (different versions and configurations). I tested on Windows 10 using both internal and external (USB) soundcard.

Open question: should we list/use all available APIs on Windows? It seems that WDM-KS and ASIO are the only ones that are relatively modern and usable. If there's a situation where WDK-KS is unavailable (?), we could maybe leave MME as an option... but it is so old, so I'm not convinced.

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

I'm making a call for testing this by Windows users. Here is testing code you can use:

//recompile the library, then:
//---- 1. ----
s.boot;
//confirm that the server booted
//note which devices were chosen
//note the list of all available devices
//test the sound
x.free; x = {PinkNoise.ar(-12.dbamp)}.play(outbus: 0);
//finish
s.quit;
//---- 2. ----
//set the devices manually to something different than the server chose before
//replace device names' below with devices available on your system (see post window)
//both input and output should be using the same API (like "MME" or "Windows WDM-KS")
s.options.inDevice_("Windows WDM-KS : Microphone Array (Realtek HD Audio Mic Array input)"); //replace device 
s.options.outDevice_("Windows WDM-KS : Speakers 1 (Realtek HD Audio output with SST)"); //replace device
s.boot;
//note if it booted to the selected device properly
//test the sound
x.free; x = {PinkNoise.ar(-12.dbamp)}.play(outbus: 0);
//finish
s.quit;
//---- 3. ---
//repeat step 2 for different devices
//if you have access to external soundcard, use that as well
//---- 4. ---
//if you have an ASIO device available, test using it
s.options.device_("ASIO : your ASIO device name"); //eg. s.options.device_("ASIO : UMC ASIO Driver")
s.boot;
//test the sound
x.free; x = {PinkNoise.ar(-12.dbamp)}.play(outbus: 0);
//finish
s.quit;

Please report the following;

  • whether the server boots properly
  • whether it selects the default audio device properly (and specify which one it is)
  • whether you can select different devices using s.options.inDevice and s.options.outDevice
  • your Windows OS version
  • your audio device / sound card model (if known)

edit: here are direct links to AppVeyor builds:
32bit and 64bit

Thanks!

@JamesWenlock

This comment has been minimized.

Copy link

commented Aug 27, 2018

Hey Marcin,

Server boots!
Here are the default initial devices
init devices

Here's what happens when you select them as the initial devices
after selecting devices

Got this error when I tried to select the MOTU
error
input

Still need to test ASIO. I'll let you know how that goes soon!

Hope that helps and have a good one!

Sincerely,
James

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

Thanks @JamesWenlock!
Can you provide code you used to select in/out devices?
Did you specify the name as it is printed in the post window, ie. with the API ("Windows WDM-KS" etc)?

@brianlheim brianlheim changed the base branch from develop to 3.10 Jan 25, 2019

@brianlheim brianlheim changed the base branch from 3.10 to develop Jan 25, 2019

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

Not sure what's up with the three commits (c0b4ca8, e92c730, 352bbec), did I mess up rebase or something?

@brianlheim

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

@dyfer I was just about to comment on that. Looks like you pulled in some extra commits via rebase. The quickest way I know of to solve that is to interactively rebase on develop and nix the commits you don't want

@brianlheim

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

(Personally, unless I am 100% certain of the result, I always interactively rebase; it gives a quick visual way of checking my work)

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch from d298486 to 2203d68 Jan 25, 2019

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

I resolved this by rebasing onto develop again (I think).
I did previous rebase (to cleanup) interactively as well, but... well... something must've go wrong. Anyway, seems OK now (?)

@brianlheim
Copy link
Member

left a comment

thank you for your patience @dyfer

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

thank you for your help @brianlheim !
Also, am I the one who's supposed to cherry pick this into 3.10.x?

@brianlheim

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

well, it would be ideal if you could rebase it on 3.10 and change the target branch on this PR. otherwise, we have a project board for keeping track of it and someone will take care of it before the release :)

@snappizz snappizz added this to to do in Cherrypick 3.10 via automation Jan 25, 2019

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

I think I can do rebase on 3.10 - but now I see

@snappizz added this to to do in Cherrypick via automation 14 hours ago

If rebasing on 3.10 is the desired course of action, I can do that.

@brianlheim

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

@dyfer please rebase

@nathan please wait a bit before adding to that board next time

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch from 2203d68 to 01b335d Jan 25, 2019

@dyfer dyfer changed the base branch from develop to 3.10 Jan 25, 2019

@dyfer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

@brianlheim I hope I did it right?

@patrickdupuis patrickdupuis modified the milestones: 3.10.2, 3.10.3 Jan 27, 2019

@brianlheim brianlheim removed this from to do in Cherrypick 3.10 Jan 29, 2019

@brianlheim
Copy link
Member

left a comment

lgtm, thank you!

@samaaron

This comment has been minimized.

Copy link

commented Apr 4, 2019

This is amazing work - thank-you so much.

@jamshark70

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

#4395: Another case of the server failing to boot using Windows' default sound card selection. This fix might help, but is held up (I assume because of codebase reformatting). Just a gentle nudge to get that process moving again.

@brianlheim

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

This PR has merge conflicts now, can you please rebase it according to the instructions here? Let me know if you have any questions. Thanks, and sorry for the inconvenience!

dyfer added some commits Sep 2, 2018

[classlib:help] provide a reference for selecting audio device
create new reference: AudioDeviceSelection.schelp
update and move Aggregate Device creation (macOS) to the new document
link the new document from other help files

@dyfer dyfer force-pushed the dyfer:topic/scsynth-portaudio-allow-separate-in-out-devices branch from 01b335d to 81937fb Jun 6, 2019

@snappizz snappizz self-assigned this Jun 8, 2019

@sonoro1234

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Forgive my ignorance: Is there any reason for still not merging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.