Unexpected reset of mixer levels

[hoffie]

Describe the bug

In a rehearsal with about 20 users which my client had seen before, I experienced unexpected resets of fader levels and mute state in my mix. I think this always happened when someone (re)connected. It always affected the same users and I suspect that mixer/mute state got restored to what was saved from a previous session.

To Reproduce

• Run a Jamulus server (should not matter if 3.6.2 or master)
• Connect Client A, B (based on current master code)
• In Client A's mix, mute B.
• Close Client A (to ensure that mixer levels are saved)
• Re-connect Client A
• In Client A's mix, unmute B.
• Connect Client C
• In Client A: Observe, that Client B gets set to muted again (to be verified)

The same happens to mixer levels.

Expected behavior

In Client A, B's mute state should not change unexpectedly.

Operating system

Arch Linux

Version of Jamulus

Current master (e7389e3) for both server and at least the problematic client.

Additional context

#1009 seems to be the culprit. My assumption there was that it would always be safe to load stored mixer settings. I suspect that this is not the case and that mixer settings are only stored on specific events (client exit?). If this is the case,

• we could revert Client: Always update gain levels #1009 and solely rely on Server: Send empty channel list on connect #1010 to keep Restarting the server on the fly ignores set levels and mixes everybody at 100% #955 fixed or
• we could ensure that mixer state is stored more regularly (e.g. any time something changes or timer-based).

I'd prefer the second option. I think it might even fix two other edge cases:

• If a Jamulus client exits unexpectedly, all changed mixer state might not get stored (even in 3.6.2).
• If a Jamulus client sees a new user for the first time and the fader levels are adjusted, then they might not be stored immediately. If that new user disconnects and re-connects later, I suspect they will start with initial levels again. Update: Fader settings are stored when a fader slot becomes unused:

  jamulus/src/audiomixerboard.cpp

  Line 1219 in 22d34ed

  StoreFaderSettings ( vecpChanFader[i] );

#1010 may also trigger this issue, but I'd argue that this is not that bad. Without #1010, mixer state for all clients is wrong. With #1010, mixer state may still not be fully up-to-date, but it will at least be close to being up-to-date (I suspect mixer state will be the same as during the initial start of each Jamulus client).

• Reproduce with master-level server and master-level, clean client code and adapt issue title.
• Decide on a fix (I'd prefer optimization of the fader storage logic)

[hoffie]

StoreAllFaderSettings is called from the following places:

• CAudioMixerBoard::HideAll, which is called on client init and disconnect
• CClientDlg::closeEvent, e.g. during explicit closing of the Jamulus client
• CClientDlg::OnClearAllStoredSoloMuteSettings
• CClientDlg::OnSaveChannelSetup, e.g. when explicitly exporting fader settings

[hoffie]

I can reproduce this issue as described above, so this is now verified.
I also verified that reverting f116811 from #1009 makes the problem go away.

[ann0see]

@hoffie can we close this? Your PR seems to be merged.

[hoffie]

@hoffie can we close this? Your PR seems to be merged.

There is no PR yet, as far as I know? :)
The PRs referenced above are the PRs which introduced the issue.

I'm currently testing different fixes, but may only finish tomorrow. :(

[ann0see]

Strange. I thought you merged something today? Probably that was something else.

[hoffie]

Strange. I thought you merged something today? Probably that was something else.

Probably. ;)

Sadly I don't have a tested fix ready yet. I'm pasting below what I'd tried so far. I think the proper solution is to ensure that fader settings (+ pan and other relevant stuff) is stored immediately when it is sent to the server.
I'm not sure if the diff below works (there may be timing problems when signals are involved) and it might be missing something (Solo state?).

I will look into it tomorrow again. If someone wants to take over, feel free to, but please comment here.

diff --git a/src/audiomixerboard.cpp b/src/audiomixerboard.cpp
index 5a1ebe2d..c3e658ab 100755
--- a/src/audiomixerboard.cpp
+++ b/src/audiomixerboard.cpp
@@ -1382,6 +1382,7 @@ void CAudioMixerBoard::UpdateSoloStates()
         if ( vecpChanFader[i]->IsVisible() )
         {
             vecpChanFader[i]->UpdateSoloState ( bAnyChannelIsSolo );
+            StoreFaderSettings ( vecpChanFader[i] );
         }
     }
 }
@@ -1418,12 +1419,14 @@ void CAudioMixerBoard::UpdateGainValue ( const int    iChannelIdx,
             }
         }
     }
+    StoreFaderSettings ( vecpChanFader[iChannelIdx] );
 }
 
 void CAudioMixerBoard::UpdatePanValue ( const int   iChannelIdx,
                                         const float fValue )
 {
     emit ChangeChanPan ( iChannelIdx, fValue );
+    StoreFaderSettings ( vecpChanFader[iChannelIdx] );
 }
 
 void CAudioMixerBoard::StoreFaderSettings ( CChannelFader* pChanFader )

[hoffie]

I think the fix works, but two things will need to be verified/extended:

• I'm not sure if it's safe to call StoreFaderSettings() in all places. To be extra sure, we could introduce a variable which tracks if we are currently in the process if loading stored settings und should therefore not store anything.
• Solo settings are still lost. This may need a new signal from the fader to the audio mixer board:

@@ -903,6 +904,9 @@ inline void CAudioMixerBoard::connectFaderSignalsToMixerBoardSlots()
     QObject::connect ( vecpChanFader[iCurChanID], &CChannelFader::soloStateChanged,
         this, &CAudioMixerBoard::UpdateSoloStates );
 
+    QObject::connect ( vecpChanFader[iCurChanID], &CChannelFader::groupStateChanged,
+        this, &CAudioMixerBoard::UpdateGroupStates );
+

[hoffie]

After discussing with @softins asynchronously, the best way forward to get 3.7.0 done seems to be to revert the problematic PR #1009. #1210 does that.

I am keeping this issue open to track progress about fixing the root cause. We might want to split this into individual tasks though. I'm moving this issue to the 3.7.1 milestone for now (@gilgongo: META: This is probably the first time where it helps to treat issue & PR seperately on the project board ;)).
@softins Can you share your findings about the unused code you've found?