Regression: Gain changes not applied in all cases (e.g. new client level = 0)

[hoffie]

I started up a fresh instance of 3.9.0beta1 (compiled from src on Ubuntu 18.04) with no existing ini file, set a username, stereo out, new client level at 0%, connected to a server with players on it (Jamsuckers 212.83.56.10 on Any Genre 1, running version 3.8.2dev-1646e905 on Linux).

I expected no audio as new client was at 0% and indeed all the sliders were at 0 yet there was audio. I double checked to make sure I hadn't accidently looped a jack port mapping but there wasn't one. I further tested this behaviour against other servers with players on them, deleting the ini file and starting from scratch with the same settings each time. Server versions tested against were 3.8.1 and 3.8.2dev-c5111db6-dirty. All had the same client behaviour, there was audio even though all players sliders were at 0%.
I repeated above procedures using 1% and got the reduced volumes expected, which were quite lower in volume than what I heard when new client level was at 0%.

I tested same procedure with 3.8.2 (compiled from src on Ubuntu 18.04) against the same servers and got the expected behaviour of no audio when new client level was set to 0%.

Bug?

Originally posted by @rdica in #2725

I can confirm this behavior.

To be checked:

• Is the actual value relevant (i.e. is 0 or 1 problematic or is this just the most extreme effect?)
• Is new client level completely broken or does it only affect initial connect?
• The sliders look correct, but the mix is wrong. It seems like the levels aren't properly sent to the server. Should confirm this via Wireshark.
• Was the level change rate limiting merged post-3.8.2 and could it be related?

[pljones]

Having looked at the code... it's a nightmare.

The big, big problem is this is implemented in the mixer panel, which is a GUI component. It should be in the client, either directly in CClient or in a non-GUI class handling the mixer.

The latter seems the better approach -- currently CClient doesn't know very much about what appears in the main client GUI. Effectively splitting mixer into behaviour/state and a UI controller tied to it with signals/slots would clean things up and make them more consistent.

Unfortunately, it's not a quick fix, of course. For 3.9.0, given I've no idea what change broke it, I'm not going to propose this is done.

[hoffie]

Reproducing instructions:

• Start a server
• Connect a client with permanent audio
• Start a 3.9.0beta client with a fresh config, set new client level to 0, connect to the server.
• Watch new client level work properly.
• Disconnect
• Connect again
• Observe fader at 0%, but audio being at 100%
• This is repeatable.

Reverting #2535 (more specifically: 2db5156 and 68d5612) fixes it for me.

[hoffie]

I'm suspecting two problems here:

jamulus/src/client.cpp

Line 404 in fa204ad

if ( newGain[iId] != oldGain[iId] )

1. It seems like this array isn't reset on reconnect. Not sure if this is the relevant part here.
2. It looks like it defaults to 0. This is a mismatch compared to the server, which sets all channels to 1. Therefore, the don't-repeat-same-client-level-protocol-messages logic might omit a required set-gain-0 message.

   jamulus/src/server.cpp

   Line 1365 in fa204ad

   vecChannels[iNewChanID].SetGain ( i, 1.0 );

With the following debug patch, the problematic situation is confirmed:

diff --git a/src/client.cpp b/src/client.cpp
index 99f74caf..2a3ca9a7 100644
--- a/src/client.cpp
+++ b/src/client.cpp
@@ -390,6 +390,8 @@ void CClient::SetRemoteChanGain ( const int iId, const float fGain, const bool b
     // send the actual gain and reset the range of channel IDs to empty
     oldGain[iId] = newGain[iId] = fGain;
     Channel.SetRemoteChanGain ( iId, fGain );
+    qInfo() << "SetRemoteChanGain: directly sending gain changed message for iId =" << iId << ", oldGain =" << oldGain[iId]
+            << ", newGain =" << newGain[iId];
 
     StartDelayTimer();
 }
@@ -407,6 +409,13 @@ void CClient::OnTimerRemoteChanGain()
             float fGain = oldGain[iId] = newGain[iId];
             Channel.SetRemoteChanGain ( iId, fGain );
             bSent = true;
+            qInfo() << "OnTimerRemoteChainGain: sending gain changed message for iId =" << iId << ", oldGain =" << oldGain[iId]
+                    << ", newGain =" << newGain[iId];
+        }
+        else
+        {
+            qInfo() << "OnTimerRemoteChainGain: skipping unchanged gain changed message for iId =" << iId << ", oldGain =" << oldGain[iId]
+                    << ", newGain =" << newGain[iId];
         }
     }

SetRemoteChanGain: directly sending gain changed message for iId = 0 , oldGain = 1 , newGain = 1 (iId=0 is the 3.9.0 client)
OnTimerRemoteChainGain: skipping unchanged gain changed message for iId = 1 , oldGain = 0 , newGain = 0 (iId=1 is the playing client; oldGain=0 is a wrong assumption by the client, because oldGain is actually 1 (the default) at the server)

[pljones]

If I remember correctly, SetRemoteChanGain also checks for a new (i.e. previously unseen) channel. There might be some point where what oldGain[iId] references can be set to 1 to start with...

Thanks for digging into this.

[hoffie]

a791cff fixes it for me, but I have to do some more thinking, if this is correct or sufficient. I suspect we might still miss the case where a client disconnects and another client takes the existing channel id.

[hoffie]

If I remember correctly, SetRemoteChanGain also checks for a new (i.e. previously unseen) channel.

Do you mean this?

jamulus/src/client.cpp

Line 380 in fa204ad

// update range of channel IDs to check in the timer

I think it only optimizes the list of channels to check, doesn't it?

[ann0see]

Pinging @softins here for more insight.

[softins]

I can cast my rusty eye over the code later, but I'm away on holiday till 30 July with no access to hardware.

[hoffie]

I suspect we might still miss the case where a client disconnects and another client takes the existing channel id.

It's hard to trigger on that case. The protocol doesn't have a concept for "new channel", as far as I can see. The UI handles it by triggering on the client list updated here:

jamulus/src/audiomixerboard.cpp

Line 1225 in fa204ad

// the fader was not in use, reset everything for new client

via

jamulus/src/clientdlg.cpp

Line 871 in fa204ad

void CClientDlg::OnConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo )

I'm currently looking into hooking into this protocol message as well (not via UI of course) and resetting oldGain for unused channels there.