Client: Avoid losing gain changes #2737

hoffie · 2022-07-17T14:32:09Z

Short description of changes
PR #2535 introduced rate limiting for gain change messages. The logic required storing the previously used gain value per channel. This logic had some flaws:

The previously used gain value defaulted to 0, despite the server-side view of the channel being set to 1 (as the default). Therefore, gain(0) changes during a series of gain changes would be lost. The most common scenario would be the initial connection, which always triggers the rate limit and therefore the faulty logic. This also affected New Client Level = 0.
The previously used gain values were not reset upon changing servers. This might have caused losing arbitrary gain change messages, e.g. stored fader values.
The previously used gain values were not reset upon a channel disconnect. This might have caused missing fader level restores.

This PR introduces a gain level memory reset to 1 (100%) on connect as well as on channel disconnects to fix these issues.

CHANGELOG: Client: Fix potential long delay in sending fader changes to the server.

Context: Fixes an issue?

Fixes: #2730

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Ready for review and testing.

What is missing until this pull request can be merged?

Needs more testing / confirmation.
@rdica Can you please try this PR?

Helpful debug patches to confirm proper working

diff --git a/src/client.cpp b/src/client.cpp
index ad9beb62..509dbaf7 100644
--- a/src/client.cpp
+++ b/src/client.cpp
@@ -314,6 +314,11 @@ void CClient::OnConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo )
             // reset oldGain and newGain as this channel id is currently unused and will
             // start with a server-side gain at 1 (100%) again.
             oldGain[iId] = newGain[iId] = 1;
+            qInfo() << "OnConClientListMesReceived: resetting old/newGain[" << iId << "] as it is unused";
+        }
+        else
+        {
+            qInfo() << "OnConClientListMesReceived: not resetting old/newGain[" << iId << "] as it is still in use";
         }
     }
 }
@@ -428,6 +433,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();
 }
@@ -445,6 +452,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];
         }
     }

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want: This PR fixes the mentioned bug for me and the debug log seems ok.
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

hoffie · 2022-07-17T14:33:59Z

src/client.cpp

