From e364b8d33e5ee01528b377a9904fc64d4945b80d Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 13 Apr 2017 13:35:59 -0700 Subject: [PATCH 1/4] avatar-mixer resends to avoid stale avatar --- .../src/avatars/AvatarMixerSlave.cpp | 21 ++++++++++++++----- libraries/avatars/src/AvatarData.h | 7 +++---- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index c4497a1066..a4e9b608de 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -175,6 +175,7 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { } }); + const uint64_t MIN_KEEP_ALIVE_PERIOD_FOR_OTHER_AVATAR = (AVATAR_UPDATE_TIMEOUT - 1) * USECS_PER_SECOND; AvatarSharedPointer thisAvatar = nodeData->getAvatarSharedPointer(); ViewFrustum cameraView = nodeData->getViewFrustom(); std::priority_queue sortedAvatars; @@ -262,11 +263,17 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // have 15 (45hz-30hz) duplicate frames. In this case, the stat // avg_other_av_skips_per_second does report 15. // - // make sure we haven't already sent this data from this sender to this receiver + // make sure we haven't already sent this data from this sender to that receiver // or that somehow we haven't sent if (lastSeqToReceiver == lastSeqFromSender && lastSeqToReceiver != 0) { - ++numAvatarsHeldBack; - shouldIgnore = true; + // don't ignore this avatar if we haven't sent any update for a long while + uint64_t lastBroadcastTime = nodeData->getLastBroadcastTime(avatarNode->getUUID()); + const AvatarMixerClientData* otherNodeData = reinterpret_cast(avatarNode->getLinkedData()); + if (lastBroadcastTime > otherNodeData->getIdentityChangeTimestamp() && + lastBroadcastTime > startIgnoreCalculation - MIN_KEEP_ALIVE_PERIOD_FOR_OTHER_AVATAR) { + ++numAvatarsHeldBack; + shouldIgnore = true; + } } else if (lastSeqFromSender - lastSeqToReceiver > 1) { // this is a skip - we still send the packet but capture the presence of the skip so we see it happening ++numAvatarsWithSkippedFrames; @@ -302,8 +309,12 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { const AvatarMixerClientData* otherNodeData = reinterpret_cast(otherNode->getLinkedData()); // If the time that the mixer sent AVATAR DATA about Avatar B to Avatar A is BEFORE OR EQUAL TO - // the time that Avatar B flagged an IDENTITY DATA change, send IDENTITY DATA about Avatar B to Avatar A. - if (nodeData->getLastBroadcastTime(otherNode->getUUID()) <= otherNodeData->getIdentityChangeTimestamp()) { + // the time that Avatar B flagged an IDENTITY DATA change + // or if no packet of any type has been sent for some time + // send IDENTITY DATA about Avatar B to Avatar A + uint64_t lastBroadcastTime = nodeData->getLastBroadcastTime(otherNode->getUUID()); + if (lastBroadcastTime <= otherNodeData->getIdentityChangeTimestamp() || + startAvatarDataPacking > lastBroadcastTime + MIN_KEEP_ALIVE_PERIOD_FOR_OTHER_AVATAR) { identityBytesSent += sendIdentityPacket(otherNodeData, node); } diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 5ade0c448e..8319eb5249 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -110,6 +110,8 @@ const char LEFT_HAND_POINTING_FLAG = 1; const char RIGHT_HAND_POINTING_FLAG = 2; const char IS_FINGER_POINTING_FLAG = 4; +const qint64 AVATAR_UPDATE_TIMEOUT = 5 * USECS_PER_SECOND; + // AvatarData state flags - we store the details about the packet encoding in the first byte, // before the "header" structure const char AVATARDATA_FLAGS_MINIMUM = 0; @@ -599,10 +601,7 @@ public: } - bool shouldDie() const { - const qint64 AVATAR_SILENCE_THRESHOLD_USECS = 5 * USECS_PER_SECOND; - return _owningAvatarMixer.isNull() || getUsecsSinceLastUpdate() > AVATAR_SILENCE_THRESHOLD_USECS; - } + bool shouldDie() const { return _owningAvatarMixer.isNull() || getUsecsSinceLastUpdate() > AVATAR_UPDATE_TIMEOUT; } static const float OUT_OF_VIEW_PENALTY; From 63ea16f24b81a34e475e5b2e66f55b71348f1012 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 13 Apr 2017 13:37:18 -0700 Subject: [PATCH 2/4] avatar-mixer to send identity packets as 'reliable' --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index a4e9b608de..56c15f7793 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -69,7 +69,7 @@ void AvatarMixerSlave::processIncomingPackets(const SharedNodePointer& node) { int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData, const SharedNodePointer& destinationNode) { int bytesSent = 0; QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(); - auto identityPacket = NLPacket::create(PacketType::AvatarIdentity, individualData.size()); + auto identityPacket = NLPacket::create(PacketType::AvatarIdentity, individualData.size(), true, true); individualData.replace(0, NUM_BYTES_RFC4122_UUID, nodeData->getNodeID().toRfc4122()); // FIXME, this looks suspicious bytesSent += individualData.size(); identityPacket->write(individualData); From 14b60165efb5c5f6126a0c19eeee8a9c24461ae5 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 13 Apr 2017 14:55:08 -0700 Subject: [PATCH 3/4] don't keep stale avatars alive --- .../src/avatars/AvatarMixerSlave.cpp | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 56c15f7793..ee3282f82b 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -67,15 +67,13 @@ void AvatarMixerSlave::processIncomingPackets(const SharedNodePointer& node) { int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData, const SharedNodePointer& destinationNode) { - int bytesSent = 0; QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(); - auto identityPacket = NLPacket::create(PacketType::AvatarIdentity, individualData.size(), true, true); individualData.replace(0, NUM_BYTES_RFC4122_UUID, nodeData->getNodeID().toRfc4122()); // FIXME, this looks suspicious - bytesSent += individualData.size(); - identityPacket->write(individualData); - DependencyManager::get()->sendPacket(std::move(identityPacket), *destinationNode); + auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true); + identityPackets->write(individualData); + DependencyManager::get()->sendPacketList(std::move(identityPackets), *destinationNode); _stats.numIdentityPackets++; - return bytesSent; + return individualData.size(); } static const int AVATAR_MIXER_BROADCAST_FRAMES_PER_SECOND = 45; @@ -175,7 +173,6 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { } }); - const uint64_t MIN_KEEP_ALIVE_PERIOD_FOR_OTHER_AVATAR = (AVATAR_UPDATE_TIMEOUT - 1) * USECS_PER_SECOND; AvatarSharedPointer thisAvatar = nodeData->getAvatarSharedPointer(); ViewFrustum cameraView = nodeData->getViewFrustom(); std::priority_queue sortedAvatars; @@ -263,17 +260,11 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // have 15 (45hz-30hz) duplicate frames. In this case, the stat // avg_other_av_skips_per_second does report 15. // - // make sure we haven't already sent this data from this sender to that receiver + // make sure we haven't already sent this data from this sender to this receiver // or that somehow we haven't sent if (lastSeqToReceiver == lastSeqFromSender && lastSeqToReceiver != 0) { - // don't ignore this avatar if we haven't sent any update for a long while - uint64_t lastBroadcastTime = nodeData->getLastBroadcastTime(avatarNode->getUUID()); - const AvatarMixerClientData* otherNodeData = reinterpret_cast(avatarNode->getLinkedData()); - if (lastBroadcastTime > otherNodeData->getIdentityChangeTimestamp() && - lastBroadcastTime > startIgnoreCalculation - MIN_KEEP_ALIVE_PERIOD_FOR_OTHER_AVATAR) { - ++numAvatarsHeldBack; - shouldIgnore = true; - } + ++numAvatarsHeldBack; + shouldIgnore = true; } else if (lastSeqFromSender - lastSeqToReceiver > 1) { // this is a skip - we still send the packet but capture the presence of the skip so we see it happening ++numAvatarsWithSkippedFrames; @@ -312,9 +303,10 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // the time that Avatar B flagged an IDENTITY DATA change // or if no packet of any type has been sent for some time // send IDENTITY DATA about Avatar B to Avatar A + const uint64_t AVATAR_UPDATE_STALE = AVATAR_UPDATE_TIMEOUT - USECS_PER_SECOND; uint64_t lastBroadcastTime = nodeData->getLastBroadcastTime(otherNode->getUUID()); if (lastBroadcastTime <= otherNodeData->getIdentityChangeTimestamp() || - startAvatarDataPacking > lastBroadcastTime + MIN_KEEP_ALIVE_PERIOD_FOR_OTHER_AVATAR) { + startAvatarDataPacking > lastBroadcastTime + AVATAR_UPDATE_STALE) { identityBytesSent += sendIdentityPacket(otherNodeData, node); } From 5bdecfbfa0b59cf1406c73aa127d768d3799e6d9 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 13 Apr 2017 16:00:11 -0700 Subject: [PATCH 4/4] send keep-alive packets but don't resend identity --- .../src/avatars/AvatarMixerSlave.cpp | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index ee3282f82b..6e3dd150a4 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -263,8 +263,16 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { // make sure we haven't already sent this data from this sender to this receiver // or that somehow we haven't sent if (lastSeqToReceiver == lastSeqFromSender && lastSeqToReceiver != 0) { - ++numAvatarsHeldBack; - shouldIgnore = true; + // don't ignore this avatar if we haven't sent any update for a long while + // in an effort to prevent other interfaces from deleting a stale avatar instance + uint64_t lastBroadcastTime = nodeData->getLastBroadcastTime(avatarNode->getUUID()); + const AvatarMixerClientData* otherNodeData = reinterpret_cast(avatarNode->getLinkedData()); + const uint64_t AVATAR_UPDATE_STALE = AVATAR_UPDATE_TIMEOUT - USECS_PER_SECOND; + if (lastBroadcastTime > otherNodeData->getIdentityChangeTimestamp() && + lastBroadcastTime + AVATAR_UPDATE_STALE > startIgnoreCalculation) { + ++numAvatarsHeldBack; + shouldIgnore = true; + } } else if (lastSeqFromSender - lastSeqToReceiver > 1) { // this is a skip - we still send the packet but capture the presence of the skip so we see it happening ++numAvatarsWithSkippedFrames; @@ -300,13 +308,8 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) { const AvatarMixerClientData* otherNodeData = reinterpret_cast(otherNode->getLinkedData()); // If the time that the mixer sent AVATAR DATA about Avatar B to Avatar A is BEFORE OR EQUAL TO - // the time that Avatar B flagged an IDENTITY DATA change - // or if no packet of any type has been sent for some time - // send IDENTITY DATA about Avatar B to Avatar A - const uint64_t AVATAR_UPDATE_STALE = AVATAR_UPDATE_TIMEOUT - USECS_PER_SECOND; - uint64_t lastBroadcastTime = nodeData->getLastBroadcastTime(otherNode->getUUID()); - if (lastBroadcastTime <= otherNodeData->getIdentityChangeTimestamp() || - startAvatarDataPacking > lastBroadcastTime + AVATAR_UPDATE_STALE) { + // the time that Avatar B flagged an IDENTITY DATA change, send IDENTITY DATA about Avatar B to Avatar A. + if (nodeData->getLastBroadcastTime(otherNode->getUUID()) <= otherNodeData->getIdentityChangeTimestamp()) { identityBytesSent += sendIdentityPacket(otherNodeData, node); }