Skip to content

Client: Avoid losing gain changes #2737

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.

Sign up for GitHub

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

Merged
hoffie merged 1 commit into from
Jul 18, 2022
Merged

Client: Avoid losing gain changes #2737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ CClient::CClient ( const quint16 iPortNumber,

QObject::connect ( &Channel, &CChannel::ReqChanInfo, this, &CClient::OnReqChanInfo );

// The first ConClientListMesReceived handler performs the necessary cleanup and has to run first:
QObject::connect ( &Channel, &CChannel::ConClientListMesReceived, this, &CClient::OnConClientListMesReceived );
Copy link
Copy Markdown
Member Author

@hoffie hoffie Jul 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

@pljones pljones Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

QObject::connect ( &Channel, &CChannel::Disconnected, this, &CClient::Disconnected );
Expand Down Expand Up @@ -261,6 +263,15 @@ void CClient::OnJittBufSizeChanged ( int iNewJitBufSize )

void CClient::OnNewConnection()
{
// The oldGain and newGain arrays are used to avoid sending duplicate gain change messages.
// As these values depend on the channel IDs of a specific server, we have
// to reset those upon connect.
// We reset to 1 because this is what the server part sets by default.
for ( int iId = 0; iId < MAX_NUM_CHANNELS; iId++ )
{
oldGain[iId] = newGain[iId] = 1;
}

// a new connection was successfully initiated, send infos and request
// connected clients list
Channel.SetRemoteInfo ( ChannelInfo );
Expand All @@ -280,6 +291,33 @@ void CClient::OnNewConnection()
//### TODO: END ###//
}

void CClient::OnConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo )
{
// Upon receiving a new client list, we have to reset oldGain and newGain
// entries for unused channels. This ensures that a disconnected channel
// does not leave behind wrong cached gain values which would leak into
// any new channel which reused this channel id.
int iNumConnectedClients = vecChanInfo.Size();

// Save what channel IDs are in use:
bool bChanIdInUse[MAX_NUM_CHANNELS] = {};
for ( int i = 0; i < iNumConnectedClients; i++ )
{
bChanIdInUse[vecChanInfo[i].iChanID] = true;
}

// Reset all gains for unused channel IDs:
for ( int iId = 0; iId < MAX_NUM_CHANNELS; iId++ )
{
if ( !bChanIdInUse[iId] )
{
// 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;
}
}
}

void CClient::CreateServerJitterBufferMessage()
{
// per definition in the client: if auto jitter buffer is enabled, both,
Expand Down
1 change: 1 addition & 0 deletions src/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ protected slots:
void OnControllerInFaderIsMute ( int iChannelIdx, bool bIsMute );
void OnControllerInMuteMyself ( bool bMute );
void OnClientIDReceived ( int iChanID );
void OnConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo );

signals:
void ConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo );
Expand Down