AVFMediaAssetWriter - fix potential race condition(s)

1. m_writerQueue is now shared by recorder control and asset writer
   to ensure it lives long enough.

2. m_delegate->method() calls from async block can be dangerous, since by the time
   this block is actually executed, delegate can be deleted already. This fix uses
   Q_INVOKABLE and invokeMethod with QueuedConnection instead.

3. -finishWritingWithCompletionHandler: is async and when the block finally gets
   executed, lock and 'if aborted' test are still needed.

4. Simplify the logic and reduce locking.

Change-Id: If23daf2fe22043244033427a7f6517a0fe3f23d1
Reviewed-by: Yoann Lopes <yoann.lopes@qt.io>

src/plugins/avfoundation/camera/avfmediaassetwriter.h

@@ -44,19 +44,9 @@
 
 QT_BEGIN_NAMESPACE
 
+class AVFMediaRecorderControlIOS;
 class AVFCameraService;
 
-class AVFMediaAssetWriterDelegate
-{
-public:
-    virtual ~AVFMediaAssetWriterDelegate();
-
-    virtual void assetWriterStarted() = 0;
-    virtual void assetWriterFailedToStart() = 0;
-    virtual void assetWriterFailedToStop() = 0;
-    virtual void assetWriterFinished() = 0;
-};
-
 typedef QAtomicInteger<bool> AVFAtomicBool;
 typedef QAtomicInteger<qint64> AVFAtomicInt64;
 
@@ -80,18 +70,15 @@ QT_END_NAMESPACE
     // Serial queue for audio output:
     QT_PREPEND_NAMESPACE(AVFScopedPointer)<dispatch_queue_t> m_audioQueue;
     // Queue to write sample buffers:
-    dispatch_queue_t m_writerQueue;
+    QT_PREPEND_NAMESPACE(AVFScopedPointer)<dispatch_queue_t> m_writerQueue;
 
     QT_PREPEND_NAMESPACE(AVFScopedPointer)<AVAssetWriter> m_assetWriter;
-    // Delegate's queue.
-    dispatch_queue_t m_delegateQueue;
 
-    QT_PREPEND_NAMESPACE(AVFMediaAssetWriterDelegate) *m_delegate;
+    QT_PREPEND_NAMESPACE(AVFMediaRecorderControlIOS) *m_delegate;
 
     bool m_setStartTime;
     QT_PREPEND_NAMESPACE(AVFAtomicBool) m_stopped;
-    bool m_stoppedInternal;
+    QT_PREPEND_NAMESPACE(AVFAtomicBool) m_aborted;
-    bool m_aborted;
 
     QT_PREPEND_NAMESPACE(QMutex) m_writerMutex;
 @public
@@ -102,8 +89,7 @@ QT_END_NAMESPACE
 }
 
 - (id)initWithQueue:(dispatch_queue_t)writerQueue
-      delegate:(QT_PREPEND_NAMESPACE(AVFMediaAssetWriterDelegate) *)delegate
+      delegate:(QT_PREPEND_NAMESPACE(AVFMediaRecorderControlIOS) *)delegate;
-      delegateQueue:(dispatch_queue_t)delegateQueue;
 
 - (bool)setupWithFileURL:(NSURL *)fileURL
         cameraService:(QT_PREPEND_NAMESPACE(AVFCameraService) *)service;

src/plugins/avfoundation/camera/avfmediaassetwriter.mm

@@ -32,6 +32,7 @@
 ****************************************************************************/
 
 #include "avfaudioinputselectorcontrol.h"
+#include "avfmediarecordercontrol_ios.h"
 #include "avfcamerarenderercontrol.h"
 #include "avfmediaassetwriter.h"
 #include "avfcameraservice.h"
@@ -39,6 +40,7 @@
 #include "avfcameradebug.h"
 
 //#include <QtCore/qmutexlocker.h>
+#include <QtCore/qmetaobject.h>
 #include <QtCore/qsysinfo.h>
 
 QT_USE_NAMESPACE
@@ -65,11 +67,7 @@ bool qt_camera_service_isValid(AVFCameraService *service)
     return true;
 }
 
-}
+} // unnamed namespace
-
-AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
-{
-}
 
 @interface QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) (PrivateAPI)
 - (bool)addAudioCapture;
@@ -83,21 +81,20 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
 @implementation QT_MANGLE_NAMESPACE(AVFMediaAssetWriter)
 
 - (id)initWithQueue:(dispatch_queue_t)writerQueue
