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

Don't insist that selected audio device is working #1378

Closed

Conversation

npostavs
Copy link
Contributor

@npostavs npostavs commented Mar 28, 2021

EDIT by maintainers: In order to move this forward, we'd need someone with macOS background who can help debugging

That is, don't immediately try to select a different working device if
the current one fails to work.

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.

uninitialized-device

@ann0see
Copy link
Member

ann0see commented Mar 29, 2021

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)?

if ( !vsErrorList.isEmpty() )
{
// create error message with all details
QString sErrorMessage = "<b>" + tr ( "No usable " ) +
Copy link
Member

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

src/soundbase.cpp Show resolved Hide resolved
@ann0see ann0see requested a review from pljones March 29, 2021 11:40
@ann0see
Copy link
Member

ann0see commented Mar 29, 2021

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 :-(.

@ann0see ann0see self-requested a review March 29, 2021 11:59
Copy link
Member

@ann0see ann0see left a 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.

@pljones
Copy link
Collaborator

pljones commented Mar 31, 2021

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?

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.

Also on Linux this now results in Jamulus not connecting to JACK. I assume this is a much bigger topic :-(.

I've always avoided looking at the sound device code... :)

@gilgongo gilgongo linked an issue Apr 2, 2021 that may be closed by this pull request
@gilgongo
Copy link
Member

gilgongo commented Apr 2, 2021

Is this PR also related to #727 ?

@pljones
Copy link
Collaborator

pljones commented Apr 3, 2021

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...

@npostavs npostavs force-pushed the np.allow-invalid-audio branch from 52eaec6 to 1ed3043 Compare April 7, 2021 02:25
@npostavs
Copy link
Contributor Author

npostavs commented Apr 7, 2021

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).

Just tested what happens if you connect to a server with an invalid ASIO Driver. Jamulus crashes. This shouldn't happen.

Hmm, doesn't happen for me. I get an error message after a few seconds of "TRYING TO CONNECT". Screenshot

@ann0see
Copy link
Member

ann0see commented Apr 7, 2021

I think we shouldn't abuse the muted message.

Doing some tests now.

@ann0see
Copy link
Member

ann0see commented Apr 7, 2021

Ok. The crash only occurs if we only have one device (ASIO4ALL) which is wrongly configured and I connect to a server

@npostavs

This comment has been minimized.

@npostavs npostavs force-pushed the np.allow-invalid-audio branch from 1ed3043 to 7c549d9 Compare April 13, 2021 01:45
@npostavs
Copy link
Contributor Author

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.

@ann0see
Copy link
Member

ann0see commented Apr 13, 2021

Thanks!

Anyway those should be fixed now, and I also disabled the "Connect" button when audio isn't working.

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.)

@npostavs npostavs force-pushed the np.allow-invalid-audio branch from 7c549d9 to fc4fd2b Compare April 15, 2021 01:18
@npostavs
Copy link
Contributor Author

think we should not disable it (since you can just open the connect dialog via "view").

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?

@ann0see
Copy link
Member

ann0see commented Apr 18, 2021

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.

@ann0see
Copy link
Member

ann0see commented Apr 18, 2021

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.

@npostavs
Copy link
Contributor Author

Ok. Just tested it. Unfortunately double clicking a server still tries to connect.

I can't reproduce that.

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 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.

@ann0see
Copy link
Member

ann0see commented Apr 18, 2021

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?

Ok. Just tested it. Unfortunately double clicking a server still tries to connect.

I can't reproduce that.

Strange. At I tried it on Windows 10 with ASIO4ALL only and it did happen. Can you try it in a VM?

@npostavs
Copy link
Contributor Author

npostavs commented Apr 18, 2021

what about a deliberate error message with a prominent "Yes, change my audio device" or "Open settings to select another audio device" button?

Yeah, I think if the user makes the choice it should be okay.

Strange. At I tried it on Windows 10 with ASIO4ALL only and it did happen. Can you try it in a VM?

Um, not easily. But since CConnectDlg::OnServerListItemDoubleClicked does nothing but call OnConnectClicked, I can't see how double clicking could behave any different. I'm on Windows 10 as well (though not using ASIO4ALL since I haven't been able to get it to disable). Just to be clear, you're not getting anything like this: Screenshot

@ann0see
Copy link
Member

ann0see commented Apr 18, 2021

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.

@npostavs npostavs force-pushed the np.allow-invalid-audio branch from b4afc90 to 83d80be Compare January 14, 2022 23:26
@npostavs npostavs force-pushed the np.allow-invalid-audio branch from 83d80be to d304a71 Compare January 14, 2022 23:32
@npostavs
Copy link
Contributor Author

Yes, the additional buttons apply across all platforms, conflicts should be resolved now.

@ann0see
Copy link
Member

ann0see commented Jan 16, 2022

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 think it would be good to have Volker have a look at the code since he's the author.

@pgScorpio
Copy link
Contributor

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.
Also after "Auto Select device" and then changing back to the faulty device the settings window still shows the previous drivers channel mapping.

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.

@npostavs
Copy link
Contributor Author

npostavs commented Jan 16, 2022

P.S. To quickly check a faulty device I use the KoordASIO driver with a mono input device selected.

I don't have such a device, so I can't reproduce this easily. Are you getting the The selected audio device is incompatible since it doesn't support 2 in/out channels. Please select another device or configuration. message?

EDIT: forcing the lNumInChan variable in CSound::CheckDeviceCapabilities to 1 with the debugger still lets me open the "ASIO Device settings".

@pgScorpio
Copy link
Contributor

I don't have such a device

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.

Are you getting the The selected audio device is incompatible since it doesn't support 2 in/out channels. Please select another device or configuration. message?

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.

@pgScorpio
Copy link
Contributor

EDIT: forcing the lNumInChan variable in...

That's no option for the average user though....

@npostavs
Copy link
Contributor Author

No I get "Couldn't initialise the audio driver. Check if your audio hardware is plugged in and verify your driver settings"

Ah, I see where that's from. I think avoiding the removeCurrentDriver call should fix this, a 2nd case of #1378 (comment). Can you try out the new executable at https://github.com/jamulussoftware/jamulus/actions/runs/1705718177

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?

EDIT: forcing the lNumInChan variable in...

That's no option for the average user though....

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.

@pgScorpio
Copy link
Contributor

pgScorpio commented Jan 17, 2022

try out the new executable

i just tested jamulus_3.8.1dev-3838fdd_win.exe with mixed results.

It's not clear to me what causes that error though

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.).
Note: The device is configured OK in windows and working with ASIO4ALL (2.15Beta1), so this might be a KoordASIO bug as well.
EDIT: Same KoordASIO config works in Reaper, so apparently no KoordASIO bug.

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?
Also I found myself accidentally hitting this button several times, maybe it would be better to have an "auto-select" option in the device drop down list? I think this would be more logical, and it also doesn't take up extra space in the UI.

@npostavs
Copy link
Contributor Author

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?)

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?

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.).

