Skip to content
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

Connection status issue #2519 #2550

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3bb34f4
Trying to ignore my vs files
pgScorpio Mar 21, 2022
9aeceb0
Connection status issue #2519 first stage
pgScorpio Mar 25, 2022
f9f5e30
Added CMsgBoxes and CCommanlineOptions to global.h,main.cpp
pgScorpio Mar 23, 2022
24f59e0
Modified messageboxes to use the new CMsgBoxes class
pgScorpio Apr 5, 2022
0923cff
clang-format update (hopefully ok now)
pgScorpio Mar 23, 2022
03a8782
Solution for headless build?
pgScorpio Mar 24, 2022
b152344
clang-format update (hopefully ok now)
ann0see Mar 24, 2022
1299e73
Bugfix non Windows exit
pgScorpio Mar 26, 2022
553d98b
Connection status issue jamulussoftware#2519 second stage
pgScorpio Mar 26, 2022
a225596
Added CMsgBoxes and CCommanlineOptions to global.h,main.cpp
pgScorpio Mar 23, 2022
a96660a
Modified messageboxes to use the new CMsgBoxes class
pgScorpio Mar 23, 2022
6af7e96
Solution for headless build?
pgScorpio Mar 24, 2022
4570ec4
Connection status issue jamulussoftware#2519 second stage
pgScorpio Mar 26, 2022
7125707
Fixing merge errors
pgScorpio Mar 28, 2022
1b17d48
Added CMsgBoxes and CCommanlineOptions to global.h,main.cpp
pgScorpio Mar 23, 2022
cd7b785
Modified messageboxes to use the new CMsgBoxes class
pgScorpio Mar 23, 2022
a675833
Bugfix non Windows exit
pgScorpio Mar 26, 2022
95a6509
Connection status issue jamulussoftware#2519 second stage
pgScorpio Mar 26, 2022
8df0e29
Fixing rebase issues again...
pgScorpio Apr 5, 2022
beed54e
Fix comments on PR
pgScorpio Apr 5, 2022
648880c
Connection status issue #2519 first stage
pgScorpio Mar 25, 2022
ae07615
rebase
pgScorpio Apr 6, 2022
2b66414
rebase
pgScorpio Mar 26, 2022
b88e704
rebase
pgScorpio Apr 6, 2022
6ea6c19
fix rebase error
pgScorpio Apr 6, 2022
20fe0b0
Fixing autobuild issues
pgScorpio Apr 8, 2022
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
17 changes: 10 additions & 7 deletions .gitignore
Copy link
Member

Choose a reason for hiding this comment

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

No changes to be made

Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,27 @@
Jamulus
Jamulus.ini
Makefile
*.pro.user*
**.cppe
**.he
.cproject
.project
.settings
.idea
.xcode
.vscode
.vs
.cache
.DS_Store
*.user
*.user.*
*.pro.user*
*.o
*.sln
*.vcxproj
*.vcxproj.filters
*.xcodeproj
*.log
Jamulus*.ps1
moc_*.cpp
ui_*.h
moc_predefs.h
Expand All @@ -30,20 +39,14 @@ build/
deploy/
build-gui/
build-nox/
jamulus.sln
jamulus.vcxproj
jamulus.vcxproj.filters
Jamulus.app/
.DS_Store
distributions/opus*
distributions/jack2
distributions/claudio_piano.sf2
distributions/fluidsynth*
distributions/jamulus.desktop
distributions/jamulus-server.desktop
.xcode
Debug-iphoneos/
Jamulus.xcodeproj
jamulus_plugin_import.cpp
autoLatestChangelog.md
debian/
29 changes: 26 additions & 3 deletions src/channel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ CChannel::CChannel ( const bool bNIsServer ) :
bIsEnabled ( false ),
bIsServer ( bNIsServer ),
bIsIdentified ( false ),
bDisconnectAndDisable ( false ),
iAudioFrameSizeSamples ( DOUBLE_SYSTEM_FRAME_SIZE_SAMPLES ),
SignalLevelMeter ( false, 0.5 ) // server mode with mono out and faster smoothing
{
Expand Down Expand Up @@ -125,7 +126,8 @@ void CChannel::SetEnable ( const bool bNEnStat )
QMutexLocker locker ( &Mutex );

// set internal parameter
bIsEnabled = bNEnStat;
bIsEnabled = bNEnStat;
bDisconnectAndDisable = false;

// The support for the packet sequence number must be reset if the client
// disconnects from a server since we do not yet know if the next server we
Expand Down Expand Up @@ -506,11 +508,20 @@ void CChannel::Disconnect()
// we only have to disconnect the channel if it is actually connected
if ( IsConnected() )
{
// for a Client we will block further audio data and disable the channel as soon as disconnected;
bDisconnectAndDisable = !bIsServer;
Copy link
Member

Choose a reason for hiding this comment

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

Reasoning to be investigated.


// set time out counter to a small value > 0 so that the next time a
// received audio block is queried, the disconnection is performed
// (assuming that no audio packet is received in the meantime)
iConTimeOut = 1; // a small number > 0
}
else if ( !bIsServer )
{
bDisconnectAndDisable = false;
bIsEnabled = false;
iConTimeOut = 0;
}
}

