From 1be56269a38f89f92258bc87148935c98db61211 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Sat, 23 Sep 2023 12:08:16 -0400 Subject: [PATCH] Fix handling of duplicate packets when OOS data was also received It's possible for a sequence of packets seqnums like 2, 0, 1, 2 to end up hitting the fast path again after filling in gap of OOS packets. The fast path assumes duplicate packets are caught by the next contiguous sequence number check, but this is only true if that value is kept updated. Since the slow path doesn't update the next contiguous sequence number, it's no longer safe to use the fast path for the remaining packets in the frame because duplicate packets would be mishandled. Queuing duplicate packets violates all sorts of queue assumptions and can cause us to return corrupt data to the decoder or attempt an FEC recovery without enough shards. --- src/RtpVideoQueue.c | 23 +++++++++++++++++------ src/RtpVideoQueue.h | 1 + 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/RtpVideoQueue.c b/src/RtpVideoQueue.c index 6662d76..02f3fdd 100644 --- a/src/RtpVideoQueue.c +++ b/src/RtpVideoQueue.c @@ -118,13 +118,14 @@ static bool queuePacket(PRTP_VIDEO_QUEUE queue, PRTPV_QUEUE_ENTRY newEntry, PRTP // If the packet is in order, we can take the fast path and avoid having // to loop through the whole list. If we get an out of order or missing // packet, the fast path will stop working and we'll use the loop instead. - if (packet->sequenceNumber == queue->nextContiguousSequenceNumber) { + // + // NB: It's not enough to just check next contiguous sequence number because + // it's possible that we hit the OOS path earlier which doesn't update the + // next contiguous sequence number. If that happens, we need to use the slow + // path for this entire frame to avoid possibly mishandling a duplicate packet. + if (queue->useFastQueuePath && packet->sequenceNumber == queue->nextContiguousSequenceNumber) { queue->nextContiguousSequenceNumber = U16(packet->sequenceNumber + 1); - - // If we received the next contiguous sequence number but already have missing - // packets, that means we received some later packets before falling back into - // sequence with this one. By definition, that's OOS data so let's tag it. - outOfSequence = queue->missingPackets != 0; + outOfSequence = false; } else { outOfSequence = false; @@ -141,6 +142,11 @@ static bool queuePacket(PRTP_VIDEO_QUEUE queue, PRTPV_QUEUE_ENTRY newEntry, PRTP entry = entry->next; } + + // If we make it here, we cannot use the fast queue path for this frame because + // we're about to queue a non-duplicate packet out of order. This will not update + // nextContiguousSequenceNumber which the fast path relies on. + queue->useFastQueuePath = false; } newEntry->packet = packet; @@ -295,6 +301,10 @@ static int reconstructFrame(PRTP_VIDEO_QUEUE queue) { } #endif + // We should never have duplicate packets enqueued + LC_ASSERT(packets[index] == NULL); + LC_ASSERT(marks[index] != 0); + packets[index] = (unsigned char*) entry->packet; marks[index] = 0; @@ -681,6 +691,7 @@ int RtpvAddPacket(PRTP_VIDEO_QUEUE queue, PRTP_PACKET packet, int length, PRTPV_ queue->receivedParityPackets = 0; queue->receivedHighestSequenceNumber = 0; queue->missingPackets = 0; + queue->useFastQueuePath = true; queue->reportedLostFrame = false; queue->bufferDataPackets = (nvPacket->fecInfo & 0xFFC00000) >> 22; queue->fecPercentage = (nvPacket->fecInfo & 0xFF0) >> 4; diff --git a/src/RtpVideoQueue.h b/src/RtpVideoQueue.h index b247a58..42ce466 100644 --- a/src/RtpVideoQueue.h +++ b/src/RtpVideoQueue.h @@ -34,6 +34,7 @@ typedef struct _RTP_VIDEO_QUEUE { uint32_t fecPercentage; uint32_t nextContiguousSequenceNumber; uint32_t missingPackets; // # of holes behind receivedHighestSequenceNumber + bool useFastQueuePath; bool reportedLostFrame; uint32_t currentFrameNumber;