From 95ae78a7179b858fce4dfcb0f3837b97231b20a7 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Thu, 23 May 2024 12:54:53 -0400 Subject: [PATCH 1/3] fix retransmitter bug --- src/source/PeerConnection/Retransmitter.c | 17 +++++++++++- src/source/Rtcp/RtcpPacket.c | 6 ++-- tst/RtcpFunctionalityTest.cpp | 34 +++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/source/PeerConnection/Retransmitter.c b/src/source/PeerConnection/Retransmitter.c index b8b66a483b..7e493cc77b 100644 --- a/src/source/PeerConnection/Retransmitter.c +++ b/src/source/PeerConnection/Retransmitter.c @@ -1,6 +1,21 @@ #define LOG_CLASS "Retransmitter" #include "../Include_i.h" +/** + Layout of Retransmitter + 0 [ PSequenceNumberList ] + 8 [ seqNumberListLen ] + 12 [ validIndexList ] + 16 [ PValidIndexList ] + 24 (PSequenceNumberList) [ seq_num_0 ] + 26 (PSequenceNumberList + 1) [ seq_num_1 ] + ... + (PSequenceNumberList + seqNumListLen - 1) [ seq_num_{seqNumListLen - 1} ] + PValidIndexList = (PSequenceNumberList + seqNumListLen) [ valid_index_0 ] + (PValidIndexList + 1) [ valid_index_1 ] + ... + (PValidIndexList + validIndexLen - 1) [ valid_index_{seqNumListLen - 1} ] +**/ STATUS createRetransmitter(UINT32 seqNumListLen, UINT32 validIndexListLen, PRetransmitter* ppRetransmitter) { @@ -105,7 +120,7 @@ STATUS resendPacketOnNack(PRtcpPacket pRtcpPacket, PKvsPeerConnection pKvsPeerCo } // putBackPacketToRollingBuffer retStatus = - rollingBufferInsertData(pSenderTranceiver->sender.packetBuffer->pRollingBuffer, pRetransmitter->sequenceNumberList[index], item); + rollingBufferInsertData(pSenderTranceiver->sender.packetBuffer->pRollingBuffer, pRetransmitter->validIndexList[index], item); CHK(retStatus == STATUS_SUCCESS || retStatus == STATUS_ROLLING_BUFFER_NOT_IN_RANGE, retStatus); // free the packet if it is not in the valid range any more diff --git a/src/source/Rtcp/RtcpPacket.c b/src/source/Rtcp/RtcpPacket.c index 9346f90af4..7dd6614ef3 100644 --- a/src/source/Rtcp/RtcpPacket.c +++ b/src/source/Rtcp/RtcpPacket.c @@ -32,7 +32,7 @@ STATUS setRtcpPacketFromBytes(PBYTE pRawPacket, UINT32 pRawPacketsLen, PRtcpPack return retStatus; } -// Given a RTCP Packet list extract the list of SSRCes, since the list of SSRCes may not be know ahead of time (because of BLP) +// Given a RTCP Packet list extract the list of SSRCes, since the list of SSRCes may not be known ahead of time (because of BLP) // we need to allocate the list dynamically STATUS rtcpNackListGet(PBYTE pPayload, UINT32 payloadLen, PUINT32 pSenderSsrc, PUINT32 pReceiverSsrc, PUINT16 pSequenceNumberList, PUINT32 pSequenceNumberListLen) @@ -55,14 +55,14 @@ STATUS rtcpNackListGet(PBYTE pPayload, UINT32 payloadLen, PUINT32 pSenderSsrc, P BLP = getInt16(*(PUINT16) (pPayload + i + 2)); // If pSsrcList is not NULL and we have space push and increment - if (pSequenceNumberList != NULL && sequenceNumberCount <= *pSequenceNumberListLen) { + if (pSequenceNumberList != NULL && sequenceNumberCount < *pSequenceNumberListLen) { pSequenceNumberList[sequenceNumberCount] = currentSequenceNumber; } sequenceNumberCount++; for (j = 0; j < 16; j++) { if ((BLP & (1 << j)) >> j) { - if (pSequenceNumberList != NULL && sequenceNumberCount <= *pSequenceNumberListLen) { + if (pSequenceNumberList != NULL && sequenceNumberCount < *pSequenceNumberListLen) { pSequenceNumberList[sequenceNumberCount] = (currentSequenceNumber + j + 1); } sequenceNumberCount++; diff --git a/tst/RtcpFunctionalityTest.cpp b/tst/RtcpFunctionalityTest.cpp index d7cc2437fe..60af25d21b 100644 --- a/tst/RtcpFunctionalityTest.cpp +++ b/tst/RtcpFunctionalityTest.cpp @@ -168,6 +168,40 @@ TEST_F(RtcpFunctionalityTest, onRtcpPacketCompoundNack) freeRtpPacket(&pRtpPacket); } +TEST_F(RtcpFunctionalityTest, createRetransmitterTest) +{ + PRtpPacket pRtpPacket = nullptr; + RtcOutboundRtpStreamStats stats{}; + BYTE validRtcpPacket[] = {0x81, 0xcd, 0x00, 0x03, 0x2c, 0xd1, 0xa0, 0xde, 0x00, 0x00, 0xab, 0xe0, 0x00, 0x00, 0x00, 0x00}; + initTransceiver(44000); + EXPECT_EQ(createRtpRollingBuffer(DEFAULT_ROLLING_BUFFER_DURATION_IN_SECONDS * DEFAULT_EXPECTED_VIDEO_BIT_RATE / 8 / DEFAULT_MTU_SIZE_BYTES, + &pKvsRtpTransceiver->sender.packetBuffer), STATUS_SUCCESS); + EXPECT_EQ(createRetransmitter(1, 1, &pKvsRtpTransceiver->sender.retransmitter), STATUS_SUCCESS); + + UINT16 x = 65000; + UINT64 y = 829482422; + + pKvsRtpTransceiver->sender.retransmitter->sequenceNumberList[0] = x; + pKvsRtpTransceiver->sender.retransmitter->validIndexList[0] = y; + + EXPECT_EQ(pKvsRtpTransceiver->sender.retransmitter->seqNumListLen, 1); + EXPECT_EQ(pKvsRtpTransceiver->sender.retransmitter->validIndexListLen, 1); + + DLOGD("ReTransmitter: \n%p\n%p\n%p\n%p\n%p\n", (void*)pKvsRtpTransceiver->sender.retransmitter, (void*)pKvsRtpTransceiver->sender.retransmitter->sequenceNumberList, + (void*)(&pKvsRtpTransceiver->sender.retransmitter->seqNumListLen),(void*)(&pKvsRtpTransceiver->sender.retransmitter->validIndexListLen), + (void*)pKvsRtpTransceiver->sender.retransmitter->validIndexList); + + //EXPECT_EQ(createRtpPacketWithSeqNum(0, &pRtpPacket), STATUS_SUCCESS); + //EXPECT_EQ(rtpRollingBufferAddRtpPacket(pKvsRtpTransceiver->sender.packetBuffer, pRtpPacket), STATUS_SUCCESS); + //EXPECT_EQ(onRtcpPacket(pKvsPeerConnection, validRtcpPacket, SIZEOF(validRtcpPacket)), STATUS_SUCCESS); + //getRtpOutboundStats(pRtcPeerConnection, nullptr, &stats); + //EXPECT_EQ(stats.nackCount, 1); + //EXPECT_EQ(stats.retransmittedPacketsSent, 1); + //EXPECT_EQ(stats.retransmittedBytesSent, 10); + EXPECT_EQ(freePeerConnection(&pRtcPeerConnection), STATUS_SUCCESS); + //freeRtpPacket(&pRtpPacket); +} + TEST_F(RtcpFunctionalityTest, onRtcpPacketCompound) { KvsPeerConnection peerConnection{}; From ce3edfe8dc60982aa74716013744eae9df7a0986 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Thu, 23 May 2024 13:01:42 -0400 Subject: [PATCH 2/3] remove partial test, will add back later once fully fleshed out --- tst/RtcpFunctionalityTest.cpp | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/tst/RtcpFunctionalityTest.cpp b/tst/RtcpFunctionalityTest.cpp index 60af25d21b..21259a87e5 100644 --- a/tst/RtcpFunctionalityTest.cpp +++ b/tst/RtcpFunctionalityTest.cpp @@ -168,39 +168,6 @@ TEST_F(RtcpFunctionalityTest, onRtcpPacketCompoundNack) freeRtpPacket(&pRtpPacket); } -TEST_F(RtcpFunctionalityTest, createRetransmitterTest) -{ - PRtpPacket pRtpPacket = nullptr; - RtcOutboundRtpStreamStats stats{}; - BYTE validRtcpPacket[] = {0x81, 0xcd, 0x00, 0x03, 0x2c, 0xd1, 0xa0, 0xde, 0x00, 0x00, 0xab, 0xe0, 0x00, 0x00, 0x00, 0x00}; - initTransceiver(44000); - EXPECT_EQ(createRtpRollingBuffer(DEFAULT_ROLLING_BUFFER_DURATION_IN_SECONDS * DEFAULT_EXPECTED_VIDEO_BIT_RATE / 8 / DEFAULT_MTU_SIZE_BYTES, - &pKvsRtpTransceiver->sender.packetBuffer), STATUS_SUCCESS); - EXPECT_EQ(createRetransmitter(1, 1, &pKvsRtpTransceiver->sender.retransmitter), STATUS_SUCCESS); - - UINT16 x = 65000; - UINT64 y = 829482422; - - pKvsRtpTransceiver->sender.retransmitter->sequenceNumberList[0] = x; - pKvsRtpTransceiver->sender.retransmitter->validIndexList[0] = y; - - EXPECT_EQ(pKvsRtpTransceiver->sender.retransmitter->seqNumListLen, 1); - EXPECT_EQ(pKvsRtpTransceiver->sender.retransmitter->validIndexListLen, 1); - - DLOGD("ReTransmitter: \n%p\n%p\n%p\n%p\n%p\n", (void*)pKvsRtpTransceiver->sender.retransmitter, (void*)pKvsRtpTransceiver->sender.retransmitter->sequenceNumberList, - (void*)(&pKvsRtpTransceiver->sender.retransmitter->seqNumListLen),(void*)(&pKvsRtpTransceiver->sender.retransmitter->validIndexListLen), - (void*)pKvsRtpTransceiver->sender.retransmitter->validIndexList); - - //EXPECT_EQ(createRtpPacketWithSeqNum(0, &pRtpPacket), STATUS_SUCCESS); - //EXPECT_EQ(rtpRollingBufferAddRtpPacket(pKvsRtpTransceiver->sender.packetBuffer, pRtpPacket), STATUS_SUCCESS); - //EXPECT_EQ(onRtcpPacket(pKvsPeerConnection, validRtcpPacket, SIZEOF(validRtcpPacket)), STATUS_SUCCESS); - //getRtpOutboundStats(pRtcPeerConnection, nullptr, &stats); - //EXPECT_EQ(stats.nackCount, 1); - //EXPECT_EQ(stats.retransmittedPacketsSent, 1); - //EXPECT_EQ(stats.retransmittedBytesSent, 10); - EXPECT_EQ(freePeerConnection(&pRtcPeerConnection), STATUS_SUCCESS); - //freeRtpPacket(&pRtpPacket); -} TEST_F(RtcpFunctionalityTest, onRtcpPacketCompound) { From c9728838bd42a3aff92bd2c6c5c778a2185b06fc Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Thu, 23 May 2024 13:12:02 -0400 Subject: [PATCH 3/3] clang format --- src/source/PeerConnection/Retransmitter.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/source/PeerConnection/Retransmitter.c b/src/source/PeerConnection/Retransmitter.c index 7e493cc77b..3e28373b0f 100644 --- a/src/source/PeerConnection/Retransmitter.c +++ b/src/source/PeerConnection/Retransmitter.c @@ -119,8 +119,7 @@ STATUS resendPacketOnNack(PRtcpPacket pRtcpPacket, PKvsPeerConnection pKvsPeerCo DLOGV("Resent packet ssrc %lu seq %lu failed 0x%08x", pRtpPacket->header.ssrc, pRtpPacket->header.sequenceNumber, retStatus); } // putBackPacketToRollingBuffer - retStatus = - rollingBufferInsertData(pSenderTranceiver->sender.packetBuffer->pRollingBuffer, pRetransmitter->validIndexList[index], item); + retStatus = rollingBufferInsertData(pSenderTranceiver->sender.packetBuffer->pRollingBuffer, pRetransmitter->validIndexList[index], item); CHK(retStatus == STATUS_SUCCESS || retStatus == STATUS_ROLLING_BUFFER_NOT_IN_RANGE, retStatus); // free the packet if it is not in the valid range any more