Comment accuracy: nine measured corrections (draft / experiment)

src/buffer.cpp

@@ -164,8 +164,16 @@ bool CNetBuf::Put ( const CVector<uint8_t>& 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<uint8_t>& 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 )

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;
 }
 

src/client.cpp

@@ -999,17 +999,27 @@ 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?";
         ClearClientChannels();
     }
 
     // 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

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