By "mic working" you can see the levels go up when you make noise then? Even though you don't get any echo back?

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?

I think this is a KoordASIO problem, it doesn't send the kAsioResetRequest message after changing settings.

And another question: Is it right that the "Auto 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?
Also I found myself accidentally hitting this button several times, maybe it would be better to have an "auto-select" option in the device drop down list? I think this would be more logical, and it also doesn't take up extra space in the UI.

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.

@pgScorpio
Copy link
Contributor

Are you in shared or exclusive mode?

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.

By "mic working" you can see the levels go up when you make noise then?

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.)

I think this is a KoordASIO problem, it doesn't send the kAsioResetRequest message after changing settings.

Might be. But isn't it always wise to reset before connecting, just to make sure the latest settings are loaded?

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.

That would be great! Shouldn't be too hard to implement the drop down option.... ;=)

@ann0see
Copy link
Member

ann0see commented Feb 25, 2022

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.

@pgScorpio
Copy link
Contributor

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'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.)

I‘d suggest to open/link the appropriate issues though.

Please explain what what you mean here...

@ann0see
Copy link
Member

ann0see commented Feb 26, 2022

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.

@ann0see ann0see added the stale label Mar 4, 2022
@pgScorpio
Copy link
Contributor

pgScorpio commented Mar 7, 2022

There are issues which could be solved with this PR. They should be found and linked to this PR.

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).

Great to hear you found issues on the macOS part.

And not only in the macOS part. ;=)

I'm quite far with the CSoundBase redesign and the Windows implementation....
I will post an update in the "audio redesign" PR soon.

@ann0see
Copy link
Member

ann0see commented Mar 7, 2022

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.

@pgScorpio
Copy link
Contributor

Great thanks. Yes, sometimes changing in/output devices in ASIO4ALL freezes Jamulus,

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.
No idea what is happening here, but it then sometimes freezes in Windows itself while trying to play the sound of a messagebox but I can't find out why, since when this happens the debugger doesn't show any Jamulus code in the call stack, anymore, just Windows code itself).

if ASIO4ALL is not the only driver you can't change it (that's what you mentioned), failed audio drivers don't reload,...

?? What I mentioned was that you can't correct a wrong driver configuration when the driver has no own "offline settings" (not just ASIO4ALL)
But with the redesign you will always be able to to open ASIO device settings when a device has incorrect configuration....

I hope your changes get accepted,

So do I ! There are a lot of improvements.... (I will post the most important ones in a separate post.)

but maybe you need to give micro pr's instead (one for each OS). Especially the ASIO part is tricky - as you know.

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...
Should I close this PR for now, while working on it on my own branch ?

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?).

@pgScorpio pgScorpio mentioned this pull request Mar 13, 2022
5 tasks
@gilgongo gilgongo removed the stale label Mar 20, 2022
@ann0see
Copy link
Member

ann0see commented Apr 11, 2022

@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!

@ann0see ann0see closed this Apr 11, 2022
@pljones pljones removed this from the Release 3.9.0 milestone May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants