Cog Audio: Made ring buffer locking mechanism more secure and/or smarter

Audio/Chain/Node.h

@@ -16,8 +16,6 @@
 @interface Node : NSObject {
 	VirtualRingBuffer *buffer;
 	Semaphore *semaphore;
-	NSLock *readLock;
-	NSLock *writeLock;
 	
 	id __weak previousNode;
 	id __weak controller;
@@ -41,9 +39,6 @@
 - (void)setShouldReset:(BOOL)s;
 - (BOOL)shouldReset;
 
-- (NSLock *)readLock;
-- (NSLock *)writeLock;
-
 - (void)setPreviousNode:(id)p;
 - (id)previousNode;
 

Audio/Chain/Node.m

@@ -20,8 +20,6 @@
 	{
 		buffer = [[VirtualRingBuffer alloc] initWithLength:BUFFER_SIZE];
 		semaphore = [[Semaphore alloc] init];
-		readLock = [[NSLock alloc] init];
-		writeLock = [[NSLock alloc] init];
 		
 		initialBufferFilled = NO;
 		
@@ -41,7 +39,6 @@
 	int amountToCopy, availOutput;
 	int amountLeft = amount;
 	
-	[writeLock lock];
 	while (shouldContinue == YES && amountLeft > 0)
 	{
 		availOutput = [buffer lengthAvailableToWriteReturningPointer:&writePtr];
@@ -55,9 +52,13 @@
 		
 		if (availOutput == 0 || shouldReset)
 		{
-			[writeLock unlock];
+            if (availOutput)
+            {
+                // must unlock buffer before waiting, may as well write silence
+                memset(writePtr, 0, availOutput);
+                [buffer didWriteLength:availOutput];
+            }
 			[semaphore wait];
-			[writeLock lock];
 		}
 		else
 		{
@@ -66,15 +67,11 @@
 				amountToCopy = amountLeft;
 			
 			memcpy(writePtr, &((char *)ptr)[amount - amountLeft], amountToCopy);
-			if (amountToCopy > 0)
-			{
-				[buffer didWriteLength:amountToCopy];
-			}
+            [buffer didWriteLength:amountToCopy];
 			
 			amountLeft -= amountToCopy;
 		}
 	}
-	[writeLock unlock];
 	
 	return (amount - amountLeft);
 }
@@ -103,7 +100,6 @@
         return 0;
     }
 	
-	[readLock lock];
 	availInput = [[previousNode buffer] lengthAvailableToReadReturningPointer:&readPtr];
 	
 /*	if (availInput <= 0) {
@@ -114,14 +110,11 @@
 	}
 */
 
-	if ([previousNode shouldReset] == YES) {
-		[writeLock lock];
-
+    if ([previousNode shouldReset] == YES) {
 		[buffer empty];
 
 		shouldReset = YES;
 		[previousNode setShouldReset: NO];
-		[writeLock unlock];
 
 		[[previousNode semaphore] signal];
 	}
@@ -134,13 +127,12 @@
 	
 	memcpy(ptr, readPtr, amountToCopy);
 	
+    [[previousNode buffer] didReadLength:amountToCopy];
+    
 	if (amountToCopy > 0)
 	{
-		[[previousNode buffer] didReadLength:amountToCopy];
-		
 		[[previousNode semaphore] signal];
 	}
-	[readLock unlock];
 	
 	return amountToCopy;
 }
@@ -179,22 +171,10 @@
 {
 	shouldReset = YES; //Will reset on next write.
 	if (previousNode == nil) {
-		[readLock lock];
 		[buffer empty];
-		[readLock unlock];
 	}
 }
 