void CChannel::PutProtocolData ( const int iRecCounter, const int iRecID, const CVector<uint8_t>& vecbyMesBodyData, const CHostAddress& RecHostAddr )
Expand All @@ -534,7 +545,7 @@ EPutDataStat CChannel::PutAudioData ( const CVector<uint8_t>& vecbyData, const i
// Only process audio data if:
// - for client only: the packet comes from the server we want to talk to
// - the channel is enabled
if ( ( bIsServer || ( GetAddress() == RecHostAddr ) ) && IsEnabled() )
if ( ( bIsServer || ( GetAddress() == RecHostAddr ) ) && IsEnabled() && !bDisconnectAndDisable )
{
MutexSocketBuf.lock();
{
Expand Down Expand Up @@ -622,6 +633,12 @@ EGetDataStat CChannel::GetData ( CVector<uint8_t>& vecbyData, const int iNumByte
eGetStatus = GS_CHAN_NOW_DISCONNECTED;
iConTimeOut = 0; // make sure we do not have negative values

if ( bDisconnectAndDisable )
{
bDisconnectAndDisable = false;
bIsEnabled = false;
}

// reset network transport properties
ResetNetworkTransportProperties();
}
Expand All @@ -643,6 +660,13 @@ EGetDataStat CChannel::GetData ( CVector<uint8_t>& vecbyData, const int iNumByte
{
// channel is disconnected
eGetStatus = GS_CHAN_NOT_CONNECTED;

if ( bDisconnectAndDisable )
{
bDisconnectAndDisable = false;
bIsEnabled = false;
iConTimeOut = 0;
}
}
}
MutexSocketBuf.unlock();
Expand All @@ -652,7 +676,6 @@ EGetDataStat CChannel::GetData ( CVector<uint8_t>& vecbyData, const int iNumByte
{
// reset the protocol
Protocol.Reset();

// emit message
emit Disconnected();
}
Expand Down
3 changes: 2 additions & 1 deletion src/channel.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class CChannel : public QObject

void PrepAndSendPacket ( CHighPrioSocket* pSocket, const CVector<uint8_t>& vecbyNPacket, const int iNPacketLen );

void ResetTimeOutCounter() { iConTimeOut = iConTimeOutStartVal; }
void ResetTimeOutCounter() { iConTimeOut = bDisconnectAndDisable ? 1 : iConTimeOutStartVal; }
Copy link
Member

Choose a reason for hiding this comment

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

How often do we use short hand if in the source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How often do we use short hand if in the source?

You mean conditional assignment ?
It is used at several places.
I use it whenever it makes the code more readable... (at least to me ;=))

Copy link
Member

Choose a reason for hiding this comment

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

I use it whenever it makes the code more readable

I usually think it makes code less readable ;-). But in this case I think it's ok - but that depends on if this is used elsewhere too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where's it's a simple "truth" test to pick between two values based on another value, it's okay, in general, I'd say. (I don't think Volker was keen on it.) Anything else (e.g. an expression getting evaluated as any part of the ?: expression).

Copy link
Member

Choose a reason for hiding this comment

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

To be investigated. May need comment.

bool IsConnected() const { return iConTimeOut > 0; }
void Disconnect();

Expand Down Expand Up @@ -216,6 +216,7 @@ void CreateReqChannelLevelListMes() { Protocol.CreateReqChannelLevelListMes(); }
bool bIsEnabled;
bool bIsServer;
bool bIsIdentified;
bool bDisconnectAndDisable;

int iNetwFrameSizeFact;
int iNetwFrameSize;
Expand Down
4 changes: 2 additions & 2 deletions src/chatdlg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ void CChatDlg::OnAnchorClicked ( const QUrl& Url )
// only allow http(s) URLs to be opened in an external browser
if ( Url.scheme() == QLatin1String ( "https" ) || Url.scheme() == QLatin1String ( "http" ) )
{
if ( QMessageBox::question ( this,
APP_NAME,
if ( QMessageBox::question ( CMsgBoxes::MainForm(),
Copy link
Member

Choose a reason for hiding this comment

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

Not part of this issue. In recreation -> don't include

CMsgBoxes::MainFormName(),
tr ( "Do you want to open the link '%1' in your browser?" ).arg ( "<b>" + Url.toString() + "</b>" ),
QMessageBox::Yes | QMessageBox::No ) == QMessageBox::Yes )
{
Expand Down
150 changes: 85 additions & 65 deletions src/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
/* Implementation *************************************************************/
CClient::CClient ( const quint16 iPortNumber,
const quint16 iQosNumber,
const QString& strConnOnStartupAddress,
const QString& strMIDISetup,
const bool bNoAutoJackConnect,
const QString& strNClientName,
Expand Down Expand Up @@ -181,13 +180,6 @@ CClient::CClient ( const quint16 iPortNumber,
// start the socket (it is important to start the socket after all
// initializations and connections)
Socket.Start();

Copy link
Member

Choose a reason for hiding this comment

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

In depth code review needed to understand changes.

// do an immediate start if a server address is given
if ( !strConnOnStartupAddress.isEmpty() )
{
SetServerAddr ( strConnOnStartupAddress );
Start();
}
}

CClient::~CClient()
Expand Down Expand Up @@ -298,7 +290,7 @@ void CClient::CreateServerJitterBufferMessage()
void CClient::OnCLPingReceived ( CHostAddress InetAddr, int iMs )
{
// make sure we are running and the server address is correct
if ( IsRunning() && ( InetAddr == Channel.GetAddress() ) )
if ( Channel.IsEnabled() && ( InetAddr == Channel.GetAddress() ) )
{
// take care of wrap arounds (if wrapping, do not use result)
const int iCurDiff = EvaluatePingMessage ( iMs );
Expand Down Expand Up @@ -435,22 +427,6 @@ void CClient::StartDelayTimer()
}
}

bool CClient::SetServerAddr ( QString strNAddr )
{
CHostAddress HostAddress;
if ( NetworkUtil().ParseNetworkAddress ( strNAddr, HostAddress, bEnableIPv6 ) )
{
// apply address to the channel
Channel.SetAddress ( HostAddress );

return true;
}
else
{
return false; // invalid address
}
}

bool CClient::GetAndResetbJitterBufferOKFlag()
{
// get the socket buffer put status flag and reset it
Expand Down Expand Up @@ -586,9 +562,12 @@ QString CClient::SetSndCrdDev ( const QString strNewDev )
// in case of an error inform the GUI about it
if ( !strError.isEmpty() )
{
emit SoundDeviceChanged ( strError );
Copy link
Member

Choose a reason for hiding this comment

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

Investigate if this is related.

QMessageBox::critical ( 0, APP_NAME, strError );
Disconnect();
}

emit SoundDeviceChanged();

return strError;
}

Expand Down Expand Up @@ -715,8 +694,13 @@ void CClient::OnSndCrdReinitRequest ( int iSndCrdResetType )
}
MutexDriverReinit.unlock();

if ( !strError.isEmpty() )
Copy link
Member

Choose a reason for hiding this comment

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

Investigate if related

{
QMessageBox::critical ( 0, APP_NAME, strError );
}

// inform GUI about the sound card device change
emit SoundDeviceChanged ( strError );
emit SoundDeviceChanged();
}

void CClient::OnHandledSignal ( int sigNum )
Expand All @@ -731,10 +715,7 @@ void CClient::OnHandledSignal ( int sigNum )
case SIGINT:
case SIGTERM:
// if connected, terminate connection (needed for headless mode)
if ( IsRunning() )
{
Stop();
}
Disconnect();
Copy link
Member

Choose a reason for hiding this comment

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

Check code validity. Semantic change? Add preconditions to function if needed.


// this should trigger OnAboutToQuit
QCoreApplication::instance()->exit();
Expand Down Expand Up @@ -819,50 +800,89 @@ void CClient::OnClientIDReceived ( int iChanID )
emit ClientIDReceived ( iChanID );
}

void CClient::Start()
bool CClient::Connect ( QString strServerAddress, QString strServerName )
{
// init object
Init();
if ( !Channel.IsEnabled() )
{
CHostAddress HostAddress;

if ( NetworkUtil().ParseNetworkAddress ( strServerAddress, HostAddress, bEnableIPv6 ) )
{
// init object
Init();

// apply address to the channel
Channel.SetAddress ( HostAddress );

// enable channel
Channel.SetEnable ( true );
// enable channel
Channel.SetEnable ( true );

// start audio interface
Sound.Start();
// start audio interface
Sound.Start();

// Notify ClientDlg
emit Connecting ( strServerName );

return true;
}
}

return false;
}

void CClient::Stop()
bool CClient::Disconnect()
{
// stop audio interface
Sound.Stop();

// disable channel
Channel.SetEnable ( false );

// wait for approx. 100 ms to make sure no audio packet is still in the
// network queue causing the channel to be reconnected right after having
// received the disconnect message (seems not to gain much, disconnect is
// still not working reliably)
QTime DieTime = QTime::currentTime().addMSecs ( 100 );
while ( QTime::currentTime() < DieTime )
if ( Channel.IsEnabled() )
Copy link
Member

Choose a reason for hiding this comment

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

Do manual diff comparison to detect changes.

{
// exclude user input events because if we use AllEvents, it happens
// that if the user initiates a connection and disconnection quickly
// (e.g. quickly pressing enter five times), the software can get into
// an unknown state
QCoreApplication::processEvents ( QEventLoop::ExcludeUserInputEvents, 100 );
// start disconnection
Channel.Disconnect();

// Channel.Disconnect() should now automatically disable Channel as soon as disconnected.
// Note that this only works if Sound is Active !

QTime DieTime = QTime::currentTime().addMSecs ( 500 );
while ( ( QTime::currentTime() < DieTime ) && Channel.IsEnabled() )
{
// exclude user input events because if we use AllEvents, it happens
// that if the user initiates a connection and disconnection quickly
// (e.g. quickly pressing enter five times), the software can get into
// an unknown state
QCoreApplication::processEvents ( QEventLoop::ExcludeUserInputEvents, 100 );
}

// Now stop the audio interface
Sound.Stop();

// in case we timed out, log warning and make sure Channel is disabled
if ( Channel.IsEnabled() )
{
Channel.SetEnable ( false );
}

// Send disconnect message to server (Since we disable our protocol
// receive mechanism with the next command, we do not evaluate any
// respond from the server, therefore we just hope that the message
// gets its way to the server, if not, the old behaviour time-out
// disconnects the connection anyway).
ConnLessProtocol.CreateCLDisconnection ( Channel.GetAddress() );

// reset current signal level and LEDs
bJitterBufferOK = true;
SignalLevelMeter.Reset();

emit Disconnected();

return true;
}
else
{
// make sure sound is stopped too
Sound.Stop();

// Send disconnect message to server (Since we disable our protocol
// receive mechanism with the next command, we do not evaluate any
// respond from the server, therefore we just hope that the message
// gets its way to the server, if not, the old behaviour time-out
// disconnects the connection anyway).
ConnLessProtocol.CreateCLDisconnection ( Channel.GetAddress() );
emit Disconnected();

// reset current signal level and LEDs
bJitterBufferOK = true;
SignalLevelMeter.Reset();
return false;
}
}

void CClient::Init()
Expand Down
Loading