From e1f905cb3667fafaa9e9935a7a6db333fb38639b Mon Sep 17 00:00:00 2001 From: wangyix Date: Fri, 1 Aug 2014 10:23:03 -0700 Subject: [PATCH 1/7] fixed >100% loss rate bug in SequenceNumberStats --- interface/src/Audio.cpp | 4 +- interface/src/ui/OctreeStatsDialog.cpp | 14 ++--- libraries/audio/src/InboundAudioStream.cpp | 2 +- libraries/audio/src/InboundAudioStream.h | 2 +- .../networking/src/SequenceNumberStats.cpp | 55 +++++++++-------- .../networking/src/SequenceNumberStats.h | 61 ++++++++++--------- 6 files changed, 73 insertions(+), 65 deletions(-) diff --git a/interface/src/Audio.cpp b/interface/src/Audio.cpp index 4ed1f7aeb3..4fcab5b949 100644 --- a/interface/src/Audio.cpp +++ b/interface/src/Audio.cpp @@ -1424,9 +1424,9 @@ void Audio::renderAudioStreamStats(const AudioStreamStats& streamStats, int hori sprintf(stringBuffer, " Packet loss | overall: %5.2f%% (%d lost), last_30s: %5.2f%% (%d lost)", streamStats._packetStreamStats.getLostRate() * 100.0f, - streamStats._packetStreamStats._numLost, + streamStats._packetStreamStats._lost, streamStats._packetStreamWindowStats.getLostRate() * 100.0f, - streamStats._packetStreamWindowStats._numLost); + streamStats._packetStreamWindowStats._lost); verticalOffset += STATS_HEIGHT_PER_LINE; drawText(horizontalOffset, verticalOffset, scale, rotation, font, stringBuffer, color); diff --git a/interface/src/ui/OctreeStatsDialog.cpp b/interface/src/ui/OctreeStatsDialog.cpp index afa799815f..7abc42d0e3 100644 --- a/interface/src/ui/OctreeStatsDialog.cpp +++ b/interface/src/ui/OctreeStatsDialog.cpp @@ -366,13 +366,13 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser QString incomingBytesString = locale.toString((uint)stats.getIncomingBytes()); QString incomingWastedBytesString = locale.toString((uint)stats.getIncomingWastedBytes()); const SequenceNumberStats& seqStats = stats.getIncomingOctreeSequenceNumberStats(); - QString incomingOutOfOrderString = locale.toString((uint)seqStats.getNumOutOfOrder()); - QString incomingLateString = locale.toString((uint)seqStats.getNumLate()); - QString incomingUnreasonableString = locale.toString((uint)seqStats.getNumUnreasonable()); - QString incomingEarlyString = locale.toString((uint)seqStats.getNumEarly()); - QString incomingLikelyLostString = locale.toString((uint)seqStats.getNumLost()); - QString incomingRecovered = locale.toString((uint)seqStats.getNumRecovered()); - QString incomingDuplicateString = locale.toString((uint)seqStats.getNumDuplicate()); + QString incomingOutOfOrderString = locale.toString((uint)seqStats.getOutOfOrder()); + QString incomingLateString = locale.toString((uint)seqStats.getLate()); + QString incomingUnreasonableString = locale.toString((uint)seqStats.getUnreasonable()); + QString incomingEarlyString = locale.toString((uint)seqStats.getEarly()); + QString incomingLikelyLostString = locale.toString((uint)seqStats.getLost()); + QString incomingRecovered = locale.toString((uint)seqStats.getRecovered()); + QString incomingDuplicateString = locale.toString((uint)seqStats.getDuplicate()); int clockSkewInMS = node->getClockSkewUsec() / (int)USECS_PER_MSEC; QString incomingFlightTimeString = locale.toString((int)stats.getIncomingFlightTimeAverage()); diff --git a/libraries/audio/src/InboundAudioStream.cpp b/libraries/audio/src/InboundAudioStream.cpp index 6ade4b17e9..36145a35f6 100644 --- a/libraries/audio/src/InboundAudioStream.cpp +++ b/libraries/audio/src/InboundAudioStream.cpp @@ -212,7 +212,7 @@ SequenceNumberStats::ArrivalInfo InboundAudioStream::frameReceivedUpdateNetworkS // discard the first few packets we receive since they usually have gaps that aren't represensative of normal jitter const int NUM_INITIAL_PACKETS_DISCARD = 3; quint64 now = usecTimestampNow(); - if (_incomingSequenceNumberStats.getNumReceived() > NUM_INITIAL_PACKETS_DISCARD) { + if (_incomingSequenceNumberStats.getReceived() > NUM_INITIAL_PACKETS_DISCARD) { quint64 gap = now - _lastFrameReceivedTime; _interframeTimeGapStatsForStatsPacket.update(gap); diff --git a/libraries/audio/src/InboundAudioStream.h b/libraries/audio/src/InboundAudioStream.h index 06bd329fee..1a196a1c61 100644 --- a/libraries/audio/src/InboundAudioStream.h +++ b/libraries/audio/src/InboundAudioStream.h @@ -108,7 +108,7 @@ public: int getSilentFramesDropped() const { return _silentFramesDropped; } int getOverflowCount() const { return _ringBuffer.getOverflowCount(); } - int getPacketReceived() const { return _incomingSequenceNumberStats.getNumReceived(); } + int getPacketsReceived() const { return _incomingSequenceNumberStats.getReceived(); } private: void starved(); diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index 66d57500a5..00a3e66877 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -14,7 +14,8 @@ #include SequenceNumberStats::SequenceNumberStats(int statsHistoryLength) - : _lastReceived(std::numeric_limits::max()), + : _received(0), + _lastReceivedSequence(0), _missingSet(), _stats(), _lastSenderUUID(), @@ -23,8 +24,10 @@ SequenceNumberStats::SequenceNumberStats(int statsHistoryLength) } void SequenceNumberStats::reset() { + _received = 0; _missingSet.clear(); _stats = PacketStreamStats(); + _lastSenderUUID = QUuid(); _statsHistory.clear(); } @@ -36,7 +39,7 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui // if the sender node has changed, reset all stats if (senderUUID != _lastSenderUUID) { - if (_stats._numReceived > 0) { + if (_received > 0) { qDebug() << "sequence number stats was reset due to new sender node"; qDebug() << "previous:" << _lastSenderUUID << "current:" << senderUUID; reset(); @@ -45,13 +48,14 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui } // determine our expected sequence number... handle rollover appropriately - quint16 expected = _stats._numReceived > 0 ? _lastReceived + (quint16)1 : incoming; + quint16 expected = _received > 0 ? _lastReceivedSequence + (quint16)1 : incoming; - _stats._numReceived++; + _received++; if (incoming == expected) { // on time arrivalInfo._status = OnTime; - _lastReceived = incoming; + _lastReceivedSequence = incoming; + _stats._expectedReceived++; } else { // out of order if (wantExtraDebugging) { @@ -76,8 +80,8 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui // ignore packet if gap is unreasonable qDebug() << "ignoring unreasonable sequence number:" << incoming - << "previous:" << _lastReceived; - _stats._numUnreasonable++; + << "previous:" << _lastReceivedSequence; + _stats._unreasonable++; return arrivalInfo; } @@ -94,10 +98,11 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui qDebug() << "this packet is earlier than expected..."; qDebug() << ">>>>>>>> missing gap=" << (incomingInt - expectedInt); } - - _stats._numEarly++; - _stats._numLost += (incomingInt - expectedInt); - _lastReceived = incoming; + int skipped = incomingInt - expectedInt; + _stats._early++; + _stats._lost += skipped; + _stats._expectedReceived += (skipped + 1); + _lastReceivedSequence = incoming; // add all sequence numbers that were skipped to the missing sequence numbers list for (int missingInt = expectedInt; missingInt < incomingInt; missingInt++) { @@ -114,7 +119,7 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui qDebug() << "this packet is later than expected..."; } - _stats._numLate++; + _stats._late++; // do not update _lastReceived; it shouldn't become smaller @@ -125,15 +130,15 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui if (wantExtraDebugging) { qDebug() << "found it in _missingSet"; } - _stats._numLost--; - _stats._numRecovered++; + _stats._lost--; + _stats._recovered++; } else { arrivalInfo._status = Duplicate; if (wantExtraDebugging) { qDebug() << "sequence:" << incoming << "was NOT found in _missingSet and is probably a duplicate"; } - _stats._numDuplicate++; + _stats._duplicate++; } } } @@ -148,7 +153,7 @@ void SequenceNumberStats::pruneMissingSet(const bool wantExtraDebugging) { // some older sequence numbers may be from before a rollover point; this must be handled. // some sequence numbers in this list may be larger than _incomingLastSequence, indicating that they were received // before the most recent rollover. - int cutoff = (int)_lastReceived - MAX_REASONABLE_SEQUENCE_GAP; + int cutoff = (int)_lastReceivedSequence - MAX_REASONABLE_SEQUENCE_GAP; if (cutoff >= 0) { quint16 nonRolloverCutoff = (quint16)cutoff; QSet::iterator i = _missingSet.begin(); @@ -159,7 +164,7 @@ void SequenceNumberStats::pruneMissingSet(const bool wantExtraDebugging) { qDebug() << "old age cutoff:" << nonRolloverCutoff; } - if (missing > _lastReceived || missing < nonRolloverCutoff) { + if (missing > _lastReceivedSequence || missing < nonRolloverCutoff) { i = _missingSet.erase(i); if (wantExtraDebugging) { qDebug() << "pruning really old missing sequence:" << missing; @@ -178,7 +183,7 @@ void SequenceNumberStats::pruneMissingSet(const bool wantExtraDebugging) { qDebug() << "old age cutoff:" << rolloverCutoff; } - if (missing > _lastReceived && missing < rolloverCutoff) { + if (missing > _lastReceivedSequence && missing < rolloverCutoff) { i = _missingSet.erase(i); if (wantExtraDebugging) { qDebug() << "pruning really old missing sequence:" << missing; @@ -202,13 +207,13 @@ PacketStreamStats SequenceNumberStats::getStatsForHistoryWindow() const { // calculate difference between newest stats and oldest stats to get window stats PacketStreamStats windowStats; - windowStats._numReceived = newestStats->_numReceived - oldestStats->_numReceived; - windowStats._numUnreasonable = newestStats->_numUnreasonable - oldestStats->_numUnreasonable; - windowStats._numEarly = newestStats->_numEarly - oldestStats->_numEarly; - windowStats._numLate = newestStats->_numLate - oldestStats->_numLate; - windowStats._numLost = newestStats->_numLost - oldestStats->_numLost; - windowStats._numRecovered = newestStats->_numRecovered - oldestStats->_numRecovered; - windowStats._numDuplicate = newestStats->_numDuplicate - oldestStats->_numDuplicate; + windowStats._expectedReceived = newestStats->_expectedReceived - oldestStats->_expectedReceived; + windowStats._unreasonable = newestStats->_unreasonable - oldestStats->_unreasonable; + windowStats._early = newestStats->_early - oldestStats->_early; + windowStats._late = newestStats->_late - oldestStats->_late; + windowStats._lost = newestStats->_lost - oldestStats->_lost; + windowStats._recovered = newestStats->_recovered - oldestStats->_recovered; + windowStats._duplicate = newestStats->_duplicate - oldestStats->_duplicate; return windowStats; } diff --git a/libraries/networking/src/SequenceNumberStats.h b/libraries/networking/src/SequenceNumberStats.h index f4e85b6fb3..d611a494ad 100644 --- a/libraries/networking/src/SequenceNumberStats.h +++ b/libraries/networking/src/SequenceNumberStats.h @@ -21,29 +21,27 @@ const int MAX_REASONABLE_SEQUENCE_GAP = 1000; class PacketStreamStats { public: PacketStreamStats() - : _numReceived(0), - _numUnreasonable(0), - _numEarly(0), - _numLate(0), - _numLost(0), - _numRecovered(0), - _numDuplicate(0) + : _expectedReceived(0), + _unreasonable(0), + _early(0), + _late(0), + _lost(0), + _recovered(0), + _duplicate(0) {} - float getUnreasonableRate() const { return (float)_numUnreasonable / _numReceived; } - float getNumEaryRate() const { return (float)_numEarly / _numReceived; } - float getLateRate() const { return (float)_numLate / _numReceived; } - float getLostRate() const { return (float)_numLost / _numReceived; } - float getRecoveredRate() const { return (float)_numRecovered / _numReceived; } - float getDuplicateRate() const { return (float)_numDuplicate / _numReceived; } + float getUnreasonableRate() const { return (float)_unreasonable / _expectedReceived; } + float getEaryRate() const { return (float)_early / _expectedReceived; } + float getLateRate() const { return (float)_late / _expectedReceived; } + float getLostRate() const { return (float)_lost / _expectedReceived; } - quint32 _numReceived; - quint32 _numUnreasonable; - quint32 _numEarly; - quint32 _numLate; - quint32 _numLost; - quint32 _numRecovered; - quint32 _numDuplicate; + quint32 _expectedReceived; + quint32 _unreasonable; + quint32 _early; + quint32 _late; + quint32 _lost; + quint32 _recovered; + quint32 _duplicate; }; class SequenceNumberStats { @@ -70,20 +68,25 @@ public: void pruneMissingSet(const bool wantExtraDebugging = false); void pushStatsToHistory() { _statsHistory.insert(_stats); } - quint32 getNumReceived() const { return _stats._numReceived; } - quint32 getNumUnreasonable() const { return _stats._numUnreasonable; } - quint32 getNumOutOfOrder() const { return _stats._numEarly + _stats._numLate; } - quint32 getNumEarly() const { return _stats._numEarly; } - quint32 getNumLate() const { return _stats._numLate; } - quint32 getNumLost() const { return _stats._numLost; } - quint32 getNumRecovered() const { return _stats._numRecovered; } - quint32 getNumDuplicate() const { return _stats._numDuplicate; } + quint32 getReceived() const { return _received; } + + quint32 getExpectedReceived() const { return _stats._expectedReceived; } + quint32 getUnreasonable() const { return _stats._unreasonable; } + quint32 getOutOfOrder() const { return _stats._early + _stats._late; } + quint32 getEarly() const { return _stats._early; } + quint32 getLate() const { return _stats._late; } + quint32 getLost() const { return _stats._lost; } + quint32 getRecovered() const { return _stats._recovered; } + quint32 getDuplicate() const { return _stats._duplicate; } + const PacketStreamStats& getStats() const { return _stats; } PacketStreamStats getStatsForHistoryWindow() const; const QSet& getMissingSet() const { return _missingSet; } private: - quint16 _lastReceived; + int _received; + + quint16 _lastReceivedSequence; QSet _missingSet; PacketStreamStats _stats; From cdcc6ece04ef0fe9e5e3576abc27729280ebe2ef Mon Sep 17 00:00:00 2001 From: wangyix Date: Fri, 1 Aug 2014 10:34:19 -0700 Subject: [PATCH 2/7] fixed compile errors caused by SequenceNumberStats changes --- interface/src/Audio.cpp | 2 +- .../src/SequenceNumberStatsTests.cpp | 96 +++++++++---------- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/interface/src/Audio.cpp b/interface/src/Audio.cpp index 4fcab5b949..1391045568 100644 --- a/interface/src/Audio.cpp +++ b/interface/src/Audio.cpp @@ -709,7 +709,7 @@ void Audio::handleAudioInput() { delete[] inputAudioSamples; } - if (_receivedAudioStream.getPacketReceived() > 0) { + if (_receivedAudioStream.getPacketsReceived() > 0) { pushAudioToOutput(); } } diff --git a/tests/networking/src/SequenceNumberStatsTests.cpp b/tests/networking/src/SequenceNumberStatsTests.cpp index de487267e0..c4205f5fb5 100644 --- a/tests/networking/src/SequenceNumberStatsTests.cpp +++ b/tests/networking/src/SequenceNumberStatsTests.cpp @@ -38,12 +38,12 @@ void SequenceNumberStatsTests::rolloverTest() { stats.sequenceNumberReceived(seq); seq = seq + (quint16)1; - assert(stats.getNumDuplicate() == 0); - assert(stats.getNumEarly() == 0); - assert(stats.getNumLate() == 0); - assert(stats.getNumLost() == 0); - assert(stats.getNumReceived() == i + 1); - assert(stats.getNumRecovered() == 0); + assert(stats.getDuplicate() == 0); + assert(stats.getEarly() == 0); + assert(stats.getLate() == 0); + assert(stats.getLost() == 0); + assert(stats.getReceived() == i + 1); + assert(stats.getRecovered() == 0); } stats.reset(); } @@ -69,12 +69,12 @@ void SequenceNumberStatsTests::earlyLateTest() { seq = seq + (quint16)1; numSent++; - assert(stats.getNumDuplicate() == 0); - assert(stats.getNumEarly() == numEarly); - assert(stats.getNumLate() == numLate); - assert(stats.getNumLost() == numLost); - assert(stats.getNumReceived() == numSent); - assert(stats.getNumRecovered() == numRecovered); + assert(stats.getDuplicate() == 0); + assert(stats.getEarly() == numEarly); + assert(stats.getLate() == numLate); + assert(stats.getLost() == numLost); + assert(stats.getReceived() == numSent); + assert(stats.getRecovered() == numRecovered); } // skip 10 @@ -89,12 +89,12 @@ void SequenceNumberStatsTests::earlyLateTest() { seq = seq + (quint16)1; numSent++; - assert(stats.getNumDuplicate() == 0); - assert(stats.getNumEarly() == numEarly); - assert(stats.getNumLate() == numLate); - assert(stats.getNumLost() == numLost); - assert(stats.getNumReceived() == numSent); - assert(stats.getNumRecovered() == numRecovered); + assert(stats.getDuplicate() == 0); + assert(stats.getEarly() == numEarly); + assert(stats.getLate() == numLate); + assert(stats.getLost() == numLost); + assert(stats.getReceived() == numSent); + assert(stats.getRecovered() == numRecovered); } // send ones we skipped @@ -106,12 +106,12 @@ void SequenceNumberStatsTests::earlyLateTest() { numLost--; numRecovered++; - assert(stats.getNumDuplicate() == 0); - assert(stats.getNumEarly() == numEarly); - assert(stats.getNumLate() == numLate); - assert(stats.getNumLost() == numLost); - assert(stats.getNumReceived() == numSent); - assert(stats.getNumRecovered() == numRecovered); + assert(stats.getDuplicate() == 0); + assert(stats.getEarly() == numEarly); + assert(stats.getLate() == numLate); + assert(stats.getLost() == numLost); + assert(stats.getReceived() == numSent); + assert(stats.getRecovered() == numRecovered); } } stats.reset(); @@ -145,12 +145,12 @@ void SequenceNumberStatsTests::duplicateTest() { seq = seq + (quint16)1; numSent++; - assert(stats.getNumDuplicate() == numDuplicate); - assert(stats.getNumEarly() == numEarly); - assert(stats.getNumLate() == numLate); - assert(stats.getNumLost() == numLost); - assert(stats.getNumReceived() == numSent); - assert(stats.getNumRecovered() == 0); + assert(stats.getDuplicate() == numDuplicate); + assert(stats.getEarly() == numEarly); + assert(stats.getLate() == numLate); + assert(stats.getLost() == numLost); + assert(stats.getReceived() == numSent); + assert(stats.getRecovered() == 0); } // skip 10 @@ -167,12 +167,12 @@ void SequenceNumberStatsTests::duplicateTest() { seq = seq + (quint16)1; numSent++; - assert(stats.getNumDuplicate() == numDuplicate); - assert(stats.getNumEarly() == numEarly); - assert(stats.getNumLate() == numLate); - assert(stats.getNumLost() == numLost); - assert(stats.getNumReceived() == numSent); - assert(stats.getNumRecovered() == 0); + assert(stats.getDuplicate() == numDuplicate); + assert(stats.getEarly() == numEarly); + assert(stats.getLate() == numLate); + assert(stats.getLost() == numLost); + assert(stats.getReceived() == numSent); + assert(stats.getRecovered() == 0); } // send 5 duplicates from before skip @@ -183,12 +183,12 @@ void SequenceNumberStatsTests::duplicateTest() { numDuplicate++; numLate++; - assert(stats.getNumDuplicate() == numDuplicate); - assert(stats.getNumEarly() == numEarly); - assert(stats.getNumLate() == numLate); - assert(stats.getNumLost() == numLost); - assert(stats.getNumReceived() == numSent); - assert(stats.getNumRecovered() == 0); + assert(stats.getDuplicate() == numDuplicate); + assert(stats.getEarly() == numEarly); + assert(stats.getLate() == numLate); + assert(stats.getLost() == numLost); + assert(stats.getReceived() == numSent); + assert(stats.getRecovered() == 0); } // send 5 duplicates from after skip @@ -199,12 +199,12 @@ void SequenceNumberStatsTests::duplicateTest() { numDuplicate++; numLate++; - assert(stats.getNumDuplicate() == numDuplicate); - assert(stats.getNumEarly() == numEarly); - assert(stats.getNumLate() == numLate); - assert(stats.getNumLost() == numLost); - assert(stats.getNumReceived() == numSent); - assert(stats.getNumRecovered() == 0); + assert(stats.getDuplicate() == numDuplicate); + assert(stats.getEarly() == numEarly); + assert(stats.getLate() == numLate); + assert(stats.getLost() == numLost); + assert(stats.getReceived() == numSent); + assert(stats.getRecovered() == 0); } } stats.reset(); From eba07b03e29a4d2901ccdecb0044a2334359412d Mon Sep 17 00:00:00 2001 From: wangyix Date: Fri, 1 Aug 2014 12:00:41 -0700 Subject: [PATCH 3/7] recursive SequenceNumberStats; untested --- .../networking/src/SequenceNumberStats.cpp | 103 +++++++++++++++++- .../networking/src/SequenceNumberStats.h | 21 +++- 2 files changed, 120 insertions(+), 4 deletions(-) diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index 00a3e66877..a700e01f07 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -13,22 +13,67 @@ #include -SequenceNumberStats::SequenceNumberStats(int statsHistoryLength) +SequenceNumberStats::SequenceNumberStats(int statsHistoryLength, int maxRecursion) : _received(0), _lastReceivedSequence(0), _missingSet(), _stats(), _lastSenderUUID(), - _statsHistory(statsHistoryLength) + _statsHistory(statsHistoryLength), + + _unreasonableTracker(NULL), + _maxRecursion(maxRecursion) { } +SequenceNumberStats::SequenceNumberStats(const SequenceNumberStats& other) + : _received(other._received), + _lastReceivedSequence(other._lastReceivedSequence), + _missingSet(other._missingSet), + _stats(other._stats), + _lastSenderUUID(other._lastSenderUUID), + _statsHistory(other._statsHistory), + _unreasonableTracker(NULL), + _maxRecursion(other._maxRecursion) +{ + if (other._unreasonableTracker) { + _unreasonableTracker = new SequenceNumberStats(*other._unreasonableTracker); + } +} + +SequenceNumberStats& SequenceNumberStats::operator=(const SequenceNumberStats& rhs) { + _received = rhs._received; + _lastReceivedSequence = rhs._lastReceivedSequence; + _missingSet = rhs._missingSet; + _stats = rhs._stats; + _lastSenderUUID = rhs._lastSenderUUID; + _statsHistory = rhs._statsHistory; + _maxRecursion = rhs._maxRecursion; + + if (rhs._unreasonableTracker) { + _unreasonableTracker = new SequenceNumberStats(*rhs._unreasonableTracker); + } else { + _unreasonableTracker = NULL; + } + return *this; +} + +SequenceNumberStats::~SequenceNumberStats() { + if (_unreasonableTracker) { + delete _unreasonableTracker; + } +} + void SequenceNumberStats::reset() { _received = 0; _missingSet.clear(); _stats = PacketStreamStats(); _lastSenderUUID = QUuid(); _statsHistory.clear(); + + if (_unreasonableTracker) { + delete _unreasonableTracker; + } } static const int UINT16_RANGE = std::numeric_limits::max() + 1; @@ -78,10 +123,64 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui } else if (absGap > MAX_REASONABLE_SEQUENCE_GAP) { arrivalInfo._status = Unreasonable; + /* // ignore packet if gap is unreasonable qDebug() << "ignoring unreasonable sequence number:" << incoming << "previous:" << _lastReceivedSequence; _stats._unreasonable++; + */ + + // do not create a child tracker for unreasonable seq nums if this instance is the last one in the chain. + // otherwise, create one if we don't have one. + + if (!_unreasonableTracker && _maxRecursion > 0) { + _unreasonableTracker = new SequenceNumberStats(0, _maxRecursion - 1); + } + + + if (_unreasonableTracker) { + + // track this unreasonable seq number with our _unreasonableTracker. + ArrivalInfo unreasonableTrackerArrivalInfo = _unreasonableTracker->sequenceNumberReceived(incoming); + + + const int UNREASONABLE_TRACKER_RECEIVED_THRESHOLD = 10; + const float UNRUNREASONABLE_TRACKER_UNREASONABLE_RATE_THRESHOLD = 0.1f; + const float UNREASONABLE_TRACKER_OUT_OF_ORDER_RATE_THRESHOLD = 0.1f; + + // when our _unreasonableTracker has received enough seq nums and doesn't have an _unreasonableTracker of its own, + // we'll either inherit its state only if we think its stream is plausible. it will then be deleted. + // (if it has an _unreasonableTracker of its own, its _unreasonableTracker may be detecting a plausible stream + // while its parent does not, so we should let it accrue seq nums and decide plausibility first) + + if (!_unreasonableTracker->hasUnreasonableTracker() && + _unreasonableTracker->_received >= UNREASONABLE_TRACKER_RECEIVED_THRESHOLD) { + + if (_unreasonableTracker->getUnreasonableRate() < UNRUNREASONABLE_TRACKER_UNREASONABLE_RATE_THRESHOLD && + _unreasonableTracker->getStats().getOutOfOrderRate() < UNREASONABLE_TRACKER_OUT_OF_ORDER_RATE_THRESHOLD) { + + // the _unreasonableTracker has detected a plausible stream of seq numbers; + // copy its state to this tracker. + + _received = _unreasonableTracker->_received; + _lastReceivedSequence = _unreasonableTracker->_lastReceivedSequence; + _missingSet = _unreasonableTracker->_missingSet; + _stats = _unreasonableTracker->_stats; + + // don't copy _lastSenderUUID; _unreasonableTracker always has null UUID for that member. + // ours should be up-to-date. + + // don't copy _statsHistory; _unreasonableTracker keeps a history of length 0. + // simply clear ours. + _statsHistory.clear(); + + arrivalInfo = unreasonableTrackerArrivalInfo; + + } + // remove our _unreasonableTracker + delete _unreasonableTracker; + } + } return arrivalInfo; } diff --git a/libraries/networking/src/SequenceNumberStats.h b/libraries/networking/src/SequenceNumberStats.h index d611a494ad..1f740d7af8 100644 --- a/libraries/networking/src/SequenceNumberStats.h +++ b/libraries/networking/src/SequenceNumberStats.h @@ -18,6 +18,10 @@ const int MAX_REASONABLE_SEQUENCE_GAP = 1000; + +const int DEFAULT_MAX_RECURSION = 5; + + class PacketStreamStats { public: PacketStreamStats() @@ -30,7 +34,7 @@ public: _duplicate(0) {} - float getUnreasonableRate() const { return (float)_unreasonable / _expectedReceived; } + float getOutOfOrderRate() const { return (float)(_early + _late) / _expectedReceived; } float getEaryRate() const { return (float)_early / _expectedReceived; } float getLateRate() const { return (float)_late / _expectedReceived; } float getLostRate() const { return (float)_lost / _expectedReceived; } @@ -61,14 +65,19 @@ public: }; - SequenceNumberStats(int statsHistoryLength = 0); + SequenceNumberStats(int statsHistoryLength = 0, int maxRecursion = DEFAULT_MAX_RECURSION); + SequenceNumberStats(const SequenceNumberStats& other); + SequenceNumberStats& operator=(const SequenceNumberStats& rhs); + ~SequenceNumberStats(); +public: void reset(); ArrivalInfo sequenceNumberReceived(quint16 incoming, QUuid senderUUID = QUuid(), const bool wantExtraDebugging = false); void pruneMissingSet(const bool wantExtraDebugging = false); void pushStatsToHistory() { _statsHistory.insert(_stats); } quint32 getReceived() const { return _received; } + float getUnreasonableRate() const { return _stats._unreasonable / _received; } quint32 getExpectedReceived() const { return _stats._expectedReceived; } quint32 getUnreasonable() const { return _stats._unreasonable; } @@ -94,6 +103,14 @@ private: QUuid _lastSenderUUID; RingBufferHistory _statsHistory; + + + // to deal with the incoming seq nums going out of sync with this tracker, we'll create another instance + // of this class when we encounter an unreasonable + SequenceNumberStats* _unreasonableTracker; + int _maxRecursion; + + bool hasUnreasonableTracker() const { return _unreasonableTracker != NULL; } }; #endif // hifi_SequenceNumberStats_h From f9ec7b6c18e9444c5d304d91d7e4282ca62dd0a5 Mon Sep 17 00:00:00 2001 From: wangyix Date: Fri, 1 Aug 2014 16:13:52 -0700 Subject: [PATCH 4/7] redesiged SequenceNumberStats child instance handling --- .../networking/src/SequenceNumberStats.cpp | 128 ++++++++++-------- .../networking/src/SequenceNumberStats.h | 11 +- .../src/SequenceNumberStatsTests.cpp | 38 +++++- .../networking/src/SequenceNumberStatsTests.h | 1 + 4 files changed, 112 insertions(+), 66 deletions(-) diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index a700e01f07..7e9022934b 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -13,16 +13,16 @@ #include -SequenceNumberStats::SequenceNumberStats(int statsHistoryLength, int maxRecursion) +SequenceNumberStats::SequenceNumberStats(int statsHistoryLength, bool canDetectOutOfSync) : _received(0), _lastReceivedSequence(0), _missingSet(), _stats(), _lastSenderUUID(), _statsHistory(statsHistoryLength), - - _unreasonableTracker(NULL), - _maxRecursion(maxRecursion) + _canHaveChild(canDetectOutOfSync), + _childInstance(NULL), + _consecutiveReasonable(0) { } @@ -33,11 +33,12 @@ SequenceNumberStats::SequenceNumberStats(const SequenceNumberStats& other) _stats(other._stats), _lastSenderUUID(other._lastSenderUUID), _statsHistory(other._statsHistory), - _unreasonableTracker(NULL), - _maxRecursion(other._maxRecursion) + _childInstance(NULL), + _canHaveChild(other._canHaveChild), + _consecutiveReasonable(other._consecutiveReasonable) { - if (other._unreasonableTracker) { - _unreasonableTracker = new SequenceNumberStats(*other._unreasonableTracker); + if (other._childInstance) { + _childInstance = new SequenceNumberStats(*other._childInstance); } } @@ -48,19 +49,20 @@ SequenceNumberStats& SequenceNumberStats::operator=(const SequenceNumberStats& r _stats = rhs._stats; _lastSenderUUID = rhs._lastSenderUUID; _statsHistory = rhs._statsHistory; - _maxRecursion = rhs._maxRecursion; + _canHaveChild = rhs._canHaveChild; + _consecutiveReasonable = rhs._consecutiveReasonable; - if (rhs._unreasonableTracker) { - _unreasonableTracker = new SequenceNumberStats(*rhs._unreasonableTracker); + if (rhs._childInstance) { + _childInstance = new SequenceNumberStats(*rhs._childInstance); } else { - _unreasonableTracker = NULL; + _childInstance = NULL; } return *this; } SequenceNumberStats::~SequenceNumberStats() { - if (_unreasonableTracker) { - delete _unreasonableTracker; + if (_childInstance) { + delete _childInstance; } } @@ -71,8 +73,9 @@ void SequenceNumberStats::reset() { _lastSenderUUID = QUuid(); _statsHistory.clear(); - if (_unreasonableTracker) { - delete _unreasonableTracker; + if (_childInstance) { + delete _childInstance; + _childInstance = NULL; } } @@ -123,68 +126,78 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui } else if (absGap > MAX_REASONABLE_SEQUENCE_GAP) { arrivalInfo._status = Unreasonable; - /* - // ignore packet if gap is unreasonable - qDebug() << "ignoring unreasonable sequence number:" << incoming - << "previous:" << _lastReceivedSequence; + qDebug() << "unreasonable sequence number:" << incoming << "previous:" << _lastReceivedSequence; + _stats._unreasonable++; - */ + _consecutiveReasonable = 0; - // do not create a child tracker for unreasonable seq nums if this instance is the last one in the chain. - // otherwise, create one if we don't have one. + + // if _canHaveChild, create a child instance of SequenceNumberStats to track this unreasonable seq num and ones in the future. + // if the child instance detects a valid stream of seq nums up to some length, the seq nums sender probably + // fell out of sync with us. - if (!_unreasonableTracker && _maxRecursion > 0) { - _unreasonableTracker = new SequenceNumberStats(0, _maxRecursion - 1); - } + if (_canHaveChild) { + + if (!_childInstance) { + _childInstance = new SequenceNumberStats(0, false); + } - if (_unreasonableTracker) { + ArrivalInfo unreasonableTrackerArrivalInfo = _childInstance->sequenceNumberReceived(incoming); - // track this unreasonable seq number with our _unreasonableTracker. - ArrivalInfo unreasonableTrackerArrivalInfo = _unreasonableTracker->sequenceNumberReceived(incoming); - - const int UNREASONABLE_TRACKER_RECEIVED_THRESHOLD = 10; - const float UNRUNREASONABLE_TRACKER_UNREASONABLE_RATE_THRESHOLD = 0.1f; - const float UNREASONABLE_TRACKER_OUT_OF_ORDER_RATE_THRESHOLD = 0.1f; - - // when our _unreasonableTracker has received enough seq nums and doesn't have an _unreasonableTracker of its own, - // we'll either inherit its state only if we think its stream is plausible. it will then be deleted. - // (if it has an _unreasonableTracker of its own, its _unreasonableTracker may be detecting a plausible stream - // while its parent does not, so we should let it accrue seq nums and decide plausibility first) + // the child instance will be used to detect some threshold number seq nums in a row that are perfectly + // in order. - if (!_unreasonableTracker->hasUnreasonableTracker() && - _unreasonableTracker->_received >= UNREASONABLE_TRACKER_RECEIVED_THRESHOLD) { + const int UNREASONABLE_TRACKER_RECEIVED_THRESHOLD = 8; - if (_unreasonableTracker->getUnreasonableRate() < UNRUNREASONABLE_TRACKER_UNREASONABLE_RATE_THRESHOLD && - _unreasonableTracker->getStats().getOutOfOrderRate() < UNREASONABLE_TRACKER_OUT_OF_ORDER_RATE_THRESHOLD) { + if (unreasonableTrackerArrivalInfo._status != OnTime) { + _childInstance->reset(); - // the _unreasonableTracker has detected a plausible stream of seq numbers; - // copy its state to this tracker. + } else if (_childInstance->getReceived() >= UNREASONABLE_TRACKER_RECEIVED_THRESHOLD) { - _received = _unreasonableTracker->_received; - _lastReceivedSequence = _unreasonableTracker->_lastReceivedSequence; - _missingSet = _unreasonableTracker->_missingSet; - _stats = _unreasonableTracker->_stats; + // the child instance has detected a threshold number of consecutive seq nums. + // copy its state to this instance. - // don't copy _lastSenderUUID; _unreasonableTracker always has null UUID for that member. - // ours should be up-to-date. + _received = _childInstance->_received; + _lastReceivedSequence = _childInstance->_lastReceivedSequence; + _missingSet = _childInstance->_missingSet; + _stats = _childInstance->_stats; - // don't copy _statsHistory; _unreasonableTracker keeps a history of length 0. - // simply clear ours. - _statsHistory.clear(); + // don't copy _lastSenderUUID; _unreasonableTracker always has null UUID for that member. + // ours should be up-to-date. - arrivalInfo = unreasonableTrackerArrivalInfo; + // don't copy _statsHistory; _unreasonableTracker keeps a history of length 0. + // simply clear ours. + _statsHistory.clear(); - } - // remove our _unreasonableTracker - delete _unreasonableTracker; + arrivalInfo = unreasonableTrackerArrivalInfo; + + // delete child instance; + delete _childInstance; + _childInstance = NULL; } } return arrivalInfo; } + _consecutiveReasonable++; + + // if we got a reasonable seq num but have a child instance tracking unreasonable seq nums, + // reset it. if many consecutive reasonable seq nums have occurred (implying the unreasonable seq num + // that caused the creation of the child instance was just a fluke), delete our child instance. + if (_childInstance) { + const int CONSECUTIVE_REASONABLE_CHILD_DELETE_THRESHOLD = 4; + if (_consecutiveReasonable >= CONSECUTIVE_REASONABLE_CHILD_DELETE_THRESHOLD) { + _childInstance->reset(); + } else { + delete _childInstance; + _childInstance = NULL; + } + } + + // now that rollover has been corrected for (if it occurred), incomingInt and expectedInt can be // compared to each other directly, though one of them might be negative @@ -241,6 +254,7 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui } } } + return arrivalInfo; } diff --git a/libraries/networking/src/SequenceNumberStats.h b/libraries/networking/src/SequenceNumberStats.h index 1f740d7af8..71092d7409 100644 --- a/libraries/networking/src/SequenceNumberStats.h +++ b/libraries/networking/src/SequenceNumberStats.h @@ -19,9 +19,6 @@ const int MAX_REASONABLE_SEQUENCE_GAP = 1000; -const int DEFAULT_MAX_RECURSION = 5; - - class PacketStreamStats { public: PacketStreamStats() @@ -65,7 +62,7 @@ public: }; - SequenceNumberStats(int statsHistoryLength = 0, int maxRecursion = DEFAULT_MAX_RECURSION); + SequenceNumberStats(int statsHistoryLength = 0, bool canDetectOutOfSync = true); SequenceNumberStats(const SequenceNumberStats& other); SequenceNumberStats& operator=(const SequenceNumberStats& rhs); ~SequenceNumberStats(); @@ -107,10 +104,10 @@ private: // to deal with the incoming seq nums going out of sync with this tracker, we'll create another instance // of this class when we encounter an unreasonable - SequenceNumberStats* _unreasonableTracker; - int _maxRecursion; + bool _canHaveChild; + SequenceNumberStats* _childInstance; - bool hasUnreasonableTracker() const { return _unreasonableTracker != NULL; } + int _consecutiveReasonable; }; #endif // hifi_SequenceNumberStats_h diff --git a/tests/networking/src/SequenceNumberStatsTests.cpp b/tests/networking/src/SequenceNumberStatsTests.cpp index c4205f5fb5..38aa901a6b 100644 --- a/tests/networking/src/SequenceNumberStatsTests.cpp +++ b/tests/networking/src/SequenceNumberStatsTests.cpp @@ -16,11 +16,12 @@ void SequenceNumberStatsTests::runAllTests() { - + /* rolloverTest(); earlyLateTest(); duplicateTest(); - pruneTest(); + pruneTest();*/ + recursiveTest(); } const quint32 UINT16_RANGE = std::numeric_limits::max() + 1; @@ -278,3 +279,36 @@ void SequenceNumberStatsTests::pruneTest() { numLost = 0; } } + +void SequenceNumberStatsTests::recursiveTest() { + + SequenceNumberStats stats(0); + + quint16 sequence; + + sequence = 89; + stats.sequenceNumberReceived(sequence); + + assert(stats.getUnreasonable() == 0); + + sequence = 2990; + for (int i = 0; i < 10; i++) { + stats.sequenceNumberReceived(sequence); + sequence += (quint16)1; + } + + assert(stats.getUnreasonable() == 0); + + + sequence = 0; + for (int R = 0; R < 7; R++) { + stats.sequenceNumberReceived(sequence); + sequence += (quint16)2000; + } + + for (int i = 0; i < 10; i++) { + stats.sequenceNumberReceived(sequence); + sequence += (quint16)1; + } + assert(stats.getUnreasonable() == 0); +} diff --git a/tests/networking/src/SequenceNumberStatsTests.h b/tests/networking/src/SequenceNumberStatsTests.h index 53a0b66480..80053fd822 100644 --- a/tests/networking/src/SequenceNumberStatsTests.h +++ b/tests/networking/src/SequenceNumberStatsTests.h @@ -23,6 +23,7 @@ namespace SequenceNumberStatsTests { void earlyLateTest(); void duplicateTest(); void pruneTest(); + void recursiveTest(); }; #endif // hifi_SequenceNumberStatsTests_h From fc670d9edb19662a91f4fd9e9cd49fb4dbcc5556 Mon Sep 17 00:00:00 2001 From: wangyix Date: Fri, 1 Aug 2014 16:44:01 -0700 Subject: [PATCH 5/7] minor threshold change --- libraries/networking/src/SequenceNumberStats.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index 7e9022934b..4d6e0bb688 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -186,9 +186,9 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui // if we got a reasonable seq num but have a child instance tracking unreasonable seq nums, // reset it. if many consecutive reasonable seq nums have occurred (implying the unreasonable seq num - // that caused the creation of the child instance was just a fluke), delete our child instance. + // that caused the creation of the child instance was probably a fluke), delete our child instance. if (_childInstance) { - const int CONSECUTIVE_REASONABLE_CHILD_DELETE_THRESHOLD = 4; + const int CONSECUTIVE_REASONABLE_CHILD_DELETE_THRESHOLD = 8; if (_consecutiveReasonable >= CONSECUTIVE_REASONABLE_CHILD_DELETE_THRESHOLD) { _childInstance->reset(); } else { From 1d74ae8197e4ba5d2451a4640b5d22f9a8eeabb9 Mon Sep 17 00:00:00 2001 From: wangyix Date: Fri, 1 Aug 2014 16:49:21 -0700 Subject: [PATCH 6/7] code cleanup --- libraries/networking/src/SequenceNumberStats.cpp | 2 -- libraries/networking/src/SequenceNumberStats.h | 1 - tests/networking/src/SequenceNumberStatsTests.cpp | 3 +-- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index 4d6e0bb688..12901e11cd 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -142,10 +142,8 @@ SequenceNumberStats::ArrivalInfo SequenceNumberStats::sequenceNumberReceived(qui _childInstance = new SequenceNumberStats(0, false); } - ArrivalInfo unreasonableTrackerArrivalInfo = _childInstance->sequenceNumberReceived(incoming); - // the child instance will be used to detect some threshold number seq nums in a row that are perfectly // in order. diff --git a/libraries/networking/src/SequenceNumberStats.h b/libraries/networking/src/SequenceNumberStats.h index 71092d7409..fb3ff68f70 100644 --- a/libraries/networking/src/SequenceNumberStats.h +++ b/libraries/networking/src/SequenceNumberStats.h @@ -67,7 +67,6 @@ public: SequenceNumberStats& operator=(const SequenceNumberStats& rhs); ~SequenceNumberStats(); -public: void reset(); ArrivalInfo sequenceNumberReceived(quint16 incoming, QUuid senderUUID = QUuid(), const bool wantExtraDebugging = false); void pruneMissingSet(const bool wantExtraDebugging = false); diff --git a/tests/networking/src/SequenceNumberStatsTests.cpp b/tests/networking/src/SequenceNumberStatsTests.cpp index 38aa901a6b..1df48b781b 100644 --- a/tests/networking/src/SequenceNumberStatsTests.cpp +++ b/tests/networking/src/SequenceNumberStatsTests.cpp @@ -16,11 +16,10 @@ void SequenceNumberStatsTests::runAllTests() { - /* rolloverTest(); earlyLateTest(); duplicateTest(); - pruneTest();*/ + pruneTest(); recursiveTest(); } From 99df05f770f73b7b5fc9ed6ed52b800a1fdedd81 Mon Sep 17 00:00:00 2001 From: wangyix Date: Fri, 1 Aug 2014 16:54:29 -0700 Subject: [PATCH 7/7] forgot member in reset() --- libraries/networking/src/SequenceNumberStats.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/networking/src/SequenceNumberStats.cpp b/libraries/networking/src/SequenceNumberStats.cpp index 12901e11cd..28ea06787a 100644 --- a/libraries/networking/src/SequenceNumberStats.cpp +++ b/libraries/networking/src/SequenceNumberStats.cpp @@ -77,6 +77,7 @@ void SequenceNumberStats::reset() { delete _childInstance; _childInstance = NULL; } + _consecutiveReasonable = 0; } static const int UINT16_RANGE = std::numeric_limits::max() + 1;