-- (NSLock *)readLock
-{
-	return readLock;
-}
-
-- (NSLock *)writeLock
-{
-	return writeLock;
-}
-
 - (Semaphore *)semaphore
 {
 	return semaphore;

Audio/Chain/RefillNode.m

@@ -21,8 +21,6 @@
         // This special node should be able to handle up to four buffers
         buffer = [[VirtualRingBuffer alloc] initWithLength:BUFFER_SIZE * 4];
         semaphore = [[Semaphore alloc] init];
-        readLock = [[NSLock alloc] init];
-        writeLock = [[NSLock alloc] init];
         
         initialBufferFilled = NO;
         

Audio/Output/OutputCoreAudio.m

@@ -97,6 +97,8 @@ static OSStatus renderCallback( void *inRefCon, AudioUnitRenderActionFlags *ioAc
         atomic_fetch_add(&_self->bytesRendered, amountRead);
         [_self->writeSemaphore signal];
     }
+    else
+        [[_self->outputController buffer] didReadLength:0];
 
     // Try repeatedly! Buffer wraps can cause a slight data shortage, as can
     // unexpected track changes.
@@ -117,6 +119,7 @@ static OSStatus renderCallback( void *inRefCon, AudioUnitRenderActionFlags *ioAc
             [_self->writeSemaphore signal];
         }
         else {
+            [[_self->outputController buffer] didReadLength:0];
             [_self->readSemaphore timedWait:500];
         }
     }
@@ -250,8 +253,8 @@ default_device_changed(AudioObjectID inObjectID, UInt32 inNumberAddresses, const
             toWrite = CHUNK_SIZE;
         if (toWrite)
             bytesRead = [outputController readData:writePtr amount:toWrite];
+        [[outputController buffer] didWriteLength:bytesRead];
         if (bytesRead) {
-            [[outputController buffer] didWriteLength:bytesRead];
             [readSemaphore signal];
             continue;
         }

Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.h

@@ -57,6 +57,8 @@
     atomic_int readPointer;
     atomic_int writePointer;
     atomic_int bufferFilled;
+    
+    NSLock *accessLock;
 }
 
 - (id)initWithLength:(UInt32)length;

Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.m

@@ -47,6 +47,8 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
     atomic_init(&writePointer, 0);
     atomic_init(&bufferFilled, 0);
 
+    accessLock = [[NSLock alloc] init];
+    
     return self;
 }
 
@@ -60,10 +62,11 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
 {
     // Assumption:
     // No one is reading or writing from the buffer, in any thread, when this method is called.
-
+    [accessLock lock];
     atomic_init(&readPointer, 0);
     atomic_init(&writePointer, 0);
     atomic_init(&bufferFilled, 0);
+    [accessLock unlock];
 }
 
 - (BOOL)isEmpty
@@ -96,6 +99,7 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
 {
     // Assumptions:
     // returnedReadPointer != NULL
+    [accessLock lock];
 
     UInt32 length;
     // Read this pointer exactly once, so we're safe in case it is changed in another thread
@@ -109,6 +113,10 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
     // Depending on out-of-order execution and memory storage, either one of these may be NULL when the buffer is empty. So we must check both.
 
     *returnedReadPointer = buffer + localReadPointer;
+    
+    if (!length)
+        [accessLock unlock];
+    
     return length;
 }
 
@@ -118,10 +126,13 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
     // [self lengthAvailableToReadReturningPointer:] currently returns a value >= length
     // length > 0
     
+
     if (atomic_fetch_add(&readPointer, length) + length >= bufferLength)
         atomic_fetch_sub(&readPointer, bufferLength);
 
     atomic_fetch_sub(&bufferFilled, length);
+    
+    [accessLock unlock];
 }
 
 
@@ -133,6 +144,7 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
 {
     // Assumptions:
     // returnedWritePointer != NULL
+    [accessLock lock];
     
     UInt32 length;
     // Read this pointer exactly once, so we're safe in case it is changed in another thread
@@ -144,6 +156,10 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
         length = bufferLength - localWritePointer;
 
     *returnedWritePointer = buffer + localWritePointer;
+    
+    if (!length)
+        [accessLock unlock];
+    
     return length;
 }
 
@@ -157,6 +173,8 @@ static void deallocateVirtualBuffer(void *buffer, UInt32 bufferLength);
         atomic_fetch_sub(&writePointer, bufferLength);
     
     atomic_fetch_add(&bufferFilled, length);
+
+    [accessLock unlock];
 }
 
 @end