@@ -119,6 +119,7 @@ CClient::CClient ( const quint16  iPortNumber,
    QObject::connect ( &Channel, &CChannel::ReqChanInfo, this, &CClient::OnReqChanInfo );

    QObject::connect ( &Channel, &CChannel::ConClientListMesReceived, this, &CClient::ConClientListMesReceived );
+    QObject::connect ( &Channel, &CChannel::ConClientListMesReceived, this, &CClient::OnConClientListMesReceived );


I'm a bit unsure what is better here:

Use a single method to handle both cases (would require to re-emit the signal though, I think?)

or use two connect() calls for those unrelated jobs.

I chose the latter as the jobs are unrelated (besides sharing the same trigger).

This, basically, is the fundamental flaw in the current design.

The Client is simply reemitting the signal on line 123 without "knowing" about it. I think it would be a better approach for the Client to handle all signals, hold state for what they "mean" and have GUI/JSONRPC/etc send state updates as signals. The client could them emit specific state change messages (i.e. in this case, which channel changed, one emit per channel, along with the new state).

However, for now, I'd be quite happy with either approach - either the new handler emitting CClient::ConClientListMesReceived when complete or the one taken here.

src/client.cpp

rdica · 2022-07-17T16:38:06Z

@hoffie Compiled 3.9.0beta2 on Ubuntu 18.04 with these changes.
Confirmed expected behaviour.

src/client.cpp

ann0see · 2022-07-17T19:20:54Z

@rdica could you please re-test with the new approach?

rdica · 2022-07-17T19:36:12Z

@ann0see Do I clone your repo/branch for this?

ann0see · 2022-07-17T19:38:03Z

No. Just use the updated code from hoffie. (Clone his repo again and ensure you have the code from this PR. My PR is somewhat related approach wise but doesn’t fix the bug.

rdica · 2022-07-17T19:54:36Z

@ann0see @hoffie Either I did something incorrectly, or the new code didn't work. I fresh cloned hoffies's repo, bulit, joined an active server with new client level set to 0, and heard audio.

ann0see · 2022-07-17T20:13:50Z

Did you check out the correct branch?

rdica · 2022-07-17T20:17:22Z

I grabbed the zip from https://github.com/hoffie/jamulus/tree/fix-lost-gain-changes

ann0see · 2022-07-17T20:21:38Z

Ok. So probably my approach is wrong. But could you please again make clean and test it (I remember it worked as I tested it with the code in the suggestion)

ann0see · 2022-07-17T20:24:41Z

@hoffie maybe The initialisation is wrong https://m.cplusplus.com/forum/general/62832/

Edit: probably not

rdica · 2022-07-17T20:36:44Z

Leave a comment
I recloned the above URL, ran git checkout fix-lost-gain-changes, rebuilt. Decided to check the program ver and its 3.8.2dev-d3022da1. I'm confused.

ann0see · 2022-07-17T20:41:56Z

d3022da That’s what the version of this PR should show if you are using git

rdica · 2022-07-17T20:51:33Z

Ok well I thought the problem was introduced in 3.9.0beta1...3.8.2 already behaves correctly, not sure what I'm testing here. Any way, I executed the binary created from above and got the expected behaviour with new client level set at 0, and joining a populated server.

PR jamulussoftware#2535 introduced rate limiting for gain change messages. The logic required storing the previously used gain value per channel. This logic had some flaws: 1. The previously used gain value defaulted to 0, despite the server-side view of the channel being set to 1 (as the default). Therefore, gain(0) changes during a series of gain changes would be lost. The most common scenario would be the initial connection, which always triggers the rate limit and therefore the faulty logic. This also affected New Client Level = 0. 2. The previously used gain values were not reset upon changing servers. This might have caused losing arbitrary gain change messages, e.g. stored fader values. 3. The previously used gain values were not reset upon a channel disconnect. This might have caused missing fader level restores. This commit introduces a gain level memory reset to 1 (100%) on connect as well as on channel disconnects to fix these issues. Fixes: jamulussoftware#2730 Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>

hoffie · 2022-07-17T21:09:35Z

Decided to check the program ver and its 3.8.2dev-d3022da1. I'm confused.

@rdica It's just that the non-release versioning is confusing. 3.8.2dev-XXX describes a version after 3.8.2 (contrary to what everyone else in the world seems to be doing; also against semantic versioning practices).

As far as I understand, a fresh binary from this PR works well for you, so that would be good.

@ann0see The code part which we changed after initially opening this PR is probably not relevant for the "joining a fresh server" case, so it's unlikely that those modifications broke the fix. I've just re-checked the debug output and it continues to seem correct.

softins

I don't have any means of trying this out for a couple of weeks, but I have studied the changes, and they look good to me. Thanks to @hoffie and @ann0see for the work!

rdica · 2022-07-17T21:37:01Z

@hoffie @ann0see Thanks for the explanations and assistance with getting the code to test with.

pljones · 2022-07-18T06:25:41Z

CHANGELOG: (List together with #2535)

Please don't do this. It makes editing the ChangeLog really hard. Put the title of the issue being fixed. Duplicates are easy to spot and the grouping mechanism works. With this approach the grouping mechanism doesn't always get the two anywhere near each other.

ann0see · 2022-07-18T18:31:47Z

@hoffie I think we can merge this.

hoffie · 2022-07-18T19:30:15Z

Put the title of the issue being fixed.

I've now put the same title as the PR which initially caused the issue, so it should be right next to each other.

hoffie added this to the Release 3.9.0 milestone Jul 17, 2022

hoffie requested review from pljones and softins July 17, 2022 14:32

hoffie commented Jul 17, 2022

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

hoffie force-pushed the fix-lost-gain-changes branch from 0b50988 to b706b50 Compare July 17, 2022 14:38

ann0see reviewed Jul 17, 2022

View reviewed changes

src/client.cpp Outdated Show resolved Hide resolved

hoffie force-pushed the fix-lost-gain-changes branch from b706b50 to d3022da Compare July 17, 2022 18:49

ann0see mentioned this pull request Jul 17, 2022

Improve algorithm to find in use channels in O(n) instead O(n²) #2738

Merged

7 tasks

hoffie force-pushed the fix-lost-gain-changes branch from d3022da to 9835897 Compare July 17, 2022 21:01

softins approved these changes Jul 17, 2022

View reviewed changes

pljones approved these changes Jul 18, 2022

View reviewed changes

hoffie merged commit 48168a6 into jamulussoftware:master Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client: Avoid losing gain changes #2737

Client: Avoid losing gain changes #2737

hoffie commented Jul 17, 2022 •

edited

Loading

hoffie Jul 17, 2022

pljones Jul 18, 2022

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022 •

edited

Loading

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

ann0see commented Jul 17, 2022 •

edited

Loading

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

rdica commented Jul 17, 2022

hoffie commented Jul 17, 2022

softins left a comment

rdica commented Jul 17, 2022

pljones commented Jul 18, 2022

ann0see commented Jul 18, 2022

hoffie commented Jul 18, 2022

Client: Avoid losing gain changes #2737

Client: Avoid losing gain changes #2737

Conversation

hoffie commented Jul 17, 2022 • edited Loading

Checklist

hoffie Jul 17, 2022

Choose a reason for hiding this comment

pljones Jul 18, 2022

Choose a reason for hiding this comment

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022 • edited Loading

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

ann0see commented Jul 17, 2022 • edited Loading

rdica commented Jul 17, 2022

ann0see commented Jul 17, 2022

rdica commented Jul 17, 2022

hoffie commented Jul 17, 2022

softins left a comment

Choose a reason for hiding this comment

rdica commented Jul 17, 2022

pljones commented Jul 18, 2022

ann0see commented Jul 18, 2022

hoffie commented Jul 18, 2022

hoffie commented Jul 17, 2022 •

edited

Loading

ann0see commented Jul 17, 2022 •

edited

Loading

ann0see commented Jul 17, 2022 •

edited

Loading