From 181c8a8b65eba7dd63e3a2d15a2d26b2381c3b29 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Mon, 31 Aug 2015 10:48:45 -0700 Subject: [PATCH 1/3] handle packet probe when no data available --- libraries/networking/src/udt/Connection.cpp | 30 ++++++++++++++++++- libraries/networking/src/udt/Connection.h | 4 ++- .../networking/src/udt/ControlPacket.cpp | 2 +- libraries/networking/src/udt/ControlPacket.h | 3 +- libraries/networking/src/udt/SendQueue.cpp | 6 ++++ 5 files changed, 41 insertions(+), 4 deletions(-) diff --git a/libraries/networking/src/udt/Connection.cpp b/libraries/networking/src/udt/Connection.cpp index b8dbc7c62e..7f5ad0c1a0 100644 --- a/libraries/networking/src/udt/Connection.cpp +++ b/libraries/networking/src/udt/Connection.cpp @@ -409,7 +409,14 @@ bool Connection::processReceivedSequenceNumber(SequenceNumber sequenceNumber, in if (((uint32_t) sequenceNumber & 0xF) == 0) { _receiveWindow.onProbePair1Arrival(); } else if (((uint32_t) sequenceNumber & 0xF) == 1) { - _receiveWindow.onProbePair2Arrival(); + // only use this packet for bandwidth estimation if we didn't just receive a control packet in its place + if (!_receivedControlProbeTail) { + _receiveWindow.onProbePair2Arrival(); + } else { + // reset our control probe tail marker so the next probe that comes with data can be used + _receivedControlProbeTail = false; + } + } _receiveWindow.onPacketArrival(); @@ -510,6 +517,11 @@ void Connection::processControl(std::unique_ptr controlPacket) { case ControlPacket::HandshakeACK: processHandshakeACK(move(controlPacket)); break; + case ControlPacket::ProbeTail: + if (_isReceivingData) { + processProbeTail(move(controlPacket)); + } + break; } } @@ -733,6 +745,21 @@ void Connection::processTimeoutNAK(std::unique_ptr controlPacket) _stats.record(ConnectionStats::Stats::ReceivedTimeoutNAK); } +void Connection::processProbeTail(std::unique_ptr controlPacket) { + // this is the second packet in a probe set so we can estimate bandwidth + // the sender sent this to us in lieu of sending new data (because they didn't have any) + +#ifdef UDT_CONNECTION_DEBUG + qCDebug(networking) << "Processing second packet of probe from control packet instead of data packet"; +#endif + + _receiveWindow.onProbePair2Arrival(); + + // mark that we processed a control packet for the second in the pair and we should not mark + // the next data packet received + _receivedControlProbeTail = true; +} + void Connection::resetReceiveState() { // reset all SequenceNumber member variables back to default @@ -767,6 +794,7 @@ void Connection::resetReceiveState() { // clear the intervals in the receive window _receiveWindow.reset(); + _receivedControlProbeTail = false; // clear any pending received messages _pendingReceivedMessages.clear(); diff --git a/libraries/networking/src/udt/Connection.h b/libraries/networking/src/udt/Connection.h index eb5961695c..f10c9dd720 100644 --- a/libraries/networking/src/udt/Connection.h +++ b/libraries/networking/src/udt/Connection.h @@ -94,6 +94,7 @@ private: void processTimeoutNAK(std::unique_ptr controlPacket); void processHandshake(std::unique_ptr controlPacket); void processHandshakeACK(std::unique_ptr controlPacket); + void processProbeTail(std::unique_ptr controlPacket); void resetReceiveState(); void resetRTT(); @@ -145,7 +146,8 @@ private: Socket* _parentSocket { nullptr }; HifiSockAddr _destination; - PacketTimeWindow _receiveWindow { 16, 64 }; // Window of interval between packets (16) and probes (64) for bandwidth and receive speed + PacketTimeWindow _receiveWindow { 16, 64 }; // Window of interval between packets (16) and probes (64) for timing + bool _receivedControlProbeTail { false }; // Marker for receipt of control packet probe tail (in lieu of probe with data) std::unique_ptr _congestionControl; diff --git a/libraries/networking/src/udt/ControlPacket.cpp b/libraries/networking/src/udt/ControlPacket.cpp index 8c799f97a6..e672ab426b 100644 --- a/libraries/networking/src/udt/ControlPacket.cpp +++ b/libraries/networking/src/udt/ControlPacket.cpp @@ -100,7 +100,7 @@ void ControlPacket::readType() { Q_ASSERT_X(bitAndType & CONTROL_BIT_MASK, "ControlPacket::readHeader()", "This should be a control packet"); uint16_t packetType = (bitAndType & ~CONTROL_BIT_MASK) >> (8 * sizeof(Type)); - Q_ASSERT_X(packetType <= ControlPacket::Type::HandshakeACK, "ControlPacket::readType()", "Received a control packet with wrong type"); + Q_ASSERT_X(packetType <= ControlPacket::Type::ProbeTail, "ControlPacket::readType()", "Received a control packet with wrong type"); // read the type _type = (Type) packetType; diff --git a/libraries/networking/src/udt/ControlPacket.h b/libraries/networking/src/udt/ControlPacket.h index 1976899c14..fea8210ba0 100644 --- a/libraries/networking/src/udt/ControlPacket.h +++ b/libraries/networking/src/udt/ControlPacket.h @@ -33,7 +33,8 @@ public: NAK, TimeoutNAK, Handshake, - HandshakeACK + HandshakeACK, + ProbeTail }; static std::unique_ptr create(Type type, qint64 size = -1); diff --git a/libraries/networking/src/udt/SendQueue.cpp b/libraries/networking/src/udt/SendQueue.cpp index 171a79dccc..7abb195005 100644 --- a/libraries/networking/src/udt/SendQueue.cpp +++ b/libraries/networking/src/udt/SendQueue.cpp @@ -463,6 +463,12 @@ bool SendQueue::maybeSendNewPacket() { // do we have a second in a pair to send as well? if (secondPacket) { sendNewPacketAndAddToSentList(move(secondPacket), getNextSequenceNumber()); + } else { + // we didn't get a second packet to send in the probe pair + // send a control packet of type ProbePairTail so the receiver can still do + // proper bandwidth estimation + static auto pairTailPacket = ControlPacket::create(ControlPacket::ProbeTail); + _socket->writeBasePacket(*pairTailPacket, _destination); } // We sent our packet(s), return here From 6c9021c2886944039b11d2252fc41b49a4121230 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 1 Sep 2015 11:10:19 -0600 Subject: [PATCH 2/3] only send second packet of pair when required --- libraries/networking/src/udt/SendQueue.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/udt/SendQueue.cpp b/libraries/networking/src/udt/SendQueue.cpp index ff53bcfc54..28f71ec93f 100644 --- a/libraries/networking/src/udt/SendQueue.cpp +++ b/libraries/networking/src/udt/SendQueue.cpp @@ -443,10 +443,12 @@ bool SendQueue::maybeSendNewPacket() { _packets.pop_front(); std::unique_ptr secondPacket; + bool shouldSendPairTail = false; if (((uint32_t) nextNumber & 0xF) == 0) { // the first packet is the first in a probe pair - every 16 (rightmost 16 bits = 0) packets // pull off a second packet if we can before we unlock + shouldSendPairTail = true; if (_packets.size() > 0) { secondPacket.swap(_packets.front()); @@ -463,7 +465,7 @@ bool SendQueue::maybeSendNewPacket() { // do we have a second in a pair to send as well? if (secondPacket) { sendNewPacketAndAddToSentList(move(secondPacket), getNextSequenceNumber()); - } else { + } else if (shouldSendPairTail) { // we didn't get a second packet to send in the probe pair // send a control packet of type ProbePairTail so the receiver can still do // proper bandwidth estimation From 56d6d95df9e9589b6e62b9c6bfca269228e01069 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 1 Sep 2015 12:09:58 -0600 Subject: [PATCH 3/3] only process cp tail if in the right spot --- libraries/networking/src/udt/Connection.cpp | 22 +++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/libraries/networking/src/udt/Connection.cpp b/libraries/networking/src/udt/Connection.cpp index 3625735c34..84928fa298 100644 --- a/libraries/networking/src/udt/Connection.cpp +++ b/libraries/networking/src/udt/Connection.cpp @@ -746,18 +746,20 @@ void Connection::processTimeoutNAK(std::unique_ptr controlPacket) } void Connection::processProbeTail(std::unique_ptr controlPacket) { - // this is the second packet in a probe set so we can estimate bandwidth - // the sender sent this to us in lieu of sending new data (because they didn't have any) - + if (((uint32_t) _lastReceivedSequenceNumber & 0xF) == 0) { + // this is the second packet in a probe set so we can estimate bandwidth + // the sender sent this to us in lieu of sending new data (because they didn't have any) + #ifdef UDT_CONNECTION_DEBUG - qCDebug(networking) << "Processing second packet of probe from control packet instead of data packet"; + qCDebug(networking) << "Processing second packet of probe from control packet instead of data packet"; #endif - - _receiveWindow.onProbePair2Arrival(); - - // mark that we processed a control packet for the second in the pair and we should not mark - // the next data packet received - _receivedControlProbeTail = true; + + _receiveWindow.onProbePair2Arrival(); + + // mark that we processed a control packet for the second in the pair and we should not mark + // the next data packet received + _receivedControlProbeTail = true; + } } void Connection::resetReceiveState() {