AVFMediaAssetWriter - fix atomics use

1. No need in two different atomics (m_stopped/m_aborted) - the single
one 'm_state' with states (Idle/Active/Aborted) should be enough.
2. QAtomicInt::load/store actually have relaxed memory ordering semantics,
(not like std::atomic with sequential ordering as the default one)
which is not always appropriate - replace with loadAquire/storeRelease instead.

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

2 files changed, 32 insertions, 27 deletions

diff --git a/src/plugins/avfoundation/camera/avfmediaassetwriter.h b/src/plugins/avfoundation/camera/avfmediaassetwriter.h
index 9b2479909..77e2a2f32 100644
--- a/src/plugins/avfoundation/camera/avfmediaassetwriter.h
+++ b/src/plugins/avfoundation/camera/avfmediaassetwriter.h
@@ -47,13 +47,10 @@ QT_BEGIN_NAMESPACE
 class AVFMediaRecorderControlIOS;
 class AVFCameraService;
 
-typedef QAtomicInteger<bool> AVFAtomicBool;
 typedef QAtomicInteger<qint64> AVFAtomicInt64;
 
 QT_END_NAMESPACE
 
-// TODO: any reasonable error handling requires smart pointers, otherwise it's getting crappy immediately.
-
 @interface QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) : NSObject<AVCaptureVideoDataOutputSampleBufferDelegate,
                                                                AVCaptureAudioDataOutputSampleBufferDelegate>
 {
@@ -78,9 +75,8 @@ QT_END_NAMESPACE
     QT_PREPEND_NAMESPACE(AVFMediaRecorderControlIOS) *m_delegate;
 
     bool m_setStartTime;
-    QT_PREPEND_NAMESPACE(AVFAtomicBool) m_stopped;
-    QT_PREPEND_NAMESPACE(AVFAtomicBool) m_aborted;
 
+    QT_PREPEND_NAMESPACE(QAtomicInt) m_state;
     QT_PREPEND_NAMESPACE(QMutex) m_writerMutex;
 @public
     QT_PREPEND_NAMESPACE(AVFAtomicInt64) m_durationInMs;
diff --git a/src/plugins/avfoundation/camera/avfmediaassetwriter.mm b/src/plugins/avfoundation/camera/avfmediaassetwriter.mm
index 59c1d1aa2..4e227114b 100644
--- a/src/plugins/avfoundation/camera/avfmediaassetwriter.mm
+++ b/src/plugins/avfoundation/camera/avfmediaassetwriter.mm
@@ -40,7 +40,6 @@
 #include "avfcameradebug.h"
 #include "avfmediacontainercontrol.h"
 
-//#include <QtCore/qmutexlocker.h>
 #include <QtCore/qmetaobject.h>
 #include <QtCore/qsysinfo.h>
 
@@ -68,6 +67,13 @@ bool qt_camera_service_isValid(AVFCameraService *service)
     return true;
 }
 
+enum WriterState
+{
+    WriterStateIdle,
+    WriterStateActive,
+    WriterStateAborted
+};
+
 } // unnamed namespace
 
 @interface QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) (PrivateAPI)
@@ -92,8 +98,7 @@ bool qt_camera_service_isValid(AVFCameraService *service)
 
         m_delegate = delegate;
         m_setStartTime = true;
-        m_stopped.store(true);
-        m_aborted.store(false);
+        m_state.store(WriterStateIdle);
         m_startTime = kCMTimeInvalid;
         m_lastTimeStamp = kCMTimeInvalid;
         m_durationInMs.store(0);
@@ -167,15 +172,22 @@ bool qt_camera_service_isValid(AVFCameraService *service)
 
 - (void)start
 {
-    // To be executed on a writer's queue.
+    // '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_aborted.load())
+    if (m_state.load() == WriterStateAborted)
         return;
 
     [self setQueues];
 
     m_setStartTime = true;
-    m_stopped.store(false);
+    m_state.storeRelease(WriterStateActive);
+
     [m_assetWriter startWriting];
     AVCaptureSession *session = m_service->session()->captureSession();
     if (!session.running)
@@ -187,26 +199,23 @@ bool qt_camera_service_isValid(AVFCameraService *service)
     // To be executed on a writer's queue.
     {
     const QMutexLocker lock(&m_writerMutex);
-    if (m_aborted.load())
-        return;
-
-    if (m_stopped.load())
+    if (m_state.load() != WriterStateActive)
         return;
-
-    m_stopped.store(true);
     }
 
     [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_aborted.load())
+        if (m_state.load() != WriterStateActive)
             return;
 
         AVCaptureSession *session = m_service->session()->captureSession();
         [session stopRunning];
         [session removeOutput:m_audioOutput];
         [session removeInput:m_audioInput];
+        m_state.storeRelease(WriterStateIdle);
         QMetaObject::invokeMethod(m_delegate, "assetWriterFinished", Qt::QueuedConnection);
     }];
 }
@@ -216,9 +225,10 @@ bool qt_camera_service_isValid(AVFCameraService *service)
     // To be executed on any thread (presumably, it's the main thread),
     // prevents writer from accessing any shared object.
     const QMutexLocker lock(&m_writerMutex);
-    m_aborted.store(true);
-    if (m_stopped.load())
+    if (m_state.fetchAndStoreRelease(WriterStateAborted) != WriterStateActive) {
+        // Not recording, nothing to stop.
         return;
+    }
 
     [m_assetWriter finishWritingWithCompletionHandler:^{
     }];
@@ -231,7 +241,7 @@ bool qt_camera_service_isValid(AVFCameraService *service)
     Q_ASSERT(sampleBuffer);
 
     const QMutexLocker lock(&m_writerMutex);
-    if (m_aborted.load() || m_stopped.load())
+    if (m_state.load() != WriterStateActive)
         return;
 
     QMetaObject::invokeMethod(m_delegate, "assetWriterStarted", Qt::QueuedConnection);
@@ -248,7 +258,7 @@ bool qt_camera_service_isValid(AVFCameraService *service)
     Q_ASSERT(sampleBuffer);
 
     // This code is executed only on a writer's queue.
-    if (!m_aborted.load() && !m_stopped.load()) {
+    if (m_state.loadAcquire() == WriterStateActive) {
         if (m_setStartTime)
             [self setStartTimeFrom:sampleBuffer];
 
@@ -264,11 +274,10 @@ bool qt_camera_service_isValid(AVFCameraService *service)
 
 - (void)writeAudioSampleBuffer:(CMSampleBufferRef)sampleBuffer
 {
-    // This code is executed only on a writer's queue.
-    // it does not touch any shared/external data.
     Q_ASSERT(sampleBuffer);
 
-    if (!m_aborted.load() && !m_stopped.load()) {
+    // This code is executed only on a writer's queue.
+    if (m_state.loadAcquire() == WriterStateActive) {
         if (m_setStartTime)
             [self setStartTimeFrom:sampleBuffer];
 
@@ -290,7 +299,7 @@ bool qt_camera_service_isValid(AVFCameraService *service)
     // 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_stopped.load())
+    if (m_state.loadAcquire() != WriterStateActive)
         return;
 
     if (!CMSampleBufferDataIsReady(sampleBuffer)) {
@@ -302,7 +311,7 @@ bool qt_camera_service_isValid(AVFCameraService *service)
 
     if (captureOutput != m_audioOutput.data()) {
         const QMutexLocker lock(&m_writerMutex);
-        if (m_aborted.load() || m_stopped.load()) {
+        if (m_state.load() != WriterStateActive) {
             CFRelease(sampleBuffer);
             return;
         }