From 17674ddc637a059c3052953c1ec8a80a7d078293 Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Wed, 29 Jun 2016 14:50:34 +0200 Subject: 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 --- .../avfoundation/camera/avfmediaassetwriter.h | 6 +-- .../avfoundation/camera/avfmediaassetwriter.mm | 53 +++++++++++++--------- 2 files changed, 32 insertions(+), 27 deletions(-) (limited to 'src/plugins') 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 AVFAtomicBool; typedef QAtomicInteger AVFAtomicInt64; QT_END_NAMESPACE -// TODO: any reasonable error handling requires smart pointers, otherwise it's getting crappy immediately. - @interface QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) : NSObject { @@ -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 #include #include @@ -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; } -- cgit v1.2.3