AVFMediaAssetWriter - fix race conditions

Apple recommends starting/setting up a session (AVCaptureSession) on a
special queue and in the past we did this on the writer's queue.
Unfortunately, we also can access the same session from a different thread
and this results in race conditions and different weird crashes, for example,
we're calling start/stopRunning from the writer's queue, while the session
is being configured (in between beginConfiguration/commitConfiguration
on the recorder control's thread). So we have to limit access to the session
by the control's thread.

Apple docs say we have to ensure all appendSampleBuffer calls done
_before_ finishWriting. We ensure this by dispatching_sync an
empty block on the writer's queue after store-release of m_state.

We also do the same with video queue to ensure it does not try to
access viewfinder's data after stop/abort executed.

All these changes also make lock/mutex unneeded.

Task-number: QTBUG-54890
Change-Id: I38e86c879b6b62306bdfbeade65405d6ac3be9f3
Reviewed-by: Yoann Lopes <yoann.lopes@qt.io>

4 files changed, 67 insertions, 65 deletions

diff --git a/src/plugins/avfoundation/camera/avfmediaassetwriter.h b/src/plugins/avfoundation/camera/avfmediaassetwriter.h
index 77e2a2f32..c36caee8b 100644
--- a/src/plugins/avfoundation/camera/avfmediaassetwriter.h
+++ b/src/plugins/avfoundation/camera/avfmediaassetwriter.h
@@ -38,7 +38,6 @@
 
 #include <QtCore/qglobal.h>
 #include <QtCore/qatomic.h>
-#include <QtCore/qmutex.h>
 
 #include <AVFoundation/AVFoundation.h>
 
@@ -63,12 +62,12 @@ QT_END_NAMESPACE
     QT_PREPEND_NAMESPACE(AVFScopedPointer)<AVAssetWriterInput> m_audioWriterInput;
     AVCaptureDevice *m_audioCaptureDevice;
 
+    // Queue to write sample buffers:
+    QT_PREPEND_NAMESPACE(AVFScopedPointer)<dispatch_queue_t> m_writerQueue;
     // High priority serial queue for video output:
     QT_PREPEND_NAMESPACE(AVFScopedPointer)<dispatch_queue_t> m_videoQueue;
     // Serial queue for audio output:
     QT_PREPEND_NAMESPACE(AVFScopedPointer)<dispatch_queue_t> m_audioQueue;
-    // Queue to write sample buffers:
-    QT_PREPEND_NAMESPACE(AVFScopedPointer)<dispatch_queue_t> m_writerQueue;
 
     QT_PREPEND_NAMESPACE(AVFScopedPointer)<AVAssetWriter> m_assetWriter;
 
@@ -77,7 +76,6 @@ QT_END_NAMESPACE
     bool m_setStartTime;
 
     QT_PREPEND_NAMESPACE(QAtomicInt) m_state;
-    QT_PREPEND_NAMESPACE(QMutex) m_writerMutex;
 @public
     QT_PREPEND_NAMESPACE(AVFAtomicInt64) m_durationInMs;
 @private
@@ -88,8 +86,7 @@ QT_END_NAMESPACE
     NSDictionary *m_videoSettings;
 }
 
-- (id)initWithQueue:(dispatch_queue_t)writerQueue
-      delegate:(QT_PREPEND_NAMESPACE(AVFMediaRecorderControlIOS) *)delegate;
+- (id)initWithDelegate:(QT_PREPEND_NAMESPACE(AVFMediaRecorderControlIOS) *)delegate;
 
 - (bool)setupWithFileURL:(NSURL *)fileURL
         cameraService:(QT_PREPEND_NAMESPACE(AVFCameraService) *)service
@@ -97,10 +94,10 @@ QT_END_NAMESPACE
         videoSettings:(NSDictionary *)videoSettings
         transform:(CGAffineTransform)transform;
 
+// This to be called from the recorder control's thread:
 - (void)start;
 - (void)stop;
-// This to be called if control's dtor gets called,
-// on the control's thread.
+// This to be called from the recorder control's dtor:
 - (void)abort;
 
 @end
diff --git a/src/plugins/avfoundation/camera/avfmediaassetwriter.mm b/src/plugins/avfoundation/camera/avfmediaassetwriter.mm
index 4e227114b..464d90db0 100644
--- a/src/plugins/avfoundation/camera/avfmediaassetwriter.mm
+++ b/src/plugins/avfoundation/camera/avfmediaassetwriter.mm
@@ -85,17 +85,11 @@ enum WriterState
 
 @implementation QT_MANGLE_NAMESPACE(AVFMediaAssetWriter)
 
