diff --git a/src/buffer.cpp b/src/buffer.cpp index 441b4121e4..93be5d52fa 100644 --- a/src/buffer.cpp +++ b/src/buffer.cpp @@ -164,8 +164,16 @@ bool CNetBuf::Put ( const CVector& vecbyData, int iInSize ) return false; } - // to get the number of input blocks we assume that the number of bytes for - // the sequence number is much smaller than the number of coded audio bytes + // This divide is exact if and only if iNumBlocks * iNumBytesSeqNum < iBlockSize, + // since the actual input is iNumBlocks * ( iBlockSize + iNumBytesSeqNum ) bytes. The + // sequence number merely being "much smaller" than the coded audio is not the + // condition: at iNumBlocks == iBlockSize the count comes out one too high whatever + // the ratio is. The bound is not enforced here but by the properties validator in + // protocol.cpp, EvaluateNetwTranspPropsMes, which rejects a base network packet size + // below CELT_MINIMUM_NUM_BYTES (10) and a block size factor outside + // { FRAME_SIZE_FACTOR_PREFERRED, _DEFAULT, _SAFE }. With iNumBytesSeqNum == 1 and + // iBlockSize == iBaseNetworkPacketSize - 1 (channel.cpp), the worst reachable case is + // a factor of 4 against a block size of 9. const int iNumBlocks = /* floor */ ( iInSize / iBlockSize ); // copy new data in internal buffer @@ -190,16 +198,23 @@ bool CNetBuf::Put ( const CVector& vecbyData, int iInSize ) iSeqNumDiff -= 256; } - // The 1-byte sequence number wraps around at a count of 256. So, if a packet is delayed - // further than this we cannot detect it. But it does not matter since such a packet is - // more than 100 ms delayed so we have a bad network situation anyway. Therefore we + // The 1-byte sequence number is folded into a signed difference above, so a + // delayed packet is mistaken for an early one once it is more than 128 counts + // late, not 256. At the fastest possible frame rate that is still 171 ms + // (64-sample frames, 750 counts/s) and at the default frame size 341 ms + // (128-sample frames, 375 counts/s), so such a packet is long useless either + // way and we have a bad network situation anyway. Therefore we // assume that the sequence number difference between the received and local counter is // correct. The idea of the following code is that we always move our "buffer window" so // that the received packet fits into the buffer. By doing this we are robust against // sample rate offsets between client/server or buffer glitches in the audio driver since // we adjust the window. The downside is that we never throw away single packets which arrive // too late so we throw away valid packets when we move the "buffer window" to the delayed - // packet and then back to the correct place when the next normal packet is received. But + // packet and then back to the correct place when the next normal packet is received. + // Note that this is not the only way a valid block is lost: a block can also be + // overwritten in its slot before it is played out. That second channel is the only + // one that exists at a buffer length of 1, while the window move dominates from a + // buffer length of 3 upwards. But // tests showed that the new buffer strategy does not perform worse than the old jitter // buffer which did not use any sequence number at all. if ( iSeqNumDiff < 0 ) diff --git a/src/channel.cpp b/src/channel.cpp index 7755b7ec92..2465e2022c 100644 --- a/src/channel.cpp +++ b/src/channel.cpp @@ -716,12 +716,20 @@ int CChannel::GetUploadRateKbps() { const int iAudioSizeOut = iNetwFrameSizeFact * iAudioFrameSizeSamples; - // we assume that the UDP packet which is transported via IP has an - // additional header size of ("Network Music Performance (NMP) in narrow - // band networks; Carot, Kraemer, Schuller; 2006") + // The 77-byte per-packet overhead below (28 + 26 + 23) models a PPPoE-over-ATM DSL + // access path, following ("Network Music Performance (NMP) in narrow band networks; + // Carot, Kraemer, Schuller; 2006"): // 8 (UDP) + 20 (IP without optional fields) = 28 bytes // 2 (PPP) + 6 (PPPoE) + 18 (MAC) = 26 bytes // 5 (RFC1483B) + 8 (AAL) + 10 (ATM) = 23 bytes + // A packet capture on a real internet path measures 46 bytes over IPv4/Ethernet + // (20 IP + 8 UDP + 14 Ethernet, no PPPoE, no ATM, no VLAN) and 66 over IPv6, so this + // constant is 31 bytes too large for IPv4 and 20 bytes too small for the IPv6 header + // it never accounts for. The figure a client can actually justify is 28 (IPv4) or + // 48 (IPv6) at L3/L4, since the access encapsulation is invisible to the endpoint. + // As it stands the returned rate overstates real IPv4/Ethernet cost by roughly 15% to + // 50%, worst at the lowest bit-rate settings. FIXME: the constant should reflect a + // measurable path, but the "right" figure is a design decision (which layer to bill). return ( iNetwFrameSize * iNetwFrameSizeFact + 28 + 26 + 23 /* header */ ) * 8 /* bits per byte */ * SYSTEM_SAMPLE_RATE_HZ / iAudioSizeOut / 1000; } diff --git a/src/client.cpp b/src/client.cpp index e1532c96b2..c35d7807e4 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -999,9 +999,12 @@ void CClient::OnControllerInMuteMyself ( bool bMute ) void CClient::OnClientIDReceived ( int iServerChanID ) { - // if we have just connected to a running server, iActiveChannels will be 0 - // if iActiveChannels is not 0, the server must have been restarted on the fly - // in that case, channels might have changed, so clear our list to get it afresh. + // If we have just connected to a running server, iActiveChannels will be 0. + // If it is not 0, the server has begun a NEW connection for us while this client kept + // running. A restart is only one way that happens: the server also drops a channel whose + // receive timeout expires ( CON_TIME_OUT_SEC_MAX, channel.h ) and treats the next packet + // from the same peer as a new connection, so a traffic gap of longer than that is enough. + // Either way the channel list we hold may be stale, so clear it and get it afresh. if ( iActiveChannels != 0 ) { qInfo() << "> Server restarted?"; @@ -1009,7 +1012,14 @@ void CClient::OnClientIDReceived ( int iServerChanID ) } // allocate and map client-side channel 0 - int iChanID = FindClientChannel ( iServerChanID, true ); // should always return channel 0 + // In normal operation this returns channel 0. It is NOT guaranteed: FindClientChannel + // returns INVALID_INDEX ( -1 ) for any iServerChanID >= MAX_NUM_CHANNELS ( 150 ), and a + // server-sent CLIENT_ID is only length-checked ( EvaluateClientIDMes, protocol.cpp ), never + // range-checked, so a malicious or buggy server can deliver an id of 150..255. The result is + // used below without a guard; with the headless mute-me-in-personal-mix flag set it reaches + // SetRemoteChanGain, which dereferences &clientChannels[-1]. FIXME: reject iServerChanID + // outside [0, MAX_NUM_CHANNELS) here or in EvaluateClientIDMes before this line. + int iChanID = FindClientChannel ( iServerChanID, true ); // for headless mode we support to mute our own signal in the personal mix // (note that the check for headless is done in the main.cpp and must not diff --git a/src/server.cpp b/src/server.cpp index a49eab776c..60c0d58a83 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -165,10 +165,17 @@ CServer::CServer ( const int iNewMaxNumChan, iServerFrameSizeSamples = SYSTEM_FRAME_SIZE_SAMPLES; } - // To avoid audio clitches, in the entire realtime timer audio processing - // routine including the ProcessData no memory must be allocated. Since we - // do not know the required sizes for the vectors, we allocate memory for - // the worst case here: + // To avoid audio glitches, the realtime timer audio routine ( OnTimer -> + // ProcessData ) should allocate no memory. The worst-case vectors below are + // pre-sized here for that reason. Note that the goal is not currently met: the + // send path still allocates once per outgoing audio packet. CSocket::SendPacket + // takes its argument as a const CVector, and the ( CVector ) cast that + // strips the const deep-copies the whole datagram -- one malloc, one memmove, one + // free for every packet sent, on this same timer thread ( attribute by stack, not + // by thread name: OnTimer runs on the Qt event-loop thread via a queued connection ). + // CNetBufWithStats::Init also allocates on this path. FIXME: remove these before + // relying on the no-allocation guarantee. Since we do not know the required sizes + // for the vectors, we allocate memory for the worst case here: // allocate worst case memory for the temporary vectors vecChanIDsCurConChan.Init ( iMaxNumChannels ); @@ -662,7 +669,11 @@ void CServer::OnTimer() bool bUseMT = false; int iNumBlocks = 0; // init number of blocks for multithreading int iMTBlockSize = 0; // init block size for multithreading - bChannelIsNowDisconnected = false; // note that the flag must be a member function since QtConcurrent::run can only take 5 params + // The flag is a member variable, not a local. Nothing in the threading forces that: the + // decode workers below are dispatched through CThreadPool::enqueue ( threadpool.h ), which is + // variadic and takes any number of arguments. The five-argument cap this note used to cite is + // Qt5 QtConcurrent::run's, and that path is no longer used here. + bChannelIsNowDisconnected = false; { // Make put and get calls thread safe. @@ -944,9 +955,14 @@ void CServer::DecodeReceiveData ( const int iChanCnt, const int iNumClients ) FreeChannel ( iCurChanID ); // note that the channel is now not in use - // note that no mutex is needed for this shared resource since it is a - // std::atomic write (not a read-modify-write operation) and also each - // thread can only set it to true and never to false + // Note that no mutex is needed for this shared resource: the store is a + // std::atomic write, not a read-modify-write operation. It is NOT true that + // a thread can only ever set this to true: OnTimer clears it to false once + // per tick, and in the default configuration ( multithreading off ) the + // decode runs inline, so the same thread writes both values. What makes the + // access safe is the ordering -- the clear is sequenced before any decode + // work is handed to the thread pool, and the flag is read back only after + // every future has been waited on. bChannelIsNowDisconnected = true; // since the channel is no longer in use, we should return