From f97e1988a6f76d46d551678666a96fa5f36a92f7 Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Tue, 19 Apr 2016 15:21:50 +0200 Subject: 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 --- .../avfoundation/camera/avfmediaassetwriter.h | 24 +--- .../avfoundation/camera/avfmediaassetwriter.mm | 136 ++++++++++----------- .../camera/avfmediarecordercontrol_ios.h | 9 +- .../camera/avfmediarecordercontrol_ios.mm | 11 +- 4 files changed, 71 insertions(+), 109 deletions(-) (limited to 'src/plugins/avfoundation') diff --git a/src/plugins/avfoundation/camera/avfmediaassetwriter.h b/src/plugins/avfoundation/camera/avfmediaassetwriter.h index de8126295..21915e9ee 100644 --- a/src/plugins/avfoundation/camera/avfmediaassetwriter.h +++ b/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 AVFAtomicBool; typedef QAtomicInteger AVFAtomicInt64; @@ -80,18 +70,15 @@ QT_END_NAMESPACE // Serial queue for audio output: QT_PREPEND_NAMESPACE(AVFScopedPointer) m_audioQueue; // Queue to write sample buffers: - dispatch_queue_t m_writerQueue; + QT_PREPEND_NAMESPACE(AVFScopedPointer) m_writerQueue; QT_PREPEND_NAMESPACE(AVFScopedPointer) 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; - bool m_aborted; + QT_PREPEND_NAMESPACE(AVFAtomicBool) 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 - delegateQueue:(dispatch_queue_t)delegateQueue; + delegate:(QT_PREPEND_NAMESPACE(AVFMediaRecorderControlIOS) *)delegate; - (bool)setupWithFileURL:(NSURL *)fileURL cameraService:(QT_PREPEND_NAMESPACE(AVFCameraService) *)service; diff --git a/src/plugins/avfoundation/camera/avfmediaassetwriter.mm b/src/plugins/avfoundation/camera/avfmediaassetwriter.mm index 37004c1db..a541956a8 100644 --- a/src/plugins/avfoundation/camera/avfmediaassetwriter.mm +++ b/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 +#include #include QT_USE_NAMESPACE @@ -65,11 +67,7 @@ bool qt_camera_service_isValid(AVFCameraService *service) return true; } -} - -AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate() -{ -} +} // unnamed namespace @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 - delegateQueue:(dispatch_queue_t)delegateQueue + delegate:(AVFMediaRecorderControlIOS *)delegate { 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 = false; + m_aborted.store(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()) { - // 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. + if (m_stopped.load()) 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, ^{ - m_delegate->assetWriterFinished(); - }); + QMetaObject::invokeMethod(m_delegate, "assetWriterFinished", Qt::QueuedConnection); }]; } - (void)abort { - // To be executed on any thread, prevents writer from - // accessing any external object (probably deleted by this time) + // 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 = 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, ^{ - m_delegate->assetWriterStarted(); - }); + const QMutexLocker lock(&m_writerMutex); + 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 - // it can access potentially deleted objects, so we - // need a lock and m_aborted flag test. - { - const QMutexLocker lock(&m_writerMutex); - if (!m_aborted && !m_stoppedInternal) { - if (m_setStartTime) - [self setStartTimeFrom:sampleBuffer]; - - if (m_cameraWriterInput.data().readyForMoreMediaData) { - [self updateDuration:CMSampleBufferGetPresentationTimeStamp(sampleBuffer)]; - [m_cameraWriterInput appendSampleBuffer:sampleBuffer]; - } + // This code is executed only on a writer's queue. + if (!m_aborted.load() && !m_stopped.load()) { + if (m_setStartTime) + [self setStartTimeFrom:sampleBuffer]; + + if (m_cameraWriterInput.data().readyForMoreMediaData) { + [self updateDuration:CMSampleBufferGetPresentationTimeStamp(sampleBuffer)]; + [m_cameraWriterInput appendSampleBuffer:sampleBuffer]; } } + CFRelease(sampleBuffer); } @@ -261,16 +256,13 @@ AVFMediaAssetWriterDelegate::~AVFMediaAssetWriterDelegate() // it does not touch any shared/external data. Q_ASSERT(sampleBuffer); - { - const QMutexLocker lock(&m_writerMutex); - if (!m_aborted && !m_stoppedInternal) { - if (m_setStartTime) - [self setStartTimeFrom:sampleBuffer]; - - if (m_audioWriterInput.data().readyForMoreMediaData) { - [self updateDuration:CMSampleBufferGetPresentationTimeStamp(sampleBuffer)]; - [m_audioWriterInput appendSampleBuffer:sampleBuffer]; - } + if (!m_aborted.load() && !m_stopped.load()) { + if (m_setStartTime) + [self setStartTimeFrom:sampleBuffer]; + + if (m_audioWriterInput.data().readyForMoreMediaData) { + [self updateDuration:CMSampleBufferGetPresentationTimeStamp(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 - // queue - it does not access any shared data except this atomic flag below. + // 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()) 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); - if (m_aborted || m_stoppedInternal) { - CFRelease(sampleBuffer); - return; - } - - // Find renderercontrol's delegate and invoke its method to - // show updated viewfinder's frame. - if (m_service && m_service->videoOutput()) { - NSObject *vfDelegate = - (NSObject *)m_service->videoOutput()->captureDelegate(); - if (vfDelegate) - [vfDelegate captureOutput:nil didOutputSampleBuffer:sampleBuffer fromConnection:nil]; - } + const QMutexLocker lock(&m_writerMutex); + if (m_aborted.load() || m_stopped.load()) { + CFRelease(sampleBuffer); + return; + } + // Find renderercontrol's delegate and invoke its method to + // show updated viewfinder's frame. + if (m_service && m_service->videoOutput()) { + NSObject *vfDelegate = + (NSObject *)m_service->videoOutput()->captureDelegate(); + if (vfDelegate) + [vfDelegate captureOutput:nil didOutputSampleBuffer:sampleBuffer fromConnection:nil]; } dispatch_async(m_writerQueue, ^{ diff --git a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h b/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h index 785769486..a055e54f6 100644 --- a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.h +++ b/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; - void assetWriterFailedToStart() Q_DECL_OVERRIDE; - void assetWriterFailedToStop() Q_DECL_OVERRIDE; - void assetWriterFinished() Q_DECL_OVERRIDE; + Q_INVOKABLE void assetWriterStarted(); + Q_INVOKABLE void assetWriterFinished(); private Q_SLOTS: void captureModeChanged(QCamera::CaptureModes); diff --git a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm b/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm index b763dbcce..73e19e683 100644 --- a/src/plugins/avfoundation/camera/avfmediarecordercontrol_ios.mm +++ b/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 - delegate:this delegateQueue:dispatch_get_main_queue()]); + m_writer.reset([[QT_MANGLE_NAMESPACE(AVFMediaAssetWriter) alloc] initWithQueue:m_writerQueue delegate:this]); 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(); -- cgit v1.2.3