From 41af76bed89c442e1dc8975186fafbe864238943 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 15 Jul 2022 15:13:03 -0400 Subject: [PATCH 1/4] Fix variable name. --- Concurrency/AsyncTaskQueue.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Concurrency/AsyncTaskQueue.hpp b/Concurrency/AsyncTaskQueue.hpp index da56e4388..c8b70be20 100644 --- a/Concurrency/AsyncTaskQueue.hpp +++ b/Concurrency/AsyncTaskQueue.hpp @@ -73,7 +73,7 @@ template class TaskQueue: [this] { ActionVector actions; - while(!should_quit) { + while(!should_quit_) { // Wait for new actions to be signalled, and grab them. std::unique_lock lock(condition_mutex_); while(actions_.empty()) { @@ -127,7 +127,7 @@ template class TaskQueue: /// The queue cannot be restarted; this is a destructive action. void stop() { if(thread_.joinable()) { - should_quit = true; + should_quit_ = true; enqueue([] {}); if constexpr (!perform_automatically) { perform(); @@ -168,7 +168,7 @@ template class TaskQueue: ActionVector actions_; // Necessary synchronisation parts. - std::atomic should_quit = false; + std::atomic should_quit_ = false; std::mutex condition_mutex_; std::condition_variable condition_; From bae47fca208ca9ca4795feeb88e8e0f0618fabad Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 15 Jul 2022 15:13:21 -0400 Subject: [PATCH 2/4] Free buffers before disposing of queue. --- OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m b/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m index 538133bcf..bc741a4c6 100644 --- a/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m +++ b/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m @@ -115,11 +115,6 @@ - (void)dealloc { [_deallocLock lock]; - if(_audioQueue) { - OSSGuard(AudioQueueDispose(_audioQueue, true)); - _audioQueue = NULL; - } - for(size_t c = 0; c < NumBuffers; c++) { if(_buffers[c]) { OSSGuard(AudioQueueFreeBuffer(_audioQueue, _buffers[c])); @@ -127,6 +122,11 @@ } } + if(_audioQueue) { + OSSGuard(AudioQueueDispose(_audioQueue, true)); + _audioQueue = NULL; + } + // nil out the dealloc lock before entering the critical section such // that it becomes impossible for anyone else to acquire. NSLock *deallocLock = _deallocLock; From ee7ef810541d23b7b3abc63aeb52e32ec3a81fb1 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 15 Jul 2022 15:21:58 -0400 Subject: [PATCH 3/4] Avoid potential attempt to free enqueued buffers at dealloc. --- OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m b/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m index bc741a4c6..e6baa6c95 100644 --- a/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m +++ b/OSBindings/Mac/Clock Signal/Audio/CSAudioQueue.m @@ -47,7 +47,11 @@ if(self) { _deallocLock = [[NSLock alloc] init]; + _deallocLock.name = @"Dealloc lock"; + _queueLock = [[NSLock alloc] init]; + _queueLock.name = @"Audio queue access lock"; + atomic_store_explicit(&_enqueuedBuffers, 0, memory_order_relaxed); _samplingRate = samplingRate; @@ -115,6 +119,12 @@ - (void)dealloc { [_deallocLock lock]; + // Ensure no buffers remain enqueued by stopping the queue. + if(_audioQueue) { + OSSGuard(AudioQueueStop(_audioQueue, true)); + } + + // Free all buffers. for(size_t c = 0; c < NumBuffers; c++) { if(_buffers[c]) { OSSGuard(AudioQueueFreeBuffer(_audioQueue, _buffers[c])); @@ -122,6 +132,7 @@ } } + // Dispose of the queue. if(_audioQueue) { OSSGuard(AudioQueueDispose(_audioQueue, true)); _audioQueue = NULL; From 3de1e762b7b489f725e518809d9d290199a0596b Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 15 Jul 2022 15:22:12 -0400 Subject: [PATCH 4/4] Avoid retain cycles. --- .../Mac/Clock Signal/Machine/CSMachine.mm | 42 +++++++++++++------ 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/OSBindings/Mac/Clock Signal/Machine/CSMachine.mm b/OSBindings/Mac/Clock Signal/Machine/CSMachine.mm index cd2a28075..e38b262cd 100644 --- a/OSBindings/Mac/Clock Signal/Machine/CSMachine.mm +++ b/OSBindings/Mac/Clock Signal/Machine/CSMachine.mm @@ -669,41 +669,57 @@ struct ActivityObserver: public Activity::Observer { #pragma mark - Timer - (void)audioQueueIsRunningDry:(nonnull CSAudioQueue *)audioQueue { - updater.enqueue([self] { - updater.performer.machine->flush_output(MachineTypes::TimedMachine::Output::Audio); + __weak CSMachine *weakSelf = self; + + updater.enqueue([weakSelf] { + CSMachine *const strongSelf = weakSelf; + if(strongSelf) { + strongSelf->updater.performer.machine->flush_output(MachineTypes::TimedMachine::Output::Audio); + } }); } - (void)scanTargetViewDisplayLinkDidFire:(CSScanTargetView *)view now:(const CVTimeStamp *)now outputTime:(const CVTimeStamp *)outputTime { - updater.enqueue([self] { + __weak CSMachine *weakSelf = self; + + updater.enqueue([weakSelf] { + CSMachine *const strongSelf = weakSelf; + if(!strongSelf) { + return; + } + // Grab a pointer to the timed machine from somewhere where it has already // been dynamically cast, to avoid that cost here. - MachineTypes::TimedMachine *const timed_machine = updater.performer.machine; + MachineTypes::TimedMachine *const timed_machine = strongSelf->updater.performer.machine; // Definitely update video; update audio too if that pipeline is looking a little dry. auto outputs = MachineTypes::TimedMachine::Output::Video; - if(_audioQueue.isRunningDry) { + if(strongSelf->_audioQueue.isRunningDry) { outputs |= MachineTypes::TimedMachine::Output::Audio; } timed_machine->flush_output(outputs); // Attempt sync-matching if this machine is a fit. - // - // TODO: either cache scan_producer(), or possibly start caching these things - // inside the DynamicMachine? - const auto scanStatus = self->_machine->scan_producer()->get_scan_status(); - const bool canSynchronise = self->_scanSynchroniser.can_synchronise(scanStatus, self.view.refreshPeriod); + const auto scanStatus = strongSelf->_machine->scan_producer()->get_scan_status(); + const bool canSynchronise = strongSelf->_scanSynchroniser.can_synchronise( + scanStatus, + strongSelf.view.refreshPeriod + ); + if(canSynchronise) { - const double multiplier = self->_scanSynchroniser.next_speed_multiplier(self->_machine->scan_producer()->get_scan_status()); + const double multiplier = strongSelf->_scanSynchroniser.next_speed_multiplier( + strongSelf->_machine->scan_producer()->get_scan_status() + ); timed_machine->set_speed_multiplier(multiplier); } else { timed_machine->set_speed_multiplier(1.0); } // Ask Metal to rasterise all that just happened and present it. - [self.view updateBacking]; + [strongSelf.view updateBacking]; dispatch_async(dispatch_get_main_queue(), ^{ - [self.view draw]; + // This is safe even if weakSelf has been nulled out in the interim. + [weakSelf.view draw]; }); }); }