-- (id)initWithQueue:(dispatch_queue_t)writerQueue
-      delegate:(AVFMediaRecorderControlIOS *)delegate
+- (id)initWithDelegate:(AVFMediaRecorderControlIOS *)delegate
 {
-    Q_ASSERT(writerQueue);
     Q_ASSERT(delegate);
 
     if (self = [super init]) {
-        // "Shared" queue:
-        dispatch_retain(writerQueue);
-        m_writerQueue.reset(writerQueue);
-
         m_delegate = delegate;
         m_setStartTime = true;
         m_state.store(WriterStateIdle);
@@ -126,6 +120,12 @@ enum WriterState
     m_audioSettings = audioSettings;
     m_videoSettings = videoSettings;
 
+    m_writerQueue.reset(dispatch_queue_create("asset-writer-queue", DISPATCH_QUEUE_SERIAL));
+    if (!m_writerQueue) {
+        qDebugCamera() << Q_FUNC_INFO << "failed to create an asset writer's queue";
+        return false;
+    }
+
     m_videoQueue.reset(dispatch_queue_create("video-output-queue", DISPATCH_QUEUE_SERIAL));
     if (!m_videoQueue) {
         qDebugCamera() << Q_FUNC_INFO << "failed to create video queue";
@@ -172,20 +172,10 @@ enum WriterState
 
 - (void)start
 {
-    // 'start' is executed on a special writer's queue.
-    // We lock a mutex for the whole function body
-    // - render control probably was deleted
-    // 'immediately' after submitting 'start'
-    // and before 'start' is actually executed ...
-    // So if we're starting, render control can not
-    // abort us 'in the middle'.
-    const QMutexLocker lock(&m_writerMutex);
-    if (m_state.load() == WriterStateAborted)
-        return;
-
     [self setQueues];
 
     m_setStartTime = true;
+
     m_state.storeRelease(WriterStateActive);
 
     [m_assetWriter startWriting];
@@ -196,40 +186,65 @@ enum WriterState
 
 - (void)stop
 {
-    // To be executed on a writer's queue.
-    {
-    const QMutexLocker lock(&m_writerMutex);
-    if (m_state.load() != WriterStateActive)
+    if (m_state.loadAcquire() != WriterStateActive)
         return;
-    }
 
+    if ([m_assetWriter status] != AVAssetWriterStatusWriting)
+        return;
+
+    // Do this here so that -
+    // 1. '-abort' should not try calling finishWriting again and
+    // 2. async block (see below) will know if recorder control was deleted
+    //    before the block's execution:
+    m_state.storeRelease(WriterStateIdle);
+    // Now, since we have to ensure no sample buffers are
+    // appended after a call to finishWriting, we must
+    // ensure writer's queue sees this change in m_state
+    // _before_ we call finishWriting:
+    dispatch_sync(m_writerQueue, ^{});
+    // Done, but now we also want to prevent video queue
+    // from updating our viewfinder:
+    dispatch_sync(m_videoQueue, ^{});
+
+    // Now we're safe to stop:
     [m_assetWriter finishWritingWithCompletionHandler:^{
         // This block is async, so by the time it's executed,
         // it's possible that render control was deleted already ...
-        // We lock so that nobody can call 'abort' in the middle ...
-        const QMutexLocker lock(&m_writerMutex);
-        if (m_state.load() != WriterStateActive)
+        if (m_state.loadAcquire() == WriterStateAborted)
             return;
 
         AVCaptureSession *session = m_service->session()->captureSession();
-        [session stopRunning];
+        if (session.running)
+            [session stopRunning];
         [session removeOutput:m_audioOutput];
         [session removeInput:m_audioInput];
-        m_state.storeRelease(WriterStateIdle);
         QMetaObject::invokeMethod(m_delegate, "assetWriterFinished", Qt::QueuedConnection);
     }];
 }
 
 - (void)abort
 {
-    // To be executed on any thread (presumably, it's the main thread),
-    // prevents writer from accessing any shared object.
-    const QMutexLocker lock(&m_writerMutex);
+    // -abort is to be called from recorder control's dtor.
+
     if (m_state.fetchAndStoreRelease(WriterStateAborted) != WriterStateActive) {
         // Not recording, nothing to stop.
         return;
     }
 
+    // From Apple's docs:
+    // "To guarantee that all sample buffers are successfully written,
+    //  you must ensure that all calls to appendSampleBuffer: and
+    //  appendPixelBuffer:withPresentationTime: have returned before
+    //  invoking this method."
+    //
+    // The only way we can ensure this is:
+    dispatch_sync(m_writerQueue, ^{});
+    // At this point next block (if any) on the writer's queue
+    // will see m_state preventing it from any further processing.
+    dispatch_sync(m_videoQueue, ^{});
+    // After this point video queue will not try to modify our
+    // viewfider, so we're safe to delete now.
+
     [m_assetWriter finishWritingWithCompletionHandler:^{
     }];
 }
@@ -240,13 +255,12 @@ enum WriterState
     Q_ASSERT(m_setStartTime);
     Q_ASSERT(sampleBuffer);
 
-    const QMutexLocker lock(&m_writerMutex);
-    if (m_state.load() != WriterStateActive)
+    if (m_state.loadAcquire() != WriterStateActive)
         return;
 
     QMetaObject::invokeMethod(m_delegate, "assetWriterStarted", Qt::QueuedConnection);
 
-    m_durationInMs.store(0);
+    m_durationInMs.storeRelease(0);
     m_startTime = CMSampleBufferGetPresentationTimeStamp(sampleBuffer);
     m_lastTimeStamp = m_startTime;
     [m_assetWriter startSessionAtSourceTime:m_startTime];
@@ -255,9 +269,9 @@ enum WriterState
 
 - (void)writeVideoSampleBuffer:(CMSampleBufferRef)sampleBuffer
 {
+    // This code is executed only on a writer's queue.
     Q_ASSERT(sampleBuffer);
 
-    // This code is executed only on a writer's queue.
     if (m_state.loadAcquire() == WriterStateActive) {
         if (m_setStartTime)
             [self setStartTimeFrom:sampleBuffer];
@@ -268,7 +282,6 @@ enum WriterState
         }
     }
 
-
     CFRelease(sampleBuffer);
 }
 
@@ -296,9 +309,6 @@ enum WriterState
 {
     Q_UNUSED(connection)
 
-    // This method can be called on either video or audio queue,
-    // never on a writer's queue, it needs access to a shared data, so
-    // lock is required.
     if (m_state.loadAcquire() != WriterStateActive)
         return;
 
@@ -310,7 +320,6 @@ enum WriterState
     CFRetain(sampleBuffer);
 
     if (captureOutput != m_audioOutput.data()) {
-        const QMutexLocker lock(&m_writerMutex);
         if (m_state.load() != WriterStateActive) {
             CFRelease(sampleBuffer);
             return;
@@ -450,7 +459,7 @@ enum WriterState
         if (!CMTimeCompare(duration, kCMTimeInvalid))
             return;
 
-        m_durationInMs.store(CMTimeGetSeconds(duration) * 1000);
+        m_durationInMs.storeRelease(CMTimeGetSeconds(duration) * 1000);
         m_lastTimeStamp = newTimeStamp;
     }
 }
diff --git a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h b/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h
index 33cb08804..589810b07 100644
--- a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h
+++ b/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h
@@ -91,8 +91,6 @@ private:
     void stopWriter();
 
     AVFCameraService *m_service;
-
-    AVFScopedPointer<dispatch_queue_t> m_writerQueue;
     AVFScopedPointer<QT_MANGLE_NAMESPACE(AVFMediaAssetWriter)> m_writer;
 
     QUrl m_outputLocation;
diff --git a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm b/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm
index e65c98257..292b449d2 100644
--- a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm
+++ b/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm
@@ -86,13 +86,7 @@ AVFMediaRecorderControlIOS::AVFMediaRecorderControlIOS(AVFCameraService *service
 {
     Q_ASSERT(service);
 
-    m_writerQueue.reset(dispatch_queue_create("asset-writer-queue", DISPATCH_QUEUE_SERIAL));
-    if (!m_writerQueue) {
-        qDebugCamera() << Q_FUNC_INFO << "failed to create an asset writer's queue";
-        return;
-    }
-
-    m_writer.reset([[QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) alloc] initWithQueue:m_writerQueue delegate:this]);
+    m_writer.reset([[QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) alloc] initWithDelegate:this]);
     if (!m_writer) {
         qDebugCamera() << Q_FUNC_INFO << "failed to create an asset writer";
         return;
@@ -289,9 +283,15 @@ void AVFMediaRecorderControlIOS::setState(QMediaRecorder::State state)
             Q_EMIT stateChanged(m_state);
             Q_EMIT statusChanged(m_lastStatus);
 
-            dispatch_async(m_writerQueue, ^{
-                [m_writer start];
-            });
+            // Apple recommends to call startRunning and do all
+            // setup on a special queue, and that's what we had
+            // initially (dispatch_async to writerQueue). Unfortunately,
+            // writer's queue is not the only queue/thread that can
+            // access/modify the session, and as a result we have
+            // all possible data/race-conditions with Obj-C exceptions
+            // at best and something worse in general.
+            // Now we try to only modify session on the same thread.
+            [m_writer start];
         } else {
             [session startRunning];
             Q_EMIT error(QMediaRecorder::FormatError, tr("Failed to start recording"));
@@ -318,7 +318,7 @@ void AVFMediaRecorderControlIOS::setMuted(bool muted)
 
 void AVFMediaRecorderControlIOS::setVolume(qreal volume)
 {
-    Q_UNUSED(volume);
+    Q_UNUSED(volume)
     qDebugCamera() << Q_FUNC_INFO << "not implemented";
 }
 
@@ -403,9 +403,7 @@ void AVFMediaRecorderControlIOS::stopWriter()
         Q_EMIT stateChanged(m_state);
         Q_EMIT statusChanged(m_lastStatus);
 
-        dispatch_async(m_writerQueue, ^{
-            [m_writer stop];
-        });
+        [m_writer stop];
     }
 }