From 494ad84ea78afb91148f4e2c9d3ea607eb743f04 Mon Sep 17 00:00:00 2001 From: Christopher Snowhill Date: Sun, 23 Jan 2022 19:36:33 -0800 Subject: [PATCH] Cog Audio: Made ring buffer locking mechanism more secure and/or smarter --- Audio/Chain/Node.h | 5 --- Audio/Chain/Node.m | 40 +++++-------------- Audio/Chain/RefillNode.m | 2 - Audio/Output/OutputCoreAudio.m | 5 ++- .../VirtualRingBuffer/VirtualRingBuffer.h | 2 + .../VirtualRingBuffer/VirtualRingBuffer.m | 20 +++++++++- 6 files changed, 35 insertions(+), 39 deletions(-) diff --git a/Audio/Chain/Node.h b/Audio/Chain/Node.h index 778dea9c0..65eca918f 100644 --- a/Audio/Chain/Node.h +++ b/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; diff --git a/Audio/Chain/Node.m b/Audio/Chain/Node.m index e8d935ad7..7315d2dee 100644 --- a/Audio/Chain/Node.m +++ b/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; diff --git a/Audio/Chain/RefillNode.m b/Audio/Chain/RefillNode.m index 927b17cd0..24bb7ed51 100644 --- a/Audio/Chain/RefillNode.m +++ b/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; diff --git a/Audio/Output/OutputCoreAudio.m b/Audio/Output/OutputCoreAudio.m index ae9627ef1..8544773b6 100644 --- a/Audio/Output/OutputCoreAudio.m +++ b/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; } diff --git a/Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.h b/Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.h index f807e3c39..f19d64854 100644 --- a/Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.h +++ b/Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.h @@ -57,6 +57,8 @@ atomic_int readPointer; atomic_int writePointer; atomic_int bufferFilled; + + NSLock *accessLock; } - (id)initWithLength:(UInt32)length; diff --git a/Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.m b/Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.m index eeb3757dc..ce9f0485f 100644 --- a/Audio/ThirdParty/VirtualRingBuffer/VirtualRingBuffer.m +++ b/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