-
Notifications
You must be signed in to change notification settings - Fork 242
Add rate-limiting for channel gain change messages #2535
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,11 @@ CClient::CClient ( const quint16 iPortNumber, | |
| // start timer so that elapsed time works | ||
| PreciseTime.start(); | ||
|
|
||
| // set gain delay timer to single-shot and connect handler function | ||
| TimerGain.setSingleShot ( true ); | ||
|
|
||
| QObject::connect ( &TimerGain, &QTimer::timeout, this, &CClient::OnTimerRemoteChanGain ); | ||
|
|
||
| // start the socket (it is important to start the socket after all | ||
| // initializations and connections) | ||
| Socket.Start(); | ||
|
|
@@ -299,6 +304,8 @@ void CClient::OnCLPingReceived ( CHostAddress InetAddr, int iMs ) | |
| const int iCurDiff = EvaluatePingMessage ( iMs ); | ||
| if ( iCurDiff >= 0 ) | ||
| { | ||
| iCurPingTime = iCurDiff; // store for use by gain message sending | ||
|
|
||
| emit PingTimeReceived ( iCurDiff ); | ||
| } | ||
| } | ||
|
|
@@ -335,15 +342,97 @@ void CClient::SetDoAutoSockBufSize ( const bool bValue ) | |
| CreateServerJitterBufferMessage(); | ||
| } | ||
|
|
||
| // In order not to flood the server with gain change messages, particularly when using | ||
| // a MIDI controller, a timer is used to limit the rate at which such messages are generated. | ||
| // This avoids a potential long backlog of messages, since each must be ACKed before the next | ||
| // can be sent, and this ACK is subject to the latency of the server connection. | ||
| // | ||
| // When the first gain change message is requested after an idle period (i.e. the timer is not | ||
| // running), it will be sent immediately, and a 300ms timer started. | ||
| // | ||
| // If a gain change message is requested while the timer is still running, the new gain is not sent, | ||
| // but just stored in newGain[iId], and the minGainId and maxGainId updated to note the range of | ||
| // IDs that must be checked when the time expires (this will usually be a single channel | ||
| // unless channel grouping is being used). This avoids having to check all possible channels. | ||
|
Comment on lines
+354
to
+356
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we are talking about worst case n=200 or something and this is not part of the critical path (sound/network processing) and not part of something which scales exponentially (e.g. server-side per-client-per-client stuff), I think I would have gone with the check-all-channels approach in order to keep the logic simpler and shorter. PS: Now this whole block comment provides value and is well-written! :)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but I figured if it is possible at little cost to limit the number of checks needed to the specific one or a few out of the 200 channels, it's worth doing. And of course include an explanation, as I did :) |
||
| // | ||
| // When the timer fires, the channels minGainId <= iId < maxGainId are checked by comparing | ||
| // the last sent value in oldGain[iId] with any pending value in newGain[iId], and if they differ, | ||
| // the new value is sent, updating oldGain[iId] with the sent value. If any new values are | ||
| // sent, the timer is restarted so that further immediate updates will be pended. | ||
|
|
||
| void CClient::SetRemoteChanGain ( const int iId, const float fGain, const bool bIsMyOwnFader ) | ||
| { | ||
| QMutexLocker locker ( &MutexGain ); | ||
|
|
||
| // if this gain is for my own channel, apply the value for the Mute Myself function | ||
| if ( bIsMyOwnFader ) | ||
| { | ||
| fMuteOutStreamGain = fGain; | ||
| } | ||
|
|
||
| if ( TimerGain.isActive() ) | ||
| { | ||
| // just update the new value for sending later; | ||
| // will compare with oldGain[iId] when the timer fires | ||
| newGain[iId] = fGain; | ||
|
|
||
| // update range of channel IDs to check in the timer | ||
| if ( iId < minGainId ) | ||
| minGainId = iId; // first value to check | ||
| if ( iId >= maxGainId ) | ||
| maxGainId = iId + 1; // first value NOT to check | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // here the timer was not active: | ||
| // send the actual gain and reset the range of channel IDs to empty | ||
| oldGain[iId] = newGain[iId] = fGain; | ||
| Channel.SetRemoteChanGain ( iId, fGain ); | ||
|
|
||
| StartDelayTimer(); | ||
| } | ||
|
|
||
| void CClient::OnTimerRemoteChanGain() | ||
| { | ||
| QMutexLocker locker ( &MutexGain ); | ||
| bool bSent = false; | ||
|
|
||
| for ( int iId = minGainId; iId < maxGainId; iId++ ) | ||
| { | ||
| if ( newGain[iId] != oldGain[iId] ) | ||
| { | ||
| // send new gain and record as old gain | ||
| float fGain = oldGain[iId] = newGain[iId]; | ||
| Channel.SetRemoteChanGain ( iId, fGain ); | ||
| bSent = true; | ||
| } | ||
| } | ||
|
|
||
| // if a new gain has been sent, reset the range of channel IDs to empty and start timer | ||
| if ( bSent ) | ||
| { | ||
| StartDelayTimer(); | ||
| } | ||
| } | ||
|
|
||
| // reset the range of channel IDs to check and start the delay timer | ||
| void CClient::StartDelayTimer() | ||
| { | ||
| maxGainId = 0; | ||
| minGainId = MAX_NUM_CHANNELS; | ||
|
|
||
| // start timer to delay sending further updates | ||
| // use longer delay when connected to server with higher ping time, | ||
| // double the ping time in order to allow a bit of overhead for other messages | ||
| if ( iCurPingTime < DEFAULT_GAIN_DELAY_PERIOD_MS / 2 ) | ||
| { | ||
| TimerGain.start ( DEFAULT_GAIN_DELAY_PERIOD_MS ); | ||
| } | ||
| else | ||
| { | ||
| TimerGain.start ( iCurPingTime * 2 ); | ||
| } | ||
| } | ||
|
|
||
| bool CClient::SetServerAddr ( QString strNAddr ) | ||
|
|
||
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.
That comment almost repeats the variable/method names and provides little added value, but I guess it's ok.. :)