From 33cc801d716621e861fc3791c79ef8c2dc227838 Mon Sep 17 00:00:00 2001 From: Divya Sampath Kumar Date: Mon, 4 Mar 2024 14:57:53 -0800 Subject: [PATCH] Address comments, disable TWCC by default --- README.md | 4 +- samples/Common.c | 1 + src/source/PeerConnection/PeerConnection.c | 31 ++++--- src/source/PeerConnection/Rtcp.c | 100 ++++++++++++--------- 4 files changed, 81 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 3fd1eeeede..3d72f7c522 100644 --- a/README.md +++ b/README.md @@ -356,11 +356,11 @@ In order to listen in on TWCC reports, the application must set up a callback us sampleSenderBandwidthEstimationHandler)); ``` -Note that TWCC is enabled by default in the SDK. In order to disable it, set the `disableSenderSideBandwidthEstimation` flag to TRUE. For example, +Note that TWCC is disabled by default in the SDK samples. In order to enable it, set the `disableSenderSideBandwidthEstimation` flag to FALSE. For example, ``` RtcConfiguration configuration; -configuration.kvsRtcConfiguration.disableSenderSideBandwidthEstimation = TRUE; +configuration.kvsRtcConfiguration.disableSenderSideBandwidthEstimation = FALSE; ``` diff --git a/samples/Common.c b/samples/Common.c index ee1295f366..3eb34c942a 100644 --- a/samples/Common.c +++ b/samples/Common.c @@ -398,6 +398,7 @@ STATUS initializePeerConnection(PSampleConfiguration pSampleConfiguration, PRtcP // Set the ICE mode explicitly configuration.iceTransportPolicy = ICE_TRANSPORT_POLICY_ALL; + configuration.kvsRtcConfiguration.disableSenderSideBandwidthEstimation = TRUE; // Set the STUN server PCHAR pKinesisVideoStunUrlPostFix = KINESIS_VIDEO_STUN_URL_POSTFIX; // If region is in CN, add CN region uri postfix diff --git a/src/source/PeerConnection/PeerConnection.c b/src/source/PeerConnection/PeerConnection.c index badfcdad3b..4a0f3a4546 100644 --- a/src/source/PeerConnection/PeerConnection.c +++ b/src/source/PeerConnection/PeerConnection.c @@ -994,7 +994,8 @@ STATUS freePeerConnection(PRtcPeerConnection* ppPeerConnection) PDoubleListNode pCurNode = NULL; UINT64 item = 0; UINT64 startTime; - UINT32 count; + UINT32 twccHashTableCount = 0; + BOOL twccLocked = FALSE; CHK(ppPeerConnection != NULL, STATUS_NULL_ARG); @@ -1069,12 +1070,16 @@ STATUS freePeerConnection(PRtcPeerConnection* ppPeerConnection) if (pKvsPeerConnection->pTwccManager != NULL) { MUTEX_LOCK(pKvsPeerConnection->twccLock); - hashTableGetCount(pKvsPeerConnection->pTwccManager->pTwccRtpPktInfosHashTable, &count); - DLOGI("Number of TWCC info packets in memory: %d", count); + twccLocked = TRUE; + hashTableGetCount(pKvsPeerConnection->pTwccManager->pTwccRtpPktInfosHashTable, &twccHashTableCount); + DLOGI("Number of TWCC info packets in memory: %d", twccHashTableCount); hashTableIterateEntries(pKvsPeerConnection->pTwccManager->pTwccRtpPktInfosHashTable, 0, freeHashEntry); hashTableFree(pKvsPeerConnection->pTwccManager->pTwccRtpPktInfosHashTable); if (IS_VALID_MUTEX_VALUE(pKvsPeerConnection->twccLock)) { - MUTEX_UNLOCK(pKvsPeerConnection->twccLock); + if (twccLocked) { + MUTEX_UNLOCK(pKvsPeerConnection->twccLock); + twccLocked = FALSE; + } MUTEX_FREE(pKvsPeerConnection->twccLock); } SAFE_MEMFREE(pKvsPeerConnection->pTwccManager); @@ -1781,19 +1786,19 @@ static STATUS twccRollingWindowDeletion(PKvsPeerConnection pKvsPeerConnection, P { ENTERS(); STATUS retStatus = STATUS_SUCCESS; - UINT16 updatedSeqNum; + UINT16 updatedSeqNum = 0; PTwccRtpPacketInfo tempTwccRtpPktInfo = NULL; - UINT64 ageOfOldest, firstRtpTime; - UINT64 value; + UINT64 ageOfOldest = 0, firstRtpTime = 0; + UINT64 twccPacketValue = 0; BOOL isCheckComplete = FALSE; - CHK(pKvsPeerConnection != NULL && pRtpPacket != NULL, STATUS_NULL_ARG); + CHK(pKvsPeerConnection != NULL && pRtpPacket != NULL && pKvsPeerConnection->pTwccManager != NULL, STATUS_NULL_ARG); updatedSeqNum = pKvsPeerConnection->pTwccManager->firstSeqNumInRollingWindow; do { // If the seqNum is not present in the hash table, it is ok. We move on to the next - if (STATUS_SUCCEEDED(hashTableGet(pKvsPeerConnection->pTwccManager->pTwccRtpPktInfosHashTable, updatedSeqNum, &value))) { - tempTwccRtpPktInfo = (PTwccRtpPacketInfo) value; + if (STATUS_SUCCEEDED(hashTableGet(pKvsPeerConnection->pTwccManager->pTwccRtpPktInfosHashTable, updatedSeqNum, &twccPacketValue))) { + tempTwccRtpPktInfo = (PTwccRtpPacketInfo) twccPacketValue; } if (tempTwccRtpPktInfo != NULL) { firstRtpTime = tempTwccRtpPktInfo->localTimeKvs; @@ -1819,6 +1824,8 @@ static STATUS twccRollingWindowDeletion(PKvsPeerConnection pKvsPeerConnection, P } else { updatedSeqNum++; } + // reset before next iteration + tempTwccRtpPktInfo = NULL; } while (!isCheckComplete && updatedSeqNum != (seqNum + 1)); // Update regardless. The loop checks until current RTP packets seq number irrespective of the failure @@ -1833,8 +1840,8 @@ STATUS twccManagerOnPacketSent(PKvsPeerConnection pKvsPeerConnection, PRtpPacket ENTERS(); STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; - UINT16 seqNum; - PTwccRtpPacketInfo pTwccRtpPktInfo; + UINT16 seqNum = 0; + PTwccRtpPacketInfo pTwccRtpPktInfo = NULL; CHK(pKvsPeerConnection != NULL && pRtpPacket != NULL, STATUS_NULL_ARG); CHK(pKvsPeerConnection->onSenderBandwidthEstimation != NULL && pKvsPeerConnection->pTwccManager != NULL, STATUS_SUCCESS); diff --git a/src/source/PeerConnection/Rtcp.c b/src/source/PeerConnection/Rtcp.c index 3e0faed17a..0b08649b0c 100644 --- a/src/source/PeerConnection/Rtcp.c +++ b/src/source/PeerConnection/Rtcp.c @@ -176,8 +176,8 @@ STATUS parseRtcpTwccPacket(PRtcpPacket pRtcpPacket, PTwccManager pTwccManager) UINT32 statuses; UINT32 i; UINT64 referenceTime; - PTwccRtpPacketInfo pTwccPacket; - UINT64 value; + PTwccRtpPacketInfo pTwccPacket = NULL; + UINT64 twccPktValue = 0; CHK(pTwccManager != NULL && pRtcpPacket != NULL, STATUS_NULL_ARG); baseSeqNum = getUnalignedInt16BigEndian(pRtcpPacket->payload + 8); @@ -220,10 +220,12 @@ STATUS parseRtcpTwccPacket(PRtcpPacket pRtcpPacket, PTwccManager pTwccManager) case TWCC_STATUS_SYMBOL_NOTRECEIVED: DLOGS("runLength packetSeqNum %u not received %lu", packetSeqNum, referenceTime); // If it does not exist it means the packet was already visited - if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &value))) { - pTwccPacket = (PTwccRtpPacketInfo) value; - pTwccPacket->remoteTimeKvs = TWCC_PACKET_LOST_TIME; - CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &twccPktValue))) { + pTwccPacket = (PTwccRtpPacketInfo) twccPktValue; + if (pTwccPacket != NULL) { + pTwccPacket->remoteTimeKvs = TWCC_PACKET_LOST_TIME; + CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + } } pTwccManager->lastReportedSeqNum = packetSeqNum; break; @@ -235,15 +237,19 @@ STATUS parseRtcpTwccPacket(PRtcpPacket pRtcpPacket, PTwccManager pTwccManager) DLOGS("runLength packetSeqNum %u received %lu", packetSeqNum, referenceTime); // If it does not exist it means the packet was already visited - if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &value))) { - pTwccPacket = (PTwccRtpPacketInfo) value; - pTwccPacket->remoteTimeKvs = referenceTime; - CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &twccPktValue))) { + pTwccPacket = (PTwccRtpPacketInfo) twccPktValue; + if (pTwccPacket != NULL) { + pTwccPacket->remoteTimeKvs = referenceTime; + CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + } } pTwccManager->lastReportedSeqNum = packetSeqNum; } packetSeqNum++; packetsRemaining--; + // Reset to NULL before next iteration + pTwccPacket = NULL; } } else { statuses = MIN(TWCC_STATUSVECTOR_COUNT(packetChunk), packetsRemaining); @@ -263,10 +269,12 @@ STATUS parseRtcpTwccPacket(PRtcpPacket pRtcpPacket, PTwccManager pTwccManager) case TWCC_STATUS_SYMBOL_NOTRECEIVED: DLOGS("statusVector packetSeqNum %u not received %lu", packetSeqNum, referenceTime); // If it does not exist it means the packet was already visited - if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &value))) { - pTwccPacket = (PTwccRtpPacketInfo) value; - pTwccPacket->remoteTimeKvs = TWCC_PACKET_LOST_TIME; - CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &twccPktValue))) { + pTwccPacket = (PTwccRtpPacketInfo) twccPktValue; + if (pTwccPacket != NULL) { + pTwccPacket->remoteTimeKvs = TWCC_PACKET_LOST_TIME; + CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + } } pTwccManager->lastReportedSeqNum = packetSeqNum; break; @@ -277,15 +285,19 @@ STATUS parseRtcpTwccPacket(PRtcpPacket pRtcpPacket, PTwccManager pTwccManager) referenceTime += KVS_CONVERT_TIMESCALE(recvDelta, TWCC_TICKS_PER_SECOND, HUNDREDS_OF_NANOS_IN_A_SECOND); DLOGS("statusVector packetSeqNum %u received %lu", packetSeqNum, referenceTime); // If it does not exist it means the packet was already visited - if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &value))) { - pTwccPacket = (PTwccRtpPacketInfo) value; - pTwccPacket->remoteTimeKvs = referenceTime; - CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, &twccPktValue))) { + pTwccPacket = (PTwccRtpPacketInfo) twccPktValue; + if (pTwccPacket != NULL) { + pTwccPacket->remoteTimeKvs = referenceTime; + CHK_STATUS(hashTableUpsert(pTwccManager->pTwccRtpPktInfosHashTable, packetSeqNum, (UINT64) pTwccPacket)); + } } pTwccManager->lastReportedSeqNum = packetSeqNum; } packetSeqNum++; packetsRemaining--; + // Reset to NULL before next iteration + pTwccPacket = NULL; } } chunkOffset += TWCC_FB_PACKETCHUNK_SIZE; @@ -299,16 +311,16 @@ STATUS parseRtcpTwccPacket(PRtcpPacket pRtcpPacket, PTwccManager pTwccManager) STATUS onRtcpTwccPacket(PRtcpPacket pRtcpPacket, PKvsPeerConnection pKvsPeerConnection) { STATUS retStatus = STATUS_SUCCESS; - PTwccManager pTwccManager; + PTwccManager pTwccManager = NULL; BOOL locked = FALSE; UINT64 sn = 0; UINT64 localStartTimeKvs, localEndTimeKvs; UINT64 sentBytes = 0, receivedBytes = 0; UINT64 sentPackets = 0, receivedPackets = 0; INT64 duration = 0; - UINT16 seqNum; - PTwccRtpPacketInfo pTwccPacket; - UINT64 value; + UINT16 seqNum = 0; + PTwccRtpPacketInfo pTwccPacket = NULL; + UINT64 twccPktValue = 0; BOOL localStartTimeRecorded = FALSE; CHK(pKvsPeerConnection != NULL && pRtcpPacket != NULL, STATUS_NULL_ARG); @@ -330,36 +342,42 @@ STATUS onRtcpTwccPacket(PRtcpPacket pRtcpPacket, PKvsPeerConnection pKvsPeerConn // This could happen if the prev packet was deleted as part of rolling window or if there // is an overlap of RTP packet statuses between TWCC packets. This could also fail if it is // the first ever packet (seqNum 0) - if (hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, seqNum - 1, &value) == STATUS_HASH_KEY_NOT_PRESENT) { + if (hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, seqNum - 1, &twccPktValue) == STATUS_HASH_KEY_NOT_PRESENT) { localStartTimeKvs = TWCC_PACKET_UNITIALIZED_TIME; } else { - pTwccPacket = (PTwccRtpPacketInfo) value; - localStartTimeKvs = pTwccPacket->localTimeKvs; - localStartTimeRecorded = TRUE; + pTwccPacket = (PTwccRtpPacketInfo) twccPktValue; + if (pTwccPacket != NULL) { + localStartTimeKvs = pTwccPacket->localTimeKvs; + localStartTimeRecorded = TRUE; + } } if (localStartTimeKvs == TWCC_PACKET_UNITIALIZED_TIME) { // time not yet set. If prev seqNum was deleted - if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, seqNum, &value))) { - pTwccPacket = (PTwccRtpPacketInfo) value; - localStartTimeKvs = pTwccPacket->localTimeKvs; - localStartTimeRecorded = TRUE; + if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, seqNum, &twccPktValue))) { + pTwccPacket = (PTwccRtpPacketInfo) twccPktValue; + if (pTwccPacket != NULL) { + localStartTimeKvs = pTwccPacket->localTimeKvs; + localStartTimeRecorded = TRUE; + } } } } // The time it would not succeed is if there is an overlap in the RTP packet status between the TWCC // packets - if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, seqNum, &value))) { - pTwccPacket = (PTwccRtpPacketInfo) value; - localEndTimeKvs = pTwccPacket->localTimeKvs; - duration = localEndTimeKvs - localStartTimeKvs; - sentBytes += pTwccPacket->packetSize; - sentPackets++; - if (pTwccPacket->remoteTimeKvs != TWCC_PACKET_LOST_TIME) { - receivedBytes += pTwccPacket->packetSize; - receivedPackets++; - if (STATUS_SUCCEEDED(hashTableRemove(pTwccManager->pTwccRtpPktInfosHashTable, seqNum))) { - SAFE_MEMFREE(pTwccPacket); + if (STATUS_SUCCEEDED(hashTableGet(pTwccManager->pTwccRtpPktInfosHashTable, seqNum, &twccPktValue))) { + pTwccPacket = (PTwccRtpPacketInfo) twccPktValue; + if (pTwccPacket != NULL) { + localEndTimeKvs = pTwccPacket->localTimeKvs; + duration = localEndTimeKvs - localStartTimeKvs; + sentBytes += pTwccPacket->packetSize; + sentPackets++; + if (pTwccPacket->remoteTimeKvs != TWCC_PACKET_LOST_TIME) { + receivedBytes += pTwccPacket->packetSize; + receivedPackets++; + if (STATUS_SUCCEEDED(hashTableRemove(pTwccManager->pTwccRtpPktInfosHashTable, seqNum))) { + SAFE_MEMFREE(pTwccPacket); + } } } }