-
Notifications
You must be signed in to change notification settings - Fork 225
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
Don't insist that selected audio device is working #1378
Conversation
Interesting approach. Nevertheless removing all the error message might be counter productive: "While configuring ASIO4ALL: Why don't I hear sound anymore? Without ASIO Driver: Why do I get an error message if I click connect?" Users would probably not look at settings on a first run. Would it be possible to show the error messages, initialise a disconnect and ask the user if he wants to quit Jamulus or open Settings to change the device (settings)? |
src/soundbase.cpp
Outdated
if ( !vsErrorList.isEmpty() ) | ||
{ | ||
// create error message with all details | ||
QString sErrorMessage = "<b>" + tr ( "No usable " ) + |
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 think this error should still be very prominent
Just tested what happens if you connect to a server with an invalid ASIO Driver. Jamulus crashes. This shouldn't happen. Can we give an error message if the user tries to connect with invalid drivers? Also on Linux this now results in Jamulus not connecting to JACK. I assume this is a much bigger topic :-(. |
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.
Yep. Also on macOS I now get an error and no sound.
It shouldn't try to connect -- connection shouldn't be possible without a working sound driver. In fact, the mixer screen isn't much use: the user should be in the settings screen until they've got it sorted, I think.
I've always avoided looking at the sound device code... :) |
Is this PR also related to #727 ? |
The whole way Jamulus handles the audio hardware is a bit problematic, really. It requires there to be a functional audio device, even it it's not using it. Whilst the "require" is in the common code, the "provide" is platform-dependent. So each of the "it doesn't work if..." scenarios has a different solution ... if you look at them one way. Looked at another - rewrite how Jamulus acquires the device - they may have the same solution... |
52eaec6
to
1ed3043
Compare
How about showing the error in the main window as well? (I've reused the mute message label, not sure if it's better to have two separate labels and swap between them instead).
Hmm, doesn't happen for me. I get an error message after a few seconds of "TRYING TO CONNECT". Screenshot |
I think we shouldn't abuse the muted message. Doing some tests now. |
Ok. The crash only occurs if we only have one device (ASIO4ALL) which is wrongly configured and I connect to a server |
This comment has been minimized.
This comment has been minimized.
1ed3043
to
7c549d9
Compare
When changing to use a separate label I bumped into several cases where I got a segfault. Actually, I'm a bit confused how I didn't see them before. Anyway those should be fixed now, and I also disabled the "Connect" button when audio isn't working. |
Thanks!
I think we should not disable it (since you can just open the connect dialog via "view"). Can you give a warning (You can't connect because your current audio device configuration isn't working: Do you want to open Jamulus's settings to change your configuration or close Jamulus?). This should be shown right before it actually starts the connection process. This would catches all cases (and probably work for CLI only clients too.) |
7c549d9
to
fc4fd2b
Compare
Ah right. Throwing an exception seems to handle both cases. I'm not sure about CLI clients though. Is it possible to change devices without a GUI? |
Not sure. I think it's not possible on Windows at least. But I don't know honestly. |
Ok. Just tested it. Unfortunately double clicking a server still tries to connect. Maybe you should add it in the Sound class? Not looked into the code yet, but it should happen in CSound or in Init()? So all the code used to be in CSoundBase::SetDev which used to throw an exception in line 200 if something went wrong: https://github.com/jamulussoftware/jamulus/pull/1378/files#diff-3826707a9164c8d7018040fddfb703fe2a72cb7777a9f1866c551a3866efa237L200 What if we catch this exception, initiate a disconnect (or disable audio) and throw a warning message which asks if you want to open settings? Also the error message "previous sound device no longer present in system" is not really an error. We might give a warning message and still initiate an automatic device selection. I'm really afraid of this PR since it seems to change complex core functionality. |
I can't reproduce that.
I disagree very strongly with this. Automatically switching to a different device because the previous happens to not be working at the moment causes a lot of confusion to people I've played with. |
Understand your point. However what about a deliberate error message with a prominent "Yes, change my audio device" or "Open settings to select another audio device" button?
Strange. At I tried it on Windows 10 with ASIO4ALL only and it did happen. Can you try it in a VM? |
Yeah, I think if the user makes the choice it should be okay.
Um, not easily. But since |
No. If I connect to a server I input into the "Server" field, it does show this message. If I connect to a server by double clicking, Jamulus tries to connect but then crashes after a few seconds. |
b4afc90
to
83d80be
Compare
83d80be
to
d304a71
Compare
Yes, the additional buttons apply across all platforms, conflicts should be resolved now. |
I assume the macOS code works fundamentally different to windows and linux. @ngocdh do you know anything about how the macOS code handles audio device failures? |
I just tested https://github.com/jamulussoftware/jamulus/actions/runs/1700022274 and indeed it doesn't change device when the device is not working, but pressing the "ASIO Device settings" button does NOT open the ASIO settings either.... So still no way to correct the problem. P.S. To quickly check a faulty device I use the KoordASIO driver with a mono input device selected. P.S.2 Why Jamulus does not accept a mono input seems a bit strange to me too, since with more than 2 inputs (also strange why not with 2 inputs) I can select the same input for both channels. |
I don't have such a device, so I can't reproduce this easily. Are you getting the EDIT: forcing the |
KoordASIO is a simple replacement for ASIO4ALL, you can download it via the link I provided. And almost any USB headset will have a mono input.
No I get "Couldn't initialise the audio driver. Check if your audio hardware is plugged in and verify your driver settings" I can generate the `The selected audio device is incompatible" message too, using another device in KoordASIO, but in that case I CAN open the ASIO Settings. |
That's no option for the average user though.... |
Ah, I see where that's from. I think avoiding the It's not clear to me what causes that error though. Seems to be unrelated to Jamulus requiring a stereo device (that check comes later). Is it possible that KoordASIO requires stereo?
Yeah, I was just trying to see if I could reproduce your problems on my end. Obviously a user wouldn't be using a debugger. |
i just tested jamulus_3.8.1dev-3838fdd_win.exe with mixed results.
Neither it is to me now, since it doesn't seem to be mic related at all (the mic. seems to have 2ch after all, my mistake) and I don't get the "Couldn't initialise the audio driver..." anymore, (Did the check itself change too?) but with the same KoordASIO config as I got this message before I now can connect to a server, mic working, but I get no audio on the headphones (though I hear some faint white noise after connecting, so the device seems to be initialized.). Second thing I noticed is that any changed ASIO settings don't take effect until the device is re-selected (You get the same error again when trying to connect after changing the ASIO settings). So maybe the driver should be re-initialized before connecting? And another question: Is it right that the Äuto Select Device" always selects the first working device in the list? Not the last used working device, and even when the current device is working ok? |
No change in Jamulus' checks. Is it possible there's another application competing for the mic/speakers and interfering? Are you in shared or exclusive mode?
By "mic working" you can see the levels go up when you make noise then? Even though you don't get any echo back?
I think this is a KoordASIO problem, it doesn't send the
Oh, I didn't consider the accidental button press, that is a good point. I'll try and see if I can make a drop down option work instead. |
Tested both. And I can force a "Your sound card is not working correctly..." in exclusive mode when making sure the device is already in use. But that message doesn't appear until a few seconds after "trying to connect". Yesterday I always immediately got the "Couldn't initialize the audio driver..." with this configuration regardless exclusive mode and in use or not. So I really don't know what changed this behavior.
Yes, and I even can hear the audio on another client.... But no audio from the server on this client with this particular config. (P.S. Maybe the change in behavior is because in the mean time I rebooted my computer?, since I re-installed jamulus_3.8.1dev-f2b59e8_win.exe and I still can't reproduce the "Couldn't initialise the audio driver", while yesterday it was very consistent. But also no return audio with this configuration on 3.8.1dev-f2b59e8 too, while the same ASIO config works in reaper.)
Might be. But isn't it always wise to reset before connecting, just to make sure the latest settings are loaded?
That would be great! Shouldn't be too hard to implement the drop down option.... ;=) |
I‘m really sorry to say that, but I‘m afraid we need to move that to a feature branch once more? (Of course unless we find someone who helps out debugging the macOS related issues) I‘d suggest to open/link the appropriate issues though. |
I'm already diving into the mac code, and I can see why there are problems. This is since the mac code has a totally different approach (channel selection via the device selector) and so quite a different implementation. (Exactly what I wanted to prevent with my "audio redesign" PR.)
Please explain what what you mean here... |
There are issues which could be solved with this PR. They should be found and linked to this PR. Great to hear you found issues on the macOS part. As I said, I think that’s much easier to get in. So I‘m not putting this to a feature branch now. |
I you know more sound relater issues, please let me know. I will solve them. I probably already solved a lot (even still unknown bugs).
And not only in the macOS part. ;=) I'm quite far with the CSoundBase redesign and the Windows implementation.... |
Great thanks. Yes, sometimes changing in/output devices in ASIO4ALL freezes Jamulus, if ASIO4ALL is not the only driver you can't change it (that's what you mentioned), failed audio drivers don't reload,... I hope your changes get accepted, but maybe you need to give micro pr's instead (one for each OS). Especially the ASIO part is tricky - as you know. |
Freezing when changing settings while connected seems to be solved! (At least on windows.) Though I encountered another "freeze" problem that seems to occur only when closing Jamulus while using Jack for Windows.
?? What I mentioned was that you can't correct a wrong driver configuration when the driver has no own "offline settings" (not just ASIO4ALL)
So do I ! There are a lot of improvements.... (I will post the most important ones in a separate post.)
But since it is a (major) change in CSoundBase this is not possible... We will have to merge them all at once. So I could use some advice here... In that case I can check in my modifications one by one, without affecting master. But anyone could check my personal branch to test some things and see the progress. Once all OS's are working with the new structure we can decide when and how to merge (new PR or re-opening PR?). |
@ngocdh @pgScorpio I will close this due to the sound redesign. But please do not remove the branch. I think it would be great if you both exchanged ideas! |
EDIT by maintainers: In order to move this forward, we'd need someone with macOS background who can help debugging
This PR is intended to make the discussion in #1338 a bit more concrete. I think this basically handles points (1) and (3) from there. I didn't make any attempt at (2); none of the messages are different.
Here's a screenshot of what happens when I've selected a device that's turned off.