-      delegate:(AVFMediaAssetWriterDelegate *)delegate
+      delegate:(AVFMediaRecorderControlIOS *)delegate
-      delegateQueue:(dispatch_queue_t)delegateQueue
 {
     Q_ASSERT(writerQueue);
     Q_ASSERT(delegate);
-    Q_ASSERT(delegateQueue);
 
     if (self = [super init]) {
-        m_writerQueue = writerQueue;
+        // "Shared" queue:
+        dispatch_retain(writerQueue);
+        m_writerQueue.reset(writerQueue);
+
         m_delegate = delegate;
-        m_delegateQueue = delegateQueue;
         m_setStartTime = true;
         m_stopped.store(true);
-        m_stoppedInternal = false;
+        m_aborted.store(false);
-        m_aborted = false;
         m_startTime = kCMTimeInvalid;
         m_lastTimeStamp = kCMTimeInvalid;
         m_durationInMs.store(0);
@@ -160,14 +157,13 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
 {
     // To be executed on a writer's queue.
     const QMutexLocker lock(&m_writerMutex);
-    if (m_aborted)
+    if (m_aborted.load())
         return;
 
     [self setQueues];
 
     m_setStartTime = true;
     m_stopped.store(false);
-    m_stoppedInternal = false;
     [m_assetWriter startWriting];
     AVCaptureSession *session = m_service->session()->captureSession();
     if (!session.running)
@@ -177,40 +173,41 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
 - (void)stop
 {
     // To be executed on a writer's queue.
+    {
     const QMutexLocker lock(&m_writerMutex);
-    if (m_aborted)
+    if (m_aborted.load())
         return;
 
-    if (m_stopped.load()) {
+    if (m_stopped.load())
-        // Should never happen, but ...
-        // if something went wrong in a recorder control
-        // and we set state stopped without starting first ...
-        // m_stoppedIntenal will be false, but m_stopped - true.
         return;
-    }
 
     m_stopped.store(true);
-    m_stoppedInternal = true;
+    }
+
     [m_assetWriter finishWritingWithCompletionHandler:^{
-        // TODO: make sure the session exist and we can call stop/remove on it.
+        // This block is async, so by the time it's executed,
+        // it's possible that render control was deleted already ...
+        const QMutexLocker lock(&m_writerMutex);
+        if (m_aborted.load())
+            return;
+
         AVCaptureSession *session = m_service->session()->captureSession();
         [session stopRunning];
         [session removeOutput:m_audioOutput];
         [session removeInput:m_audioInput];
-        dispatch_async(m_delegateQueue, ^{
+        QMetaObject::invokeMethod(m_delegate, "assetWriterFinished", Qt::QueuedConnection);
-            m_delegate->assetWriterFinished();
-        });
     }];
 }
 
 - (void)abort
 {
-    // To be executed on any thread, prevents writer from
+    // To be executed on any thread (presumably, it's the main thread),
-    // accessing any external object (probably deleted by this time)
+    // prevents writer from accessing any shared object.
     const QMutexLocker lock(&m_writerMutex);
-    m_aborted = true;
+    m_aborted.store(true);
     if (m_stopped.load())
         return;
+
     [m_assetWriter finishWritingWithCompletionHandler:^{
     }];
 }
@@ -221,9 +218,11 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
     Q_ASSERT(m_setStartTime);
     Q_ASSERT(sampleBuffer);
 
-    dispatch_async(m_delegateQueue, ^{
+    const QMutexLocker lock(&m_writerMutex);
-        m_delegate->assetWriterStarted();
+    if (m_aborted.load() || m_stopped.load())
-    });
+        return;
+
+    QMetaObject::invokeMethod(m_delegate, "assetWriterStarted", Qt::QueuedConnection);
 
     m_durationInMs.store(0);
     m_startTime = CMSampleBufferGetPresentationTimeStamp(sampleBuffer);
@@ -236,22 +235,18 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
 {
     Q_ASSERT(sampleBuffer);
 
-    // This code is executed only on a writer's queue, but
+    // This code is executed only on a writer's queue.
-    // it can access potentially deleted objects, so we
+    if (!m_aborted.load() && !m_stopped.load()) {
-    // need a lock and m_aborted flag test.
+        if (m_setStartTime)
-    {
+            [self setStartTimeFrom:sampleBuffer];
-        const QMutexLocker lock(&m_writerMutex);
-        if (!m_aborted && !m_stoppedInternal) {
-            if (m_setStartTime)
-                [self setStartTimeFrom:sampleBuffer];
 
-            if (m_cameraWriterInput.data().readyForMoreMediaData) {
+        if (m_cameraWriterInput.data().readyForMoreMediaData) {
-                [self updateDuration:CMSampleBufferGetPresentationTimeStamp(sampleBuffer)];
+            [self updateDuration:CMSampleBufferGetPresentationTimeStamp(sampleBuffer)];
-                [m_cameraWriterInput appendSampleBuffer:sampleBuffer];
+            [m_cameraWriterInput appendSampleBuffer:sampleBuffer];
-            }
         }
     }
 
+
     CFRelease(sampleBuffer);
 }
 
@@ -261,16 +256,13 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
     // it does not touch any shared/external data.
     Q_ASSERT(sampleBuffer);
 
-    {
+    if (!m_aborted.load() && !m_stopped.load()) {
-        const QMutexLocker lock(&m_writerMutex);
+        if (m_setStartTime)
-        if (!m_aborted && !m_stoppedInternal) {
+            [self setStartTimeFrom:sampleBuffer];
-            if (m_setStartTime)
-                [self setStartTimeFrom:sampleBuffer];
 
-            if (m_audioWriterInput.data().readyForMoreMediaData) {
+        if (m_audioWriterInput.data().readyForMoreMediaData) {
-                [self updateDuration:CMSampleBufferGetPresentationTimeStamp(sampleBuffer)];
+            [self updateDuration:CMSampleBufferGetPresentationTimeStamp(sampleBuffer)];
-                [m_audioWriterInput appendSampleBuffer:sampleBuffer];
+            [m_audioWriterInput appendSampleBuffer:sampleBuffer];
-            }
         }
     }
 
@@ -283,13 +275,12 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
 {
     Q_UNUSED(connection)
 
-    // This method can be called on either video or audio queue, never on a writer's
+    // This method can be called on either video or audio queue,
-    // queue - it does not access any shared data except this atomic flag below.
+    // never on a writer's queue, it needs access to a shared data, so
+    // lock is required.
     if (m_stopped.load())
         return;
 
-    // Even if we are stopped now, we still do not access any data.
-
     if (!CMSampleBufferDataIsReady(sampleBuffer)) {
         qDebugCamera() << Q_FUNC_INFO << "sample buffer is not ready, skipping.";
         return;
@@ -298,21 +289,18 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate()
     CFRetain(sampleBuffer);
 
     if (captureOutput != m_audioOutput.data()) {
-        {
+        const QMutexLocker lock(&m_writerMutex);
-            const QMutexLocker lock(&m_writerMutex);
+        if (m_aborted.load() || m_stopped.load()) {
-            if (m_aborted || m_stoppedInternal) {
+            CFRelease(sampleBuffer);
-                CFRelease(sampleBuffer);
+            return;
-                return;
+        }
-            }
+        // Find renderercontrol's delegate and invoke its method to
-
+        // show updated viewfinder's frame.
-            // Find renderercontrol's delegate and invoke its method to
+        if (m_service && m_service->videoOutput()) {
-            // show updated viewfinder's frame.
+            NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> *vfDelegate =
-            if (m_service && m_service->videoOutput()) {
+                (NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> *)m_service->videoOutput()->captureDelegate();
-                NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> *vfDelegate =
+            if (vfDelegate)
-                    (NSObject<AVCaptureVideoDataOutputSampleBufferDelegate> *)m_service->videoOutput()->captureDelegate();
+                [vfDelegate captureOutput:nil didOutputSampleBuffer:sampleBuffer fromConnection:nil];
-                if (vfDelegate)
-                    [vfDelegate captureOutput:nil didOutputSampleBuffer:sampleBuffer fromConnection:nil];
-            }
         }
 
         dispatch_async(m_writerQueue, ^{

src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h

@@ -51,7 +51,7 @@ class AVFCameraService;
 class QString;
 class QUrl;
 
-class AVFMediaRecorderControlIOS : public QMediaRecorderControl, public AVFMediaAssetWriterDelegate
+class AVFMediaRecorderControlIOS : public QMediaRecorderControl
 {
     Q_OBJECT
 public:
@@ -76,13 +76,10 @@ public Q_SLOTS:
     void setMuted(bool muted) Q_DECL_OVERRIDE;
     void setVolume(qreal volume) Q_DECL_OVERRIDE;
 
-    // Writer delegate:
 private:
 
-    void assetWriterStarted() Q_DECL_OVERRIDE;
+    Q_INVOKABLE void assetWriterStarted();
-    void assetWriterFailedToStart() Q_DECL_OVERRIDE;
+    Q_INVOKABLE void assetWriterFinished();
-    void assetWriterFailedToStop() Q_DECL_OVERRIDE;
-    void assetWriterFinished() Q_DECL_OVERRIDE;
 
 private Q_SLOTS:
     void captureModeChanged(QCamera::CaptureModes);

src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm

@@ -86,8 +86,7 @@ AVFMediaRecorderControlIOS::AVFMediaRecorderControlIOS(AVFCameraService *service
         return;
     }
 
-    m_writer.reset([[QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) alloc] initWithQueue:m_writerQueue
+    m_writer.reset([[QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) alloc] initWithQueue:m_writerQueue delegate:this]);
-                    delegate:this delegateQueue:dispatch_get_main_queue()]);
     if (!m_writer) {
         qDebugCamera() << Q_FUNC_INFO << "failed to create an asset writer";
         return;
@@ -259,14 +258,6 @@ void AVFMediaRecorderControlIOS::assetWriterStarted()
     Q_EMIT statusChanged(QMediaRecorder::RecordingStatus);
 }
 
-void AVFMediaRecorderControlIOS::assetWriterFailedToStart()
-{
-}
-
-void AVFMediaRecorderControlIOS::assetWriterFailedToStop()
-{
-}
-
 void AVFMediaRecorderControlIOS::assetWriterFinished()
 {
     AVFCameraControl *cameraControl = m_service->cameraControl();