From 1c386a898731ae07168d52c835657409b54f7790 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 13 Nov 2017 10:15:43 -0800 Subject: [PATCH] Fix race condition that could result in loss of NALUs in an IDR frame Unfortunately, this was a design flaw in the way we handled IDR frames NALUs. Because we split them all up and handled them as separate "frames" in the system, some of them could be discarded due to a video decode unit overflow, for example, and cause the decoder to get confused due to receiving an SPS without a following PPS. To address this, NALUs in the IDR frame are always delivered together as one frame. The SPS, PPS, and VPS are now in separate buffers in the IDR frame's buffer list and tagged as such with the new buffer type field. The IDR frames themselves have a new frame type field which marks them as special frames that clients may need to process in a special way. This will likely be a breaking change for your clients! --- src/Limelight.h | 23 ++++++++++++++++ src/VideoDepacketizer.c | 59 ++++++++++++++++++++++++++++++++++------- 2 files changed, 73 insertions(+), 9 deletions(-) diff --git a/src/Limelight.h b/src/Limelight.h index 26531cf..8bbe596 100644 --- a/src/Limelight.h +++ b/src/Limelight.h @@ -61,6 +61,13 @@ typedef struct _STREAM_CONFIGURATION { // Use this function to zero the stream configuration when allocated on the stack or heap void LiInitializeStreamConfiguration(PSTREAM_CONFIGURATION streamConfig); +// These identify codec configuration data in the buffer lists +// of frames identified as IDR frames. +#define BUFFER_TYPE_PICDATA 0x00 +#define BUFFER_TYPE_SPS 0x01 +#define BUFFER_TYPE_PPS 0x02 +#define BUFFER_TYPE_VPS 0x03 + typedef struct _LENTRY { // Pointer to the next entry or NULL if this is the last entry struct _LENTRY* next; @@ -70,13 +77,29 @@ typedef struct _LENTRY { // Size of data in bytes (never <= 0) int length; + + // Buffer type (listed above) + int bufferType; } LENTRY, *PLENTRY; +// This is a standard frame which references the IDR frame and +// previous P-frames. +#define FRAME_TYPE_PFRAME 0x00 + +// Indicates this frame contains SPS, PPS, and VPS (if applicable) +// as the first buffers in the list. Each NALU will appear as a separate +// buffer in the buffer list. The I-frame data follows immediately +// after the codec configuration NALUs. +#define FRAME_TYPE_IDR 0x01 + // A decode unit describes a buffer chain of video data from multiple packets typedef struct _DECODE_UNIT { // Frame number int frameNumber; + // Frame type + int frameType; + // Receive time of first buffer // NOTE: This will be populated from gettimeofday() if !HAVE_CLOCK_GETTIME and // populated from clock_gettime(CLOCK_MONOTONIC) if HAVE_CLOCK_GETTIME diff --git a/src/VideoDepacketizer.c b/src/VideoDepacketizer.c index 5a73649..ba045fd 100644 --- a/src/VideoDepacketizer.c +++ b/src/VideoDepacketizer.c @@ -231,6 +231,14 @@ static void reassembleFrame(int frameNumber) { qdu->decodeUnit.frameNumber = frameNumber; qdu->decodeUnit.receiveTimeMs = firstPacketReceiveTime; + // IDR frames will have leading CSD buffers + if (nalChainHead->bufferType != BUFFER_TYPE_PICDATA) { + qdu->decodeUnit.frameType = FRAME_TYPE_IDR; + } + else { + qdu->decodeUnit.frameType = FRAME_TYPE_PFRAME; + } + nalChainHead = NULL; nalChainDataLength = 0; @@ -274,7 +282,43 @@ static void reassembleFrame(int frameNumber) { } } -static void queueFragment(char*data, int offset, int length) { + +#define AVC_NAL_TYPE_SPS 0x67 +#define AVC_NAL_TYPE_PPS 0x68 +#define HEVC_NAL_TYPE_VPS 0x40 +#define HEVC_NAL_TYPE_SPS 0x42 +#define HEVC_NAL_TYPE_PPS 0x44 + +static int getBufferFlags(char* data, int length) { + BUFFER_DESC buffer; + BUFFER_DESC candidate; + + buffer.data = data; + buffer.length = (unsigned int)length; + buffer.offset = 0; + + if (!getSpecialSeq(&buffer, &candidate) || !isSeqFrameStart(&candidate)) { + return BUFFER_TYPE_PICDATA; + } + + switch (candidate.data[candidate.offset + candidate.length]) { + case AVC_NAL_TYPE_SPS: + case HEVC_NAL_TYPE_SPS: + return BUFFER_TYPE_SPS; + + case AVC_NAL_TYPE_PPS: + case HEVC_NAL_TYPE_PPS: + return BUFFER_TYPE_PPS; + + case HEVC_NAL_TYPE_VPS: + return BUFFER_TYPE_VPS; + + default: + return BUFFER_TYPE_PICDATA; + } +} + +static void queueFragment(char* data, int offset, int length) { PLENTRY entry = (PLENTRY)malloc(sizeof(*entry) + length); if (entry != NULL) { entry->next = NULL; @@ -283,6 +327,8 @@ static void queueFragment(char*data, int offset, int length) { memcpy(entry->data, &data[offset], entry->length); + entry->bufferType = getBufferFlags(entry->data, entry->length); + nalChainDataLength += entry->length; if (nalChainHead == NULL) { @@ -305,6 +351,9 @@ static void processRtpPayloadSlow(PNV_VIDEO_PACKET videoPacket, PBUFFER_DESC cur BUFFER_DESC specialSeq; int decodingVideo = 0; + // We should not have any NALUs when processing the first packet in an IDR frame + LC_ASSERT(nalChainHead == NULL); + while (currentPos->length != 0) { int start = currentPos->offset; @@ -317,9 +366,6 @@ static void processRtpPayloadSlow(PNV_VIDEO_PACKET videoPacket, PBUFFER_DESC cur // Now we're working on a frame decodingFrame = 1; - // Reassemble any pending frame - reassembleFrame(videoPacket->frameIndex); - if (isSeqReferenceFrameStart(&specialSeq)) { // No longer waiting for an IDR frame waitingForIdrFrame = 0; @@ -334,11 +380,6 @@ static void processRtpPayloadSlow(PNV_VIDEO_PACKET videoPacket, PBUFFER_DESC cur currentPos->offset += specialSeq.length; } else { - // Check if this is padding after a full frame - if (decodingVideo && isSeqPadding(currentPos)) { - reassembleFrame(videoPacket->frameIndex); - } - // Not decoding video decodingVideo = 0;