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/avfoundation') 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 From 7788feea5d55418ab61681e122c1d2d29f7bddce Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Fri, 22 Jul 2016 14:29:45 +0200 Subject: 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 --- .../avfoundation/camera/avfmediaassetwriter.h | 13 ++-- .../avfoundation/camera/avfmediaassetwriter.mm | 91 ++++++++++++---------- .../camera/avfmediarecordercontrol_ios.h | 2 - .../camera/avfmediarecordercontrol_ios.mm | 26 +++---- 4 files changed, 67 insertions(+), 65 deletions(-) (limited to 'src/plugins/avfoundation') 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 #include -#include #include @@ -63,12 +62,12 @@ QT_END_NAMESPACE QT_PREPEND_NAMESPACE(AVFScopedPointer) m_audioWriterInput; AVCaptureDevice *m_audioCaptureDevice; + // Queue to write sample buffers: + QT_PREPEND_NAMESPACE(AVFScopedPointer) m_writerQueue; // High priority serial queue for video output: QT_PREPEND_NAMESPACE(AVFScopedPointer) m_videoQueue; // Serial queue for audio output: QT_PREPEND_NAMESPACE(AVFScopedPointer) m_audioQueue; - // Queue to write sample buffers: - QT_PREPEND_NAMESPACE(AVFScopedPointer) m_writerQueue; QT_PREPEND_NAMESPACE(AVFScopedPointer) 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 m_writerQueue; AVFScopedPointer 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]; } } -- cgit v1.2.3