From 184c91da77cf72ba8d17b79acdf3770463aad5da Mon Sep 17 00:00:00 2001 From: James Delaplane Date: Wed, 30 Mar 2022 10:27:38 -0700 Subject: [PATCH 1/3] Addressing error handling concerns for an edge case --- src/source/PeerConnection/JitterBuffer.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/source/PeerConnection/JitterBuffer.c b/src/source/PeerConnection/JitterBuffer.c index 2d919d07c6..4b61fdd1a3 100644 --- a/src/source/PeerConnection/JitterBuffer.c +++ b/src/source/PeerConnection/JitterBuffer.c @@ -221,12 +221,16 @@ STATUS jitterBufferInternalParse(PJitterBuffer pJitterBuffer, BOOL bufferClosed) } else { lastNonNullIndex = index; retStatus = hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue); - pCurPacket = (PRtpPacket) hashValue; - if (retStatus == STATUS_SUCCESS || retStatus == STATUS_HASH_KEY_NOT_PRESENT) { + if (retStatus == STATUS_SUCCESS) { retStatus = STATUS_SUCCESS; + } else if (retStatus == STATUS_HASH_KEY_NOT_PRESENT) { + // should be unreachable, this means hashTablContains() said we had it but hashTableGet() couldn't find it. + continue; } else { CHK(FALSE, retStatus); } + pCurPacket = (PRtpPacket) hashValue; + CHK(pCurPacket != NULL, STATUS_NULL_ARG); curTimestamp = pCurPacket->header.timestamp; // new timestamp on an RTP packet means new frame if (curTimestamp != pJitterBuffer->headTimestamp) { @@ -315,7 +319,9 @@ STATUS jitterBufferDropBufferData(PJitterBuffer pJitterBuffer, UINT16 startIndex if (hasEntry) { CHK_STATUS(hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue)); pCurPacket = (PRtpPacket) hashValue; - freeRtpPacket(&pCurPacket); + if(pCurPacket) { + freeRtpPacket(&pCurPacket); + } CHK_STATUS(hashTableRemove(pJitterBuffer->pPkgBufferHashTable, index)); } } @@ -344,13 +350,8 @@ STATUS jitterBufferFillFrameData(PJitterBuffer pJitterBuffer, PBYTE pFrame, UINT CHK(pJitterBuffer != NULL && pFrame != NULL && pFilledSize != NULL, STATUS_NULL_ARG); for (; UINT16_DEC(index) != endIndex; index++) { hashValue = 0; - retStatus = hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue); + CHK_STATUS(hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue)); pCurPacket = (PRtpPacket) hashValue; - if (retStatus == STATUS_SUCCESS || retStatus == STATUS_HASH_KEY_NOT_PRESENT) { - retStatus = STATUS_SUCCESS; - } else { - CHK(FALSE, retStatus); - } CHK(pCurPacket != NULL, STATUS_NULL_ARG); partialFrameSize = remainingFrameSize; CHK_STATUS(pJitterBuffer->depayPayloadFn(pCurPacket->payload, pCurPacket->payloadLength, pCurPtrInFrame, &partialFrameSize, NULL)); From 5cf46c96919a4da446c4c11fd54d9959f7c77533 Mon Sep 17 00:00:00 2001 From: James Delaplane Date: Wed, 30 Mar 2022 10:29:49 -0700 Subject: [PATCH 2/3] Clang format --- src/source/PeerConnection/JitterBuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/PeerConnection/JitterBuffer.c b/src/source/PeerConnection/JitterBuffer.c index 4b61fdd1a3..3fa4ac2201 100644 --- a/src/source/PeerConnection/JitterBuffer.c +++ b/src/source/PeerConnection/JitterBuffer.c @@ -319,7 +319,7 @@ STATUS jitterBufferDropBufferData(PJitterBuffer pJitterBuffer, UINT16 startIndex if (hasEntry) { CHK_STATUS(hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue)); pCurPacket = (PRtpPacket) hashValue; - if(pCurPacket) { + if (pCurPacket) { freeRtpPacket(&pCurPacket); } CHK_STATUS(hashTableRemove(pJitterBuffer->pPkgBufferHashTable, index)); From 7adf1e77b0ebe376464f32ac1c82ed01fa875c4a Mon Sep 17 00:00:00 2001 From: James Delaplane Date: Thu, 31 Mar 2022 16:58:45 -0700 Subject: [PATCH 3/3] Removed useless status_success check, changed test to look for correct error value --- src/source/PeerConnection/JitterBuffer.c | 6 ++---- tst/JitterBufferFunctionalityTest.cpp | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/source/PeerConnection/JitterBuffer.c b/src/source/PeerConnection/JitterBuffer.c index 3fa4ac2201..aea092edc0 100644 --- a/src/source/PeerConnection/JitterBuffer.c +++ b/src/source/PeerConnection/JitterBuffer.c @@ -221,13 +221,11 @@ STATUS jitterBufferInternalParse(PJitterBuffer pJitterBuffer, BOOL bufferClosed) } else { lastNonNullIndex = index; retStatus = hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue); - if (retStatus == STATUS_SUCCESS) { - retStatus = STATUS_SUCCESS; - } else if (retStatus == STATUS_HASH_KEY_NOT_PRESENT) { + if (retStatus == STATUS_HASH_KEY_NOT_PRESENT) { // should be unreachable, this means hashTablContains() said we had it but hashTableGet() couldn't find it. continue; } else { - CHK(FALSE, retStatus); + CHK_STATUS(retStatus); } pCurPacket = (PRtpPacket) hashValue; CHK(pCurPacket != NULL, STATUS_NULL_ARG); diff --git a/tst/JitterBufferFunctionalityTest.cpp b/tst/JitterBufferFunctionalityTest.cpp index d88717b8da..c857bea78a 100644 --- a/tst/JitterBufferFunctionalityTest.cpp +++ b/tst/JitterBufferFunctionalityTest.cpp @@ -448,7 +448,7 @@ TEST_F(JitterBufferFunctionalityTest, fillDataReturnErrorWithImcompleteFrame) EXPECT_EQ(STATUS_SUCCESS, jitterBufferPush(mJitterBuffer, mPRtpPackets[i], nullptr)); } - EXPECT_EQ(STATUS_NULL_ARG, jitterBufferFillFrameData(mJitterBuffer, buffer, 2, &filledSize, 0, 1)); + EXPECT_EQ(STATUS_HASH_KEY_NOT_PRESENT, jitterBufferFillFrameData(mJitterBuffer, buffer, 2, &filledSize, 0, 1)); clearJitterBufferForTest(); MEMFREE(buffer);