summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorTimur Pocheptsov <timur.pocheptsov@theqtcompany.com>2016-07-22 14:29:45 +0200
committerTimur Pocheptsov <timur.pocheptsov@theqtcompany.com>2016-07-26 13:01:56 +0000
commit7788feea5d55418ab61681e122c1d2d29f7bddce (patch)
tree7e79602d22aef1bb079b70b55ffca2ea54a888f4
parentab940d8fe9affd2ae1bb9cdf0e3f9d41fa49a662 (diff)
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>
-rw-r--r--src/plugins/avfoundation/camera/avfmediaassetwriter.h13
-rw-r--r--src/plugins/avfoundation/camera/avfmediaassetwriter.mm91
-rw-r--r--src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h2
-rw-r--r--src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm26
4 files changed, 67 insertions, 65 deletions
diff --git a/src/plugins/avfoundation/camera/avfmediaassetwriter.h b/src/plugins/avfoundation/camera/avfmediaassetwriter.h
index 77e2a2f3..c36caee8 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 4e227114..464d90db 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 33cb0880..589810b0 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 e65c9825..292b449d 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];
}
}