From f14a2a66ddcd2f24dbd432b6c19f1f92bae2a79b Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 20 Oct 2014 12:55:23 -0400 Subject: [PATCH 1/4] Fix the memory leak in the video decoder --- Limelight/Connection.m | 3 +- Limelight/VideoDecoderRenderer.m | 103 ++++++++++++++++++++++--------- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/Limelight/Connection.m b/Limelight/Connection.m index a4c7747..fe9c783 100644 --- a/Limelight/Connection.m +++ b/Limelight/Connection.m @@ -65,9 +65,8 @@ void DrSubmitDecodeUnit(PDECODE_UNIT decodeUnit) entry = entry->next; } + // This function will take our buffer [renderer submitDecodeBuffer:data length:decodeUnit->fullLength]; - - free(data); } } diff --git a/Limelight/VideoDecoderRenderer.m b/Limelight/VideoDecoderRenderer.m index 813e257..7f3cd18 100644 --- a/Limelight/VideoDecoderRenderer.m +++ b/Limelight/VideoDecoderRenderer.m @@ -46,38 +46,69 @@ OSStatus status; size_t oldOffset = CMBlockBufferGetDataLength(existingBuffer); - // Append a 4 byte buffer to this block for the length prefix - status = CMBlockBufferAppendMemoryBlock(existingBuffer, NULL, - NAL_LENGTH_PREFIX_SIZE, - kCFAllocatorDefault, NULL, 0, - NAL_LENGTH_PREFIX_SIZE, 0); - if (status != noErr) { - NSLog(@"CMBlockBufferAppendMemoryBlock failed: %d", (int)status); - return; + // If we're at index 1 (first NALU in frame), enqueue this buffer to the memory block + // so it can handle freeing it when the block buffer is destroyed + if (offset == 1) { + int dataLength = nalLength - NALU_START_PREFIX_SIZE; + + // Pass the real buffer pointer directly (no offset) + // This will give it to the block buffer to free when it's released. + // All further calls to CMBlockBufferAppendMemoryBlock will do so + // at an offset and will not be asking the buffer to be freed. + status = CMBlockBufferAppendMemoryBlock(existingBuffer, data, + nalLength + 1, // Add 1 for the offset we decremented + kCFAllocatorDefault, + NULL, 0, nalLength + 1, 0); + if (status != noErr) { + NSLog(@"CMBlockBufferReplaceDataBytes failed: %d", (int)status); + return; + } + + // Write the length prefix to existing buffer + const uint8_t lengthBytes[] = {(uint8_t)(dataLength >> 24), (uint8_t)(dataLength >> 16), + (uint8_t)(dataLength >> 8), (uint8_t)dataLength}; + status = CMBlockBufferReplaceDataBytes(lengthBytes, existingBuffer, + oldOffset, NAL_LENGTH_PREFIX_SIZE); + if (status != noErr) { + NSLog(@"CMBlockBufferReplaceDataBytes failed: %d", (int)status); + return; + } } - - // Write the length prefix to the new buffer - int dataLength = nalLength - NALU_START_PREFIX_SIZE; - const uint8_t lengthBytes[] = {(uint8_t)(dataLength >> 24), (uint8_t)(dataLength >> 16), - (uint8_t)(dataLength >> 8), (uint8_t)dataLength}; - status = CMBlockBufferReplaceDataBytes(lengthBytes, existingBuffer, - oldOffset, NAL_LENGTH_PREFIX_SIZE); - if (status != noErr) { - NSLog(@"CMBlockBufferReplaceDataBytes failed: %d", (int)status); - return; - } - - // Append the rest of the data by simply attaching a reference to the buffer - status = CMBlockBufferAppendMemoryBlock(existingBuffer, &data[offset+NALU_START_PREFIX_SIZE], - dataLength, - kCFAllocatorNull, // Don't deallocate data on free - NULL, 0, dataLength, 0); - if (status != noErr) { - NSLog(@"CMBlockBufferReplaceDataBytes failed: %d", (int)status); - return; + else { + // Append a 4 byte buffer to this block for the length prefix + status = CMBlockBufferAppendMemoryBlock(existingBuffer, NULL, + NAL_LENGTH_PREFIX_SIZE, + kCFAllocatorDefault, NULL, 0, + NAL_LENGTH_PREFIX_SIZE, 0); + if (status != noErr) { + NSLog(@"CMBlockBufferAppendMemoryBlock failed: %d", (int)status); + return; + } + + // Write the length prefix to the new buffer + int dataLength = nalLength - NALU_START_PREFIX_SIZE; + const uint8_t lengthBytes[] = {(uint8_t)(dataLength >> 24), (uint8_t)(dataLength >> 16), + (uint8_t)(dataLength >> 8), (uint8_t)dataLength}; + status = CMBlockBufferReplaceDataBytes(lengthBytes, existingBuffer, + oldOffset, NAL_LENGTH_PREFIX_SIZE); + if (status != noErr) { + NSLog(@"CMBlockBufferReplaceDataBytes failed: %d", (int)status); + return; + } + + // Attach the buffer by reference to the block buffer + status = CMBlockBufferAppendMemoryBlock(existingBuffer, &data[offset+NALU_START_PREFIX_SIZE], + dataLength, + kCFAllocatorNull, // Don't deallocate data on free + NULL, 0, dataLength, 0); + if (status != noErr) { + NSLog(@"CMBlockBufferReplaceDataBytes failed: %d", (int)status); + return; + } } } +// This function must free data - (void)submitDecodeBuffer:(unsigned char *)data length:(int)length { unsigned char nalType = data[FRAME_START_PREFIX_SIZE] & 0x1F; @@ -120,21 +151,25 @@ if (status != noErr) { NSLog(@"Failed to create format description: %d", (int)status); formatDesc = NULL; - return; } } + // Free the data buffer + free(data); + // No frame data to submit for these NALUs return; } if (formatDesc == NULL) { // Can't decode if we haven't gotten our parameter sets yet + free(data); return; } if (nalType != 0x1 && nalType != 0x5) { // Don't submit parameter set data + free(data); return; } @@ -144,9 +179,10 @@ status = CMBlockBufferCreateEmpty(NULL, 0, 0, &blockBuffer); if (status != noErr) { NSLog(@"CMBlockBufferCreateEmpty failed: %d", (int)status); + free(data); return; } - + int lastOffset = -1; for (int i = 0; i < length - FRAME_START_PREFIX_SIZE; i++) { // Search for a NALU @@ -166,6 +202,8 @@ [self updateBufferForRange:blockBuffer data:data offset:lastOffset length:length - lastOffset]; } + // From now on, CMBlockBuffer owns the data pointer and will free it when it's dereferenced + CMSampleBufferRef sampleBuffer; status = CMSampleBufferCreate(kCFAllocatorDefault, @@ -176,6 +214,7 @@ &sampleBuffer); if (status != noErr) { NSLog(@"CMSampleBufferCreate failed: %d", (int)status); + CFRelease(blockBuffer); return; } @@ -197,6 +236,10 @@ } [displayLayer enqueueSampleBuffer:sampleBuffer]; + + // Dereference the buffers + CFRelease(blockBuffer); + CFRelease(sampleBuffer); } @end From b1938610dd285f7d339babdb95dee6958649fae0 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 20 Oct 2014 12:56:37 -0400 Subject: [PATCH 2/4] Revert "Use a semaphore to prevent us from having to busy wait on the audio buffer queue" This reverts commit f3b3d2bd089508d1cf245b3447c50f981c2cd98e. --- Limelight/Connection.m | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/Limelight/Connection.m b/Limelight/Connection.m index fe9c783..e275524 100644 --- a/Limelight/Connection.m +++ b/Limelight/Connection.m @@ -11,8 +11,6 @@ #import #import -#include - #include "Limelight.h" #include "opus.h" @@ -42,7 +40,6 @@ struct AUDIO_BUFFER_QUEUE_ENTRY { static short decodedPcmBuffer[512]; static NSLock *audioLock; static struct AUDIO_BUFFER_QUEUE_ENTRY *audioBufferQueue; -static dispatch_semaphore_t audioQueueSemaphore; static int audioBufferQueueLength; static AudioComponentInstance audioUnit; static VideoDecoderRenderer* renderer; @@ -94,8 +91,6 @@ void ArInit(void) opusDecoder = opus_decoder_create(48000, 2, &err); audioLock = [[NSLock alloc] init]; - - audioQueueSemaphore = dispatch_semaphore_create(0); } void ArRelease(void) @@ -140,15 +135,9 @@ void ArDecodeAndPlaySample(char* sampleData, int sampleLength) // Clear all values from the buffer queue struct AUDIO_BUFFER_QUEUE_ENTRY *entry; while (audioBufferQueue != NULL) { - // Unlink the current entry entry = audioBufferQueue; audioBufferQueue = entry->next; - - // Decrease the semaphore count and queue length audioBufferQueueLength--; - dispatch_semaphore_wait(audioQueueSemaphore, DISPATCH_TIME_NOW); - - // Free the entry free(entry); } } @@ -163,13 +152,9 @@ void ArDecodeAndPlaySample(char* sampleData, int sampleLength) } lastEntry->next = newEntry; } - - // Increment the queue size and unlock audioBufferQueueLength++; - [audioLock unlock]; - // Signal the other thread - dispatch_semaphore_signal(audioQueueSemaphore); + [audioLock unlock]; } } } @@ -333,13 +318,10 @@ static OSStatus playbackCallback(void *inRefCon, } // Wait for a buffer to be available + // FIXME: This needs optimization to avoid busy waiting for buffers struct AUDIO_BUFFER_QUEUE_ENTRY *audioEntry = NULL; while (audioEntry == NULL) { - // Wait for an entry to be present in the queue - dispatch_semaphore_wait(audioQueueSemaphore, DISPATCH_TIME_FOREVER); - - // If there's an entry there, dequeue it [audioLock lock]; if (audioBufferQueue != NULL) { // Dequeue this entry temporarily @@ -368,7 +350,6 @@ static OSStatus playbackCallback(void *inRefCon, audioBufferQueue = audioEntry; audioBufferQueueLength++; [audioLock unlock]; - dispatch_semaphore_signal(audioQueueSemaphore); } else { // This entry is fully depleted so free it From 35e2ca1e005e79cc12912f437293bdefe9aad779 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 20 Oct 2014 13:18:23 -0400 Subject: [PATCH 3/4] Don't try to be clever and stall the audio thread while we await samples. Start audio output earlier to allow it some extra warmup time. --- Limelight/Connection.m | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/Limelight/Connection.m b/Limelight/Connection.m index e275524..8c7993f 100644 --- a/Limelight/Connection.m +++ b/Limelight/Connection.m @@ -106,7 +106,6 @@ void ArRelease(void) void ArStart(void) { printf("Start audio\n"); - AudioOutputUnitStart(audioUnit); } void ArStop(void) @@ -292,6 +291,13 @@ void ClDisplayTransientMessage(char* message) NSLog(@"Unable to initialize audioUnit: %d", (int32_t)status); } + // We start here because it seems to need some warmup time + // before it starts accepting samples + status = AudioOutputUnitStart(audioUnit); + if (status) { + NSLog(@"Unable to start audioUnit: %d", (int32_t)status); + } + return self; } @@ -305,9 +311,15 @@ static OSStatus playbackCallback(void *inRefCon, // Fill them up as much as you can. Remember to set the size value in each buffer to match how // much data is in the buffer. + bool ranOutOfData = false; for (int i = 0; i < ioData->mNumberBuffers; i++) { ioData->mBuffers[i].mNumberChannels = 2; + if (ranOutOfData) { + ioData->mBuffers[i].mDataByteSize = 0; + continue; + } + if (ioData->mBuffers[i].mDataByteSize != 0) { int thisBufferOffset = 0; @@ -317,19 +329,22 @@ static OSStatus playbackCallback(void *inRefCon, continue; } - // Wait for a buffer to be available - // FIXME: This needs optimization to avoid busy waiting for buffers struct AUDIO_BUFFER_QUEUE_ENTRY *audioEntry = NULL; - while (audioEntry == NULL) - { - [audioLock lock]; - if (audioBufferQueue != NULL) { - // Dequeue this entry temporarily - audioEntry = audioBufferQueue; - audioBufferQueue = audioBufferQueue->next; - audioBufferQueueLength--; - } - [audioLock unlock]; + + [audioLock lock]; + if (audioBufferQueue != NULL) { + // Dequeue this entry temporarily + audioEntry = audioBufferQueue; + audioBufferQueue = audioBufferQueue->next; + audioBufferQueueLength--; + } + [audioLock unlock]; + + if (audioEntry == NULL) { + // No data left + ranOutOfData = true; + ioData->mBuffers[i].mDataByteSize = thisBufferOffset; + continue; } // Figure out how much data we can write From 5ffdc1d2bd05a6ed1329cb2269ab1dba4facfd1c Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Mon, 20 Oct 2014 13:18:53 -0400 Subject: [PATCH 4/4] Change video background to black --- Limelight/VideoDecoderRenderer.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Limelight/VideoDecoderRenderer.m b/Limelight/VideoDecoderRenderer.m index 7f3cd18..50f602c 100644 --- a/Limelight/VideoDecoderRenderer.m +++ b/Limelight/VideoDecoderRenderer.m @@ -23,7 +23,7 @@ displayLayer = [[AVSampleBufferDisplayLayer alloc] init]; displayLayer.bounds = view.bounds; - displayLayer.backgroundColor = [UIColor greenColor].CGColor; + displayLayer.backgroundColor = [UIColor blackColor].CGColor; displayLayer.position = CGPointMake(CGRectGetMidX(view.bounds), CGRectGetMidY(view.bounds)); displayLayer.videoGravity = AVLayerVideoGravityResizeAspect; [view.layer addSublayer:displayLayer];